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

Fix bad yaml when only secretKeyRef #267

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

asiegman
Copy link
Contributor

@asiegman asiegman commented Mar 2, 2021

Found a bug with my previous feature.

If you specified only a secretKeyRef, and not an accompanying fieldRef, the env: tag would be missing in the yaml section, causing invalid yaml as such:

      containers:
      - name: mything
        image: mything:latest
        imagePullPolicy: Always

        envFrom:
        - configMapRef:
            name: mything-env-default

          - name: MYTHING_USERNAME
            valueFrom:
              secretKeyRef:
                name: mything-user
                key: username
          - name: MYTHING_PASSWORD
            valueFrom:
              secretKeyRef:
                name: mything-user
                key: password

This fixes this bug, so that if either fieldKeyRef or secretKeyRefs are referenced, the env: section will have the appropriate yaml

      containers:
      - name: mything
        image: mything:latest
        imagePullPolicy: Always

        envFrom:
        - configMapRef:
            name: mything-env-default

        env:

          - name: MYTHING_USERNAME
            valueFrom:
              secretKeyRef:
                name: mything-user
                key: username
          - name: MYTHING_PASSWORD
            valueFrom:
              secretKeyRef:
                name: mything-user
                key: password

@asiegman asiegman marked this pull request as ready for review March 2, 2021 08:03
@Nuru Nuru added the bug label Mar 2, 2021
@@ -64,8 +64,9 @@ env:
{{/*
https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-pod-fields-as-values-for-environment-variables
*/}}
{{- with $root.Values.envFromFieldRefFieldPath }}
{{- if or (not (empty $root.Values.envFromFieldRefFieldPath)) (not (empty $root.Values.envFromSecretKeyRef)) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to consider

{{- with $root.Values.env }}
env:
{{- range $name, $value := . }}
- name: {{ $name }}
value: {{ default "" $value | quote }}
{{- end }}
{{- end }}

And you can use a more concise syntax like

{{- if $root.Values.configMaps | or $root.Values.secrets | or $root.Values.envFrom }}

  • empty is redundant, conditionals calculate emptiness for you

Copy link
Contributor Author

@asiegman asiegman Mar 3, 2021

Choose a reason for hiding this comment

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

I really didn't like my if statement there, but missed the better syntax. Updated that.

Is the forced quoting of values for safety? I didn't see it done in the fieldRef feature just above the secretKeyRef feature, so I did not do it. This is all part of the following Kubernetes features:
https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-environment-variables

This should all be Kubernetes field paths and secret names and safe for YAML without unnecessary quoting. I'm always a bit fuzzy on this, the rules around quoting in YAML seem inconsistent to me everytime I read them.

Copy link
Contributor Author

@asiegman asiegman Mar 3, 2021

Choose a reason for hiding this comment

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

Added in the string safety after testing it. Seems Kubernetes doesn't mind the explicit strings there, in my limited testing.

@asiegman asiegman requested a review from Nuru March 3, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants