Chart should allow specifying existing secrets

[josecv]

Right now, this helm chart generates all the secrets, unconditionally, from values in the values.yaml. For those of us who like to commit our values.yamls this is not ideal, as it requires that we commit our passwords, for example our S3 access key.

It seems like the best practice is to allow the user to supply an existingSecret (e.g. in the postgres chart).

Something similar could be done here -- for example for the registry, if registry.existingSecret is set, don't generate a secret and just trust the user to figure out what the secret should look like on their own (which allows them to self-manage the lifecycle of their passwords).

If there's interest, I can find some time to do this PR.

[josecv]

It looks like this would also mitigate #181

[ywk253100]

@josecv I think your requirement has been fixed in PR #126?

[josecv]

Hi @ywk253100 -- that's not exactly what I had in mind. That PR will let me specify the contents of the secret via values.yaml but what I'd like is for the chart to not generate any k8s secrets at all, and leave the user to create the secrets themselves.

For example, in registry-secret.yaml do something like:

{{- if (not .Values.registry.existingSecret) -}}
apiVersion: v1
kind: Secret
metadata:
  name: "{{ template "harbor.registry" . }}"
  labels:
{{ include "harbor.labels" . | indent 4 }}
type: Opaque
data:
   # etc
{{- end }}

and then registry-dpl.yaml:

# ...
        envFrom:
        - secretRef:
           {{- if (not .Values.registry.existingSecret) }}
              name: "{{ template "harbor.registry" . }}"
           {{- else }}
              name: "{{ .Values.registry.existingSecret }}"
           {{- end }}
# ...

core.secretName is already handled like that, I'd just like to extend that to all other secrets.

[ywk253100]

@josecv I understand what your requirement is.
But why do you commit the sensitive information? I think you should set them when deploying the chart.

[josecv]

So we usually like to make our CI system deploy to our kubernetes clusters, instead of manually running helm install/helm upgrade. Usually what this looks like is:

• An engineer sets up some secrets
• commits a values.yaml without any secrets
• the CI picks up the values.yaml and installs things to the cluster.
• Subsequent commits to the values.yaml (e.g. make resources bigger, scale up) trigger the CI.

This lets us use our github as the single source of truth for what's deployed in the cluster.

Making the chart not generate secrets could also let us forbid tiller from reading secrets, which means that we can set up our environments so that developers/CI can do helm operations, but only SREs can read credentials to certain systems.

[bmcustodio]

I'd like to second this request.

[ywk253100]

@josecv I don't think the #199 is a good solution for Clair and Notary. The secrets of Clair and Notary contains the configuration files which are part of source code of Harbor chart, they may change between different versions. If we allow users to configure their own secrets for Clair and Notary, that means the users must maintain the correct structure of these files, I think this is unreasonable and complicated.

[ywk253100]

And the reason why we put the configuration files of Clair and Notary is that these files contain database password and the password cannot be set via environment variable.

[bmcustodio]

