start using the CCO config object #227

joelddiaz · 2020-07-15T17:28:26Z

to determine whether CCO is in manual/disabled mode (old configmap is deprecated and the new config object takes precedence)
to bypass permissions checking when annotating the cloud secret (AWS-only presently)
update role to grant access to read the config
copy generated config CRD from openshift/api into manifests so that it gets registered
update clusteroperator status reporting for bad modes (and conflicting legacy settings and new modes)

Also consolidate scheme registration.
Fix conditions merging to not drop old conditions when new StatusHandler has no conditions to set.
Fix fallout from that fix where StatusHandlers registered during tests were stacking up and interfering with each other.

joelddiaz · 2020-07-15T17:28:49Z

/hold
this PR depends on openshift/api#692

joelddiaz · 2020-07-20T20:01:37Z

/test e2e-aws-upgrade

joelddiaz · 2020-07-22T16:59:45Z

@staebler PTAL, this set of changes is the minimal set (IMO) to get CCO to read/respect the new operator config

The other PR will handle making the operator config required for runtime operation and handling the bootstrap mode behavior

pkg/operator/secretannotator/aws/reconciler.go

pkg/util/scheme.go

staebler

The bones of this look sound.

pkg/azure/actuator.go

pkg/operator/constants/constants.go

pkg/operator/utils/utils.go

pkg/operator/secretannotator/aws/reconciler.go

staebler · 2020-07-24T13:46:13Z

pkg/util/clusteroperator/status.go

@@ -36,6 +37,11 @@ var (
 	statusHandlers = []StatusHandler{}
 )

+// ClearHandlers so that test cases don't endlessly add handlers


This may lead to some flaky tests since tests should be able to be run in parallel.

pkg/operator/secretannotator/azure/status.go

pkg/operator/secretannotator/status/status.go

pkg/operator/secretannotator/aws/reconciler.go

joelddiaz · 2020-07-24T16:19:02Z

@staebler addressed everything except the registering/clearing of the status handlers. Need to think about that one.

pkg/cmd/operator/cmd.go

pkg/operator/secretannotator/aws/reconciler.go

pkg/operator/secretannotator/status/status.go

staebler · 2020-07-24T17:21:45Z

pkg/operator/secretannotator/vsphere/reconciler.go

