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

Upgrade keycloak to 18.0.0 #18441

Merged
merged 1 commit into from
May 23, 2022
Merged

Upgrade keycloak to 18.0.0 #18441

merged 1 commit into from
May 23, 2022

Conversation

yelhouti
Copy link
Contributor


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@yelhouti yelhouti mentioned this pull request Apr 22, 2022
6 tasks
@yelhouti
Copy link
Contributor Author

yelhouti commented Apr 22, 2022

fixes: #17850

@yelhouti
Copy link
Contributor Author

@DanielFran @vishal423
can anyone give access to the cypress dashboard so I can see why some tests are sporadic. Ouath2/keycloak seems to work only half the time :s

@vishal423
Copy link
Contributor

@yelhouti, I suggest trying failed workflows on local env to reproduce issues

@yelhouti
Copy link
Contributor Author

@vishal423 I could not reproduce locally, also they fail sporadically in the CI. This is why some ouath2 test pass and other fails. Is there a way to access cypress dashboard ?

@yelhouti
Copy link
Contributor Author

yelhouti commented Apr 24, 2022

are you saying there is a recent change in docker ports mapping behaviour? If not, then, your configuration is incorrect. The previous configuration used to work and was thoroughly tested in local and CI (with docker compose).

@vishal423 no what I am saying is the way keycloak starts changed (no way to have a port offset of 1000) https://github.com/jhipster/generator-jhipster/pull/18441/files#diff-1bfaf6e31fe218cd507027450c04913b93f510df3f63b4a2ea84163c6423e9fbL339

But we still want to offset ports to have docker and keycloak not fight for port 8080 on localhost so we map keycloak:8080 to localhost:9080

https://github.com/jhipster/generator-jhipster/pull/18441/files#diff-8eef712ac6e760fcb9b7b54a8cb6d5579608b146d14323efa75a30bb1397f82bR107

Also please give others the benefit of the doubt :)

@vishal423
Copy link
Contributor

are you saying there is a recent change in docker ports mapping behaviour? If not, then, your configuration is incorrect. The previous configuration used to work and was thoroughly tested in local and CI (with docker compose).

@vishal423 no what I am saying is the way keycloak starts changed (no way to have a port offset of 1000) https://github.com/jhipster/generator-jhipster/pull/18441/files#diff-1bfaf6e31fe218cd507027450c04913b93f510df3f63b4a2ea84163c6423e9fbL339

But we still want to offset ports to have docker and keycloak not fight for port 8080 on localhost so we map keycloak:8080 to localhost:9080

https://github.com/jhipster/generator-jhipster/pull/18441/files#diff-8eef712ac6e760fcb9b7b54a8cb6d5579608b146d14323efa75a30bb1397f82bR107

Also please give others the benefit of the doubt :)

Got it. You are trying to access within the docker network. Failure tests seems related to micro-frontend and micro-services. Can you try those flows locally?

@yelhouti
Copy link
Contributor Author

I tried ms-angular locally which one would you recommend.

Also --monolith and --worksoace doesn't seem to work correctly maybe I am doing something wrong

I use scripts 11 and 12 the same way they are used on the workflow

@mraible
Copy link
Contributor

mraible commented May 15, 2022

Can your please fix conflicts @yelhouti?

@yelhouti
Copy link
Contributor Author

yelhouti commented May 17, 2022

I spent the last few hours understanding the issue with my PR and it is as follows:

Since keycloak doesn't support port offset anymore we are "forced" to change the port mapping at the docker level.
The issue then becomes as follows:
Since our code uses the same issuer-uri to:

  1. talk directly to the IDP (verify tokens...)
  2. redirect the user for login
    If we set it to:
  3. keycloak:8080 the browser is unable to talk to it because it can't resolve keycloak correctly (and pointing to localhost as I suspect we do now doesn't solve the problem) as mentioned in docker-compose.yaml: # For Keycloak to work, you need to add '127.0.0.1 keycloak' to your hosts file (which is cheating and I don't know how github actions was configured to do that) EDIT: which is done in 03-system.sh
  4. localhost:9080 the gateway is not able to talk to localhost correctly because it is inside the container.

Possible solution:
The gateway should know 2 urls: 1 for internal use (to verify token, retrieve oidc config in Security Configuration...) and one to return to the frontend or redirect the frontend to.
There are other solutions I could think of but they are much more complicated, change the used authentication flow, or don't work in some edge cases...

What the you all think we should do about this ? @mraible @vishal423

echo "127.0.0.1 keycloak" | sudo tee -a /etc/hosts

@mraible
Copy link
Contributor

mraible commented May 17, 2022 via email

@yelhouti
Copy link
Contributor Author

@mraible I don't think they should, the only reason our code was working is because we where using a "hack". I think its our code that needs fixing... Maybe there is a way to retrieve the IP of the keycloak container and make keycloak resolve to that instead of 127.0.0.1 which would be a correct way of doing this.

