Allow injection of env vars in any deployment by valentintorikian · Pull Request #120 · StackStorm/stackstorm-k8s

valentintorikian · 2020-03-10T15:51:55Z

This allows the user to inject any custom env variable in any deployment.

The need was the ability to configure a proxy so that st2 pack install doesn't fail when behind said proxy. The env variable needed to be injected in the st2actionrunner deployments.

There is a BC break on st2chatops configuration, if secrets are provided, they need to be added to secrets.st2.st2chatops.env instead of st2chatops.env.

This should also closes #119 and partially solves #14.

linked to StackStorm#119 StackStorm#14

mickmcgrath13 · 2020-03-10T16:10:00Z

values.yaml

 # TODO: Alternatively as part of reorganizing Helm values, consider moving values to existing `st2` and `st2web` sections ? (#14)
 secrets:
  st2:
+    st2chatops:


Any reason this is secrets.st2.st2chatops instead of secrets.st2chatops (i.e. why the nesting under st2)?

Also, any reason not to put the secrets specific to st2chatops under st2chatops (and the same for all other services) instead of aggregating all secrets to a central place?

i.e. instead of

secrets: st2: st2chatops: foo: bar

do

st2chatops: secrets: foo: bar

?

Any reason this is secrets.st2.st2chatops instead of secrets.st2chatops (i.e. why the nesting under st2)?

For this part, no particular reason, it's true that it's a bit too verbose.

Also, any reason not to put the secrets specific to st2chatops under st2chatops (and the same for all other services) instead of aggregating all secrets to a central place?

See issue #14 :)

Makes sense. I think it'd be preferable to spread the secrets out into their respective services, but we can tackle that when we tackle #14

Thanks!

I agree that secrets.st2.st2chatops looks too verbose and even counter-intuitive.

This also raises some inconsistencies in the context of #119 discussion. I'm thinking about exploring the alternatives for the Helm values definition.

mickmcgrath13

The changes look good to me. I haven't tested them, though..

arm4b

I absolutely like an idea of adding env for every new service, - that's a great addition! 👍

However secrets.st2.st2chatops is something that worries me 👎 in this PR and uncovers some inconsistencies described in issues like #14 #119.

arm4b · 2020-03-10T23:41:45Z

values.yaml

 # TODO: Alternatively as part of reorganizing Helm values, consider moving values to existing `st2` and `st2web` sections ? (#14)
 secrets:
  st2:
+    st2chatops:


I agree that secrets.st2.st2chatops looks too verbose and even counter-intuitive.

This also raises some inconsistencies in the context of #119 discussion. I'm thinking about exploring the alternatives for the Helm values definition.

arm4b · 2020-06-26T09:49:34Z

templates/secrets_st2chatops.yaml

 type: Opaque
 data:
-{{- range $env, $value := .Values.st2chatops.env }}
+{{- range $env, $value := .Values.secrets.st2.st2chatops.env }}


I think there is at least an agreement about the 💯 need of raw ENV for every Pod and that's super helpful.
Can we keep raw ENV functionality in this PR to be able to merge it asap, but split the st2chatops secrets ENV enhancement into another PR?

cognifloyd

Awesome start!

I think we might also want to add an env block to the jobs (all the jobs would share the same block).
In values.yaml, it would be something like:

jobs:
  env: {}
  # HTTP_PROXY: http://proxy:1234

cognifloyd · 2021-06-18T02:20:26Z

templates/deployments.yaml

+        {{- if .Values.st2auth.env }}
+        env:
+        {{- range $env, $value := .Values.st2auth.env }}
+        - name: {{ $env | quote }}
+          value: {{ $value | quote }}
+        {{- end }}
+        {{- end }}


I think this could be extracted into a helper to avoid some repetition.
So, here it would have:

{{ include "custom-env" .Values.st2auth | indent 8 }}

And then the template would be:

{{- define "custom-env" -}} {{- if .env }} env: {{- range $env, $value := .env }} - name: {{ $env | quote }} value: {{ $value | quote }} {{- end }} {{- end }} {{- end -}}

cognifloyd · 2021-06-18T02:22:22Z

templates/deployments.yaml

