wave-k8s / wave

Kubernetes configuration tracking controller
Apache License 2.0
647 stars 82 forks source link

Support for env.valueFrom using configMapKeyRef and secretKeyRef #33

Closed andyedwardsibm closed 5 years ago

andyedwardsibm commented 5 years ago

This operator is exactly what we're looking for, but our use of secrets involves explicitly pulling in individual secrets to the deployment as separate envs. Wave only considers whole ConfigMap/Secret entries in the containers.envFrom and volumes.volumeSource sections though.

For example, for the deployment snippet below, changes to EXPLICIT_A in ConfigMap config-explicit, and SECRET_A in Secret secrets are not noticed.

...
          env:
            # Explicit config
            - name: EXPLICIT_A
              valueFrom:
                configMapKeyRef:
                  name: config-explicit
                  key: EXPLICIT_A
            # Explicit secrets
            - name: SECRET_A
              valueFrom:
                secretKeyRef:
                  name: secrets
                  key: SECRET_A
          # Global config
          envFrom:
            - configMapRef:
                name: config-global
...

I've been prototyping some changes in a fork that includes discovery of env entries, and only includes the data from the specified *KeyRef in the hash calculation. This has basically involved adding a new ConfigObject type and putting the Object inside (so Wave can still pass it to k8s), along with metadata on whether an individual field is specified, and what that field is (that can be used by the hashing code). I have something that seems to work in a minikube test environment. Would you be open to me submitting a PR?

JoelSpeed commented 5 years ago

You're right, Wave should definitely include support for these! I completely missed this when doing the original implementation

At present, when parsing we just hash the whole configmap/secret for simplicity's sake, so I'd be tempted to start with a PR to add the parsing of the envVar section and just adding the whole secret/configmap (as we do with other sources) and then follow up with a second PR which makes the changes to only hash the individual keys within the secrets and configmaps.

WDYT?

andyedwardsibm commented 5 years ago

That's a possibility, yes, and I might be able to whip up a quick PR, but for our use case we'd want the final behaviour of spotting individual key changes.

I'm really close to finishing the code that handles specific keyed items: I've just got to handle the optional field and those fields being potentially missing (and extend the unit tests for that situation).

andyedwardsibm commented 5 years ago

PR submitted (https://github.com/pusher/wave/pull/34) that includes the env section, only using the specified keys in the hash, and handling the optional flag as best I can.

JoelSpeed commented 5 years ago

@andyedwardsdfdl Can this issue be closed now #34 is merged?

andyedwardsibm commented 5 years ago

Yep, I believe this is now resolved, and ready to go into the next release.