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

chart: add extraEnv to enable secrets injection through secrets operator #514

Merged
merged 1 commit into from
Dec 5, 2021

Conversation

goshlanguage
Copy link
Contributor

Add extraEnv pattern to enable secrets injection through secrets operator

add extraEnv to enable secrets injection through secrets operator. This enables operators to bring their own secrets that don't necessarily conform to the defaults the chart sets, so this chart can be used as a chart dependency or otherwise, for users who need to leverage secrets operators like onepass-operator or external-secrets, etc.

How to use

Set extraEnv to your desired value(s), ie:

extraEnv: 
- name: "NEBRASKA_OIDC_CLIENT_SECRET"
  valueFrom:
    secretKeyRef:
      name: my-non-standard-secret-name
      key: nonStandardOidcClientSecretName

This chart change has been reviewed and tested by templating out the chart with the standard, and with valid spec in the extraEnv block with the following command:

helm template  --output-dir test-output/ . --debug

In the default case, nothing is appended to env, and in the case that the preceding example is populated in values, the resulting env spec is:

          env:
            - name: DB_PASSWORD
              valueFrom:
                secretKeyRef:
                  name: RELEASE-NAME-postgresql
                  key: postgresql-password
            - name: NEBRASKA_DB_URL
              value: "postgres://postgres:$(DB_PASSWORD)@RELEASE-NAME-postgresql:5432/nebraska?sslmode=disable&connect_timeout=10"
            - name: TZ
              value: "UTC"
            # Extra environment variables
            - name: NEBRASKA_OIDC_CLIENT_SECRET
              valueFrom:
                secretKeyRef:
                  key: nonStandardOidcClientSecretName
                  name: my-non-standard-secret-name

Testing done

Please see above

@joaquimrocha joaquimrocha requested a review from yolossn November 22, 2021 09:24
@goshlanguage goshlanguage changed the title add extraEnv to enable secrets injection through secrets operator chart: add extraEnv to enable secrets injection through secrets operator Nov 22, 2021
@goshlanguage
Copy link
Contributor Author

I did need to add a toggle in order to bypass the required fields being set in values. I'm open to other approaches if desired, this seemed to be the cleanest way to avoid creating a false secret that wouldn't be used.

Per usage, this would enable me to create a chart that embeds nebraska, setting it as a dependency, and providing with it a secrets CRD to be consumed by my secrets operator that pulls my secret from another source. Then, I can inject that secret in extraEnv like so:

extraEnv:
- name: NEBRASKA_OIDC_CLIENT_ID
  valueFrom:
    secretKeyRef:
      name: nebraska-oidc
      key: oidc-client-id
- name: NEBRASKA_OIDC_CLIENT_SECRET
  valueFrom:
    secretKeyRef:
      name: nebraska-oidc
      key: oidc-client-secret

and then I can set the oidc config as follows in order for it to be picked up:

  auth:
    mode: oidc
    bypassSecrets: true
    oidc:
      clientID:
      clientSecret:
      issuerURL: https://my.issuer.com/
      validRedirectURLs: https://nebraska.my.url.com/
      managementURL:
      logoutURL:
      adminRoles: myAdminTeam
      viewerRoles:
      rolesPath:
      scopes:
      sessionAuthKey:
      sessionCryptKey:

@joaquimrocha
Copy link
Collaborator

