allows normal reconciliation to continue even if secrets are not present - #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.ui.account2.DeleteAccountTest#deleteOwnAccountAIACancellationSucceeds

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.ui.account2.DeleteAccountTest.deleteOwnAccountAIACancellationSucceeds(DeleteAccountTest.java:102)
...

Report flaky test

An enhancement to ErrorStatusUpdateControl could be to allow for configurable retry similar to the main loop. cc @csviri This could ideally be made more sophisticated eventually by using a back-off time influenced by the hasErrors condition lastTransitionTime.

I can imaging a re-schdule for the ErrorStatusUpdateControl, interfering directly with the retry (in the sense, like setting the next retry interval) is going against the current philosophy of retries (which can be also a discussion for v5 if you want to open that). But a simple re-schedule that actually cancels the current retry process, and just reschedules an execution makes sense imo.

added an issue: operator-framework/java-operator-sdk#2016

What is the purpose of the operator watching the secrets. Does it actually do anything with them ?
My understanding is that the secrets are used by the stateful set therefore the operator does not really need to concern itself with them.

I have found it difficult to use the operator in #22170

The issue I face in a CD environment we do not control order of creating resources in K8s. We use externalsecrets with the secret content residing in vault. These take approx 2 mins to sync onto the cluster and be created as secrets.

Because the operator is expecting to see the secrets at the point in time the Keycloak object is created then an error is thrown by the operator. More critically this error is not recoverable and the only way to solve it is to update the keycloak object (or delete and reapply it).

I was thinking that if the operator did not get involved with the secrets then the sts woudld be created immaterial if the secrets existed or not. And then the sts would stall out on pod construction waiting for the secrets to be available.
When the secrets do become available the pod/scheduler logic would then create the pod and fulfil the sts.

This would mean the responsibility of the operator is to generate the STS and service and initial secret. The operator would not be responsible for anything to do with the secrets.

From a security perspective: removing the ability for the operator to access secrets would be a good thing (least privilege) .... but I think that would require some rethinking of how the initial secret is created by the operator (that would need to be descoped from the responsibility of the operator).

TLDR: keen to see how to progress usage of operator in context of secrets not being available when keycloak object is created.

What is the purpose of the operator watching the secrets. Does it actually do anything with them ?
My understanding is that the secrets are used by the stateful set therefore the operator does not really need to concern itself with them.

The operator is taking the reponsibility of ensuring that changes to secrets are accounted for for by the keycloak instance.
Secrets are currently utilized by the statefulset via env variables. If the content of the secret changes the environment of the pods are not changed - it must be restarted to pickup the changes. Even if/when we move to using mounted secrets it will depend on how the property is handled - basic quarkus config values are effectively resolved on first use and will not react to changes. Meaning a restart is still needed.

And then the sts would stall out on pod construction waiting for the secrets to be available.

That is what this pr does.

From a security perspective: removing the ability for the operator to access secrets would be a good thing (least privilege) .... but I think that would require some rethinking of how the initial secret is created by the operator (that would need to be descoped from the responsibility of the operator).

Other than removing the watched secrets feature you would just limit the rbac to create and remove the other verbs.

A related issue is #10773 - if you took the responsibility away from the operator of watching secrets, something would still need to trigger the restarts. This basically boils down to eventually adding an annotation to the sts pod template that triggers another rollout. The secret watching logic is computing a hash of all the watched secrets for this purpose.

All watched secrets in our Operator are defined as SecretKeyReference which can be marked as optional.

To clarify my concern in the pr description, if we want to support declaratively optional secrets (currently we do not as the optional SecretKeySelector field is ignored), then we'll have a very similar issue later - that is optional secrets once they exist won't be picked up until the keycloak is reconciled again.

By requiring Secrets to exist on the Operator level, we're basically breaking the contract as is defined in the CRD (that the Secret can be optional).

Are you indicating that we should support declaratively optional secrets?

that is optional secrets once they exist won't be picked up until the keycloak is reconciled again.

We should probably have a periodic reconciliation then (every 10 seconds or so).

Are you indicating that we should support declaratively optional secrets?

Possibly... Or we should stop exposing the optional field in our contract. But of course full support of optional Secrets is beyond the scope this PR.

Possibly... Or we should stop exposing the optional field in our contract.

