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

Fix the keycloak devservice when using shared network #46710

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

jedla97
Copy link
Contributor

@jedla97 jedla97 commented Mar 10, 2025

Fixes #45793

This is follow up of #46141 as there is seems still be some problems per #45793 (comment)

If you want more info I describe it in #45793 (comment)

@sberyozkin
Copy link
Member

Thanks @jedla97, I believe we need to get back the test, please look at restoring it soon 🙏

@jedla97
Copy link
Contributor Author

jedla97 commented Mar 10, 2025

@sberyozkin I have lot of things in past month, but I have plan look at it this weekend.

@amoscatelli
Copy link
Contributor

@jedla97 thank you

@jedla97 jedla97 marked this pull request as ready for review March 11, 2025 10:37
@sberyozkin
Copy link
Member

Thanks @jedla97, please don't do it on the weekend, if you can find time to do it in the next few weeks it would be great

@jedla97 jedla97 force-pushed the fix-kc-shared-network branch from fcec035 to c6ee979 Compare March 11, 2025 10:42
@sberyozkin
Copy link
Member

@jedla97 You probably need to recreate this PR, it is better to rebase, see https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#before-you-contribute

@jedla97
Copy link
Contributor Author

jedla97 commented Mar 11, 2025

@sberyozkin branch updated, the issue should be fixed, it's confirmed to work. Also we should probably add label to backport it to 3.19.

Copy link

quarkus-bot bot commented Mar 11, 2025

Status for workflow Quarkus CI

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

✅ 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.

You can consult the Develocity build scans.

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 @jedla97

@sberyozkin sberyozkin merged commit 8b51493 into quarkusio:main Mar 11, 2025
29 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Mar 11, 2025
@jedla97 jedla97 deleted the fix-kc-shared-network branch March 11, 2025 11:50
@gsmet
Copy link
Member

gsmet commented Mar 11, 2025

Folks, I'm a bit worried about backporting this as it's a bit hard to follow what this fixes exactly and how. I read the comments in the issue and I'm not sure I fully grasp why we removed the port and why we removed the newly added parameter.

Given this parameter was just added, I'm a bit worried about us breaking other things?

In any case, some clarification would be welcome.

@jedla97
Copy link
Contributor Author

jedla97 commented Mar 11, 2025

Hi @gsmet ,

I start from previous PR #46141

It removed the --hostname-port=" + fixedExposedPort.getAsInt() as the hostname-port was deprecated in KC 25 and in KC 26 it was removed (https://www.keycloak.org/docs/latest/upgrading/#new-hostname-options)

To fix it I found two option the one which was implemented in #46141 .
This set KC hostname to something like http://keycloak-hq3uo but without port so for KC to be able comunicate, the --hostname-backchannel-dynamic was need to to enable KC internal comunication and redirections (don't know exactly how as looked at it more then month ago but I was experience that the redirect to login page was not successful as it redirection somewhere else and the connection timeouted).

With this KC dev service started but was available only on http://localhost:45969/realms/quarkus but the http://keycloak-hq3uo was not available. Not have it available was probably fine as it was working on all usecases which we had.

But yesterday the problem #45793 (comment) was found. I don't know the exact setup what they have but the iss value was different from what they expect. So this PR was created to fix it.

It adding the KC port which in case of shared networks should be always default as the Quarkus app and KC is on same network. The --hostname-backchannel-dynamic was removed as it was no longer needed, as KC have all information defined in hostname. Before it was missing port so there was need to dynamicaly resolve it.

Probably issue was when ussing mp.jwt.verify.issuer property that the JWT token was resolved as invalid (but this is only my speculation base on reading few docs)

Maybe @sberyozkin have some insight to add

Also note this happen and is fixed only in docker/podman Quarkus native image (maybe even jvm image but I didn't check it)

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.

Devservices Keycloak breaks integration tests
4 participants