Hi @goshlanguage . Some of the code owners in the team have been on vacation but they should be back tomorrow.
In the meanwhile, and in the interest of speeding up the review, I can advise to follow the format for the commit titles: prefix with backend: Title and to add a description/body to the commits that are not super obvious (I know you have explained some of them in the PRs comments, but when doing a bisect or going back to that commit, it's good to have the info about it in git itself).

If you need more details about the style, check out: https://kinvolk.io/docs/nebraska/latest/contributing/

@joaquimrocha
Copy link
Collaborator

Actually, backend: and frontend: are the prefixes we commonly use for those parts. For chart, I have been using charts:.
If a commit has to touch several files in more than one "module", then it's fine to exclude the prefix. Hopefully you'll find these fine and not too nitpicky 🙂

@goshlanguage
Copy link
Contributor Author

Apologies, I now see the bits about the commit messages:
https://github.com/kinvolk/contribution/blob/master/topics/git.md

I'll see if I can amend these git commit messages. Thanks for your reply

@goshlanguage goshlanguage force-pushed the enable-configurable-extra-envs branch from 82c55f9 to 903b71b Compare November 24, 2021 19:42
@yolossn
Copy link
Contributor

yolossn commented Nov 25, 2021

Hey @goshlanguage,

Thanks for the PR

I did need to add a toggle in order to bypass the required fields being set in values. I'm open to other approaches if desired, this seemed to be the cleanest way to avoid creating a false secret that wouldn't be used.

An empty array, map is evaluated as false value in Helm Pipelines, perhaps we can use that instead of an extra variable.
Refer: https://helm.sh/docs/chart_template_guide/control_structures/#ifelse

The secrets.yaml can be converted to something like this, to achieve the same without the need for an extra variable
Note: I have used the variable ownEnv instead of extraEnv as it can be confusing with extraEnvVars.

{{- $useGhAuth := eq .Values.config.auth.mode "github" }}
{{- $useOidcAuth := eq .Values.config.auth.mode "oidc" }}
{{- $useDbPassword := not .Values.config.database.passwordExistingSecret.enabled }}
{{- $ownEnv := .Values.ownEnv}}
{{- if and (or $useDbPassword $useOidcAuth $useGhAuth) $ownEnv}}
apiVersion: v1
kind: Secret
metadata:
  name: {{ include "nebraska.fullname" . }}
  labels:
    {{- include "nebraska.labels" . | nindent 4 }}
type: Opaque
data:
  {{- if $useDbPassword }}
  dbPassword: {{ (tpl .Values.config.database.password .) | b64enc }}
  {{- end }}
  {{- if $ownEnv }}
    {{- if $useOidcAuth }}
    oidcClientID: {{ required "A valid 'clientId' is required when using oidc authentication" .Values.config.auth.oidc.clientID | toString | b64enc }}
    oidcClientSecret: {{ required "A valid 'clientSecret' is required when using oidc authentication" .Values.config.auth.oidc.clientSecret | toString | b64enc }}
      {{- with .Values.config.auth.oidc.sessionAuthKey }}
    oidcSessionAuthKey: {{ . | toString | b64enc }}
      {{- end }}
      {{- with .Values.config.auth.oidc.sessionCryptKey }}
    oidcSessionCryptKey: {{ . | toString | b64enc }}
      {{- end }}
    {{- end }}
    {{- if $useGhAuth }}
    ghClientSecret: {{ required "A valid 'clientSecret' is required when using github authentication." .Values.config.auth.github.clientSecret | toString | b64enc }}
    ghSessionAuthKey: {{ required "A valid 'sessionAuthKey' is required when using github authentication." .Values.config.auth.github.sessionAuthKey | toString | b64enc }}
    ghSessionCryptKey: {{ required "A valid 'sessionCryptKey' is required when using github authentication." .Values.config.auth.github.sessionCryptKey | toString | b64enc }}
    ghWebhookSecret: {{ required "A valid 'webhookSecret' is required when using github authentication." .Values.config.auth.github.webhookSecret | toString | b64enc }}
    {{- end }}
  {{- end }}
{{- end }}

What do you think?

@goshlanguage
Copy link
Contributor Author

I wouldn't mind, but this deviates from a well established idiom/pattern in the community, including but not limited to the dependencies of this very chart.

For example, see the bitnami Postgres chart:
https://github.com/bitnami/charts/blob/master/bitnami/postgresql/templates/statefulset.yaml#L253-L255

This is specifically called extraEnv because that's the established pattern.

@goshlanguage
Copy link
Contributor Author

Per using the contents of extraEnv as a toggle in secrets, this is imprecise, as the user may wish to add variables to the environment that don't pertain to the oidc secret. In this instance, the logic would be flawed.

@yolossn
Copy link
Contributor

yolossn commented Dec 3, 2021

@goshlanguage Can you rebase the PR with the main branch? The CI is failing because the chart version is not up to date.

Previously, the chart only took a key value map for data that is commonly treated as secret. This patch creates a mechanism that allows users to bring these secrets in with a secrets operator or manually, and define them, setting the relevant environment variables to the value of their secret. This requires a bypass toggle since the charts secret currently requires the secret to be passed in through values.
@goshlanguage goshlanguage force-pushed the enable-configurable-extra-envs branch from 903b71b to 90c7a45 Compare December 3, 2021 16:51
@goshlanguage
Copy link
Contributor Author

all set, thank you

@yolossn yolossn merged commit f85f97f into flatcar:main Dec 5, 2021
@goshlanguage goshlanguage deleted the enable-configurable-extra-envs branch December 6, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants