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

Added keycloak-server test framework #16035

Merged

Conversation

cemnura
Copy link
Contributor

@cemnura cemnura commented Mar 26, 2021

Fixes #13957

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/oidc area/testing labels Mar 26, 2021
@sberyozkin
Copy link
Member

Hi @cemnura Great work :-)
But I don't think test-containers are needed here just yet - may be in the next iteration where we would have to remove the profiles - but for now this test support code should not be dealing with starting Keycloak before it is tuned (I'll doc how to use a profile to get it started and users will just copy and paste it.); this specific test also uses HTTPS - so please retain it for now - users would be able to set http location if the need it.
Thanks

@cemnura
Copy link
Contributor Author

cemnura commented Mar 26, 2021

Hi @sberyozkin thanks for your review :)

I added test-containers as the original integration-tests/oidc module uses testcontainers via a maven plugin. Thats why I thought I had to enable keycloak to run via testcontainers.

Is there any reference I can look up to run KC instance? I had difficulties with https I will try to add it back in.

@sberyozkin
Copy link
Member

@cemnura Having said that, may be it is the right time to switch to test-containers - for integration-tests/oidc to start with (-Dstart-containers is activating the docker-keycloak - so we should drop this profile from integration-tests/oidc/pom.xml if we will keep test-containers)
@gsmet Hi Guillaume - is there any restriction in place on using test-containers as part of the CI build ?

@sberyozkin
Copy link
Member

@cemnura Do you mean this one, https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc/pom.xml#L124 ? That is a property set by CI :-), while start-containers will start a docker using fabric plugin:

@sberyozkin
Copy link
Member

@cemnura for the users it would likely be simpler to use the test-containers, we just need to get a clearance on using it in CI :-)

@cemnura
Copy link
Contributor Author

cemnura commented Mar 26, 2021

@cemnura Do you mean this one, https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc/pom.xml#L124 ? That is a property set by CI :-), while start-containers will start a docker using fabric plugin:

Yes sorry I meant that module runs KC docker container with fabric 8 😄

@sberyozkin
Copy link
Member

@cemnura Re getting test-containers getting KC support HTTPS, please see
https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-keycloak-authorization-quickstart/src/test/java/org/acme/security/keycloak/authorization/KeycloakServer.java#L16.
Please also update the code to use the configured version

@cemnura
Copy link
Contributor Author

cemnura commented Mar 26, 2021

@cemnura for the users it would likely be simpler to use the test-containers, we just need to get a clearance on using it in CI :-)

Using it in CI might be difficult I just thought I was going to run a full KC instance programmatically. If there is a way to launch a KC instance without testcontainers I would gladly give it a try 😄

@sberyozkin
Copy link
Member

@cemnura sorry, may be I'm missing something. integration-tests/oidc/pom.xml has a profile which launches KC - so we should either keep it - but then do not use test-containers to launch KC - or we should remove it if we keep your current code which uses the test-containers to launch it. For this test alone I'd go with your option as indeed it would also be fine with CI - as long as @gsmet and also @famod confirm it is ok to use them

@sberyozkin
Copy link
Member

@cemnura It is grand, test-containers are already used for ex by Vault. so lets use for integration-tests/oidc

@sberyozkin
Copy link
Member

@cemnura please use keycloak.docker.image property; also please add a client which can be used for the code flow tests, I'll find some code to reuse

@sberyozkin
Copy link
Member

sberyozkin commented Mar 26, 2021

@cemnura OK, so you have:

RealmRepresentation realm = createRealm("quarkus"); // via KEYCLOAK_REALM
realm.getClients().add(createClient("quarkus-app"));
realm.getUsers().add(createUser("alice", "user"));
realm.getUsers().add(createUser("admin", "user", "admin"));
realm.getUsers().add(createUser("jdoe", "user", "confidential"));

First I propose to rename the realm name from quarkus to quarkus-service-realm and client id from quarkus-app to quarkus-service-app (and update the application.properties, etc) (and collapse the whole quarkus-service-realm setup in a single function similarly to the next one - or if you prefer not to - fine as well)

and also create quarkus-webapp-realm

and copy this code:

