fix: ensures that cli and env messages are the same by shawkins · Pull Request #23797 · keycloak/keycloak

shawkins · 2023-10-06T15:39:25Z

Fix for inconsistency between cli and env usage. This seems to be the simplest way to handle this - pulling the relevant env as cli options. There is of course some messiness to doing this, and it's adding an extra parsing of the command line options, but I didn't see another way to quickly determine the target command.

closes #13608

ghost

Unreported flaky test detected, please review

ghost · 2023-10-06T17:16:00Z

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#deleteOwnAccountSectionNotVisibleWithoutDeleteAccountActionEnabled

Keycloak CI - Account Console IT (chrome)

org.openqa.selenium.NoSuchElementException: 
no such element: Unable to locate element: {"method":"css selector","selector":"#username"}
  (Session info: headless chrome=117.0.5938.132)
For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
...

Report flaky test

org.keycloak.testsuite.ui.account2.DeleteAccountTest#navigationTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Expected PersonalInfoPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/personal-info&state=5c3fffa9-8448-4932-80da-1b82d5425e07&session_state=e76b9e5f-7e71-4fde-a579-29a2608c4774&iss=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Ftest&code=334abea2-eb74-41e1-84a8-0d2df43ceee3.e76b9e5f-7e71-4fde-a579-29a2608c4774.de99c740-4edb-42e4-88b4-5d53a7ee73af)
	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 java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

org.keycloak.testsuite.federation.ldap.LDAPProvidersFullNameMapperTest#testUpdatingAttributesWorksEvenWithEmptyAttributes

Keycloak CI - Base IT (5)

jakarta.ws.rs.InternalServerErrorException: HTTP 500 Internal Server Error
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.handleErrorStatus(ClientInvocation.java:250)
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.extractResult(ClientInvocation.java:216)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.extractors.BodyEntityExtractor.extractEntity(BodyEntityExtractor.java:59)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invokeSync(ClientInvoker.java:136)
...

Report flaky test

shawkins · 2023-10-06T17:56:31Z

@vmuzikar @mabartos is there a better way to do this?

vmuzikar

I think this approach doesn't fully fix it as we need to account for fast start that skips CLI args parsing.

shawkins · 2023-10-09T12:24:08Z

I think this approach doesn't fully fix it as we need to account for fast start that skips CLI args parsing.

@vmuzikar it seems that the operator already is effectively disabling fast-start because of the inclusion of --verbose.

I see 3 options:

Remove the fast start check - how much of the startup time is it avoiding?
Proceed with the pr as is and document that --verbose or some other parameter is needed if you to get the same cli validation with your env variables
Create a separate validation layer post cli to make the experience similar - but it will need to effectively duplicate what the cli validation is doing.

vmuzikar · 2023-10-09T12:30:39Z

I'd go with either 1 or 3.

For 1 we'd need to analyze the perf impact.

For 3 we could possibly find some way to unify the logic for CLI and env vars. I wonder if config file (and potentially any config source) is affected too.

shawkins · 2023-10-09T13:17:02Z

For 3 we could possibly find some way to unify the logic for CLI and env vars.

As far as I can tell what the cli is primary giving us is expected value validation. Let me see what things look like if that is added separately.

I wonder if config file (and potentially any config source) is affected too.

My focus here was strickly on the env because of the issue, but yes if all checks are moved post-cli, then they would be against the full Config, including any config file.

mabartos · 2023-10-09T15:05:38Z

@shawkins @vmuzikar I don't like the way we're handling the configuration.

Seems to me that we're making millions of workarounds in our server configuration, which will not be possible to maintain. It probably resolves the problem, but the other one will arise soon. We should provide a better approach for the configuration in general.

I like the idea around SmallRye Config philosophy and its multiple configuration sources. In the end, you don't care about the configuration source, whether it's an ENV var, CLI option, or properties file. You just want to get information about a particular property of a system, asses the behavior of the system,...

IMO, we should create some sort of abstraction about the configuration. We might just gather the configuration from SmallRye(don't care about the source) and create some sort of representation of the Keycloak configuration. We would be able to use the Keycloak config in a more declarative way, we could execute certain validations at the representation/model level and avoid costly operations around config changes. Something similar to the Operator config.

We could easily check the changes in the configuration by, not going through the PropertyMappers again and again, but by checking the hash/revision of the representation and its transitive components.

I'd rather focus on the proper design of the server configuration and not provide other workarounds. But if you think otherwise, I'd be glad to discuss it more :))

