Ability to add environment variables to helm-core container #1147

PhilipNelson5 · 2022-02-16T18:08:53Z

I am configuring OIDC via the environment variable CONFIG_OVERWRITE_JSON. However, the helm chart does not expose a place to add this environment variable. This feature modifies the helm chart to allow users to add additional environment variables to the core deployment container so they can specify CONFIG_OVERWRITE_JSON.

With this addition, a values file like this can be used to configure the user settings via an environment variable

core:
  containerEnv:
    - name: CONFIG_OVERWRITE_JSON
      value: >
        {
          "auth_mode": "oidc_auth",
          "oidc_name": "Dex",
          "oidc_endpoint": "https://.../dex",
...
        }

The environment variable configuration feature was proposed here goharbor/community#158, implemented here goharbor/harbor#14777, and released as part of harbor v2.3.0

zyyw · 2022-02-25T07:31:08Z

values.yaml

@@ -450,6 +450,8 @@ core:
  affinity: {}
  ## Additional deployment annotations
  podAnnotations: {}
+  ## Additional environment variables
+  containerEnv: []


should this be containerEnv: {}?

I think it should be a list based on the Kubernetes API docs

Field Description

env
EnvVar array
patch strategy: merge
patch merge key: name List of environment variables to set in the container. Cannot be updated.

by the way it is injected, wonder how array or map would fit into the listentry expected having

- name: value:

lemme suggest:

{{- if .Values.core.containerEnv }} - name: CONFIG_OVERWRITE_JSON value: {{ .Values.core.containerEnv }} {{- end }}

thereby .Values.core.containerEnv is expected string, otherwise you would probably need yaml2json.

I think this is a good idea. In this case I'd suggest changing the name to something like .Values.core.configOverwriteJson or .Values.core.configureUserSettings. Your change limits this feature to only modifying the CONFIG_OVERWRITE_JSON environment variable instead of allowing arbitrary environment variables to be injected. This might be a good thing.

I don't know of any other environment variables someone would want to set. If there are multiple environment variables that make sense to set, then my way makes more sense. If CONFIG_OVERWRITE_JSON is the only one, then your change makes more sense. I'm happy to use your suggestion. @janesser @zyyw what do you think?

zyyw · 2022-02-25T08:34:16Z

I'm afraid we may not expose this in harbor helm. You may use Post Rendering to manifest of your choice.

PhilipNelson5 · 2022-02-25T20:26:22Z

I know this is not exposed in harbor helm. The purpose of this pull request is to create a place to add environment variables.

PhilipNelson5 · 2022-03-16T17:38:38Z

What is the status of reviewing this feature? Is there something else that needs to be addressed?

I don't mean to be annoying if it's still in progress, I'm not sure what kind of timeline to expect, this is my first time contributing.

PhilipNelson5 · 2022-03-22T17:39:59Z

@zyyw are you able to look at this again? Is there someone else if you are too busy? Thanks!

janesser · 2022-03-23T11:30:19Z

@zyyw @PhilipNelson5 coming here having the need to configure harbor for oidc via helm

looking into present concern, shifting the env-vars from cm to secret might give it better security (at least obfuscation).

What do you think?

janesser

injection of helm values in deployment.yaml needs adjustment

@PhilipNelson5 any rough idea when you will continue here, otherwise i can propose to fork-the-fork.

PhilipNelson5 · 2022-03-23T14:41:52Z

@janesser is this what you are thinking? PhilipNelson5@5131b29

janesser · 2022-03-23T16:34:22Z

Absolutely! Last but not least the value could go into a secret, which is ref'd like few lines above.

@janesser is this what you are thinking? PhilipNelson5@5131b29

PhilipNelson5 · 2022-03-23T16:45:16Z

Isn't this already possible with the current state of this branch with something like this?

core:
  containerEnv:
    - name: CONFIG_OVERWRITE_JSON
        valueFrom:
          secretKeyRef:
            name: harborSecrets
            key: configureUserSettings

janesser · 2022-03-23T18:09:13Z

Isn't this already possible with the current state of this branch with something like this?

core:
  containerEnv:
    - name: CONFIG_OVERWRITE_JSON
        valueFrom:
          secretKeyRef:
            name: harborSecrets
            key: configureUserSettings

