Skip to content

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
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