Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Keycloak Admin Client with TLS registry #43303

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik force-pushed the feature/kc-admin-client-tls-registry-integration branch 2 times, most recently from 1c0ca5f to e5d8e34 Compare September 15, 2024 14:01

This comment has been minimized.

Copy link

github-actions bot commented Sep 15, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/kc-admin-client-tls-registry-integration branch from e5d8e34 to 5b5a4d1 Compare September 15, 2024 14:46
@sberyozkin sberyozkin self-requested a review September 15, 2024 14:49
@michalvavrik
Copy link
Member Author

Thanks @sberyozkin; cc @geoand in case you wanna check, I configure REST client with TLS registry just as you did in #41153 so I expect it's alright

This comment has been minimized.

@michalvavrik
Copy link
Member Author

michalvavrik commented Sep 15, 2024

sorry @sberyozkin , I didn't know there are native tests so I didn't run them. it seems that Keycloak's org.keycloak.admin.client.ClientBuilderWrapper#create must be initialized it during static init as it uses static initializataion block, so I'll stop using it and request your review again.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/kc-admin-client-tls-registry-integration branch from 5b5a4d1 to c4d9750 Compare September 15, 2024 15:21
@michalvavrik michalvavrik marked this pull request as draft September 15, 2024 15:36
@michalvavrik michalvavrik marked this pull request as ready for review September 15, 2024 15:51
@michalvavrik michalvavrik force-pushed the feature/kc-admin-client-tls-registry-integration branch from c4d9750 to c973daf Compare September 15, 2024 15:51
@michalvavrik michalvavrik force-pushed the feature/kc-admin-client-tls-registry-integration branch from c973daf to 4115e48 Compare September 15, 2024 16:10
Copy link

quarkus-bot bot commented Sep 15, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4115e48.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

Copy link

quarkus-bot bot commented Sep 15, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 4115e48.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@michalvavrik
Copy link
Member Author

michalvavrik commented Sep 15, 2024

Changes inside Keycloak Admin Resteasy Client are due to the fact that I changed time when we set Keycloak Rest client provider (from static to runtime). This meant that during static init when native image is build this was executed: https://github.com/keycloak/keycloak/blob/75973157aa10bdbcb04d82af490a495dbb0b35a7/integration/admin-client/src/main/java/org/keycloak/admin/client/Keycloak.java#L46
Which meant that static initialization block here was executed https://github.com/keycloak/keycloak/blob/75973157aa10bdbcb04d82af490a495dbb0b35a7/integration/admin-client/src/main/java/org/keycloak/admin/client/ClientBuilderWrapper.java#L11 which failed (I didn't spend time looking why, it's not important as we don't need that class at all). So I just made sure that we don't call https://github.com/keycloak/keycloak/blob/75973157aa10bdbcb04d82af490a495dbb0b35a7/integration/admin-client/src/main/java/org/keycloak/admin/client/Keycloak.java#L48 considering we don't use it anyway.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @michalvavrik

@sberyozkin sberyozkin merged commit 47daf46 into quarkusio:main Sep 16, 2024
23 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Sep 16, 2024
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 16, 2024
@michalvavrik michalvavrik deleted the feature/kc-admin-client-tls-registry-integration branch September 16, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance keycloak-admin-client extension to support TLS trust and key stores
4 participants