i guess it requires a little extra magic over here: https://github.com/goharbor/harbor-helm/blob/master/templates/core/core-secret.yaml

PhilipNelson5 · 2022-03-23T18:44:48Z

It seems like the current version is more flexible. Is your intent to lock it down and force users to put the CONFIG_OVERWRITE_JSON string in a secret? I'm not opposed to that, just trying to get on the same page and figure out what would be the best solution going forward.

janesser · 2022-03-23T19:16:26Z

@PhilipNelson5 in case of oidc secrets and most other use-cases we have credentials in hand - so yeah, don't offer bad practice like having credentials in configmaps.

PhilipNelson5 · 2022-03-23T19:35:42Z

@janesser, ok I'm onboard. So the core-dpl.yaml would look like this?

{{- if .Values.core.configureUserSettings }}
          - name: CONFIG_OVERWRITE_JSON
            valueFrom:
              secretKeyRef:
                name: "{{ .Values.core.configureUserSettings }}"
                key: CONFIG_OVERWRITE_JSON
{{- end }}

If you want to supply user configuration then you create a secret

apiVersion: v1
kind: Secret
metadata:
  name: myHarborSecret
type: Opaque
data:
  CONFIG_OVERWRITE_JSON: eyAiYXV0aF9tb2RlIjogIm9pZGNfYXV0aCIsICJvaWRjX25hbWUiOiAiRGV4IiwgIm9pZGNfZW5kcG9pbnQiOiAiaHR0cHM6Ly8uLi4vZGV4IiwgIm9pZGNfY2xpZW50X2lkIjogImRleC1jb250cm9sbGVyLWhhcmJvci1jbGllbnQiLCAib2lkY19jbGllbnRfc2VjcmV0IjogInBhc3N3b3JkIiwgIm9pZGNfc2NvcGUiOiAib3BlbmlkLG9mZmxpbmVfYWNjZXNzLHByb2ZpbGUsZW1haWwiLCAib2lkY192ZXJpZnlfY2VydCI6ICJ0cnVlIiB9Cg==

Then put the name of the secret into the helm values

core:
  configureUserSettings: myHarborSecret

Does this align with what you are thinking?

janesser · 2022-03-24T09:07:20Z

@PhilipNelson5 We are close ;-)

If you would append a line such as

CONFIG_OVERWRITE_JSON: {{ .Values.core.configureUserSettings }}

to https://github.com/goharbor/harbor-helm/blob/master/templates/core/core-secret.yaml

{{- if .Values.core.configureUserSettings }}
         - name: CONFIG_OVERWRITE_JSON
            valueFrom:
            secretKeyRef:
                name: {{ template "harbor.core" . }}
                key: CONFIG_OVERWRITE_JSON
{{- end }}

to the deploy.yaml refering the harbor-std-secret.

PhilipNelson5 · 2022-03-24T13:50:35Z

CONFIG_OVERWRITE_JSON: {{ .Values.core.configureUserSettings }}

Do we want .Values.core.configureUserSettings to already be base64 encoded or do we want to add this instead

CONFIG_OVERWRITE_JSON: {{ .Values.core.configureUserSettings | b64enc | quote }}

janesser · 2022-03-24T15:29:42Z

@PhilipNelson5 You have a point about b64enc. Just like you proposed, looks perfectly fine.

BTW you could refresh the PR - depending on your branching, just push to https://github.com/PhilipNelson5/harbor-helm/tree/core-env-vars again.

PhilipNelson5 · 2022-03-24T15:54:50Z

@janesser of course. I was just waiting to hear about the b64enc to update the PR. I think this resolves everything we've talked about.

janesser

Reviewed latest PR with all improvements. Looks good to me.

@zyyw Please have a look.

janesser · 2022-04-01T08:39:15Z

@zyyw hope your points were covered in a best-practice fashion.

any idea when you will be able to review this?

mac-chaffee · 2022-04-02T00:35:52Z

templates/core/core-dpl.yaml

@@ -85,6 +85,13 @@ spec:
              secretKeyRef:
                name: "{{ template "harbor.jobservice" . }}"
                key: JOBSERVICE_SECRET
+{{- if .Values.core.configureUserSettings }}
+          - name: CONFIG_OVERWRITE_JSON


Thanks for doing this! I literally just now ran into the same issue that this PR solves!

