Support ignoring artigacts when running re-augmentation by pedroigor · Pull Request #20976 · keycloak/keycloak

pedroigor · 2023-06-13T15:16:40Z

Closes #20974

mabartos · 2023-06-14T15:03:33Z

@pedroigor The issue is super valid. Code LGTM. However, there are still some insufficiencies as the tests failed.

pedroigor · 2023-06-16T15:16:58Z

@mabartos Looks like we can make this much simpler. @aloubyansky is working on a change to Quarkus.

Initial tests show that it is enough to solve our problem. The main change on our side would be to set quarkus.class-loading.removed-artifacts before running the re-augmentation in the Build command to force removing new store (and vice-versa) when running build steps.

ghost · 2023-06-16T17:07:09Z

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.LinkedAccountsTest#linkAccountTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Expected LinkedAccountsPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/security/linked-accounts&state=62bc3da3-22a9-4de1-9b7e-eda628a88002&session_state=1e7b2df3-c5fb-4ba4-9167-4f0a4e14e6bc&code=49bcc294-8e3f-4be2-8288-d34a20f43cb9.1e7b2df3-c5fb-4ba4-9167-4f0a4e14e6bc.d502b72b-4fac-4629-b403-8ddd025a130d)
	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.GeneratedMethodAccessor103.invoke(Unknown Source)
...

Report flaky test

ghost

Unreported flaky test detected, please review

pedroigor · 2023-06-19T12:03:58Z

The fix from quarkusio/quarkus#34107 should help to remove the workarounds in hibernate (e.g.: duplicate of the Integrator SPI impl in dist runtime module).

Based on discussions with @aloubyansky, we should be able to get this change before 22.

vmuzikar · 2023-06-29T08:27:47Z

@pedroigor Is it still valid we want this change to get to 22?

aloubyansky · 2023-06-29T08:46:14Z

Just FYI, io.quarkus:quarkus-bom:3.2.0.Final is available on Central now.

hmlnarik · 2023-06-29T10:04:29Z

@pedroigor Is it still valid we want this change to get to 22?

Would be good to have this change for 22 as long as it can be based on the Quarkus 3.2.0 changes so that the whole modules can be ignored upon build. Then this PR would be cleaner, without the need to override the EventListenerIntegrator class.

vmuzikar · 2023-06-30T09:25:32Z

If I understand correctly, this is then effectively waiting for Quarkus 3.2, correct?

aloubyansky · 2023-06-30T09:31:39Z

See my previous comment, it's already available. Not the platform but you are not using the platform BOM anyway.

vmuzikar · 2023-06-30T09:34:25Z

@aloubyansky Yeah, I meant waiting for us to upgrade to 3.2. ;)

vmuzikar · 2023-07-03T13:49:26Z

Since #21352 was merged, we can continue with this PR.

ghost · 2023-07-03T16:02:48Z

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

Keycloak CI - Forms IT (firefox)

java.lang.AssertionError: Expected ErrorPage but was Sign in to test (https://localhost:8543/auth/realms/test/login-actions/reset-credentials?client_id=test-app&tab_id=Z35EXfACAfY)
	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)
...

org.openqa.selenium.TimeoutException: 
Navigation timed out after 10000 ms
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az548-577', ip: '10.1.0.158', os.name: 'Linux', os.arch: 'amd64', os.version: '5.15.0-1040-azure', java.version: '17.0.7'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Keycloak CI - Store Model Tests

java.lang.AssertionError: 
threads didn't terminate in time: [main (TIMED_WAITING): locked on java.util.concurrent.FutureTask@705af15f waiting for 98343 ms blocked for 8576 ms
	at java.base@17.0.7/jdk.internal.misc.Unsafe.park(Native Method)
	at java.base@17.0.7/java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:252)
	at java.base@17.0.7/java.util.concurrent.FutureTask.awaitDone(FutureTask.java:444)
...

Report flaky test

ghost

Unreported flaky test detected, please review

ghost · 2023-07-03T16:37:13Z

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

Keycloak CI - Forms IT (firefox)

java.lang.AssertionError: Expected ErrorPage but was Sign in to test (https://localhost:8543/auth/realms/test/login-actions/reset-credentials?client_id=test-app&tab_id=Z35EXfACAfY)
	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)
...

org.openqa.selenium.TimeoutException: 
Navigation timed out after 10000 ms
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az548-577', ip: '10.1.0.158', os.name: 'Linux', os.arch: 'amd64', os.version: '5.15.0-1040-azure', java.version: '17.0.7'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Keycloak CI - Store Model Tests

