Dev services check property value existence with expression expansion

[ozangunalp]

Fixes #41128 replaces #41143

Thanks to @The-Funk !

[gsmet]

Makes sense.

[geoand]

👌

[The-Funk]

@ozangunalp @gsmet @geoand can this be backported to 3.8 for those of us on the LTS?

[ozangunalp]

I don't see any conflicts for 3.8 so it should be fine.

[gsmet]

I added the label but we discuss backports for 3.8 in a committee so we will see how it goes when we discuss the next round of backports.

[gsmet]

@ozangunalp the extensions/oidc-token-propagation-reactive/deployment errors look suspicious. I wouldn't be surprised if there was an issue with the Keycloak Dev Services new behavior?

[ozangunalp]

@gsmet I am checking this right now.

I'll also disable those flaky Kafka ITs and revisit them later.

[ozangunalp]

Found the problems:

• The oidc extension IT application.properties was self referencing the auth-server config
• The oidc extension tests did use wiremock to simulate the keycloak, so auth-server config needed to be present but replaced by the tests.

[quarkus-bot]

Status for workflow Quarkus CI

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

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

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

• Expecting actual: ["-6","-7","-9","-10","-11","-12","-13","-14"] to start with: ["-6", "-7", "-8", "-9"] - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual:
  ["-6","-7","-9","-10","-11","-12","-13","-14"]
to start with:
  ["-6", "-7", "-8", "-9"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:41)

---

⚙️ Gradle Tests - JDK 17 Windows

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.MultiSourceProjectDevModeTest.main - History

• Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes 30 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes  30 seconds.
	at app//org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at app//org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at app//org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at app//org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at app//org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at app//io.quarkus.test.devmode.util.DevModeClient.getHttpResponse(DevModeClient.java:164)
	at app//io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:164)

[ozangunalp]

Impacted tests are now green. There is however a behavior change that I've noticed fixing OIDC tests:

Before when a config property was supplied with an expression that is not resolved, the dev service looking at that property would not kick in, because the property value is still non-null.
Now with this change, when the expression is not resolved the dev service property check resolves to null, enabling the dev service.

In most cases this is not an issue, you either have a value for that property or not, but for OIDC tests in the quarkus core, there was an interesting case :

Some tests use the OidcWiremockTestResource provided by the test library quarkus-test-oidc-server, which mocks the OIDC flow and provides only the keycloak.url property, not quarkus.oidc.auth-server-url. The OIDC extension was configured in properties with
quarkus.oidc.auth-server-url=${keycloak.url}/realms/quarkus. So when the dev service processor looks at the property, the expression is not resolved thus the property null and starts the dev service. But the test expects the mocked OIDC provider not the dev service.

The workaround I applied is to change the expression to quarkus.oidc.auth-server-url=${keycloak.url:replaced-by-tests}/realms/quarkus.

I am not sure if this would qualify as a breaking change and block the backport. But worth discussing.

@The-Funk @gsmet @geoand

[geoand]

Thanks for raising that.

To be honest, I had been +0 on the backport thing, so this probably tips me to -0.5 :)

[ozangunalp]

Same for me. It is a very minor breaking change, but we should refrain from backporting.

[gsmet]

I actually missed the ping and I think it's a bit problematic. I don't see a way to fix this given how things are ordered (and the order makes sense) but we already have one complaint here: #42273 .

And I suspect we will have more.

I added a note in the migration guide as it's the least we have to do.

[sberyozkin]

Hi @ozangunalp, @gsmet I've missed this whole conversation, but I'm seeing a 2nd issue related to this change,

Can you clarify please, given quarkus.oidc.auth-server-url=${keycloak.url}/realms/quarkus, what exactly has changed that made keycloak.url unresolvable, given that the OIDC test library sets keycloak.url ?

I see #42392, but keycloak.url is defined. What am I missing ?

[ozangunalp]

To summarize, every dev service checks whether a service has already been configured AT BUILD TIME to decide whether to kick in. Before this change, those checks were done without taking the expression expansion into account.

So for a given configuration with quarkus.oidc.auth-server-url=${keycloak.url}/realms/quarkus, when keycloak.url is not set, the value evaluation:

• Without the expression expansion, the value would be /realms/quarkus (a non-empty value)
• With the expression expansion, the value would be null, because a used expression is not defined, like : Expanding ${keycloak.url}/realms/quarkus/ when keycloak.url is not defined leads to null value #42392

After this change taking the expression expansion into account, even if a test resource would inject the keycloak.url property, the dev service would've already kicked in because quarkus.oidc.auth-server-url would be evaluated to null.

The workaround I've employed was to provide a fallback to the expression expansion like %test.quarkus.oidc.auth-server-url=${keycloak.url:replaced-by-tests}/realms/quarkus

[sberyozkin]

OK, thanks for the summary @ozangunalp