Since we set envFrom: - secretRef:, I think that means CONFIG_OVERWRITE_JSON is always set as an environment variable for the container. So wouldn't this block be unneeded?

Would that also mean we need to add {{- if .Values.core.configureUserSettings }} around the environment variable in the Secret? That way the environment variable isn't automatically set (by envFrom) even when the value is empty.

Glad this will help!

Correct, we only want to define CONFIG_OVERWRITE_JSON if .Values.core.configureUserSettings is set. We need to add the if statement in the core secret.

Since we set envFrom: - secretRef:, I think that means CONFIG_OVERWRITE_JSON is always set as an environment variable for the container. So wouldn't this block be unneeded?

I don't understand what you mean here. Why is CONFIG_OVERWRITE_JSON always set making this block unneeded?

"envFrom" works by taking every key in the Secret and automatically inserting it as an environment variable in the container: https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables

So no need to explicitly include the env var here; it is already implicitly included by "envFrom".

Thanks for doing this! I literally just now ran into the same issue that this PR solves!

Since we set envFrom: - secretRef:, I think that means CONFIG_OVERWRITE_JSON is always set as an environment variable for the container. So wouldn't this block be unneeded?

Would that also mean we need to add {{- if .Values.core.configureUserSettings }} around the environment variable in the Secret? That way the environment variable isn't automatically set (by envFrom) even when the value is empty.

The JSON may contain credentials, see above remarks. Preferably let's keep it "secret".

"envFrom" works by taking every key in the Secret and automatically inserting it as an environment variable in the container: https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables

So no need to explicitly include the env var here; it is already implicitly included by "envFrom".

@mac-chaffee I understand now, you're talking about lines 72-76. I'll remove the explicit inclusion of the environment variable and add the missing if statement to the core secret.

The JSON may contain credentials, see above remarks. Preferably let's keep it "secret".

@janesser I think your concerns about the secret are still taken care of. The config will stay in the core secret. We just don't need to be explicitly including it in the core deployment. This is handled by the envFrom line linked above

PhilipNelson5 · 2022-04-06T14:26:25Z

@reasonerjt or @ywk253100, we've been waiting on a review from @zyyw for many days without hearing from them. Do either of you know what's up? Would you be able to take a look at this PR? We'd super appreciate it!

janesser · 2022-05-29T19:47:29Z

@zyyw don't forget about us here. thanks in advance.

BulatSaif · 2022-06-27T06:11:02Z

@zyyw, can we merge this PR? I am deploying new harbor server and would like to have this feature.

BulatSaif · 2022-06-28T12:27:29Z

My current workaround is to use bitnami helm chart:
bitnami/charts#9534

taemon1337 · 2022-08-10T12:33:06Z

I would also like to use this feature

PhilipNelson5 · 2022-08-31T20:40:02Z

@zyyw I resolved the conflicts with master. Will you please review this PR?

PhilipNelson5 · 2022-09-02T05:17:33Z

@ywk253100 it's been months without hearing from @zyyw. I've recently updated this PR so it is up to date with master. Can we get some feedback or timeline on when to expect a review? Thanks in advance!

PhilipNelson5 · 2022-09-13T15:50:59Z

@reasonerjt maybe you can help us move this PR forward. I'm not sure what happened but we are ready for a review and revision or approval. If you could help us we would be very thankful!

Coo-ops · 2023-03-30T13:05:16Z

@zyyw bump, need this.

The JSON string set in core.configureUserSettings string will be added to the core secret and loaded into the environment variable CONFIG_OVERWRITE_JSON Signed-off-by: Philip Nelson <philipnelson5@gmail.com>

Vad1mo

I am in to merge this PR,

@zyyw @stonezdj @reasonerjt, we should reconsider our decision to not support CONFIG_OVERWRITE_JSON in the default workflow. Abusing Helm's post render capabilities is IMHO the wrong approach to usability.

igor-nikiforov · 2023-05-15T17:11:54Z

@Vad1mo sorry for ping. Any timeline when this PR will be merged? It is really pain to use this chart without CONFIG_OVERWRITE_JSON.

The JSON string set in core.configureUserSettings string will be added to the core secret and loaded into the environment variable CONFIG_OVERWRITE_JSON Signed-off-by: Philip Nelson <philipnelson5@gmail.com> Signed-off-by: Raul Garcia Sanchez <info@raulgarcia.de>