java.lang.AssertionError: 
threads didn't terminate in time: [main (TIMED_WAITING): locked on java.util.concurrent.FutureTask@705af15f waiting for 98343 ms blocked for 8576 ms
	at java.base@17.0.7/jdk.internal.misc.Unsafe.park(Native Method)
	at java.base@17.0.7/java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:252)
	at java.base@17.0.7/java.util.concurrent.FutureTask.awaitDone(FutureTask.java:444)
...

Report flaky test

ghost

Unreported flaky test detected, please review

ghost

Unreported flaky test detected, please review

ghost · 2023-07-03T18:32:48Z

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

Keycloak CI - Forms IT (firefox)

java.lang.AssertionError: Expected ErrorPage but was Sign in to test (https://localhost:8543/auth/realms/test/login-actions/reset-credentials?client_id=test-app&tab_id=Z35EXfACAfY)
	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)
...

org.openqa.selenium.TimeoutException: 
Navigation timed out after 10000 ms
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az548-577', ip: '10.1.0.158', os.name: 'Linux', os.arch: 'amd64', os.version: '5.15.0-1040-azure', java.version: '17.0.7'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Keycloak CI - Store Model Tests

java.lang.AssertionError: 
threads didn't terminate in time: [main (TIMED_WAITING): locked on java.util.concurrent.FutureTask@705af15f waiting for 98343 ms blocked for 8576 ms
	at java.base@17.0.7/jdk.internal.misc.Unsafe.park(Native Method)
	at java.base@17.0.7/java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:252)
	at java.base@17.0.7/java.util.concurrent.FutureTask.awaitDone(FutureTask.java:444)
...

Report flaky test

… stores Closes keycloak#20974

mabartos

@pedroigor Code looks good to me, nice job! I've put only one comment below.

As the IGNORE_ARTIFACTS option is hidden, does it mean it's intended to use only internally, right? As users cannot use the property class-loader-ignore-artifacts to ignore other artifacts, right?

If that's the case, it works as expected as the user is not capable to ignore other artifacts via the property.

However, we could maybe consider providing such a capability as users could also ignore some artifacts for their deployments.

@pedroigor WDYT?

pedroigor · 2023-07-04T15:55:37Z

@mabartos Regarding making it a user-facing option, I would keep it hidden for now and only available internally. Not sure about the use cases we are going to address with it.

mabartos · 2023-07-04T15:58:23Z

Regarding making it a user-facing option, I would keep it hidden for now and only available internally. Not sure about the use cases we are going to address with it.

Agree to have it hidden for now.

mabartos

@pedroigor LGTM

vmuzikar · 2023-07-04T16:08:19Z

+        if (ignoredArtifacts != null && StringUtil.isNotBlank(ignoredArtifacts.getValue())) {
+            // ignored artifacts must be set prior to starting re-augmentation
+            System.setProperty("quarkus.class-loading.removed-artifacts", ignoredArtifacts.getValue());
+        }


Just out of curiosity, why do we need this if we already have mapping here:

keycloak/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/ClassLoaderPropertyMappers.java

Lines 25 to 32 in 563b379

public static PropertyMapper[] getMappers() {

return new PropertyMapper[] {

fromOption(ClassLoaderOptions.IGNORE_ARTIFACTS)

.to("quarkus.class-loading.removed-artifacts")

.transformer(ClassLoaderPropertyMappers::resolveIgnoredArtifacts)

.build()

};

}

Can't we remove either of them?

@vmuzikar To leverage the related fix from Quarkus, we need to make sure the build class loader is built taking this property into account so that the artifacts are ignored accordingly.

By keeping only within our mapping layer, it is too late because the build class loader was already built.

We could potentially keep everything within the Build command but I did not test it. Also, I think keeping the resolution within our configuration is more natural and aligned with what we are doing for others.

pedroigor requested a review from a team June 13, 2023 15:16

pedroigor requested review from a team as code owners June 13, 2023 15:16

ghost added team/cloud-native team/store labels Jun 13, 2023

pedroigor force-pushed the issue-20974 branch from d0fd84a to 644e079 Compare June 13, 2023 15:19

pedroigor changed the title ~~Avoid loading classes and resources from jpa store if current store is enabled~~ Avoid loading classes and resources from jpa store if legacy store is enabled Jun 13, 2023

ghost added the flaky-test label Jun 13, 2023

