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.

Sign up for GitHub

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

Jump to bottom

When using Secrets, config functionality should be auto enabled #11748

Merged
merged 1 commit into from
Sep 2, 2020
Merged

When using Secrets, config functionality should be auto enabled #11748

merged 1 commit into from
Sep 2, 2020

Conversation

ketoketo
Copy link
Contributor

@gsmet gsmet requested a review from geoand August 31, 2020 16:27
@geoand
Copy link
Contributor

geoand commented Aug 31, 2020

Thanks for this!

Can you please check the output of CI? Something is failing

@geoand
Copy link
Contributor

geoand commented Aug 31, 2020

The relevant CI failure is:

2020-08-31T14:40:00.2255552Z [INFO] Running io.quarkus.it.kubernetes.client.SecretPropertiesTest
2020-08-31T14:40:05.7507108Z 2020-08-31 14:40:02,825 WARN  [io.qua.dep.QuarkusAugmentor] (main) Using Java versions older than 11 to build Quarkus applications is deprecated and will be disallowed in a future release!
2020-08-31T14:40:05.7507902Z 2020-08-31 14:40:05,156 INFO  [okh.moc.MockWebServer] (MockWebServer 55545) MockWebServer[55545] starting to accept connections
2020-08-31T14:40:05.7508464Z 2020-08-31 14:40:05,160 WARN  [io.fab.kub.cli.Config] (main) Error reading service account token from: [/var/run/secrets/kubernetes.io/serviceaccount/token]. Ignoring.
2020-08-31T14:40:05.7509100Z 2020-08-31 14:40:05,499 WARN  [io.qua.kub.cli.run.KubernetesConfigRecorder] (main) Configuration is read from Secrets [s1], but quarkus.kubernetes-config.secrets.enabled is false. Check if your application's service account has enough permissions to read secrets.
2020-08-31T14:40:05.7509836Z 2020-08-31 14:40:05,560 INFO  [okh.moc.MockWebServer] (MockWebServer /127.0.0.1:58024) MockWebServer[55545] received request: GET /api/v1/namespaces/test/configmaps/cmap1 HTTP/1.1 and responded: HTTP/1.1 200 OK
2020-08-31T14:40:05.7510801Z 2020-08-31 14:40:05,627 INFO  [okh.moc.MockWebServer] (MockWebServer /127.0.0.1:58024) MockWebServer[55545] received request: GET /api/v1/namespaces/test/configmaps/cmap2 HTTP/1.1 and responded: HTTP/1.1 200 OK
2020-08-31T14:40:05.7511384Z 2020-08-31 14:40:05,725 ERROR [io.qua.application] (main) Failed to start application (with profile test): javax.enterprise.inject.spi.DeploymentException: No config value of type [java.lang.String] exists for: secret.prop1
2020-08-31T14:40:05.7511642Z 	at io.quarkus.arc.runtime.ConfigRecorder.validateConfigProperties(ConfigRecorder.java:37)

@ketoketo
Copy link
Contributor Author

ketoketo commented Sep 1, 2020

Thanks for the review. I'll try to fix it.

@ketoketo ketoketo changed the title When using Secrets, config functionality should be auto enabled WIP - When using Secrets, config functionality should be auto enabled Sep 1, 2020
@geoand
Copy link
Contributor

geoand commented Sep 1, 2020

Can you please squash the commits?

@ketoketo ketoketo changed the title WIP - When using Secrets, config functionality should be auto enabled When using Secrets, config functionality should be auto enabled Sep 1, 2020
@ketoketo ketoketo changed the title When using Secrets, config functionality should be auto enabled WIP - When using Secrets, config functionality should be auto enabled Sep 1, 2020
@ketoketo ketoketo closed this Sep 1, 2020
@ketoketo ketoketo changed the title WIP - When using Secrets, config functionality should be auto enabled When using Secrets, config functionality should be auto enabled Sep 1, 2020
@ketoketo ketoketo reopened this Sep 1, 2020
@geoand
Copy link
Contributor

geoand commented Sep 1, 2020

There is one thing I don't like: With the current approach, having quarkus.kubernetes-config.enabled=true also enables configmap reading, which is something we probably don't want

@ketoketo
Copy link
Contributor Author

ketoketo commented Sep 1, 2020

I thought that being able to read configMap when quarkus.kubernetes-config.enabled=true was the same behavior as before, but am I mistaken?

@geoand
Copy link
Contributor

geoand commented Sep 1, 2020

Sorry, I meant that when enabling secrets, then config maps are enabled - that is what I am saying is a little weird

geoand
geoand reviewed Sep 1, 2020
View reviewed changes
...e/src/test/java/io/quarkus/kubernetes/client/runtime/KubernetesConfigSourceProviderTest.java Outdated
import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
import io.fabric8.kubernetes.api.model.ConfigMapList;
import io.fabric8.kubernetes.api.model.DoneableConfigMap;
import io.fabric8.kubernetes.api.model.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: Please avoid using start imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it

@ketoketo
Fix to read secrets when secrets.enabled is true
f350ec7 
fix integration test

fix log

fix import
@ketoketo ketoketo requested a review from geoand September 2, 2020 01:44
geoand
geoand approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@geoand geoand merged commit 126cd55 into quarkusio:master Sep 2, 2020
@geoand geoand added this to the 1.8.0 - master milestone Sep 2, 2020
@geoand geoand mentioned this pull request Sep 2, 2020
Closed
@Ladicek Ladicek mentioned this pull request Sep 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/kubernetes
Projects
None yet
Milestone
1.8.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ketoketo @geoand