PhilipNelson5 force-pushed the core-env-vars branch 3 times, most recently from df301fc to 9bd223f Compare February 16, 2022 18:43

zyyw reviewed Feb 25, 2022

View reviewed changes

PhilipNelson5 requested a review from zyyw February 28, 2022 17:49

janesser suggested changes Mar 23, 2022

View reviewed changes

janesser approved these changes Mar 25, 2022

View reviewed changes

mac-chaffee reviewed Apr 2, 2022

View reviewed changes

mac-chaffee approved these changes Apr 4, 2022

View reviewed changes

jimsnab approved these changes Jun 8, 2022

View reviewed changes

Allow CONFIG_OVERWRITE_JSON to be set in values file

5000171

The JSON string set in core.configureUserSettings string will be added to the core secret and loaded into the environment variable CONFIG_OVERWRITE_JSON Signed-off-by: Philip Nelson <philipnelson5@gmail.com>

PhilipNelson5 force-pushed the core-env-vars branch from 5950300 to 5000171 Compare April 14, 2023 14:51

Vad1mo approved these changes Apr 14, 2023

View reviewed changes

zyyw approved these changes May 18, 2023

View reviewed changes

Vad1mo merged commit ab98a78 into goharbor:master May 19, 2023

PhilipNelson5 deleted the core-env-vars branch May 19, 2023 19:42

zyyw added the target/1.13.0 label May 22, 2023

zyyw mentioned this pull request Jul 26, 2023

Harbor core configuration managed by helm's triggered Job #1544

Closed

Ability to add environment variables to helm-core container #1147

Ability to add environment variables to helm-core container #1147

Conversation

PhilipNelson5 commented Feb 16, 2022

zyyw Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

PhilipNelson5 Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

janesser Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

PhilipNelson5 Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

zyyw commented Feb 25, 2022

PhilipNelson5 commented Feb 25, 2022

PhilipNelson5 commented Mar 16, 2022

PhilipNelson5 commented Mar 22, 2022

janesser commented Mar 23, 2022

janesser left a comment • edited Loading

Choose a reason for hiding this comment

PhilipNelson5 commented Mar 23, 2022 • edited Loading

janesser commented Mar 23, 2022

PhilipNelson5 commented Mar 23, 2022

janesser commented Mar 23, 2022

PhilipNelson5 commented Mar 23, 2022

janesser commented Mar 23, 2022

PhilipNelson5 commented Mar 23, 2022

janesser commented Mar 24, 2022

PhilipNelson5 commented Mar 24, 2022

janesser commented Mar 24, 2022

PhilipNelson5 commented Mar 24, 2022

janesser left a comment • edited Loading

Choose a reason for hiding this comment

janesser commented Apr 1, 2022

mac-chaffee Apr 2, 2022 • edited Loading

Choose a reason for hiding this comment

PhilipNelson5 Apr 3, 2022

Choose a reason for hiding this comment

mac-chaffee Apr 3, 2022

Choose a reason for hiding this comment

janesser Apr 4, 2022

Choose a reason for hiding this comment

PhilipNelson5 Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

PhilipNelson5 commented Apr 6, 2022

janesser commented May 29, 2022

BulatSaif commented Jun 27, 2022 • edited Loading

BulatSaif commented Jun 28, 2022

taemon1337 commented Aug 10, 2022

PhilipNelson5 commented Aug 31, 2022

PhilipNelson5 commented Sep 2, 2022

PhilipNelson5 commented Sep 13, 2022

Coo-ops commented Mar 30, 2023

Vad1mo left a comment • edited Loading

Choose a reason for hiding this comment

igor-nikiforov commented May 15, 2023

zyyw Feb 25, 2022 •

edited

Loading

PhilipNelson5 Feb 25, 2022 •

edited

Loading

janesser Mar 23, 2022 •

edited

Loading

PhilipNelson5 Mar 23, 2022 •

edited

Loading

janesser left a comment •

edited

Loading

PhilipNelson5 commented Mar 23, 2022 •

edited

Loading

janesser left a comment •

edited

Loading

mac-chaffee Apr 2, 2022 •

edited

Loading

PhilipNelson5 Apr 4, 2022 •

edited

Loading

BulatSaif commented Jun 27, 2022 •

edited

Loading

Vad1mo left a comment •

edited

Loading