+        {{- if .env }}
+        env:
+        {{- range $env, $value := .env }}
+        - name: {{ $env | quote }}
+          value: {{ $value | quote }}
+        {{- end }}
+        {{- end }}


And the template here would just be:

{{ include "custom-env" . | indent 8 }}

cognifloyd · 2021-06-18T02:25:15Z

templates/deployments.yaml

+{{- range $env, $value := .Values.st2chatops.env }}
+        - name: {{ $env | quote }}
+          value: {{ $value | quote }}
+{{- end }}


I think I would drop this, and leave the st2chatops env as is (in envFrom[1].secretRef below).

Suggested change

{{- range $env, $value := .Values.st2chatops.env }}

- name: {{ $env | quote }}

value: {{ $value | quote }}

{{- end }}

cognifloyd · 2021-06-18T02:28:08Z

templates/deployments.yaml

+{{- range $env, $value := .Values.st2client.env }}
+        - name: {{ $env | quote }}
+          value: {{ $value | quote }}
+{{- end }}


A couple of jobs also have an env: block already. So, we could split the template into two to accommodate this usage.

{{- define "custom-env-entries" -}} {{- range $env, $value := .env }} - name: {{ $env | quote }} value: {{ $value | quote }} {{- end }} {{- end -}} {{- define "custom-env" -}} {{- if .env }} env: {{ include "custom-env-entries" . }} {{- end }} {{- end -}}

Which would make this block be:

{{ include "custom-env-entries" .Values.st2client }}

templates/secrets_st2chatops.yaml

values.yaml

cognifloyd · 2021-06-18T02:30:57Z

values.yaml

+        env: {}
+        # HTTP_PROXY: http://proxy:1234


Nice. I like the commented example. 👍

values.yaml

cognifloyd · 2021-09-09T03:51:27Z

I want to clear out old PRs by merging them if possible.
So, I just merged in master and made a few updates to address all the comments here.

I think a Changelog entry is the only thing left before merging this. @angrydeveloper what do you think of these changes?

arm4b

The changes look really helpful! 👍

Thanks @cognifloyd. I think it's also OK to inject the changelog as well to merge it.

Allow injection of env vars in any deployment

c994ed6

linked to StackStorm#119 StackStorm#14

pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Mar 10, 2020

mickmcgrath13 reviewed Mar 10, 2020

View reviewed changes

mickmcgrath13 approved these changes Mar 10, 2020

View reviewed changes

arm4b reviewed Mar 10, 2020

View reviewed changes

arm4b added the enhancement New feature or request label Mar 10, 2020

arm4b suggested changes Jun 26, 2020

View reviewed changes

cognifloyd requested changes Jun 18, 2021

View reviewed changes

cognifloyd added 2 commits September 8, 2021 21:50

Merge branch 'master' into env-injection

72ce516

DRY customEnv definition using a _helper

8388b0b

pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Sep 9, 2021

add customEnv for jobs

7f5ec95

cognifloyd self-requested a review September 9, 2021 03:31

arm4b approved these changes Sep 9, 2021

View reviewed changes

cognifloyd added 2 commits September 9, 2021 12:46

changelog entry

3ced2e4

Merge branch 'master' into env-injection

291d37d

cognifloyd approved these changes Sep 12, 2021

View reviewed changes

cognifloyd enabled auto-merge September 12, 2021 18:00

cognifloyd merged commit 718d769 into StackStorm:master Sep 12, 2021

Uh oh!

Conversation

valentintorikian commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickmcgrath13 Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentintorikian Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arm4b Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickmcgrath13 left a comment

Choose a reason for hiding this comment

Uh oh!

arm4b left a comment

Choose a reason for hiding this comment

Uh oh!

arm4b Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cognifloyd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cognifloyd commented Sep 9, 2021

Uh oh!

arm4b left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

valentintorikian commented Mar 10, 2020 •

edited

Loading

mickmcgrath13 Mar 10, 2020 •

edited

Loading

valentintorikian Mar 10, 2020 •

edited

Loading

arm4b Mar 10, 2020 •

edited

Loading

arm4b Mar 10, 2020 •

edited

Loading

cognifloyd left a comment •

edited

Loading