OAuthRevisionLabelPodDeployer #348

p0lyn0mial · 2020-04-01T13:14:12Z

builds on #347 and adds oauth revision label deployer (1702200)

This PR adds NewUnionRevisionLabelPodDeployer that combines a standard OpenShiftAPIServer deployer and a temporal OpenSiftAuthAPI deployer into one.

OpenSiftAuthAPI is on/off based on the annotation of the openshift-oauth-apiserver-encryption-cofngi secret. It will be removed in future releases ( >= 4.7).

When OpenSiftAuthAPI deployer is on encryption controllers for this operator will wait until OpenShiftAuthAPI servers converge onto a single revision.

This PR also starts OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver in openshift-config-managed namespace as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md

p0lyn0mial · 2020-04-01T13:15:01Z

/assign @sttts

p0lyn0mial · 2020-04-01T13:15:37Z

requires openshift/library-go#763

p0lyn0mial · 2020-04-01T13:15:50Z

/hold

for testing

p0lyn0mial · 2020-04-01T14:17:44Z

/retest

p0lyn0mial · 2020-04-02T08:11:54Z

/retest

pkg/operator/encryptionprovider/oauthrevisionpoddeployer.go

pkg/operator/revisionpoddeployer/unionrevisionedpod.go

p0lyn0mial · 2020-07-14T11:24:31Z

/test e2e-aws-upgrade

p0lyn0mial · 2020-07-14T13:04:47Z

e2e-aws-operator, e2e-aws-operator-encryption-perf, e2e-aws-operator-encryption are failing because this PR requires openshift/cluster-authentication-operator#243

p0lyn0mial · 2020-07-14T13:13:04Z

pkg/operator/oauthapiencryptioncontroller/oauthapiencryptioncontroller.go

@@ -34,7 +34,7 @@ type oauthAPIServerController struct {
 }

 // New creates OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver in openshift-config-managed namespace as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md
-// Note that this code will be removed in the future release (4.6)
+// Note that this code will be removed in the future release (4.7)


tracked in https://bugzilla.redhat.com/show_bug.cgi?id=1817629

p0lyn0mial · 2020-07-14T13:13:30Z

pkg/operator/encryptionprovider/encryptionprovider.go

+// case 1 encryption off or the secret was annotated - we are in charge
+// case 2 otherwise let CAO manage its own encryption configuration
+// TODO:
+// - change case 1 in in 4.7 so that this operator doesn't manage CAO's encryption config when encryption is off


tracked in https://bugzilla.redhat.com/show_bug.cgi?id=1819723

sttts · 2020-07-15T09:17:53Z

pkg/operator/encryptionprovider/encryptionprovider.go

 	allEncryptedGRs                     []schema.GroupResource
 	encryptedGRsManagedByExternalServer sets.String

-	secretLister corev1listers.SecretNamespaceLister
+	isOAuthEncryptionConfigManagedByThisOperatorFunc func() bool


nit: s/Func//

sttts · 2020-07-15T09:25:31Z

pkg/operator/revisionpoddeployer/oauthrevisionpoddeployer.go

+// Whether this deployer is on/off is determined by isOAuthEncryptionConfigManagedByThisOperatorFunc
+// TODO:
+//  remove this deployer in 4.7
+func NewOAuthRevisionLabelPodDeployerAdapter(


MaybeDisabledDeployer might be a better name? This is nothing special about oauth.

DisabledByPredicateDeployer even. And the interface would be MaybeDisabledDeployer.

sttts · 2020-07-15T09:27:16Z

pkg/operator/revisionpoddeployer/unionrevisionedpod.go

+	"github.com/openshift/library-go/pkg/operator/encryption/statemachine"
+)
+
+type DeployerExtended interface {


MaybeDisabledDeployer

sttts · 2020-07-15T09:28:52Z

pkg/operator/revisionpoddeployer/unionrevisionedpod.go

+	Disabled() bool
+}
+
+// UnionRevisionLabelPodDeployer provides unified state from multiple distinct deployers.


... it's converged if all delegates are converged. Each deployer can be disabled.

What if all are disabled?

What if all are disabled?

we will treat it as if encryption was off

sttts · 2020-07-15T09:37:18Z

pkg/operator/revisionpoddeployer/unionrevisionedpod.go

+	return &UnionRevisionLabelPodDeployer{delegates: delegates}, nil
+}
+
+// DeployedEncryptionConfigSecret returns the actual encryption configuration across multiple deployers


.... if they all agree.

sttts · 2020-07-15T10:00:56Z

pkg/operator/starter.go

@@ -232,6 +255,14 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 		return err
 	}