ghost mentioned this pull request Jun 13, 2023

Flaky test: org.keycloak.testsuite.model.session.SessionTimeoutsTest#testOfflineUserClientIdleTimeoutSmallerThanSessionNoRefresh #20874

Closed

mabartos self-requested a review June 14, 2023 13:57

pedroigor marked this pull request as draft June 14, 2023 15:38

pedroigor force-pushed the issue-20974 branch from 644e079 to 5e3a9cc Compare June 15, 2023 11:58

ghost mentioned this pull request Jun 15, 2023

Flaky test: org.keycloak.testsuite.admin.authentication.FlowTest#testAddRemoveFlow #20763

Closed

pedroigor force-pushed the issue-20974 branch from 5e3a9cc to a320c0a Compare June 15, 2023 13:53

hmlnarik reviewed Jun 16, 2023

View reviewed changes

pedroigor force-pushed the issue-20974 branch from a320c0a to bd7916e Compare June 16, 2023 15:51

ghost reviewed Jun 16, 2023

View reviewed changes

pedroigor added the status/hold PR should not be merged. On hold for later. label Jun 19, 2023

pedroigor force-pushed the issue-20974 branch from bd7916e to 06958b8 Compare July 3, 2023 14:27

ghost reviewed Jul 3, 2023

View reviewed changes

pedroigor force-pushed the issue-20974 branch from 06958b8 to 06b304c Compare July 3, 2023 20:04

pedroigor marked this pull request as ready for review July 3, 2023 22:41

Ignoring artifacts when running re-aug to isolate the current and new…

563b379

… stores Closes keycloak#20974

pedroigor force-pushed the issue-20974 branch from 06b304c to 563b379 Compare July 3, 2023 22:52

pedroigor changed the title ~~Avoid loading classes and resources from jpa store if legacy store is enabled~~ Support ignoring artigacts when running re-augmentation Jul 3, 2023

vmuzikar requested a review from hmlnarik July 4, 2023 08:55

mabartos reviewed Jul 4, 2023

View reviewed changes

Comment thread ...main/java/org/keycloak/quarkus/runtime/configuration/mappers/ClassLoaderPropertyMappers.java

mabartos approved these changes Jul 4, 2023

View reviewed changes

mabartos mentioned this pull request Jul 4, 2023

Handle ignored artifacts separately #21440

Merged

vmuzikar approved these changes Jul 4, 2023

View reviewed changes

pedroigor merged commit bde57ca into keycloak:main Jul 5, 2023

pedroigor deleted the issue-20974 branch July 5, 2023 10:56

pedroigor mentioned this pull request Jul 12, 2023

Version 22.0.0 not started in dev mode and build mode #21625

Closed

2 tasks

This was referenced Jul 23, 2023

Unable setup keycloak in windows from source code. #21716

Closed

Profile activation for WF app server doesn't properly work for Windows #21284

Closed

mabartos mentioned this pull request Aug 14, 2023

Handle ignored artifacts separately #22430

Closed

	public static PropertyMapper[] getMappers() {
	return new PropertyMapper[] {
	fromOption(ClassLoaderOptions.IGNORE_ARTIFACTS)
	.to("quarkus.class-loading.removed-artifacts")
	.transformer(ClassLoaderPropertyMappers::resolveIgnoredArtifacts)
	.build()
	};
	}

Uh oh!

Conversation

pedroigor commented Jun 13, 2023

Uh oh!

mabartos commented Jun 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pedroigor commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 16, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.LinkedAccountsTest#linkAccountTest

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jun 19, 2023

Uh oh!

vmuzikar commented Jun 29, 2023

Uh oh!

aloubyansky commented Jun 29, 2023

Uh oh!

hmlnarik commented Jun 29, 2023

Uh oh!

vmuzikar commented Jun 30, 2023

Uh oh!

aloubyansky commented Jun 30, 2023

Uh oh!

vmuzikar commented Jun 30, 2023

Uh oh!

vmuzikar commented Jul 3, 2023

Uh oh!

ghost commented Jul 3, 2023

Unreported flaky test detected

org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordWrongSmtp

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 3, 2023

Unreported flaky test detected

org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordWrongSmtp

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Uh oh!

ghost 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 Jul 3, 2023

Unreported flaky test detected

org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordWrongSmtp

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pedroigor commented Jul 4, 2023

Uh oh!

mabartos commented Jul 4, 2023

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

pedroigor Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

pedroigor commented Jun 16, 2023 •

edited

Loading