We'd have to create our own SecretKeySelector replacement for that - do you want to track that with a separate pr, or work towards supporting optional instead?

But of course full support of optional Secrets is beyond the scope this PR.

Sure, I just wanted to bring it up because another solution common to both problems is to just watch all secrets - but unless the change in this pr were made (or the secret logic retargeted to the keycloak cr) - there would be a problem as the secret watching scans the statefulsets to find who is watching. The pro of this approach is that we'd get away from manipulating the secret labels. The con being that we currently scan the statefulsets with a api server listing - we'd want that to use the cache instead.

We should probably have a periodic reconciliation then (every 10 seconds or so).

That would be the alternative to watching all secrets. There we'd need to detect this special case in addition to the current check for not ready that is triggering the 10 second delayed reconciliation.

I don't think we should be watching all secrets within a namespace. I can imagine folks doing all kinds of strange things having larger number of Secrets and we might see some issues with being informed on all of them. WDYT?

do you want to track that with a separate pr, or work towards supporting optional instead?

I think we should first discuss a bit if it even make sense to have optional Secrets. Do you see any use cases for it in the context of Keycloak? I'm not sure... If you specify a Secret in CR, we could expect it exists.

I don't think we should be watching all secrets within a namespace. I can imagine folks doing all kinds of strange things having larger number of Secrets and we might see some issues with being informed on all of them. WDYT?

Yes it could be a large number, but no I don't think there would be much of an issue being informed on all of them - the event volume shouldn't be that high and the informer can be customized to only store the hash rather than the entire secret. As long as we are efficient about which keycloaks / statefulsets that maps to, there shouldn't be much of a problem.

I think we should first discuss a bit if it even make sense to have optional Secrets. Do you see any use cases for it in the context of Keycloak? I'm not sure... If you specify a Secret in CR, we could expect it exists.

I haven't seen any yet. It would have to be where some default would apply otherwise - the database and tls secrets don't fall into that category, but it seems plausible for the whole gamit of additional options.

@Bengreen related to all of this, can you open an enhancement request / discussion for about the operator not taking the responsibility for watching secrets?

@shawkins Thinking about this more, I'm still more inclined towards not watching all Secrets in the namespace. Feels a bit weird that the Operator would get events (incl. the Secret's data) for Secrets that might be unrelated to the Keycloak deployment. Almost feels like a minor security issue. Unless there's a very strong benefit in this (like significantly simpler implementation), I would not go this way.

Regarding the optional Secrets. I can see two reasons why we should support it.

1. We're following standard K8s pattern where Secrets can be marked as optional (either as env vars or mounted as files).
2. Even in cases where the Secret might be seemingly required in Keycloak (like the aforementioned TLS or DB Secret), it's actually optional too. TLS cert/key, DB creds can both be already present and configured in a custom built image.

Feels a bit weird that the Operator would get events (incl. the Secret's data) for Secrets that might be unrelated to the Keycloak deployment. Almost feels like a minor security issue.

From an RBAC perspective the operator already has access to all secrets in the namespaces it is watching. Upstream kubernetes has also rejected adding the ability to specify RBAC based upon a label.

Unless there's a very strong benefit in this (like significantly simpler implementation), I would not go this way.

It would:

• remove the need add / remove secret labels
• prevent reconciliation loops that are currently noisy (several info level logs), by making it event driven instead
• works for missing required or optional secrets

I'm not sure if that qualifies as strong enough though.

The approach that would simplify things the most is to remove watching entirely and leave it to the user to trigger restarts of the keycloak, but that is a very different direction.

Regarding the optional Secrets. I can see two reasons why we should support it.

Captured as #22610

Feel free to add some tests for this PR. :)

Ok added an integration test to ensure that we still create the statefulset even if there's a missing secret. We don't yet pick this condition up in any meaningful way in the status because pod status condition is just waiting to create container without any specifics that a required secret is missing. Another enhancement would be to add to our status that a required secret does not yet exist.

Captured as #22610

After looking at things more, that does need to be addressed here - we are passing through the optional flag, so we will not pick up those secrets until after there's been a reconciliation of the keycloak.

The second commit adds handling for missing optional secrets using the polling mechanism. Those changes will conflict with the final pr to convert to dependent resources, but not too much. I'm guessing that you prefer this direction instead of watching all secrets correct?

I started a discussion on option of Operator not watching secrets in #22647