Allow reading configuration from Kubernetes secrets #10939

Ladicek · 2020-07-23T15:29:39Z

@geoand, @iocanel, this is a follow-up to #8644 as discussed in https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Using.20k8s.20secrets.20as.20a.20configuration.20source. See commit messages for details.

I'm creating this as a draft so that CI can run in my fork, and also because:

this needs Dekorate release with role binding names dekorateio/dekorate#594;
I need to write some documentation;
there's one TODO in the code that I'd like to discuss with you guys;
I added a test, but if you guys think more tests are needed, I'd be happy to add them.

Ladicek · 2020-07-23T15:33:46Z

Ah, and one more important thing that also needs a bit of a discussion: this PR contains an SPI breaking change. I renamed KubernetesRoleBuildItem to KubernetesRoleBindingBuildItem (because that's what it does) and created a new KubernetesRoleBuildItem (which no longer creates role bindings, but roles). If you think that's not acceptable, I'm happy to move back, but would need a good name for a build item that requests creating a role.

(Also, I originally envisioned a generic build item that would carry arbitrary Kubernetes resources, but that would require exposing Dekorate as public SPI. Which is probably a bad idea at this point. So I went with adding a build item for the one Kubernetes resource type I needed, which is a role.)

geoand · 2020-07-23T15:34:54Z

Cool, thanks @Ladicek ! I'll have a look tomorrow

geoand · 2020-07-24T05:53:45Z

Although breaking the SPI is obviously something we don't want to do, I think in this case it's warranted. The new names make perfect sense and furthermore it's very unlikely that any piece of code outside this extension depends on the changed build item.

geoand

I really like this and would definitely want to have it in for 1.7

...s/kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesRoleBindingBuildItem.java

extensions/kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesRoleBuildItem.java

geoand · 2020-07-24T05:56:14Z

...s/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesProcessor.java

@@ -732,11 +736,16 @@ private void applyBuildItems(Session session,
                .forEach(p -> session.configurators().add(new AddPort(p)));

        //Handle RBAC
+        // TODO why this condition?


I don't remember TBH... To me it doesn't seem reasonable (it might have made sense in the past?).

@iocanel do you remember?

The condition seems wrong. The actual condition should be when kubernetes-client is added to the project or something like that.

extensions/kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesRoleBuildItem.java

...runtime/src/main/java/io/quarkus/kubernetes/client/runtime/KubernetesConfigSourceConfig.java

...kus-standard-way/src/test/java/io/quarkus/it/kubernetes/KubernetesConfigWithSecretsTest.java

geoand · 2020-07-24T06:03:16Z

bom/application/pom.xml

@@ -145,7 +145,7 @@
        <aws-alexa-sdk.version>2.30.0</aws-alexa-sdk.version>
        <azure-functions-java-library.version>1.3.0</azure-functions-java-library.version>
        <kotlin.version>1.3.72</kotlin.version>
-        <dekorate.version>0.12.6</dekorate.version>
+        <dekorate.version>0.12-SNAPSHOT</dekorate.version>


Let's not forget to change this one :P

Yea. Hopefully dekorateio/dekorate#594 will get some attention :-)

I pinged @iocanel on this

I'll release later today

geoand · 2020-07-24T06:03:45Z

...illa/deployment/src/main/java/io/quarkus/kubernetes/deployment/AddRoleResourceDecorator.java

+import io.dekorate.kubernetes.decorator.ResourceProvidingDecorator;
+import io.quarkus.kubernetes.spi.KubernetesRoleBuildItem;
+
+class AddRoleResourceDecorator extends ResourceProvidingDecorator<KubernetesListBuilder> {


LGTM, but this is one that @iocanel will have to give the OK for

geoand · 2020-07-24T08:42:23Z

...fig/runtime/src/main/java/io/quarkus/kubernetes/client/runtime/KubernetesConfigRecorder.java

+                && !buildTimeConfig.secretsEnabled) {
+            log.warn("Configuration is read from Secrets " + config.secrets.get()
+                    + ", but quarkus.kubernetes-config.secrets.enabled is false."
+                    + " Check if your application's service account has enough permissions to read secrets.");


Is it just me, or does the last sentence seem out of place?

I wanted to say that what the user ultimately needs to do is not setting our config property, but make sure that the app's service account has access to secrets.

Obviously quarkus.kubernetes-config.secrets.enabled is the easiest way, if people use the Kubernetes extension. But that doesn't always have to be the case.

Ladicek · 2020-07-24T09:08:51Z

Added docs.

geoand · 2020-07-28T11:10:39Z

@iocanel can you please take a look at this? It would be nice to have it in for 1.7

iocanel · 2020-07-28T14:03:54Z

Will look later today

geoand · 2020-07-29T11:33:57Z

@iocanel @Ladicek do you think we can get this one in for 1.7 (CR1 is tomorrow)?

iocanel

LGTM

geoand · 2020-07-29T12:14:17Z

We'll obviously need a dekorate release and a rebase to fix the confict.

Ladicek · 2020-07-29T14:08:50Z

If we can get a Dekorate release, I think this can go into 1.7, yea.

Ladicek · 2020-07-29T14:31:00Z

Rebased and resolved the conflict.

geoand · 2020-07-29T14:38:24Z

Great, so all we need is a dekorate release and we're good

iocanel · 2020-07-29T14:41:33Z

@Ladicek @geoand: dekorate 0.12.7 is out and includes the desired fix.

geoand · 2020-07-29T14:48:17Z

Excellent, thank you both for driving this one home!

Ladicek · 2020-07-29T14:50:45Z

Updated PR, will let CI run in my fork and then un-WIP.

This reverts commit 0c71c47 and adds a couple of cleanups. Most importantly, the issue which caused this functionality to be removed remains: the application by default runs under a role that doesn't have access to secrets. This is marked with one TODO in the code and will be addressed in a subsequent commit.

The `kubernetes` extension automatically generates a RoleBinding that refers to the `view` ClusterRole. This ClusterRole doesn't allow access to secrets. This commit therefore adds a configuration property which, when enabled, makes the `kubernetes` extension generate a special Role `view-secrets` and a second RoleBinding referring to that role. This configuration property is build-time only and has no other effect. With this configuration in place, there's nothing preventing the application from reading Secrets directly from the API server. For convenience, a warning is printed at runtime if configuration is read from Secrets yet the property is disabled.

Ladicek · 2020-07-29T16:13:33Z

Fixed a failing unit test (I didn't run it locally before, just the integration tests) and un-WIP-ed.

geoand · 2020-07-30T08:17:41Z

There is a weird CI failure that is almost certainly a system failure, but because it happened on the Kubernetes tests, I want to avoid any surprises, so I'll restart CI

gsmet · 2020-07-30T12:17:37Z

And merged! Thanks!

geoand · 2020-07-30T12:19:18Z

🎉

boring-cyborg bot added area/dependencies Pull requests that update a dependency file area/kubernetes labels Jul 23, 2020

Ladicek requested review from geoand and iocanel July 23, 2020 15:33

geoand reviewed Jul 24, 2020

View reviewed changes

Ladicek force-pushed the kubernetes-config-secrets branch from 9ffc17d to 74eb333 Compare July 24, 2020 08:39

geoand reviewed Jul 24, 2020

View reviewed changes

Ladicek force-pushed the kubernetes-config-secrets branch from 74eb333 to dfc1f6e Compare July 24, 2020 09:08

Ladicek force-pushed the kubernetes-config-secrets branch from dfc1f6e to 412d10a Compare July 24, 2020 12:48

boring-cyborg bot added the area/documentation label Jul 24, 2020

gsmet changed the title ~~allow reading configuration from Kubernetes secrets~~ Allow reading configuration from Kubernetes secrets Jul 28, 2020

gsmet added this to the 1.7.0 - master milestone Jul 28, 2020

iocanel approved these changes Jul 29, 2020

View reviewed changes

Ladicek force-pushed the kubernetes-config-secrets branch from 412d10a to a19aab0 Compare July 29, 2020 14:30

Ladicek force-pushed the kubernetes-config-secrets branch from a19aab0 to 29c6d11 Compare July 29, 2020 14:50

Ladicek added 2 commits July 29, 2020 18:12

make sure ConfigMap/Secret content isn't accidentally exposed

571aaef

Ladicek force-pushed the kubernetes-config-secrets branch from c33bc0c to 571aaef Compare July 29, 2020 16:12

Ladicek marked this pull request as ready for review July 29, 2020 16:12

gsmet merged commit f538ba5 into quarkusio:master Jul 30, 2020

Ladicek deleted the kubernetes-config-secrets branch July 30, 2020 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reading configuration from Kubernetes secrets #10939

Allow reading configuration from Kubernetes secrets #10939

Ladicek commented Jul 23, 2020

Ladicek commented Jul 23, 2020

geoand commented Jul 23, 2020

geoand commented Jul 24, 2020

geoand left a comment

geoand Jul 24, 2020

iocanel Jul 29, 2020

geoand Jul 24, 2020

Ladicek Jul 24, 2020

geoand Jul 24, 2020

iocanel Jul 29, 2020

geoand Jul 24, 2020

geoand Jul 24, 2020

Ladicek Jul 24, 2020

geoand Jul 24, 2020

Ladicek commented Jul 24, 2020

geoand commented Jul 28, 2020

iocanel commented Jul 28, 2020

geoand commented Jul 29, 2020

iocanel left a comment

geoand commented Jul 29, 2020

Ladicek commented Jul 29, 2020

Ladicek commented Jul 29, 2020

geoand commented Jul 29, 2020

iocanel commented Jul 29, 2020

geoand commented Jul 29, 2020

Ladicek commented Jul 29, 2020

Ladicek commented Jul 29, 2020

geoand commented Jul 30, 2020 •

edited

Loading

gsmet commented Jul 30, 2020

geoand commented Jul 30, 2020

Allow reading configuration from Kubernetes secrets #10939

Allow reading configuration from Kubernetes secrets #10939

Conversation

Ladicek commented Jul 23, 2020

Ladicek commented Jul 23, 2020

geoand commented Jul 23, 2020

geoand commented Jul 24, 2020

geoand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Jul 24, 2020

geoand commented Jul 28, 2020

iocanel commented Jul 28, 2020

geoand commented Jul 29, 2020

iocanel left a comment

Choose a reason for hiding this comment

geoand commented Jul 29, 2020

Ladicek commented Jul 29, 2020

Ladicek commented Jul 29, 2020

geoand commented Jul 29, 2020

iocanel commented Jul 29, 2020

geoand commented Jul 29, 2020

Ladicek commented Jul 29, 2020

Ladicek commented Jul 29, 2020

geoand commented Jul 30, 2020 • edited Loading

gsmet commented Jul 30, 2020

geoand commented Jul 30, 2020

geoand commented Jul 30, 2020 •

edited

Loading