Skip to content

Removes the watched secret store by directly using the statefulsets - #21162

Merged
vmuzikar merged 1 commit into
keycloak:mainfrom
shawkins:iss21125a
Jul 17, 2023
Merged

Removes the watched secret store by directly using the statefulsets#21162
vmuzikar merged 1 commit into
keycloak:mainfrom
shawkins:iss21125a

Conversation

@shawkins

@shawkins shawkins commented Jun 22, 2023

Copy link
Copy Markdown
Contributor

What's going on here:

  • decided that the additional complexity of Removes the watched secret store by directly using the statefulsets #21161 wasn't warrented. The only thing this pr does much differently is not use a cache to scan the statefulsets.
  • removes the usage of a direct call to rollingUpdate, instead a rollingUpdate will be triggered by a secret hash on the statefulset template.
  • this first point also means that the upgrade of the operator will trigger a rolling restart
  • removes the need for the watched secret store, under the assumption there's nothing else using that resource. Instead we'll add an annotation to the statefulset to directly identify the secrets being watched.
  • This moves things in the direction of dependent resources / controllers that only modify their resource. This adds a separate controller for watched secrets, but keeps a programatic path for applying labels.
  • This largely mimics the old handling, but you could try to do more event filtering - differentiate statefulsets that have no watched secrets [*], omit secret events if the data is not changing, etc. That seems best to tackle later.
  • The changes assume the new label prefix of operator.keycloak.org.

closes #21125

[*] I'm wondering if at some point the initial admin secret will need to be watched as well.

@shawkins
shawkins requested review from a team as code owners June 22, 2023 21:41
This was referenced Jun 22, 2023
Closed
Closed
@shawkins

shawkins commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Cleared up the type issue with a generic parameter. To minimize conflicts with the other open prs this change was just limited to KeycloakDeployment for now.

ghost
ghost reviewed Jun 23, 2023
View reviewed changes

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@ghost

ghost commented Jun 23, 2023

Copy link
Copy Markdown

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.forms.ResetPasswordTest#resetPasswordWithSpnegoEnabled

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected LoginPage but was  (https://localhost:8543/auth/realms/test/login-actions/authenticate?session_code=yG3wIXoXoBkZfN7Gyk0KYVtarLH7FE5NPTSAzWJz2Nw&execution=68317b51-9119-4228-b970-2dbc3fe8dab1&client_id=test-app&tab_id=6FzCFqkvH7I)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at org.keycloak.testsuite.pages.LoginPage.open(LoginPage.java:236)
...

Report flaky test

@vmuzikar
vmuzikar self-requested a review June 26, 2023 15:45
@ghost ghost mentioned this pull request Jun 26, 2023
Closed
ghost
ghost reviewed Jun 26, 2023
View reviewed changes

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@ghost

ghost commented Jun 26, 2023

Copy link
Copy Markdown

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

@vmuzikar vmuzikar added the status/hold PR should not be merged. On hold for later. label Jun 28, 2023
@vmuzikar

vmuzikar commented Jun 28, 2023

Copy link
Copy Markdown
Contributor

@shawkins Given it's a more major change, does it work for you to merge after 22 is released?

@shawkins

shawkins commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

@shawkins Given it's a more major change, does it work for you to merge after 22 is released?

Yes that's fine.

@shawkins

shawkins commented Jun 30, 2023

Copy link
Copy Markdown
Contributor Author

One consideration to add is that MD5 won't be available in fips mode unless the additional java flag is set to non-fips.

@shawkins

shawkins commented Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

@vmuzikar rebased and changed the hashing to sha-256.

@ghost ghost mentioned this pull request Jul 13, 2023
Closed
This was referenced Jul 13, 2023
Closed
Closed
vmuzikar
vmuzikar reviewed Jul 14, 2023
View reviewed changes
Comment thread operator/src/main/java/org/keycloak/operator/Constants.java Outdated
Comment thread operator/src/main/java/org/keycloak/operator/controllers/WatchedSecrets.java Outdated
Comment thread operator/src/main/java/org/keycloak/operator/controllers/WatchedSecretsController.java Outdated
@shawkins
shawkins requested a review from vmuzikar July 14, 2023 16:26
@vmuzikar
vmuzikar merged commit fa83034 into keycloak:main Jul 17, 2023
Jamstah pushed a commit to Jamstah/keycloak that referenced this pull request Oct 4, 2023
@shawkins @Jamstah
Also replaces md5 usage with sha-256 (keycloak#21162)
73de629 
closes keycloak#21125
@Jamstah Jamstah mentioned this pull request Oct 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test status/hold PR should not be merged. On hold for later. team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove watched secrets store from the operator

2 participants

@shawkins @vmuzikar