8000 Additional datasources now require XA by vmuzikar · Pull Request #32403 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Additional datasources now require XA - #32403

Merged
vmuzikar merged 5 commits into
keycloak:mainfrom
vmuzikar:transactions
Aug 29, 2024
Merged

Additional datasources now require XA#32403
vmuzikar merged 5 commits into
keycloak:mainfrom
vmuzikar:transactions

Conversation

@vmuzikar
@vmuzikar vmuzikar commented Aug 26, 2024
Copy link
Copy Markdown
Contributor

Closes #32402

shawkins
shawkins reviewed Aug 26, 2024
View reviewed changes
@shawkins shawkins left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a little nuanced, but we can still support the case of aurora as the default datasource with a tweak to recommendation / check.

Comment thread docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc Outdated
@shawkins shawkins Aug 26, 2024
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should also mention that you'll get a warning about recovery not being enabled, and likely mention that there isn't out-of-the box support via the operator.

@vmuzikar vmuzikar Aug 26, 2024
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think I noticed any warning.

Operator supports the .spec.transaction. xaEnabled field.

@shawkins shawkins Aug 26, 2024
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I had forgotten that we set quarkus.transaction-manager.enable-recovery=true along with the other changes to disable XA by default - so there will be no warning. In that case we just need to document that recovery isn't supported when you don't have persistent storage for the transaction store which is especially true when using the operator.

@vmuzikar vmuzikar Aug 28, 2024
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a note on the recovery.

@shawkins shawkins Aug 28, 2024
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Added a note on the recovery.

We should expand on that with a WARNING and call out containerized / operator usage specifically. If it's not clear when the full xa features are not available, then this solution is not much better than setting the workaround flag.

cc @yrodiere

@vmuzikar vmuzikar Aug 28, 2024
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shawkins I wonder if we could do that as a follow-up. This change here is to just document the breaking change we did that XA is now required. The upgrading guide is IMHO not supposed to supplement a proper docs for additional datasources which will be added as part of #29116. On a side note, AFAIK, Quarkus itself doesn't provide such warning in their docs.

@shawkins shawkins Aug 28, 2024
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we could do that as a follow-up.

Sounds good.

On a side note, AFAIK, Quarkus itself doesn't provide such warning in their docs.

I agree https://quarkus.io/guides/transaction does not spell out the containerized scenario, but it seems like it could be less obvious or assumed to be turn-key when utilized via Keycloak so we should be clear about expectations.

@vmuzikar vmuzikar Aug 29, 2024
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in #29116 to remind us later.

Comment thread quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc Resolved
@vmuzikar
vmuzikar requested a review from shawkins August 28, 2024 13:45
@vmuzikar
vmuzikar marked this pull request as ready for review August 28, 2024 13:45
vmuzikar requested review from a team as code owners August 28, 2024 13:45
@vmuzikar
vmuzikar requested a review from shawkins August 28, 2024 14:22
@keycloak-github-bot
keycloak-github-bot Bot commented Aug 28, 2024
Copy link
Copy Markdown

Unreported flaky test detected

If the 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#resetPasswordDisabledUser

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected LoginPasswordResetPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=a0ef0939-21bc-46c2-9182-ecedd3eb1e6a&scope=openid)
	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 java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordTwiceInNewBrowser

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected LoginPasswordResetPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=7b3cdfa2-5c3b-42f2-9aff-a90ab33ee908&scope=openid)
	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 java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordWrongSmtp

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected LoginPasswordResetPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=bfddbf33-76e0-4494-b58f-2edadcb2e54f&scope=openid)
	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 java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.forms.ResetPasswordTest#resetPasswordExpiredCodeAndAuthSessionPerActionLifespan

Keycloak CI - Forms IT (chrome)

java.lang.AssertionError: Expected LoginPasswordResetPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=ece6376f-2109-4ff2-84ab-ba3abbba3a3d&scope=openid)
	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 java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Aug 28, 2024
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot 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

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Aug 28, 2024
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot 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

@keycloak-github-bot
keycloak-github-bot Bot commented Aug 28, 2024
Copy link
Copy Markdown

Unreported flaky test detected

If the 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.WebAuthnPropertyTest#timeout

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=4d904561-319b-45d0-8495-8db0e4841eba&scope=openid)
	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 java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.webauthn.WebAuthnRegisterAndLoginTest#webAuthnPasswordlessAlternativeWithWebAuthnAndPassword

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected LoginUsernameOnlyPage but was AUTH_RESPONSE (https://localhost:8543/auth/realms/master/app/auth?state=3e86930b-1cdb-45ed-943a-1cfb2a628565&session_state=eb2dfead-1d41-45a8-ad3b-0b654e9f2e6c&iss=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Ftest&code=efa7c9b2-3de4-4f20-807d-f2f4ed18f38e.eb2dfead-1d41-45a8-ad3b-0b654e9f2e6c.fdd82f9f-6c3c-47c3-b4a5-72bbadfff5dd)
	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:246)
...

java.lang.AssertionError: Event expected
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertNotNull(Assert.java:713)
	at org.keycloak.testsuite.AssertEvents.poll(AssertEvents.java:87)
...

Report flaky test

vmuzikar and others added 5 commits August 28, 2024 18:00
@vmuzikar
Additional datasources now require XA
8f2b4a5 
Closes keycloak#32402

Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>
@vmuzikar @shawkins
Apply suggestions from code review
07bae3f 
Co-authored-by: Steven Hawkins <shawkins@redhat.com>
Signed-off-by: Václav Muzikář <vaclav@muzikari.cz>
@vmuzikar
Relax validation
0dea863 
Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>
@vmuzikar
Added a note on recovery
989c19d 
Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>
@vmuzikar
Fix CustomJpaEntityProviderDistTest
0397863 
Signed-off-by: Václav Muzikář <vmuzikar@redhat.com>
shawkins
shawkins approved these changes Aug 28, 2024
View reviewed changes
@shawkins shawkins left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @vmuzikar

@vmuzikar vmuzikar mentioned this pull request Aug 29, 2024
Closed
@vmuzikar
vmuzikar merged commit 7d3dcae into keycloak:main Aug 29, 2024
@vmuzikar
vmuzikar deleted the transactions branch August 29, 2024 09:16
@ahus1
ahus1 commented Aug 29, 2024
Copy link
Copy Markdown
Member

@vmuzikar - thank you for putting into this extra effort to have a well-defined error message with actionable instructions when there more than one non-XA datasource. This will make a great difference to users, so they don't need to figure out the cause and possible remedies!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abstractj abstractj abstractj approved these changes
@shawkins shawkins shawkins approved these changes
@Pepo48 Pepo48 Pepo48 approved these changes
+1 more reviewer
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Additional datasources do not work

5 participants

@vmuzikar @ahus1 @abstractj @shawkins @Pepo48
0