@@ -129,6 +144,17 @@ func (r *ReconcileCloudCredSecret) Reconcile(request reconcile.Request) (reconci
 }

 func (r *ReconcileCloudCredSecret) validateCloudCredsSecret(secret *corev1.Secret) error {
+	// We only support passthrough so make sure the operator mode is either unset or "passthrough"


Thought it worth mentioning that the installer will not allow the mode to be set to "passthrough". Of course, the user can set the mode directly in the config CR afterwards.

staebler · 2020-07-24T17:23:22Z

pkg/operator/secretannotator/vsphere/reconciler.go

+	if err != nil {
+		return err
+	}
+	if mode != "" && mode != operatorv1.CloudCredentialsModePassthrough {


Not something that we should tackle for this PR, but this is also something that should cause the operator to be degraded.

staebler · 2020-07-24T17:44:45Z

pkg/operator/credentialsrequest/credentialsrequest_controller.go

 	if err != nil {
 		logger.WithError(err).Error("error checking if operator is disabled")
 		return reconcile.Result{}, err
 	} else if conflict {
 		logger.Error("configuration conflict betwen legacy configmap and operator config")
 		return reconcile.Result{}, fmt.Errorf("configuration conflict")
-	} else if operatorIsDisabled {
+	} else if mode == operatorv1.CloudCredentialsModeManual {


Don't need to change it, but I find the else distracting. There is no need for it to be an else since each of the previous blocks return from the function.

mode, conflict, err := ... if err != nil { ... } if conflict { ... } if mode == operatatorv1.CloudCredentialsModeManual { ... }

staebler · 2020-07-24T17:46:33Z

pkg/operator/metrics/metrics.go

 	if err != nil {
 		mc.log.WithError(err).Error("failed to determine whether CCO is disabled")
 		return
 	}
+	ccoDisabled := mode == operatorv1.CloudCredentialsModeManual


Also not something to change now, but we should make an effort in the future to remove the "disabled" concept from the CCO and replace it with "manual mode".

staebler

This looks good. Let's squash it down and wait for the api changes to merge.

joelddiaz · 2020-07-27T19:35:35Z

/retest

Pull in updated openshift/api with CCO config CRD. manual overide vendor of openshift/api move to client-go 0.19.0-rc.2 vendor controller-runtime as github.com/joelanford/controller-runtime@k8s-1.19 pull master branch for openshift/library-go and openshift/client-go

- [x] to determine whether CCO is in manual/disabled mode - [x] to bypass permissions checking when annotating the cloud secret (AWS/Azure/GCP) - [x] update role to grant access to read the config - [x] copy generated CRD from openshift/api into manifests so that it gets applied - [x] add clusteroperator status reporting for bad modes (and conflicting legacy settings and new modes). Fix conditions merging to not drop old conditions when new StatusHandler has no conditions to set. Fix fallout from that fix where StatusHandlers registered during tests were stacking up and interfering with each other.

staebler

/lgtm

openshift-ci-robot · 2020-07-29T14:53:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [joelddiaz,staebler]

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

joelddiaz · 2020-07-29T15:36:19Z

/unhold
openshift/api#692 merged

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020

openshift-ci-robot requested review from abutcher and csrwng July 15, 2020 17:28

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020

joelddiaz force-pushed the force-mode branch from d298ae3 to 557605d Compare July 17, 2020 20:44

joelddiaz mentioned this pull request Jul 17, 2020

handle bootstrap user-defined mode #228

Merged

9 tasks

joelddiaz force-pushed the force-mode branch from 557605d to 20d7078 Compare July 17, 2020 21:03

joelddiaz requested a review from staebler July 22, 2020 16:58

staebler reviewed Jul 22, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2020

joelddiaz force-pushed the force-mode branch from ff24640 to 8cece45 Compare July 23, 2020 21:50

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2020

staebler reviewed Jul 24, 2020

View reviewed changes

joelddiaz force-pushed the force-mode branch from b777de0 to c9cc599 Compare July 24, 2020 16:18

staebler reviewed Jul 24, 2020

View reviewed changes

joelddiaz force-pushed the force-mode branch from a466570 to 52065ff Compare July 24, 2020 17:48

staebler reviewed Jul 24, 2020

View reviewed changes

joelddiaz force-pushed the force-mode branch from 235145f to e59280d Compare July 27, 2020 13:14

joelddiaz force-pushed the force-mode branch from e59280d to 9d9de20 Compare July 28, 2020 17:59

joelddiaz force-pushed the force-mode branch from 9d9de20 to b3a3526 Compare July 28, 2020 19:33

joelddiaz force-pushed the force-mode branch from b3a3526 to 840ad99 Compare July 28, 2020 19:43

staebler approved these changes Jul 29, 2020

View reviewed changes

openshift-ci-robot assigned staebler Jul 29, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020

joelddiaz changed the title ~~WIP: start using the CCO config object~~ start using the CCO config object Jul 29, 2020

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 Jul 29, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2020

openshift-merge-robot merged commit caef52c into openshift:master Jul 29, 2020

joelddiaz mentioned this pull request Jul 30, 2020

Bug 1855625: vendor in updated golang/x/text #225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start using the CCO config object #227

start using the CCO config object #227

joelddiaz commented Jul 15, 2020 •

edited

Loading

joelddiaz commented Jul 15, 2020

joelddiaz commented Jul 20, 2020

joelddiaz commented Jul 22, 2020

staebler left a comment

staebler Jul 24, 2020

joelddiaz commented Jul 24, 2020

staebler Jul 24, 2020

staebler Jul 24, 2020

staebler Jul 24, 2020

staebler Jul 24, 2020

staebler left a comment

joelddiaz commented Jul 27, 2020

staebler left a comment

openshift-ci-robot commented Jul 29, 2020

joelddiaz commented Jul 29, 2020

start using the CCO config object #227

start using the CCO config object #227

Conversation

joelddiaz commented Jul 15, 2020 • edited Loading

joelddiaz commented Jul 15, 2020

joelddiaz commented Jul 20, 2020

joelddiaz commented Jul 22, 2020

staebler left a comment

Choose a reason for hiding this comment

staebler Jul 24, 2020

Choose a reason for hiding this comment

joelddiaz commented Jul 24, 2020

staebler Jul 24, 2020

Choose a reason for hiding this comment

staebler Jul 24, 2020

Choose a reason for hiding this comment

staebler Jul 24, 2020

Choose a reason for hiding this comment

staebler Jul 24, 2020

Choose a reason for hiding this comment

staebler left a comment

Choose a reason for hiding this comment

joelddiaz commented Jul 27, 2020

staebler left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 29, 2020

joelddiaz commented Jul 29, 2020

joelddiaz commented Jul 15, 2020 •

edited

Loading