Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1940142: Correct incorrect CACert in secrets created prior to 4.6 #314

Merged
merged 4 commits into from Apr 2, 2021
Merged

Bug 1940142: Correct incorrect CACert in secrets created prior to 4.6 #314

merged 4 commits into from Apr 2, 2021

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Mar 24, 2021

Prior to OpenShift 4.6 the installer would create secrets with an invalid cacert which prevented them from working when validating the cert. This has been fixed in the installer for new clusters, but existing cluster may still contain these invalid credentials. We need a way to seamlessly fix these broken root credential secrets. This series fixes the problem by adding a secretannotator for OpenStack and applying the fix in its reconcile loop.

OpenStack did not previously have a secretannotator. The default behaviour of CCO on an OpenStack cluster is to run the AWS secretannotator. This is a noop on an OpenStack cluster, as the AWS root secret will not exist.

The stub secretannotator does nothing other than validate that the cluster does not specify the 'mint' mode, which is not supported by OpenStack. This is new behaviour and could potentially cause a regression if a user had specified this invalid configuration, which was previously being ignored. However, we plan to add support for all modes in the future so this code is useful, as well as conveniently following the pattern of the other cloud providers.

We add the secret validation to the reconcile loop. If we detect an invalid secret we directly update the root secret and re-reconcile.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. labels Mar 24, 2021
@openshift-ci-robot
Copy link
Contributor

@mdbooth: This pull request references Bugzilla bug 1940142, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @lwan-wanglin

In response to this:

Bug 1940142: Correct incorrect CACert in secrets created prior to 4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Mar 24, 2021
@openshift-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 24, 2021

Please take a look at where this is going, but it is currently completely untested. I am adding unit tests for all new code.

@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 24, 2021

This may supercede #309

joelddiaz
joelddiaz reviewed Mar 25, 2021
View reviewed changes
pkg/openstack/actuator.go Outdated Show resolved Hide resolved
pkg/openstack/actuator_test.go Outdated Show resolved Hide resolved
pkg/openstack/actuator_test.go Outdated Show resolved Hide resolved
pkg/operator/secretannotator/openstack/reconciler.go Outdated Show resolved Hide resolved
pkg/operator/secretannotator/openstack/reconciler.go Show resolved Hide resolved
@mdbooth mdbooth force-pushed the correct_cacert branch 2 times, most recently from 92e239c to 443d6ae Compare March 30, 2021 15:36
@mdbooth mdbooth marked this pull request as ready for review March 31, 2021 09:25
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2021
@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 31, 2021

/cc @pierreprinetti @Fedosin

@Fedosin Fedosin mentioned this pull request Mar 31, 2021
Closed
@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 31, 2021

/hold I'm just reworking the tests a bit to remove the go-test dependency

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2021
@mdbooth
Copy link
Contributor Author

mdbooth commented Mar 31, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2021
mdbooth and others added 4 commits April 1, 2021 09:08
@mdbooth
go mod tidy
420c678
@mdbooth
openstack: Add stub secretannotator
e8294ad 
This secretannotator does nothing other than validate the configuration
of the OpenStack root secret. It will raise an error if the
configuration is anything other than the default or explicitly
'Passthrough'.
@mdbooth
openstack: Pull non-actuator method and constants into utils.go
ab037f9 
This refactor is in preparation for subsequent patches which reuse
GetRootCloudCredentialsSecretData and openstack constants from another
package.
@mdbooth @Fedosin
Bug 1940142: prepare correct clouds.yaml data
405271d 
In OCP versions <4.6 we included the original host cacert path in
clouds.yaml. It is wrong because internally we always use
"/etc/kubernetes/static-pod-resources/configmaps/cloud-config/ca-bundle.pem".

The issue was fixed in the installer [1] in 4.6 but we still have to
replace incorrect values in the generated clouds.yaml for the
previous versions.

This commit checks if the value is not correct and fixes it in the root
secret.

[1] openshift/installer#4227

Co-Authored-By: Mike Fedosin <mfedosin@redhat.com>
@mdbooth
Copy link
Contributor Author

mdbooth commented Apr 1, 2021

/retest

2 similar comments
@mdbooth
Copy link
Contributor Author

mdbooth commented Apr 1, 2021

/retest

@mdbooth
Copy link
Contributor Author

mdbooth commented Apr 1, 2021

/retest

joelddiaz
joelddiaz reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelddiaz, mdbooth

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 1808ef1 into openshift:master Apr 2, 2021
@openshift-ci-robot
Copy link
Contributor

@mdbooth: All pull requests linked via external trackers have merged:

Bugzilla bug 1940142 has been moved to the MODIFIED state.

In response to this:

Bug 1940142: Correct incorrect CACert in secrets created prior to 4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Fedosin Fedosin mentioned this pull request Apr 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelddiaz joelddiaz joelddiaz left review comments

@lwan-wanglin lwan-wanglin Awaiting requested review from lwan-wanglin

@dgoodwin dgoodwin Awaiting requested review from dgoodwin

@twiest twiest Awaiting requested review from twiest

@Fedosin Fedosin Awaiting requested review from Fedosin

@pierreprinetti pierreprinetti Awaiting requested review from pierreprinetti

Assignees

@joelddiaz joelddiaz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mdbooth @openshift-ci-robot @openshift-bot @joelddiaz @openshift-merge-robot