Fix Keycloak devservice base URL

[Eng-Fouad]

• Fixes Keycloak devservice uses wrong port when https is enabled with fixed port #49842

[Eng-Fouad]

Alternative implementation would be to introduce a new config for fixed-https-port, then use it in the base URL.

[holly-cummins]

It would be ideal to have a test for this, especially since #49542 is coming soon, but fix looks sensible.

[sberyozkin]

@Eng-Fouad Thanks for the PR. I'm not sure setting the http: scheme is right, it should be https: if https is expected.
This indeed works as expected if no fixed port is used, but with the fixed port it is getting a bit confusing, several tests expect https. You checked that it is

12345:8080
randomPort:8443

when the fixed port is used and https is expected. Should we make it work such that in this case it is only 12345:8443 ?

[Eng-Fouad]

when the fixed port is used and https is expected. Should we make it work such that in this case it is only 12345:8443 ?

Yes, I think that would work. If so, then http port would be random?

[sberyozkin]

when the fixed port is used and https is expected. Should we make it work such that in this case it is only 12345:8443 ?

Yes, I think that would work. If so, then http port would be random?

Hmm, I don't recall why we even had to have http port when https is expected. For Quarkus itself, we support http: when https: is used, but I'm not sure if that is really needed for Keycloak. Oh, we probably did that to make Dev UI simpler...

Are you also using a shared network ? Can you please give me a favor and check how many ports are exposed, what is the mapping, when no fixed port is used ?

I agree with Holly the fix looks sensible. But unfortunately the tests fail.

[Eng-Fouad]

Are you also using a shared network ?

I am not setting quarkus.keycloak.devservices.shared.

Can you please give me a favor and check how many ports are exposed, what is the mapping, when no fixed port is used ?

Got random mapping for both ports:

60634:8080
60635:8443⁠

[sberyozkin]

Thanks @Eng-Fouad, so I guess we can try to retain the randomness for 8080, the bug is about the fixed port used for the wrong mapping in your case, it should be used for 8443, please update the PR and we'll see if the tests like it, thanks

[sberyozkin]

@Eng-Fouad Maybe, we'd also have to do what you proposed about the fixed port https configuration, but for now, just using the fixed port for https should do

[Eng-Fouad]

Thanks @Eng-Fouad, so I guess we can try to retain the randomness for 8080, the bug is about the fixed port used for the wrong mapping in your case, it should be used for 8443, please update the PR and we'll see if the tests like it, thanks

Original:

quarkus/extensions/devservices/keycloak/src/main/java/io/quarkus/devservices/keycloak/KeycloakDevServicesProcessor.java

Lines 552 to 588 in 97cebb8

	if (fixedExposedPort.isPresent()) {
	addFixedExposedPort(fixedExposedPort.getAsInt(), KEYCLOAK_PORT);
	if (useSharedNetwork) {
	// expose random port for which we are able to ask Testcontainers for the actual mapped port at runtime
	// as from the host's perspective Testcontainers actually expose container ports on random host port
	addExposedPort(KEYCLOAK_PORT);
	}
	} else {
	addExposedPort(KEYCLOAK_PORT);
	}
	if (sharedContainer && LaunchMode.current() == LaunchMode.DEVELOPMENT) {
	withLabel(DEV_SERVICE_LABEL, containerLabelValue);
	withLabel(QUARKUS_DEV_SERVICE, containerLabelValue);
	}
	if (javaOpts.isPresent()) {
	addEnv(JAVA_OPTS, javaOpts.get());
	}
	if (keycloakX) {
	addEnv(KEYCLOAK_QUARKUS_ADMIN_PROP, KEYCLOAK_ADMIN_USER);
	addEnv(KEYCLOAK_QUARKUS_ADMIN_PASSWORD_PROP, KEYCLOAK_ADMIN_PASSWORD);
	String finalStartCommand = startCommand.orElse(KEYCLOAK_QUARKUS_START_CMD);
	if (features.isPresent()) {
	finalStartCommand += (" --features=" + features.get().stream().collect(Collectors.joining(",")));
	}
	withCommand(finalStartCommand);
	addUpConfigResource();
	if (isHttps()) {
	addExposedPort(KEYCLOAK_HTTPS_PORT);
	}
	} else {
	addEnv(KEYCLOAK_WILDFLY_USER_PROP, KEYCLOAK_ADMIN_USER);
	addEnv(KEYCLOAK_WILDFLY_PASSWORD_PROP, KEYCLOAK_ADMIN_PASSWORD);
	addEnv(KEYCLOAK_WILDFLY_VENDOR_PROP, KEYCLOAK_WILDFLY_DB_VENDOR);
	}