vmuzikar · 2023-10-10T08:48:13Z

IMO, we should create some sort of abstraction about the configuration. We might just gather the configuration from SmallRye(don't care about the source) and create some sort of representation of the Keycloak configuration.

I agree but we have it already, kinda... I think we just need to unify validation.

shawkins · 2023-10-10T14:34:19Z

I think we just need to unify validation.

@vmuzikar the next stab at this creates a utility method for validating the non-cli properties. It is a much larger change, but it does have specific handling for the optimized start case.

If the overall approach seems fine, some additional questions / thoughts:

The config source name has been tacked on to the error message to make it clearer where the property is defined, but this exposes a ConfigSource name that may not be well-defined from a user perspective.
validateNonCliConfig was based on hasConfigChanges, which scans over all config. It may be better to scan based upon the applicable PropertyMappers instead - as we have seen with the method too large issue raised recently some environments will have thousands of env variables. In particular for the "fast start" case this doing quite a bit more work just to validate a handful of runtime properties that have expected values (like --log) - switched the logic to based upon the applicable properymappers instead.
how the message looks is not quite the same in the fast start case, but with some additional logic around creating a CommandLine instance (or just letting regular handling proceed) it could be the same. This point, along with the narrow nature under which the fast start option works (such as --verbose by-passing it), could point towards simply removing the fast start option.
https://github.com/keycloak/keycloak/pull/23797/files#diff-5e7526010bd1f3d3c2735d6b5d600aaf3457a83eb1699ef7e10c91e2257ae6d2R145 - could be moved to a separate pr. It's to better cover the case where a user mistaken does something like "./kc.sh --something" where there's no command present. Moved to fix: allows --version to work #24161 since there's another issue and this could get back ported.
~~Similarly it seems like we should be setting the exit code here https://github.com/keycloak/keycloak/pull/23797/files#diff-c1907932c84285e376fbc3a66277c655f30b91476db47834c3f5a81a7a45b319R80~~
part of what was requested with SSL Mode not working? #22667 was to make it clearer when --optimized was used what env values would not get used. There is a placeholder for this in the validateNonCliConfig - more than likely it should log a warning rather than error out.
A lot of tests are erroring out because keycloak won't start with: "Invalid value for option 'kc.https-client-auth': REQUEST. Expected values are: none, request, required". In the integration test keycloak.conf it's using the upper case REQUEST, so I've changed that to lower case. There may need to be some clarification on what casing is acceptable - via the cli, REQUEST would be rejected.
The main method name Picocli.validateNonCliConfig is a bit of a misnomer - it's not actually filtering out cli values, so it's effectively redoing the check that was already done. These can either be filtered out, or could be left in case additional high-level validations were to ever be added. The other thought here is to remove the expected value validation from picocli and move it to this new logic.

vmuzikar · 2023-10-19T09:33:49Z

TBD:

export KC_DB=invalid
./kc.sh start --optimized
currently marked as a TODO - we may at least want a log indicating which build-time properties were ignored. If we don't want to add further validation to the operator, then something is needed here to address the original operator issue - log scaping or even a flag that would cause a failure to start with incorrect usage of build-time properties.

export KC_LOG=invalid
./kc.sh build
Similar to the above - we could at least log what runtime values are not used.

+1 for just logging that with info level. Anything more than (failure) that might be a bit complicated and confusing. Would we consider env vars? Config file? What if the env var remain set after the previous kc.sh build? Etc., etc. But in any case, I'd address that as a follow-up.

shawkins · 2023-10-19T11:43:30Z

+1 for just logging that with info level. Anything more than (failure) that might be a bit complicated and confusing.

I've added something for this already: Picocli.outputIgnoredProperties - it is directly to the stdout because the quarkus deferred logger will swallow it sometimes otherwise.

Would we consider env vars? Config file? What if the env var remain set after the previous kc.sh build? Etc., etc.

It's all the same "configurable" sources that we're already checking. The message just says they are being ignored - it's not trying to compare to the existing value, if any.

But in any case, I'd address that as a follow-up.

It can be done in a separate pr if you want.

vmuzikar · 2023-10-19T10:28:54Z

One last nitpick. :) We could use better naming of config sources in the error messages to avoid stuff like:

... From ConfigSource KcEnvVarConfigSource

We can't probably rely on the getName() as we don't own all config sources so we can't change the names there. We'd might need some sort of mapping.

But we already use the same naming e.g. in show-config. So definitely follow-up (if anything).

Agreed, mentioned this as well in #23797 (comment) - so we'll capture that as a follow-up.

vmuzikar · 2023-10-19T10:33:32Z

It'd be nice if we could use logger here to allow users to filter this? But as already discussed offline, it seems not something we can easily achieve as deferred logging sometimes swallow messages.

vmuzikar · 2023-10-19T15:37:32Z

@mabartos @pedroigor I'd like to merge this soon. Let me know if you want to review. :)

shawkins · 2023-10-19T18:44:39Z

I'd like to merge this soon. Let me know if you want to review. :)

It should be nearly there in terms of tests passing. There may be one more change needed.

ghost

Unreported flaky test detected, please review

ghost · 2023-10-19T22:07:00Z

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.cluster.GroupInvalidationClusterTest#crudWithFailover

Keycloak CI - Legacy Clustering IT

java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "entity" is null
	at org.keycloak.testsuite.cluster.AbstractInvalidationClusterTest.sortFields(AbstractInvalidationClusterTest.java:169)
	at org.keycloak.testsuite.cluster.AbstractInvalidationClusterTest.assertEntityOnSurvivorNodesEqualsTo(AbstractInvalidationClusterTest.java:151)
	at org.keycloak.testsuite.cluster.AbstractInvalidationClusterTest.verifyEntityUpdateDuringFailover(AbstractInvalidationClusterTest.java:137)
	at org.keycloak.testsuite.cluster.GroupInvalidationClusterTest.testEntityUpdates(GroupInvalidationClusterTest.java:148)
...

Report flaky test

mabartos

@shawkins @vmuzikar As we don't have any better solution for it ATM, and it quite remarkably improves the UX, I'm ok to proceed in this form.

Pepo48

I went through the changes yesterday, I just forgot about the approve. I'm fine with the proposed, it's actually a huge improvement from my perspective.

Thanks @shawkins 👍

ghost

Unreported flaky test detected, please review

ghost · 2023-10-20T13:31:43Z

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.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (non-strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile

Keycloak CI - FIPS IT (non-strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

Keycloak CI - FIPS IT (non-strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.forms.RegisterTest#registerExistingEmailAllowed

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:60)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor556.invoke(Unknown Source)
...

Report flaky test

org.keycloak.testsuite.forms.RegisterTest#registerExistingEmailForbidden

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/login-actions/registration?execution=b8891ad8-ac05-4da8-b283-22aeb4f19be0&client_id=test-app&tab_id=nRF8uQsnCUE)
	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 jdk.internal.reflect.GeneratedMethodAccessor41.invoke(Unknown Source)
...

Report flaky test

org.keycloak.testsuite.forms.RegisterTest#registerExistingUser_emailAsUsername

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/login-actions/registration?execution=b8891ad8-ac05-4da8-b283-22aeb4f19be0&client_id=test-app&tab_id=fnMe_JZh_1U)
	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 jdk.internal.reflect.GeneratedMethodAccessor41.invoke(Unknown Source)
...

Report flaky test

shawkins · 2023-10-20T13:36:38Z

There errors seem to be from an unrelated issue with liquibase - there's a sql related exception in the failed pod logs. I'll rebase to see if we can get this pr clean.

also adds better messages for unknown options closes keycloak#13608

vmuzikar

LGTM. Are we ready to merge this yet? :)

shawkins · 2023-10-20T13:54:53Z

LGTM. Are we ready to merge this yet? :)

I think once it tests cleanly yes - see above that there some liqiubase errors in the previous run.

shawkins requested review from a team as code owners October 6, 2023 15:39

ghost added team/cloud-native flaky-test labels Oct 6, 2023

ghost mentioned this pull request Oct 6, 2023

Flaky test: org.keycloak.testsuite.ui.account2.ApplicationsTest#toggleApplicationDetailsTest #23362

Closed

ghost reviewed Oct 6, 2023

View reviewed changes

