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

Make basic realm and form authentication configuration properties only used during runtime the runtime properties #37022

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Nov 11, 2023

There is no reason to have properties only used at runtime defined as build time properties. BasicAuthenticationMechanism constructors have been deprecated for 3 years now.

Copy link

quarkus-bot bot commented Nov 11, 2023

Failing Jobs - Building b08cf2f

Status Name Step Failures Logs Raw logs Build scan
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 21

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.CustomManifestArgumentsTest.shouldContainsSpecificManifestProperty line 37 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual not to be null

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/hibernate-search-orm-opensearch 

📦 integration-tests/hibernate-search-orm-opensearch

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-hibernate-search-orm-opensearch: I/O Error

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

LGTM, please check test failures just in case, there is no obvious link but who knows

@michalvavrik
Copy link
Member Author

michalvavrik commented Nov 11, 2023

@sberyozkin

io.quarkus.gradle.CustomManifestArgumentsTest#shouldContainsSpecificManifestProperty has no security, so it can't be related (checked source code).

OpenSearch test has something to do with container, it's probably flaky due to resources

2023-11-11T02:51:13.5676197Z 02:51:13.566 OpenSearch:Error: Could not find or load main class Cannot
2023-11-11T02:51:13.5677427Z 02:51:13.566 OpenSearch:Caused by: java.lang.ClassNotFoundException: Cannot
2023-11-11T02:51:13.9699114Z [ERROR] DOCKER> [docker.io/opensearchproject/opensearch:2.9.0] "opensearch": Container stopped with exit code 1 unexpectedly after 2014 ms while waiting on url http://localhost:9200
2023-11-11T02:51:13.9708386Z [ERROR] DOCKER> Error occurred during container startup, shutting down...
2023-11-11T02:51:13.9714594Z [INFO] DOCKER> [docker.io/opensearchproject/opensearch:2.9.0] "opensearch": Stop and removed container ff0ac6a3382b after 0 ms
2023-11-11T02:51:13.9760741Z [ERROR] DOCKER> I/O Error [[docker.io/opensearchproject/opensearch:2.9.0] "opensearch": Container stopped with exit code 1 unexpectedly after 2014 ms while waiting on url http://localhost:9200]

@michalvavrik
Copy link
Member Author

also the Gradle test also failed in my other PR that is not related

@sberyozkin
Copy link
Member

Sounds good thanks, good idea to fix it now alongside http policies to avoid native side-effects, etc.

@zakkak
Copy link
Contributor

zakkak commented Nov 14, 2023

@michalvavrik this PR results in warnings like the following being printed:

$ java -jar target/quarkus-app/quarkus-run.jar 
__  ____  __  _____   ___  __ ____  ______ 
 --/ __ \/ / / / _ | / _ \/ //_/ / / / __/ 
 -/ /_/ / /_/ / __ |/ , _/ ,< / /_/ /\ \   
--\___\_\____/_/ |_/_/|_/_/|_|\____/___/   
2023-11-13 18:08:21,863 WARN  [io.qua.run.con.DeprecatedRuntimePropertiesRecorder] (main) The 'quarkus.http.auth.form.redirect-after-login' config property is deprecated and should not be used anymore
2023-11-13 18:08:22,130 INFO  [io.quarkus] (main) quarkus-full-microprofile 1.0.0-SNAPSHOT on JVM (powered by Quarkus 999-SNAPSHOT) started in 0.917s. Listening on: http://0.0.0.0:8080
2023-11-13 18:08:22,131 INFO  [io.quarkus] (main) Profile prod activated. 
2023-11-13 18:08:22,131 INFO  [io.quarkus] (main) Installed features: [cdi, jaeger, rest-client, resteasy, security, smallrye-context-propagation, smallrye-fault-tolerance, smallrye-health, smallrye-jwt, smallrye-metrics, smallrye-openapi, smallrye-opentracing, vertx]
^C2023-11-13 18:08:29,397 INFO  [io.quarkus] (Shutdown thread) quarkus-full-microprofile stopped in 0.221s

Even if the application is not setting (at least to our understanding) the deprecated config property.
See Karm/mandrel-integration-tests#220 (comment)

@michalvavrik
Copy link
Member Author

I didn't deprecate this property @zakkak , it has been deprecated for a while now. I can only assume there is a difference in how deprecated properties are reported.

I'll have a look. Could you please point me to exact source code of that Mandrel application?

@michalvavrik
Copy link
Member Author

It is called DeprecatedRuntimePropertiesRecorder so just by name (before I go deeper) it doesn't report build time properties.

@michalvavrik
Copy link
Member Author

Okay, you are using quarkus-full-microprofile, I'll check it out.

@zakkak
Copy link
Contributor

zakkak commented Nov 14, 2023

@michalvavrik we saw the warning using quarkus-full-microprofile from https://github.com/Karm/mandrel-integration-tests/tree/master/apps/quarkus-full-microprofile

If you need more help to rerproduce the issue let me know.

I suspect that the deprecation warning is related to this line of code

@michalvavrik
Copy link
Member Author

This line is correct, it's legit to support deprecated properties until we decide to remove them. It was there before as well, only thing that changes is BUILD TIME => RUNTIME.

Anyway @zakkak I've run https://github.com/Karm/mandrel-integration-tests/tree/master/apps/quarkus-full-microprofile with mvn clean install -Pnative -Dgloba.quarkus.version=999-SNAPSHOT and HEAD on 830ef6e. The native-image 22.3.0.1-Final Mandrel Distribution (Java Version 17.0.5+8) was used. and I can't reproduce it with ./target/quarkus-runner.

Could you maybe provide steps to reproduce?

@jerboaa
Copy link
Contributor

jerboaa commented Nov 14, 2023

Anyway @zakkak I've run https://github.com/Karm/mandrel-integration-tests/tree/master/apps/quarkus-full-microprofile with mvn clean install -Pnative -Dgloba.quarkus.version=999-SNAPSHOT

You don't need to run native to reproduce. JVM mode is sufficient. Steps should be:

cd apps/quarkus-full-microprofile/
mvn clean package -Dquarkus.version=999-SNAPSHOT
java -jar target/quarkus-app/quarkus-run.jar
__  ____  __  _____   ___  __ ____  ______ 
 --/ __ \/ / / / _ | / _ \/ //_/ / / / __/ 
 -/ /_/ / /_/ / __ |/ , _/ ,< / /_/ /\ \   
--\___\_\____/_/ |_/_/|_/_/|_|\____/___/   
2023-11-14 10:56:07,714 WARN  [io.qua.run.con.DeprecatedRuntimePropertiesRecorder] (main) The 'quarkus.http.auth.form.redirect-after-login' config property is deprecated and should not be used anymore
2023-11-14 10:56:07,999 INFO  [io.quarkus] (main) quarkus-full-microprofile 1.0.0-SNAPSHOT on JVM (powered by Quarkus 999-SNAPSHOT) started in 0.976s. Listening on: http://0.0.0.0:8080
2023-11-14 10:56:07,999 INFO  [io.quarkus] (main) Profile prod activated. 
2023-11-14 10:56:07,999 INFO  [io.quarkus] (main) Installed features: [cdi, jaeger, rest-client, resteasy, security, smallrye-context-propagation, smallrye-fault-tolerance, smallrye-health, smallrye-jwt, smallrye-metrics, smallrye-openapi, smallrye-opentracing, vertx]

That is with quarkus main (revision 2d702826c2642c6db0eccda3b0f9c699fdb0d5ca)

@michalvavrik
Copy link
Member Author

@zakkak @jerboaa this property is added by DefaultValuesConfigSource which is IMHO config system bug - we should have right to set default value to deprecated. @radcortez WDYT?

@michalvavrik
Copy link
Member Author

for context @radcortez io.quarkus.runtime.configuration.DeprecatedRuntimePropertiesRecorder#reportDeprecatedProperties collect all property names and one of config sources that provides this name is DefaultValuesConfigSource, hence warning is reported even though this property is never set.

@michalvavrik
Copy link
Member Author

@radcortez I could just add condition to the recorder that ignores config source with the DefaultValuesConfigSource name, or do you have in mind something more complex?

@jerboaa
Copy link
Contributor

jerboaa commented Nov 14, 2023

Could we move this to a separate issue, please?

@michalvavrik
Copy link
Member Author

Could we move this to a separate issue, please?

Sure thing, I created #37072 and I'll add detail when I find a little time.

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.

4 participants