Patch:

            if (fixedExposedPort.isPresent()) {
                if (isHttps()) {
                    addFixedExposedPort(fixedExposedPort.getAsInt(), KEYCLOAK_HTTPS_PORT);
                    addExposedPort(KEYCLOAK_PORT);
                } else {
                    addFixedExposedPort(fixedExposedPort.getAsInt(), KEYCLOAK_PORT);
                }
                if (useSharedNetwork) {
                    // expose random port for which we are able to ask Testcontainers for the actual mapped port at runtime
                    // as from the host's perspective Testcontainers actually expose container ports on random host port
                    addExposedPort(KEYCLOAK_PORT);
                    if (isHttps()) {
                        addExposedPort(KEYCLOAK_HTTPS_PORT);
                    }
                }
            } else {
                addExposedPort(KEYCLOAK_PORT);
                if (isHttps()) {
                    addExposedPort(KEYCLOAK_HTTPS_PORT);
                }
            }

            if (sharedContainer && LaunchMode.current() == LaunchMode.DEVELOPMENT) {
                withLabel(DEV_SERVICE_LABEL, containerLabelValue);
                withLabel(QUARKUS_DEV_SERVICE, containerLabelValue);
            }

            if (javaOpts.isPresent()) {
                addEnv(JAVA_OPTS, javaOpts.get());
            }

            if (keycloakX) {
                addEnv(KEYCLOAK_QUARKUS_ADMIN_PROP, KEYCLOAK_ADMIN_USER);
                addEnv(KEYCLOAK_QUARKUS_ADMIN_PASSWORD_PROP, KEYCLOAK_ADMIN_PASSWORD);
                String finalStartCommand = startCommand.orElse(KEYCLOAK_QUARKUS_START_CMD);
                if (features.isPresent()) {
                    finalStartCommand += (" --features=" + features.get().stream().collect(Collectors.joining(",")));
                }
                withCommand(finalStartCommand);
                addUpConfigResource();
            } else {
                addEnv(KEYCLOAK_WILDFLY_USER_PROP, KEYCLOAK_ADMIN_USER);
                addEnv(KEYCLOAK_WILDFLY_PASSWORD_PROP, KEYCLOAK_ADMIN_PASSWORD);
                addEnv(KEYCLOAK_WILDFLY_VENDOR_PROP, KEYCLOAK_WILDFLY_DB_VENDOR);
            }

@sberyozkin Could you please check if it is a valid implementation? Thanks.

[sberyozkin]

Thanks @Eng-Fouad, it does look fine, easier to understand how the HTTPS port allocation is done with or without the fixed port, and will be also easier to update it further should we see some demand for managing fixed port for both http and https.

Please update the PR and let's see what happens

[sberyozkin]

@Eng-Fouad Unfortunately it does not work either or may be some more updates are needed...

[sberyozkin]

@Eng-Fouad But it does look better compared to the previous build, only keycloak admin tests are failing

[Eng-Fouad]

So, I discovered something interesting.

In this line:

quarkus.keycloak.devservices.start-command=start --https-client-auth=required --hostname-strict=false --https-key-store-file=/etc/server-keystore.p12 --https-key-store-password=secret --truststore-paths=/etc/server-ca.crt --https-port=8080

--https-port=8080 will force Keycloak to run inside the container on port 8080 using https (no http is listening), and the exposed ports are:

• 8083:8080
• randomPort:8443

and the Keycloak base URL would be valid and accessible: https://localhost:8083/realms/quarkus, but becomes invalid and inaccessible: http://localhost:8083/realms/quarkus if there is already running Keycloak container for reuse? 🤨

However, when we remove -https-port=8080, Keycloak runs inside the container on port 8443 using https (no http is listening), and the exposed ports are:

• 8083:8080
• randomPort:8443

and the Keycloak base URL would be invalid and inaccessible in both cases: https://localhost:8083/realms/quarkus and http://localhost:8083/realms/quarkus.

---

CI fails on this PR because on those tests Keycloak running https on 8080 inside the container, and the exposed ports are:

• randomPort:8080
• 8083:8443

and the Keycloak base URL would be invalid and inaccessible: https://localhost:8083/realms/quarkus

If we remove --https-port=8080 on those tests, I think it would pass CI. However, users won't be able to specify --https-port in their configs.

[Eng-Fouad]

The --https-port=8080 part is a workaround which makes these tests usually pass in Quarkus CI:

quarkus/extensions/keycloak-admin-rest-client/deployment/src/test/resources/app-mtls-config.properties

Line 4 in a747f3d

quarkus.keycloak.devservices.start-command=start --https-client-auth=required --hostname-strict=false --https-key-store-file=/etc/server-keystore.p12 --https-key-store-password=secret --truststore-paths=/etc/server-ca.crt --https-port=8080

quarkus/extensions/keycloak-admin-resteasy-client/deployment/src/test/resources/app-mtls-config.properties

Line 4 in a747f3d

quarkus.keycloak.devservices.start-command=start --https-client-auth=required --hostname-strict=false --https-key-store-file=/etc/server-keystore.p12 --https-key-store-password=secret --truststore-paths=/etc/server-ca.crt --https-port=8080

cc @michalvavrik

[sberyozkin]

Interesting. So, with the current PR, when HTTPS is expected and the fixed port is mapped to the HTTPS port (as I proposed), the fixed port configuration conflicts with the Keycloak --https-port configuration, so I'm little bit worried my idea can cause side-effects...

I guess we need a bit more time to figure out the best option, the current one seems not bad, but we'll probably break some existing tests...

Now that we have it all working (thanks a million for all the analysis), what about the alternative option where you proposed adding https specific fixed port, say quarkus.keycloak.devservices.https-port=8083 ?

I.e, the fixed port, if set with quarkus.keycloak.devservices.port=8083, remains there to support HTTP only, it does not work for HTTPS anyway right now. If users want a fixed port for HTTPS, they'd use quarkus.keycloak.devservices.https-port=8083. This way there are no conflicts/unexpected side-effects of any kind.

What do you think, @Eng-Fouad, @holly-cummins ? And also @michalvavrik

[quarkus-bot]

Status for workflow Quarkus CI

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

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

[sberyozkin]

May be, the current approach is right after all, ultimately, it is the startup command that dictates the choice of either HTTP or HTTPS, with a possible new quarkus.keycloak.devservices.https-port=8083 it is unclear what the base URL is when quarkus.keycloak.devservices.port=8080 is also set

[holly-cummins]

Interesting. So, with the current PR, when HTTPS is expected and the fixed port is mapped to the HTTPS port (as I proposed), the fixed port configuration conflicts with the Keycloak --https-port configuration, so I'm little bit worried my idea can cause side-effects...

I guess we need a bit more time to figure out the best option, the current one seems not bad, but we'll probably break some existing tests...

Now that we have it all working (thanks a million for all the analysis), what about the alternative option where you proposed adding https specific fixed port, say quarkus.keycloak.devservices.https-port=8083 ?

I.e, the fixed port, if set with quarkus.keycloak.devservices.port=8083, remains there to support HTTP only, it does not work for HTTPS anyway right now. If users want a fixed port for HTTPS, they'd use quarkus.keycloak.devservices.https-port=8083. This way there are no conflicts/unexpected side-effects of any kind.