vmuzikar self-assigned this Oct 9, 2023

vmuzikar requested review from Pepo48, mabartos and vmuzikar October 9, 2023 09:55

vmuzikar requested changes Oct 9, 2023

View reviewed changes

shawkins requested a review from vmuzikar October 9, 2023 12:24

shawkins force-pushed the iss13608 branch from 7a9929e to 5dc8a4b Compare October 10, 2023 14:20

shawkins force-pushed the iss13608 branch 5 times, most recently from 219d83b to ec514c8 Compare October 11, 2023 16:12

shawkins requested a review from a team as a code owner October 11, 2023 16:12

shawkins requested a review from a team October 11, 2023 16:12

ghost added team/core labels Oct 11, 2023

ghost mentioned this pull request Oct 19, 2023

Flaky test: org.keycloak.testsuite.forms.BrowserFlowTest#testAlternativeNonInteractiveExecutorInSubflow #22644

Closed

shawkins force-pushed the iss13608 branch from f80e7c1 to 90f5fe7 Compare October 19, 2023 11:31

This was referenced Oct 19, 2023

Flaky test: org.keycloak.testsuite.adapter.servlet.OfflineServletsAdapterTest#testServlet #21536

Closed

Flaky test: org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordLink #21422

Closed

vmuzikar reviewed Oct 19, 2023

View reviewed changes

shawkins force-pushed the iss13608 branch from 90f5fe7 to 3bfc878 Compare October 19, 2023 17:40

shawkins force-pushed the iss13608 branch from 3bfc878 to 5efe737 Compare October 19, 2023 20:08

ghost reviewed Oct 19, 2023

View reviewed changes

shawkins force-pushed the iss13608 branch from 1b4f71e to c839cdb Compare October 20, 2023 00:41

mabartos approved these changes Oct 20, 2023

View reviewed changes

Pepo48 approved these changes Oct 20, 2023

View reviewed changes

shawkins force-pushed the iss13608 branch from c839cdb to 423ca29 Compare October 20, 2023 11:52

ghost reviewed Oct 20, 2023

View reviewed changes

improvement: validates the expected values of non-cli properties

f7b7f4d

also adds better messages for unknown options closes keycloak#13608

shawkins force-pushed the iss13608 branch from 423ca29 to f7b7f4d Compare October 20, 2023 13:36

vmuzikar approved these changes Oct 20, 2023

View reviewed changes

vmuzikar enabled auto-merge (squash) October 20, 2023 15:49

vmuzikar merged commit f4d1dd9 into keycloak:main Oct 20, 2023

Uh oh!

Conversation

shawkins commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 6, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.DeleteAccountTest#deleteOwnAccountSectionNotVisibleWithoutDeleteAccountActionEnabled

org.keycloak.testsuite.ui.account2.DeleteAccountTest#navigationTest

org.keycloak.testsuite.federation.ldap.LDAPProvidersFullNameMapperTest#testUpdatingAttributesWorksEvenWithEmptyAttributes

Uh oh!

shawkins commented Oct 6, 2023

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins commented Oct 9, 2023

Uh oh!

vmuzikar commented Oct 9, 2023

Uh oh!

shawkins commented Oct 9, 2023

Uh oh!

mabartos commented Oct 9, 2023

Uh oh!

vmuzikar commented Oct 10, 2023

Uh oh!

shawkins commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmuzikar commented Oct 19, 2023

Uh oh!

shawkins commented Oct 19, 2023

Uh oh!

vmuzikar Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

shawkins Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

vmuzikar Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

vmuzikar commented Oct 19, 2023

Uh oh!

shawkins commented Oct 19, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 19, 2023

Unreported flaky test detected

org.keycloak.testsuite.cluster.GroupInvalidationClusterTest#crudWithFailover

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Pepo48 left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 20, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

org.keycloak.testsuite.forms.RegisterTest#registerExistingEmailAllowed

org.keycloak.testsuite.forms.RegisterTest#registerExistingEmailForbidden

org.keycloak.testsuite.forms.RegisterTest#registerExistingUser_emailAsUsername

Uh oh!

shawkins commented Oct 20, 2023

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins commented Oct 20, 2023

shawkins commented Oct 6, 2023 •

edited

Loading

shawkins commented Oct 10, 2023 •

edited

Loading