@ywk253100 while I understand your point, the chart as it currently stands—and unless I am missing something—is not "usable" with a GitOps pipeline based on Flux, the Helm operator and Sealed Secrets in which you check your configuration as HelmRelease resources into source control (as the HelmRelease resource for Harbor would need to contain values which shouldn't be checked into source control). It would be great to find a solution which is "GitOps-friendly".

[bmcustodio]

And the reason why we put the configuration files of Clair and Notary is that these files contain database password and the password cannot be set via environment variable.

@ywk253100 I believe that for Notary this is possible using NOTARY_SERVER_STORAGE_DB_URL and NOTARY_SIGNER_STORAGE_DB_URL as per this. As for Clair, it seems not to be possible, but WDYT about...

1. Storing the password (only) in a secret, managed by the user and referenced in values.yaml.
2. Keeping the Clair configmap as-is (managed by the chart) but having ${PASSWORD} in place of the actual password.
3. Adding an init container to the Clair deployment that mounts the secret and configmap above, runs envsubst and places that in a volume shared with the actual Clair container so it is picked up when it starts?

[tux-00]

Any news here ? An existingSecret value for s3 credentials could be a great improvoment. As @bmcstdio said, with the actual chart we can't use GitOps as the secret is clear into the chart values file.

[hendrikhalkow]

What's the issue that this isn't moving forward? What can I do to help resolving this this issue?

[reasonerjt]

Hi,

Quite different from the situation in postgres chart mentioned in the OP, there are too many attributes in values.yaml of this chart may potentially be moved to secret and I don't think we have the bandwidth to maintain all the combination and answer questions regarding misconfigurations.

I believe there are ways to solve the problem without having to check in the secret keys to the repos, for example, render the values.yaml dynamically in the deployment time via an overlay.

[mmiller1]

Would a PR for a subset of the configuration options (excluding clair / trivy) be accepted? perhaps something along the lines of this format in values.yaml would be less confusing to end users and easier to support:

registry
  credentials:
     # overrides anything set for "password"
    passwordSecretName: secret
    passwordSecretKey: key    
...
database:
  external:
    # overrides anything set for "password"
    passwordSecretName: secret
    passwordSecretKey: key

I would gladly help contribute something towards a solution here. maintaining forks is not fun :(

[Bak3y]

At the very least setting existing secrets for external DB connections should be supported in values.yaml

[alfsch]

@ywk253100 while I understand your point, the chart as it currently stands—and unless I am missing something—is not "usable" with a GitOps pipeline based on Flux, the Helm operator and Sealed Secrets in which you check your configuration as HelmRelease resources into source control (as the HelmRelease resource for Harbor would need to contain values which shouldn't be checked into source control). It would be great to find a solution which is "GitOps-friendly".

Also it's absolutely not usable with ArgoCD GitOps, if you don't want to check in passwords!

[alfsch]

@josecv I understand what your requirement is.
But why do you commit the sensitive information? I think you should set them when deploying the chart.

I think he's doing GitOps and not ClickOps and don't want to fiddle around by deploying manually with helm.

[hendrikhalkow]

Dear maintainer team,

We would like to deploy Harbor via GitOps. This means, we commit what we want to deploy to a GitRepo. In our clusters runs some stuff like Argo CD Flux and a Helm Controller that fetches the repo and deploys whatever is in there to the cluster. Our repo content – a HelmRelease – would look like this.

To avoid storing secrets in our GitOps repos, we use tools like Sealed Secrets. It uses a CRD where you commit an encrypted password and Sealed Secrets decrypts it into a regular Kubernetes Secret, which is the one we want to reference from the HelmRelease mentioned above.

As we currently have no way of referencing our secrets, we would have to commit our secrets in cleartext into our repos, which is why we need this issue to be fixed. This issue blocks us from using GitOps for Harbor deployment and forces us back into use the command line via helm install.

Could you please, please implement this or would you accept a pull request when somebody from outside the team does it?

[wmgroot]

+1. We are having to fork this chart to use it when deploying with ArgoCD to avoid committing secrets into Git.
Most other charts I've worked with support this use case by allowing the user to provide a Secret outside of the chart.

See external-dns as an example that allows you to provide AWS credentials through an external Secret.
https://github.com/helm/charts/blob/master/stable/external-dns/values.yaml#L83-L86

[r8474]

+1. We are having to fork this chart to use it when deploying with ArgoCD to avoid committing secrets into Git.
Most other charts I've worked with support this use case by allowing the user to provide a Secret outside of the chart.

We have also forked the chart for the same reason. Would much rather be using the official release but committing secrets to git is a deal-breaker.

[Geethree]

I too hit this issue when attempting to figure out how to get the AWS credentials into the workload (this would be negated by IAM Role Service Accounts.. goharbor/harbor#12553).

Anyways.. personally I'm not a large fan of helm charts and didn't want to fork/host/vendor/maintain a modified chart just for this feature request.

So

Here is my hack for ArgoCD for those who are interested..

I added a new configManagementPlugin kustomized-helm to the argocd-cm config map

kind: ConfigMap
metadata:
  name: argocd-cm
data:
  configManagementPlugins: |
    - name: kustomized-helm
      init:
        command: ["/bin/sh", "-c"]
        args: ["helm dependency build"]
      generate:
        command: [sh, -c]
        args: ['helm template -f $HELM_VALUE_FILE $HELM_RELEASE_NAME . > helm-out.yaml && kustomize build']

When defining the Argo Application one can use this plugin like so..

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: harbor
spec:
  ...
  source:
    ...
    plugin:
      name: kustomized-helm
      env:
        - name: HELM_VALUE_FILE
          value: values.yaml
        - name: HELM_RELEASE_NAME
          value: harbor

Then.. we can "kustomize" the templated helm resources by leveraging the ArgoCD hooks to Skip applying the harbor-registry secret to the cluster with this patch

apiVersion: v1
kind: Secret
metadata:
  name: "harbor-registry"
  annotations:
    argocd.argoproj.io/hook: Skip

Kustomization.yml file..

patches:
  - patches/patch-registry-secret.yaml
resources:
  - helm-out.yaml

Then I added a harbor-registry Secret that populates the appropriate key=value fields as desired - I used a ExternalSecret, but any secret with the correct name and keys would work.

Fortunately, the harbor-registry secret is pretty simple. Your mileage may vary for larger more complex/tempalted secrets/configmaps.

Anyways, this works as expected.. just a little dirty ArgoCD specific hack.

[alfsch]

Thanx @Geethree for inspiration and this hack. But still I would prefer a clean solution in the helm chart.

[dajudge]

The rationale for this issue seems pretty clear given the increasing adoption of GitOps bast-practices where everything infrastructure is checked in.

There also have been numerous offers to contribute suitable patches.

So far there has been very little response from the maintainers.

Would it be possible to provide feedback regarding the chances of having this rather severe issue fixed that is known since May 2019?

[acobaugh]

Just to add a requirement here, in case it wasn't captured in a previous comment:

We should be able to specify not just external Secrets, but the key within those Secrets that contain the value we're looking for. The helm lookup function might be useful here, in there case of things like the notary database settings, which are a composite of multiple values.

The example use case is using a postgresql operator (in our case, zalando) to create the database. Zalando has the option of storing the username and password in a Secret with the keys username and password.

[sathieu]

Please don't use the lookup function. It doesn't work with helm template, and breaks ArgoCD (see argoproj/argo-cd#5202).

[sathieu]

@ywk253100 @reasonerjt @ninjadq Please review #199 by @josecv 🙏 . Allowing external secrets is a much-needed feature for gitops practices.

[j14s]

Please support this! For a stack that is so fundamental to so many of our environments....foundational...it is hard to believe this isn't supported.

signed: Another GitOps implementer

[acobaugh]

@j14s What gitops operator are you using? If flux, could you not use valuesFrom in your HelmRelease? That's what has worked for me.

[memogarcia]

Adding myself as yet another implementer.

Or even better, Harbor team, would you be open to a PR?

[florentinadolf]

+1 for non-clickOps ready chart.

[corinz]

So, has this work been declined? Because, I'm about to fork it as well to add just a few lines to allow for an external secret...

[sathieu]

@corinz If you fork, please create a PR...

[corinz]

@sathieu What about this work: #199

Looks like the PR went stale? Why no response from maintainers?

[sathieu]

@corinz

@sathieu What about this work: #199

Looks like the PR went stale? Why no response from maintainers?

At least this PR needs rebase: This branch has conflicts that must be resolved. But I agree that maintainers are not very responsive ...

[corinz]

@sathieu urghhh! OK, I will need to shelf this for now -- but no doubt I will need to come back. I will keep this on my radar

[akw123]

+1 this is a deal breaker for GitOps

[siegenthalerroger]

So this is now implemented for S3 (though I don't understand why Chart and Registry are seperate if the directory is shared). #1165

Hopefully it'll come for database, etc.

[florentinadolf]

Apparently this has been taken care of in #1179

[tomaszkrzyzanowski]

Hello!

I realized yesterday, that there is still problem with Notary database password.

The newest released chart (1.11.1) is still pushing the .Values.database.external.password as Notary DB password.

What do you think about reconstructing the Pod template and make use of interdependent envs?
https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/

As noted by @bmcustodio COMMENT it's possible to override the configuration with env variable.

It would look like:
templates/notary/notary-sever.yaml

containers:
    - name: notary-server
        (...)
      env:
        - name: MIGRATIONS_PATH
          value: migrations/server/postgresql
       {{- if .Values.database.external.existingSecret }}
        - name: POSTGRESQL_PASSWORD
          valueFrom:
            secretKeyRef:
              name: {{ .Values.database.external.existingSecret }}
              key: password
       {{- end }}
        - name: DB_URL
        {{- if .Values.database.external.existingSecret }}
          value: {{ template "harbor.database.notaryServer" . }}
        {{- else -}}
          valueFrom:
            secretKeyRef:
              name: {{ template "harbor.notary-server" . }}
              key: NOTARY_SERVER_DB_URL
       {{- end }}
        - name: NOTARY_SERVER_STORAGE_DB_URL
        {{- if .Values.database.external.existingSecret }}
          value: {{ template "harbor.database.notaryServer" . }}
        {{- else -}}
          valueFrom:
            secretKeyRef:
              name: {{ template "harbor.notary-server" . }}
              key: NOTARY_SERVER_DB_URL
       {{- end }}

templates/_helpers.tpl

{{- define "harbor.database.notaryServer" -}}
  {{- if .Values.database.external.existingSecret -}}
    postgres://{{ template "harbor.database.username" . }}:$(POSTGRESQL_PASSWORD)@{{ template "harbor.database.host" . }}:{{ template "harbor.database.port" . }}/{{ template "harbor.database.notaryServerDatabase" . }}?sslmode={{ template "harbor.database.sslmode" . }}
  {{- else -}}
    postgres://{{ template "harbor.database.username" . }}:{{ template "harbor.database.escapedRawPassword" . }}@{{ template "harbor.database.host" . }}:{{ template "harbor.database.port" . }}/{{ template "harbor.database.notaryServerDatabase" . }}?sslmode={{ template "harbor.database.sslmode" . }}
  {{- end -}}
{{- end -}}

As far as I know NOTARY_SERVER_STORAGE_DB_URL should work both with notary db configuration and migrations as well as this env should override config file configuration

Let me know about your thoughts - I can prepare PR with required changes

The changes are working on my local fork - both signer and server are starting without connection issues. 😅

FYI: @zyyw

[PrivatePuffin]

Please don't use the lookup function. It doesn't work with helm template, and breaks ArgoCD (see argoproj/argo-cd#5202).

Helm lookup works fine with helm-template, it's just that the context of helm-template would be "nothing", hence the output of helm-lookup is "nothing". This can be solved by things like dryruns just fine.

About ArgoCD:
If a company singlehandly decides to go against(!) Helm standards by writhing their own sub-par parser, it's their bug to fix. It's not for helm-chart creators to compensate for the FUBAR-parser of ArgoCD. Considering FluxCD can handle things just fine.

[sathieu]

About ArgoCD: If a company singlehandly decides to go against(!) Helm standards by writhing their own sub-par parser, it's their bug to fix. It's not for helm-chart creators to compensate for the FUBAR-parser of ArgoCD. Considering FluxCD can handle things just fine.

ArgoCD has no sub-par parser. It's calling helm template. It's not calling helm install for security reasons (the helm template is done in a sandboxed pod, with no access to k8s API).

NB: I'm not an ArgoCD dev.

[siegenthalerroger]

@sathieu how do other charts solve this issue? Because just saying "don't use X" isn't helpful obviously ;)

Also funny enough that FluxCD has no security issues with this though that's probably because they completely reimplemented the helm functionality afaik

[sathieu]

@siegenthalerroger

how do other charts solve this issue

With existingSecret.

Also funny enough that FluxCD has no security issues with this though that's probably because they completely reimplemented the helm functionality afaik

They have not reimplemented it, but use Helm SDK. About security issues, there is always a trade-off between security and features. Here Flux has one more feature (access to k8s resources with the lookup function), with probably reduced security.

https://github.com/fluxcd/helm-controller#helm-controller

[MinerYang]

Right now, this helm chart generates all the secrets, unconditionally, from values in the values.yaml. For those of us who like to commit our values.yamls this is not ideal, as it requires that we commit our passwords, for example our S3 access key.

It seems like the best practice is to allow the user to supply an existingSecret (e.g. in the postgres chart).

Something similar could be done here -- for example for the registry, if registry.existingSecret is set, don't generate a secret and just trust the user to figure out what the secret should look like on their own (which allows them to self-manage the lifecycle of their passwords).

If there's interest, I can find some time to do this PR.

Since the existing secrets has been supported in the recent latest release. Will close this issue for now.
Thanks all of you and feel free to leave a comments/open a issue if there's still a problem.

Best,
Miner

[PrivatePuffin]

ArgoCD has no sub-par parser. It's calling helm template. It's not calling helm install for security reasons (the helm template is done in a sandboxed pod, with no access to k8s API).

I would call that non-standard sub-par parsing.
Using template without context for a use-case it isn't meant for.

Here Flux has one more feature (access to k8s resources with the lookup function), with probably reduced security.

While I agree about the reduced security, there is more than one area where argocd helm functions differently than native Helm.
Afaik there is a page about that on there website somewhere, but i'm honestly too lazy to look it up again