What do you think, @Eng-Fouad, @holly-cummins ? And also @michalvavrik

Tricky. This morning, my instinct was that adding extra config wasn't ideal, both since it's verbose and since the 'default' behaviour would be surprising to users and violate the principle of least surprise. Papering over surprising behaviour with extra config is something we'd ideally avoid. But then I saw all the test failures and debugging over the course of today, and I wondered if maybe there isn't any non-surprising behaviour.

I'm also unsure how all this will interact with my dev service rewrite, and if that might change the current behaviour in any way. It shouldn't change much except for preventing port conflicts when multiple tests use the same fixed port. But it did happen for lambdas that my changes broke ephemeral ports, so there might be other side effects in the keycloak area, since it's apparently quite complex!

[Eng-Fouad]

I am thinking of an implementation like this:

String internalHttpPort = getPortFromStartCommandOrNullIfNotSet(config.startCommand, "--http-port");
String internalHttpsPort = getPortFromStartCommandOrNullIfNotSet(config.startCommand, "--https-port");

if (internalHttpsPort == null && config.startCommand.contains("--https")) {
    internalHttpsPort = KEYCLOAK_DEFAULT_HTTPS_PORT;
}

if (internalHttpPort == null && internalHttpsPort == null) {
    internalHttpPort = KEYCLOAK_DEFAULT_HTTP_PORT;
}

if (config.httpPort != null) {
    if (internalHttpPort == null) {
        internalHttpPort = KEYCLOAK_DEFAULT_HTTP_PORT;
        config.startCommand = addPortToStartCommand(config.startCommand, "--http-port", internalHttpPort);
    }
    addFixedExposedPort(config.httpPort, internalHttpPort);
} else if (internalHttpPort != null) {
    addExposedPort(internalHttpPort);
}

if (config.httpsPort != null) {
    if (internalHttpsPort == null) {
        internalHttpsPort = KEYCLOAK_DEFAULT_HTTPS_PORT;
        config.startCommand = addPortToStartCommand(config.startCommand, "--https-port", internalHttpsPort);
    }
    addFixedExposedPort(config.httpsPort, internalHttpsPort);
} else if (internalHttpsPort != null) {
    addExposedPort(internalHttpsPort);
}

[Eng-Fouad]

but becomes invalid and inaccessible: http://localhost:8083/realms/quarkus if there is already running Keycloak container for reuse? 🤨

Also this behavior is weird (uses https if there is no running Keycloak containers, and http if there is an existing running container), and it shouldn't happen :)

[michalvavrik]

You are fixing legit issue, thanks for that and yes, this test worked around that (and it wasn't hard to do 😀 ), but did you mention that Keycloak only exposes one port - HTTPS port? So why should be an issue to have --https-port=8080? Does it fail with it? If yes, it is a problem because it is unnecessary (KC only listens to one port).

So my question is, how often is it that users want to combine HTTPS port and HTTP port? I have vague memory that Keycloak team mentioned that they want to support that in the future, but I don't know if they already do and if so, let's wait for a feature request from someone that needs it.

My solution would be to determine port once and use it everywhere in the KeycloakDevServicesProcessor, it will be either HTTPS port or HTTP port. I think current state is too complicated, I spend last 20 minutes running this test and I can't tell you what is expected behavior....

[Eng-Fouad]

I am OK with using HTTPS only. If I remember correctly, when I used --https-port=8080, I faced a problem in integration testing as Keycloak devservice was starting with wrong KEYCLOAK_URL, something like http://keycloak-xxxxx:8080. I had to disable discovery of the OIDC endpoints (because all URLs were wrongly starting with http://) and I had to specify quarkus.oidc.jwks-path and quarkus.oidc.token-path manually.

[michalvavrik]

[OPTIONAL] It would also be interesting to drop quarkus.keycloak.admin-client.server-url=https://localhost:${quarkus.keycloak.devservices.port} as it should be inferred automatically, so that we know it works.