CONSOLE-3322: Update cluster-authentication-operator not to go degraded without console #583

jhadvig · 2022-09-27T22:44:09Z

Adding check if the Console capability is disabled in the ClusterVersion in case the console config is not present on the cluster. In this case cluster-authentication-operator should not get degraded.

/assign @stlaz

wking · 2022-09-27T22:46:48Z

pkg/controllers/configobservation/console/observe_consoleurl.go

+	isConsoleCapabilityEnabled := false
+	for _, capability := range clusterVersionConfig.Status.Capabilities.EnabledCapabilities {
+		if capability == configv1.ClusterVersionCapabilityConsole {
+			isConsoleCapabilityEnabled = true


nit: you could break here, but 🤷, iterating through the remainder of the list isn't expensive.

there's a break in the next line now, so this thread can be marked resolved.

wking · 2022-09-27T22:55:45Z

pkg/controllers/configobservation/console/observe_consoleurl.go

+
 	consoleConfig, err := listers.ConsoleLister.Get("cluster")
 	if err != nil {
+		if errors.IsNotFound(err) && isConsoleCapabilityEnabled {


I think you want !isConsoleCapabilityEnabled instead of isConsoleCapabilityEnabled here, for "we can't find the cluster Console, but that's ok, because this is a console-less cluster".

I'm a bit unsure about errors.IsNotFound(err). Does that match the errors we get when requesting a resource whose backing CRD is missing?

Thats a fair point @wking ... Not entirely sure what should be the other then the IsNotFound error

wking · 2022-09-27T22:57:14Z

pkg/controllers/configobservation/console/observe_consoleurl.go

 	consoleConfig, err := listers.ConsoleLister.Get("cluster")
 	if err != nil {
+		if errors.IsNotFound(err) && isConsoleCapabilityEnabled {
+			return nil, nil


Is it worth return existingConfig, nil here? I'm not sure how much the caller assumes about existingConfig completion, but the actually-failing returns in surrounding lines include existingConfig, and there may be useful stuff in there, even in the "cluster is no-console" case.

bparees · 2022-10-18T15:37:11Z

@stlaz can we move this forward? it's critical we resolve the degraded condition before 4.12 ships so that disabling the Console doesn't result in a degraded auth operator.

jhadvig · 2022-10-19T12:56:39Z

@wking comments addressed.

@stlaz PTAL :)

wking · 2022-10-19T16:08:01Z

pkg/controllers/configobservation/console/observe_consoleurl_test.go

+			if tt.clusterVersion != nil {
+				if err := clusterVersionIndexer.Add(&configv1.ClusterVersion{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "cluster",


This probably needs to move to version too.

wking · 2022-10-20T04:27:27Z

platform.gcp.project: Internal error: googleapi: Error 500: Internal error encountered., backendError:

/retest

wking

/lgtm

openshift-ci · 2022-10-20T04:28:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jhadvig, wking
Once this PR has been reviewed and has the lgtm label, please ask for approval from stlaz by writing /assign @stlaz in a comment. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

bparees · 2022-10-20T21:11:53Z

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

openshift-ci · 2022-10-20T21:11:58Z

@bparees: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d29f6000-50bb-11ed-9541-8b5ab96ea429-0

wking · 2022-10-21T04:42:28Z

That^ run didn't have Ben's new code from openshift/origin#27481:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/openshift-cluster-authentication-operator-583-ci-4.12-e2e-aws-sdn-no-capabilities/1583204398492815360/artifacts/release/artifacts/release-payload-latest/image-references | jq -r '.spec.tags[] | select(.name == "tests").annotations["io.openshift.build.commit.id"]'
9da7e86531a7ed93591692be6acafe9ae3e452c5
$ git --no-pager log --oneline --first-parent -2 origin/master
77afd1ee7d (origin/release-4.13, origin/release-4.12, origin/master, origin/HEAD) Merge pull request #27481 from bparees/nocaps
9da7e86531 Merge pull request #27482 from ardaguclu/fix-flaky-cli-tests

Trying again:

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

openshift-ci · 2022-10-21T04:42:50Z

@wking: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c4785560-50fa-11ed-9924-3fbb0aff9287-0

bparees · 2022-10-21T21:30:38Z

I don't know what to make of this error, or why it would be tied to any particular capability being turned off:

Oct 21 05:36:12.519: INFO: Waiting for the OAuth server route to be ready: x509: certificate signed by unknown authority

so:

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

But that'll have to be our debugging starting point if this fails again.

openshift-ci · 2022-10-21T21:30:42Z

@bparees: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9b56df70-5187-11ed-8a47-1066141d15d7-0

wking · 2022-10-22T05:35:42Z

Yup, stdout for failed test-cases in the new run has lots of Waiting for the OAuth server route to be ready: x509: certificate signed by unknown authority too :/

stlaz · 2022-10-24T06:46:55Z

pkg/controllers/configobservation/console/observe_consoleurl.go

+	isConsoleCapabilityEnabled := false
+	for _, capability := range clusterVersionConfig.Status.Capabilities.EnabledCapabilities {
+		if capability == configv1.ClusterVersionCapabilityConsole {
+			isConsoleCapabilityEnabled = true


shouldn't the execution be stopped with a hardcoded value once the capability is not allowed?

it should 👍

stlaz · 2022-10-24T06:48:52Z

pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go

 			IngressLister:   configInformer.Config().V1().Ingresses().Lister(),

 			APIServerLister_:     configInformer.Config().V1().APIServers().Lister(),
 			ConsoleLister:        configInformer.Config().V1().Consoles().Lister(),


If the console type does not exist, the lister presence will fill our logs with error messages in a loop. We can't have that.

stlaz · 2022-10-24T06:50:30Z

pkg/controllers/configobservation/console/observe_consoleurl_test.go

 	}{
 		{
-			name:           "NoConsoleConfig",
+			name:           "NoConsoleConfigConsoleCapabilityEnabled",


what happens when the capability is not enabled but someone creates the CRD and the object?

Then I think the cluster would be in an inconsistent state since the the CRD would be there but quite a lot of other resrouces would be missing.
Also is that even a valid scenario? meaning that only a cluster admin can create CRDs, not mentioning that he would/should be the one that decides which set of capabilities should be enabled when provisioning the cluster.

Also is that even a valid scenario?

Unless you can prevent it, yes.

Add a unit test, please.

cluster admin can create CRDs

Cluster admin and likely anyone installing an operator.

stlaz · 2022-10-24T06:51:16Z

~~Once all the comments are fixed, please provide a must-gather from a cluster that does not have the console capabiity enabled.~~

edit: I see from the above comments that we can already test this, awesome 👍

stlaz · 2022-10-24T06:53:19Z

/lgtm cancel

bparees · 2022-10-24T13:47:36Z

@stlaz here's the must-gather from the most recent run:
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/openshift-cluster-authentication-operator-583-ci-4.12-e2e-aws-sdn-no-capabilities/1583571500747722752/artifacts/e2e-aws-sdn-no-capabilities/gather-must-gather/artifacts/

(doesn't include any changes since friday, not sure if you think that is likely to affect the failure)

jhadvig · 2022-10-24T15:11:48Z

Comments addressed. PTAL

pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go

pkg/operator/starter.go

stlaz · 2022-10-24T15:18:35Z

pkg/controllers/configobservation/console/observe_consoleurl_test.go

 	}{
 		{
-			name:           "NoConsoleConfig",
+			name:           "NoConsoleConfigConsoleCapabilityEnabled",


Also is that even a valid scenario?

Unless you can prevent it, yes.

Add a unit test, please.

cluster admin can create CRDs

Cluster admin and likely anyone installing an operator.

stlaz · 2022-10-26T08:23:12Z

pkg/controllers/configobservation/console/observe_consoleurl.go

-	if !isConsoleCapabilityEnabled {
-		return existingConfig, errs
-	}



wrap this whole piece with the condition block and remove the capability special case from the error handling

stlaz · 2022-10-26T08:23:50Z

pkg/controllers/configobservation/console/observe_consoleurl.go

-		return existingConfig, errs
-	}

 	consoleConfig, err := listers.ConsoleLister.Get("cluster")


nil exception panic here in case the capability is not enabled

…ed without console

jhadvig · 2022-10-26T08:54:20Z

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

openshift-ci · 2022-10-26T08:54:23Z

@jhadvig: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c7b40b60-550b-11ed-9b2e-b9f3e74a45b6-0

bparees · 2022-10-26T13:33:15Z

level=error msg=failed to initialize the cluster: Multiple errors are preventing progress:
level=error msg=* Cluster operator authentication is not available

from latest run.

stlaz · 2022-10-26T14:13:34Z

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

let's see what happens with the latest changes

openshift-ci · 2022-10-26T14:13:56Z

@stlaz: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/60cd7300-5538-11ed-8f53-acf499b096a8-0

jhadvig · 2022-10-27T21:14:27Z

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

openshift-ci · 2022-10-27T21:14:31Z

@jhadvig: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/57259260-563c-11ed-9cce-e3b4f93a9592-0

bparees · 2022-10-27T21:31:39Z

pkg/operator/starter.go

 	oauthInformers := oauthinformers.NewSharedInformerFactory(oauthClient, resync)

+	clusterVersionLister := operatorCtx.operatorConfigInformer.Config().V1().ClusterVersions().Lister()
+	clusterVersionConfig, err := clusterVersionLister.Get("version")


I strongly recommend you add some debugging that prints out these two values (the clusterversion object and the err object), until you sort out what is going on (and then you can remove it)

I bet my money that the issue is in the informer... 💸

jhadvig · 2022-10-27T22:37:02Z

/payload-job periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

openshift-ci · 2022-10-27T22:37:17Z

@jhadvig: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.12-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e07823b0-5647-11ed-8d79-857c7ed15d09-0

openshift-ci · 2022-10-28T09:03:26Z

@jhadvig: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-console-login	`c779d16`	link	true	`/test e2e-console-login`
ci/prow/e2e-agnostic	`c779d16`	link	true	`/test e2e-agnostic`
ci/prow/e2e-agnostic-upgrade	`c779d16`	link	true	`/test e2e-agnostic-upgrade`
ci/prow/e2e-agnostic-ipv6	`c779d16`	link	false	`/test e2e-agnostic-ipv6`
ci/prow/e2e-aws-single-node	`c779d16`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-operator	`c779d16`	link	true	`/test e2e-operator`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

jhadvig · 2022-11-11T11:48:56Z

Closing in favour of #587

openshift-ci bot assigned stlaz Sep 27, 2022

wking reviewed Sep 27, 2022

View reviewed changes

jhadvig force-pushed the CONSOLE-3322 branch from 8508055 to dbba866 Compare October 19, 2022 12:56

jhadvig force-pushed the CONSOLE-3322 branch from dbba866 to 9bbc203 Compare October 19, 2022 15:44

wking reviewed Oct 19, 2022

View reviewed changes

jhadvig force-pushed the CONSOLE-3322 branch from 9bbc203 to 484a04e Compare October 19, 2022 16:14

wking approved these changes Oct 20, 2022

View reviewed changes

openshift-ci bot assigned wking Oct 20, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2022

stlaz reviewed Oct 24, 2022

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2022

stlaz suggested changes Oct 24, 2022

View reviewed changes

jhadvig force-pushed the CONSOLE-3322 branch from 5d270f9 to 3efec7f Compare October 25, 2022 19:01

stlaz suggested changes Oct 26, 2022

View reviewed changes

CONSOLE-3322: Update cluster-authentication-operator not to go degrad…

cad2b81

…ed without console

jhadvig force-pushed the CONSOLE-3322 branch from 92ad01b to cad2b81 Compare October 26, 2022 08:37

addressing comments

0a1714b

wait for it

3da2e5c

bparees reviewed Oct 27, 2022

View reviewed changes

wait for it vol.2

a6c1ea8

jhadvig added 2 commits October 28, 2022 07:22

debug

022752c

start sync

c779d16

openshift-ci-robot mentioned this pull request Nov 10, 2022

OCPBUGS-3441: Update cluster-authentication-operator not to go degraded without console [4.13] #587

Merged

jhadvig closed this Nov 11, 2022

CONSOLE-3322: Update cluster-authentication-operator not to go degraded without console #583

CONSOLE-3322: Update cluster-authentication-operator not to go degraded without console #583

Uh oh!

Conversation

jhadvig commented Sep 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Oct 18, 2022

Uh oh!

jhadvig commented Oct 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Oct 20, 2022

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 20, 2022

Uh oh!

bparees commented Oct 20, 2022

Uh oh!

openshift-ci bot commented Oct 20, 2022

Uh oh!

wking commented Oct 21, 2022

Uh oh!

openshift-ci bot commented Oct 21, 2022

Uh oh!

bparees commented Oct 21, 2022

Uh oh!

openshift-ci bot commented Oct 21, 2022

Uh oh!

wking commented Oct 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stlaz commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stlaz commented Oct 24, 2022

Uh oh!

bparees commented Oct 24, 2022

Uh oh!

jhadvig commented Oct 24, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhadvig commented Oct 26, 2022

Uh oh!

openshift-ci bot commented Oct 26, 2022

Uh oh!

bparees commented Oct 26, 2022

Uh oh!

stlaz commented Oct 26, 2022

Uh oh!

stlaz commented Oct 24, 2022 •

edited

Loading