https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/KeycloakRealmResourceManager.java#L75
(except https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/KeycloakRealmResourceManager.java#L96)

It is nearly the same as for the service realm but there is an extra sso property and the client creation code also has a redirect uri property. Probably makes sense to have 1 realm/clients/users for testing the service apps and one for - testing web-app applications.
But no need to add a CodeFlow test for now, I may do it later

@cemnura cemnura force-pushed the feature/quarkus-test-keycloak-server branch from c4d7715 to 6df24e3 Compare March 28, 2021 22:21
@cemnura
Copy link
Contributor Author

cemnura commented Mar 28, 2021

Hey @sberyozkin

I configured the first realm and added the webapp realm as you said (and collapsed them to seperate createRealm methods (I also thought it would be cleaner 😄).

I removed -Pdocker-keycloak since it would not be used anymore as we talked.

I only had a problem to load the keycloak.docker.image property during building.

https://github.com/cemnura/quarkus/blob/6df24e3555e1ff07bc79d1b8fe46b13c3840db53/test-framework/keycloak-server/src/main/java/io/quarkus/test/keycloak/server/KeycloakTestResourceLifecycleManager.java#L30

We need to be able to load the keycloak.docker.image maven property to the class. Could I use a application.properties similar property file and load the property in the property such as;

#application.properties
keycloak.docker.image=${keycloak.docker.image}

Perhaps there is a better way to load configurations for QuarkusTestResourceLifecycleManager we could use?

@sberyozkin
Copy link
Member

sberyozkin commented Mar 29, 2021

@cemnura Hi, thanks, it is nearly ready to go then :-) (sorry mistyped the smiley the prev time)
I'm not sure what is the problem with keycloak.docker.image, may be it is to do with the fact it is set in build-parent/pom.xml and it is not visible to the test code; so please try then:

quay.io/keycloak/keycloak:" + ${keycloak.version} as it is set in bom/application/pom.xml.
If needed the test project's pom can set a keycloak.version property
thanks

@cemnura
Copy link
Contributor Author

cemnura commented Mar 30, 2021

Hello @sberyozkin I believe the problem is more to access the maven property in the java code rather then the property build-parent/pom.xml being invisible to the test code.

I am not sure how to use maven properties it in the java code hence bringing me to the following search 😄

https://www.google.com/search?q=maven+use+properties+in+code

I tried to load the property as a system property via <systemPropertyVariables> and load it using System.getProperty("keycloak.docker.image", "quay.io/keycloak/keycloak:12.0.4") but apparently that is maven-surefire-plugin feature.

@sberyozkin
Copy link
Member

@cemnura Yes, the users would be asked to do something like this. IMHO keycloak.version is a simpler property (and I'm not sure keycloak.docker.image would be visible in the custom application poms)

@cemnura cemnura force-pushed the feature/quarkus-test-keycloak-server branch from 6df24e3 to 5714317 Compare March 31, 2021 01:03
@cemnura
Copy link
Contributor Author

cemnura commented Mar 31, 2021

Hello @sberyozkin

I successfully tested out whether we can access keycloak.docker.image and load it as a system variable.

Example:

<systemPropertyVariables>
    <keycloak.docker.image>${keycloak.docker.image}</keycloak.docker.image>
    <keycloak.url>${keycloak.ssl.url}</keycloak.url>
</systemPropertyVariables>

the keycloak.docker.image system property is loaded here


I agree that keycloak.version would be a simpler property. However, since test container parameter is a full docker image name we would lose the ability for the user use a different keycloak docker image (highly unlikely).

Such as;

<systemPropertyVariables>
    <keycloak.docker.image>jboss/keycloak:12.0.4</keycloak.docker.image>
    <keycloak.url>${keycloak.ssl.url}</keycloak.url>
</systemPropertyVariables>

That being said I would gladly implement keycloak.version if you see fit 😄

The balls in your court 😄

Cheers

@sberyozkin
Copy link
Member

@cemnura Makes sense, we can support keycloak.url as well if the docker image property is not set as a fallback - but it is good to go IMHO, I'll follow up a bit later on with some doc updates and might use for the code flow test as well, nice work, thanks
I'll ping you to suggest some more issues to look at :-)

@sberyozkin sberyozkin self-requested a review March 31, 2021 10:44
@sberyozkin sberyozkin merged commit 9d36ef7 into quarkusio:main Mar 31, 2021
@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Mar 31, 2021
@cemnura
Copy link
Contributor Author

cemnura commented Mar 31, 2021

🎉 @sberyozkin great thanks looking forward 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/oidc area/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add KeycloakTestResourceLifecycleManager
2 participants