+	oauthEncryptionController := oauthapiencryptioncontroller.New(


this is just the secret sync controller, isn't it? Can we make that clearer?

sttts · 2020-07-20T09:45:33Z

pkg/operator/revisionpoddeployer/deployers.go

+// That means:
+//  - none has reported an error
+//  - all have converged
+//  - all have observed exactly the same encryption configuration


nit: ... Otherwise it returns converged=false.

sttts · 2020-07-20T09:47:32Z

Lgtm.

sttts · 2020-07-20T11:08:06Z

/lgtm
/approve

…iftAPIServer deployer and a temporal OpenSiftAuthAPI deployer into one OpenSiftAuthAPI is on/off based on the annotation of the openshift-oauth-apiserver-encryption-cofngi secret. It will be removed in future releases ( >= 4.7), when OpenSiftAuthAPI deployer is on encryption controllers for this operator will wait until OpenShiftAuthAPI servers converge onto a single revision. it also starts OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver in openshift-config-managed namespace as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md

p0lyn0mial · 2020-07-28T10:48:03Z

/retest

p0lyn0mial · 2020-07-28T15:01:34Z

/retest

p0lyn0mial · 2020-07-29T12:59:25Z

/test e2e-aws

the test will be off unitl openshift/cluster-openshift-apiserver-operator#348 merges

sttts · 2020-07-30T07:30:49Z

/retest
/lgtm

openshift-ci-robot · 2020-07-30T07:31:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, sttts

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~~ [sttts]

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

p0lyn0mial · 2020-07-30T07:34:05Z

/test all

p0lyn0mial · 2020-07-30T07:36:41Z

/retest

p0lyn0mial · 2020-07-30T07:40:00Z

/test all

p0lyn0mial · 2020-07-30T08:29:28Z

/retest

p0lyn0mial · 2020-07-30T09:10:39Z

/hold cancel

sttts · 2020-07-30T09:16:05Z

e2e-aws "Pod openshift-infra/recycler-for-nfs-lr9st was pending entire time: unknown error",

/retest

openshift-bot · 2020-07-30T09:21:28Z

/retest

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

openshift-bot · 2020-07-30T09:34:19Z

/retest

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

openshift-ci-robot requested review from deads2k and mfojtik April 1, 2020 13:14

openshift-ci-robot assigned sttts Apr 1, 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 Apr 1, 2020

p0lyn0mial mentioned this pull request Apr 1, 2020

changes UnionRevisionLabelPodDeployer to deal with nil secrets openshift/library-go#763

Closed

p0lyn0mial force-pushed the with-custom-deployer branch 3 times, most recently from e574939 to b5d7329 Compare April 1, 2020 18:51

sttts reviewed Apr 2, 2020

View reviewed changes

pkg/operator/encryptionprovider/oauthrevisionpoddeployer.go Outdated Show resolved Hide resolved

p0lyn0mial force-pushed the with-custom-deployer branch from b5d7329 to 386d0c5 Compare April 2, 2020 14:53

sttts reviewed Apr 3, 2020

View reviewed changes

pkg/operator/revisionpoddeployer/unionrevisionedpod.go Outdated Show resolved Hide resolved

sttts reviewed Apr 3, 2020

View reviewed changes

pkg/operator/revisionpoddeployer/unionrevisionedpod.go Outdated Show resolved Hide resolved

p0lyn0mial force-pushed the with-custom-deployer branch from 386d0c5 to f89924c Compare April 3, 2020 10:08

p0lyn0mial force-pushed the with-custom-deployer branch from f89924c to e8271d0 Compare July 14, 2020 09:47

p0lyn0mial force-pushed the with-custom-deployer branch from e8271d0 to d58abbb Compare July 14, 2020 13:03

p0lyn0mial commented Jul 14, 2020

View reviewed changes

sttts reviewed Jul 15, 2020

View reviewed changes

p0lyn0mial force-pushed the with-custom-deployer branch from 7d650bf to a05479a Compare July 15, 2020 14:54

sttts reviewed Jul 20, 2020

View reviewed changes

p0lyn0mial force-pushed the with-custom-deployer branch from a05479a to daf8d3d Compare July 20, 2020 10:29

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 20, 2020

p0lyn0mial force-pushed the with-custom-deployer branch from daf8d3d to f4f436e Compare July 27, 2020 09:21

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

bump(*)

382f682

p0lyn0mial added a commit to p0lyn0mial/cluster-authentication-operator that referenced this pull request Jul 29, 2020

adds the encryption tests

d200ff3

the test will be off unitl openshift/cluster-openshift-apiserver-operator#348 merges

p0lyn0mial added a commit to p0lyn0mial/cluster-authentication-operator that referenced this pull request Jul 29, 2020

adds the encryption tests

40a51b4

the test will be off unitl openshift/cluster-openshift-apiserver-operator#348 merges

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

p0lyn0mial mentioned this pull request Jul 30, 2020

enables the encryption tests openshift/cluster-authentication-operator#309

Merged

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 30, 2020

openshift-merge-robot merged commit ed0e055 into openshift:master Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAuthRevisionLabelPodDeployer #348

OAuthRevisionLabelPodDeployer #348

p0lyn0mial commented Apr 1, 2020 •

edited

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 2, 2020

p0lyn0mial commented Jul 14, 2020

p0lyn0mial commented Jul 14, 2020

p0lyn0mial Jul 14, 2020

p0lyn0mial Jul 14, 2020

sttts Jul 15, 2020

sttts Jul 15, 2020

sttts Jul 15, 2020

sttts Jul 15, 2020

sttts Jul 15, 2020

p0lyn0mial Jul 15, 2020

sttts Jul 15, 2020

sttts Jul 15, 2020

p0lyn0mial Jul 15, 2020

sttts Jul 20, 2020

sttts commented Jul 20, 2020

sttts commented Jul 20, 2020

p0lyn0mial commented Jul 28, 2020

p0lyn0mial commented Jul 28, 2020

p0lyn0mial commented Jul 29, 2020

sttts commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

sttts commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

OAuthRevisionLabelPodDeployer #348

OAuthRevisionLabelPodDeployer #348

Conversation

p0lyn0mial commented Apr 1, 2020 • edited

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 1, 2020

p0lyn0mial commented Apr 2, 2020

p0lyn0mial commented Jul 14, 2020

p0lyn0mial commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 20, 2020

sttts commented Jul 20, 2020

p0lyn0mial commented Jul 28, 2020

p0lyn0mial commented Jul 28, 2020

p0lyn0mial commented Jul 29, 2020

sttts commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

p0lyn0mial commented Jul 30, 2020

sttts commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

p0lyn0mial commented Apr 1, 2020 •

edited