Fix dev service ephemeral ports: Make test resource config override dev service config and system props

[holly-cummins]

• Resolves Support ephemeral ports in dev services config #48784
• Resolves Testing Lambda resource endpoint with rest-assured and random test-port results in an IllegalArgumentException #50389

After the big test classloading rewrite, ephemeral ports for the lambda dev service stopped working (that is, if a user sets the port as 0 in application.properties, in hopes that they get a randomly assigned one, injected clients will attempt to connect to a port "0"). This issue would also affect other services where a user explicitly configured a port of 0. To fix that, the dev service needs to be moved to the new model, but that's not enough. The lazy config source that knows the random port needs to have a higher priority than the '0' hardcoded in the application properties.

Fixing this is (almost) a straightforward matter of bumping the priority (ordinal) to something 400 or higher. Sadly that simple fix creates a ripple of other problems:

• The MultiClientImportPreloadingTest in the redis-client test suite uses a TestResource to start and stop a redis instance, but the test doesn't disable dev services, so there are actually two redis instances. (I've checked, and this is the case even on 3.21.) For some of the test to pass, they needs to connect to the test resource one, not the dev service one, and for that to work, the dev services config needs to be lower priority than application properties. Arguably, this is a slightly silly test for starting things twice, but I don't want to regress behaviour that currently works.
• The KakfaCompanionResource test resource used by the integration-tests/reactive-messaging-hibernate-orm and integration-tests/opentelemetry-reactive-messaging tests interacts with the dev services, and for it to work, test resources need to have a higher priority than dev services
• The random (ie ephemeral) port test in extensions/resteasy-classic/resteasy-client uses a QuarkusUnitTest.overrideConfigKey to configure the port to 0. QuarkusUnitTest.overrideConfigKey uses the same RuntimeOverrideConfigSource config source as test resources do. For this test to pass, unit test sources need to have a lower priority than dev service sources.

So we have the following desired behaviours:

• Config for test resources should have a lower priority than system properties, but higher than dynamic dev service config.
• Config for QuarkusUnitTest().overrideConfigKey should have a lower priority than dynamic dev services config
• Config from dev services should be higher than system properties (or -Dservice.port=0 wouldn't work).
• Config from dev services should be lower than application properties (or coexistence with test resources wouldn't work).

As a hierarchy of priorities, that would be (from highest to lowest):

• Dev services 'overrides', such as replacing "0" with an ephemeral port
• System properties
• TestResourceManager.start [currently the same as QuarkusUnitTest]
• QuarkusUnitTest.overrideConfigKey [currently the same as TestResourceManager]
• Application properties
• Dev services 'filling in the blanks' config, such as defining a service port where there wasn't one before

So I see two options. One is to split the RuntimeOverrideConfigSource into two so it can have two priorities for the two different scenarios, and the other is to split the dev service source into an 'underride' and 'override' pair. I worried that I might keep finding scenarios where the test resource config wasn't quite at the right relative priority, whereas the "this should override" vs "this should not override" distinction for dev services seemed clearer. So I went that way.

It should be really clean to just say 'only have this as an override if the same property is found in the original, user-defined, config' but getting that list wasn't so easy, and passing in the hardcoded config just made the API really confusing. I wondered about using ConfigUtils.isPropertyNonEmpty(propName) but that would be introducing the same problem of fighting with test resource overrides, but with some extra sequencing fun.

As part of this, I also had to make the port a runtime config element. Otherwise, CI was green, but there was a warning in the logs.

[github-actions]

🎊 PR Preview 34e5e88 has been successfully built and deployed to https://quarkus-pr-main-48815-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[radcortez]

It should be really clean to just say 'only have this as an override if the same property is found in the original, user-defined, config' but getting that list wasn't so easy, and passing in the hardcoded config just made the API really confusing. I wondered about using ConfigUtils.isPropertyNonEmpty(propName) but that would be introducing the same problem of fighting with test resource overrides, but with some extra sequencing fun.

You can do this by using a ConfigSourceFactory: https://smallrye.io/smallrye-config/Latest/config-sources/factories/.

Internally, SmallRyeConfig initializes sources in two phases. In the second phase, where ConfigSourceFactory is initialized, you can access the current ConfigSourceContext and retrieve configuration from the first phase. The first phase contains all sources that are not factories (System Prop, Env, implementations of ConfigSource, etc.).

Since the DevServices configuration is already resolved before the runtime Config, the DevServices config could be used in a factory to selectively set the desired values. If required, a factory can provide multiple sources with different ordinals. All the resolved sources are ordered with the main ones to assemble the final list, but having the ability to query the current config and set what you need should be enough, and you shouldn't require creating different sources with different ordinals.

[hamburml]

Is there an eta? :( We are blocked to update to the new quarkus lts cause we use random test-ports.

[radcortez]

@holly-cummins do you want me to write my alternate proposal?

[lucakes]

Hi, are there any updates?

[hamburml]

@holly-cummins @radcortez Is there an update pls?

[radcortez]

I'm working on #51209, which would create the foundations to fix this, but it may still take a while... sorry for the inconvenience, and thank you for your patience.

[holly-cummins]

Should we consider merging the fix in this PR while we wait for the longer term solution, @radcortez?

[radcortez]

#51209 must go in a new minor, hopefully in 3.31, but most likely we are skipping the December release and doing it only in January: https://github.com/quarkusio/quarkus/wiki/Release-Planning

So, if this cannot wait until then, I'm in favor of merging this.

I already have a prototype using #51209 to fix this, which conceptually is not very different from what you have done here (but without relying directly on Config), but I'm still exploring other options.

Since we need to rebase this, I would ask to add a couple of @Deprecated annotations to the new methods, because most likely this will change, and it would be easier to remove them in the future and signal to be careful to use them.

Do you want me to do the rebase?

[holly-cummins]

Since we need to rebase this, I would ask to add a couple of @Deprecated annotations to the new methods, because most likely this will change, and it would be easier to remove them in the future and signal to be careful to use them.

Do you want me to do the rebase?

Yes please, thank you! +1 on the deprecated annotations, seems reasonable.

[radcortez]

Ok, rebased and added the @Deprecated annotations.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 296c455.

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

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[radcortez]

@holly-cummins, please check if this is ok.

[holly-cummins]

@holly-cummins, please check if this is ok.

Yes it looks good!

[radcortez]

Rerunning the job with test failures. The failures seem unrelated.

[quarkus-bot]

Status for workflow Quarkus CI

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

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

[hamburml]

Could this be ported to 3.27 lts please?