Backport operator secret sequencing fix - #23705

Its definitely affected, we've been testing with it.

I'm happy to look at a more targeted fix, but it would be a different fix to the one in 23.x. I thought it would be cleaner to apply the same fix.

I guess its the WatchedSecretsController that you want to avoid bringing in? That's only there to know when to apply the missing secrets annotation used to trigger the scheduled reconcile. I could have a go at applying the label with based on the previous WatchedSecretsStore implementation, if that suits?

I'm happy to look at a more targeted fix, but it would be a different fix to the one in 23.x. I thought it would be cleaner to apply the same fix.

That's generarly true backports should be 1:1. However, in 23 we did more major changes to Operator implementation we want to avoid dragging in back to 22 to limit the risk of regressions in a micro.

In this case, I think we need a tailored fix for 22 to keep previous implementation.

@shawkins WDYT?

Other than some additional risk, probably the biggest issue for a point release is that this will trigger a rolling restart of the keycloak instance.

As long as we're okay with that, then I think the additional risk is worth not having a skew in the implementations.

the biggest issue for a point release is that this will trigger a rolling restart of the keycloak instance.

You mean upgrading to the micro will cause the restart? It will trigger a restart in any case to roll the new image.

I'm not really comfortable with backporting anything this big, especially not now before prod release. I'd rather have a different fix implementation.

OK. I need to fix the tests anyway (I only ran the unit tests, whoops!), so shall I go for an alternative implementation?

shall I go for an alternative implementation?

+1 from me

You mean upgrading to the micro will cause the restart? It will trigger a restart in any case to roll the new image.

Just to make sure I'm following you, are you saying that every release of the operator will be accompanied by a different keycloak image?

You mean upgrading to the micro will cause the restart? It will trigger a restart in any case to roll the new image.

Just to make sure I'm following you, are you saying that every release of the operator will be accompanied by a different keycloak image?

Let me ask a question - I thought that the operator <-> operand is 1:1 currently, and every time there is (even a security update) of the operator, it means that there is also updated operand image. For example, UBI9 security updates are there in both operator and operands.

So, isn't it that ALWAYS when operator is updated, operand also will be updated, thru the rolling update?

Otherwise, the Keycloak CR would need to have something like .spec.version indicating the exact version of the operand image to use. Currently I see that operator can cope with single operand image.

Having said that, what is wrong with the direct backport, i.e. is the only concern due to the rolling update of the operand?
(I do understand the motivation to lower the risk of introducing regression in the last runaway before the release - but can we assess where the risks are perhaps?)

Just to make sure I'm following you, are you saying that every release of the operator will be accompanied by a different keycloak image?

Yes, the release processes of Operator and Operand are now 100% aligned. We can't release new Keycloak version without releasing a new Operator version and vice versa.

Having said that, what is wrong with the direct backport, i.e. is the only concern due to the rolling update of the operand?
(I do understand the motivation to lower the risk of introducing regression in the last runaway before the release - but can we assess where the risks are perhaps?)

I see the risk in the potential regressions. #21162 significantly changed how we handle watching Secrets. This change has not yet been released in any Keycloak version, just nightlies. We have integration tests of course, but there's always risk we missed something. And from my perspective it's safer to release something like that in a major version than risk breaking stuff in micro release (just before a major prod release). And I see less risk in creating a smaller targeted fix just for #22404.

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.account.WebAuthnTransportLocaleTest#localizationTransportUSB

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected OIDCLogin but was  (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?client_id=account-console&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Ftest%2Faccount%2F%23%2Fsecurity%2Fsigningin&state=5775c4d3-b5b8-4034-a987-1f751ca46d9e&response_mode=fragment&response_type=code&scope=openid&nonce=7520efb8-3462-4098-b8da-50ec8a620ecf&code_challenge=gsD4llxxzug04eg1UECSqa73yMieW_Gx22JhkRo9sRc&code_challenge_method=S256)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at jdk.internal.reflect.GeneratedMethodAccessor627.invoke(Unknown Source)
...

Report flaky test

Looking at the failure on 22.0, it's actually crashing because it can't find the secret and then giving up on reconcile because it crashed too much. I think that's a different cause (although the same symptoms).

I have a more targetted fix coming in a second pr, just letting the tests run this time :)

Closing for a more targeted fix for 22.0 in #23741