@yelhouti
Copy link
Contributor Author

@mraible 03-system.sh is the one pointing keycloak to 127.0.0.1 we could fix the ip of keycloak an change 03-system.sh. Or we could run a similar script once keycloak has started and the ip is read using docker container inspect | grep ...

@mraible
Copy link
Contributor

mraible commented May 17, 2022

I agree the current requirement to edit your hosts file isn't ideal. However, it's only required when running everything with Docker or in CI. I've yet to figure out a better solution but would love to find one!

@yelhouti
Copy link
Contributor Author

yelhouti commented May 17, 2022

As I mentioned, this can be achieved by specifying a different URL for server to server communication than the one used to redirect the user to... But for now what d you think I should do ?

  • Add a network to app.yml and set a fixed ip to keycloak (and use it in 03-system.sh)
    Or
  • retrieve it and set in /etc/hosts to just before running the e2e tests ?

@mshima
Copy link
Member

mshima commented May 17, 2022

Making the keycloak host dynamic will be a pain for development.

Add --http-port 9080 to entrypoint.
Tried locally seem to work:

17 14:25:33,330 INFO  [io.quarkus] (main) Keycloak 18.0.0 on JVM (powered by Quarkus 2.7.5.Final) started in 13.276s. Listening on: http://0.0.0.0:9080

@yelhouti
Copy link
Contributor Author

@mshima this is a hack, I would rather configure a network in app.yaml and set the IP in the compose file to a fixed one, and tell people to add it to /etc*hosts.
Also this is not needed for development since in development the app is not started using docker-compose so using localhost:9080 works perfectly.

@yelhouti
Copy link
Contributor Author

@mshima I understand what you mean, although the approach of changing ports is wrong, I think its easier to do for most people, so sadly I think I will switch back to it.
@mraible I would gladly discuss what I think is going wrong, but since I am reverting to switching ports maybe you don't want to waster your time with this for now. Feel free to ask me anything if I am wrong.

@yelhouti yelhouti force-pushed the keycloak-18 branch 3 times, most recently from e2835fb to 47d9bbd Compare May 21, 2022 11:53
@yelhouti
Copy link
Contributor Author

@mshima @mraible here you go :)

generators/cleanup.js Outdated Show resolved Hide resolved
generators/cleanup.js Outdated Show resolved Hide resolved
@mraible
Copy link
Contributor

mraible commented May 21, 2022

I tested this today on my M1 and everything worked great. I have the following in my /etc/hosts.

127.0.0.1 keycloak

I tested with jhipster jdl reactive-ms and proved everything worked when starting Docker containers and each app using ./gradlew. Then, I tested by running docker compose up in the docker-compose directory and running npm run e2e. All tests pass too.

@yelhouti
Copy link
Contributor Author

@mraible thanks a lot for you time :)

@yelhouti
Copy link
Contributor Author

@mshima I made the changes you requested for, could you check them please :)

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Cleanup is fixed.

@mshima mshima merged commit 82cefd1 into jhipster:main May 23, 2022
@mshima
Copy link
Member

mshima commented May 23, 2022

Thanks @yelhouti
The port change provides the best developer experience for now.

@DanielFran
Copy link
Member

Many thanks @yelhouti for the huge work.
Can you review the PR in other projects and the documentation, please?

@yelhouti
Copy link
Contributor Author

@DanielFran of course I will in few, but they can safely be closed

@yelhouti
Copy link
Contributor Author

@mshima indeed thank you for your feedback :) goes to prove that correct doesn't always mean better

@yelhouti
Copy link
Contributor Author

yelhouti commented May 23, 2022

claiming bounty :) https://opencollective.com/generator-jhipster/expenses/78206 (if no one minds)

@pascalgrimaud
Copy link
Member

can #17850 be closed ?
so the bounty can be claimed ?

@DanielFran DanielFran mentioned this pull request May 23, 2022
1 task
@pascalgrimaud
Copy link
Member

@DanielFran : thanks
@yelhouti : approved

@yelhouti
Copy link
Contributor Author

Thank you all :)

@DanielFran DanielFran added this to the 7.9.0 milestone Jun 22, 2022
@DanielFran DanielFran changed the title chore: upgrade keycloak to 18.0.0 Upgrade keycloak to 18.0.0 Jul 31, 2022
@mraible
Copy link
Contributor

mraible commented Oct 11, 2022 via email

@yelhouti
Copy link
Contributor Author

@mraible what is the exact change you are proposing please ? doesn't docker-compose.yml already use 9080 ?

@mraible
Copy link
Contributor

mraible commented Oct 11, 2022

@yelhouti No changes need to be made as far as I know. When I made the comment above, it was using a different port.

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.

7 participants