harbor-helm icon indicating copy to clipboard operation
harbor-helm copied to clipboard

Helm Chart 1.13.1: Error with 'existingSecret' for External Redis Database

Open pierreblais opened this issue 9 months ago • 14 comments

Issue Description

I have encountered a regression in the Helm Chart with the latest patch (1.13.1). Specifically, when attempting to provide an existingSecret for an external Redis database, the following error occurs:

$ helm template my-release harbor/harbor --values helm-values.yaml -n harbor --debug --version 1.13.1
install.go:194: [debug] Original chart version: "1.13.1"
install.go:211: [debug] CHART PATH: /home/vscode/.cache/helm/repository/harbor-1.13.1.tgz


Error: template: harbor/templates/trivy/trivy-sts.yaml:26:28: executing "harbor/templates/trivy/trivy-sts.yaml" at <include (print $.Template.BasePath "/trivy/trivy-secret.yaml") .>: error calling include: template: harbor/templates/trivy/trivy-secret.yaml:10:15: executing "harbor/templates/trivy/trivy-secret.yaml" at <include "harbor.redis.urlForTrivy" .>: error calling include: template: harbor/templates/_helpers.tpl:198:48: executing "harbor.redis.urlForTrivy" at <include "harbor.redis.url" $>: error calling include: template: harbor/templates/_helpers.tpl:166:64: executing "harbor.redis.url" at <include "harbor.redis.cred" $>: error calling include: template: harbor/templates/_helpers.tpl:155:25: executing "harbor.redis.cred" at <include "harbor.redis.pwdfromsecret" $>: error calling include: template: harbor/templates/_helpers.tpl:149:56: executing "harbor.redis.pwdfromsecret" at <.Values.redis.external.existingSecret>: nil pointer evaluating interface {}.REDIS_PASSWORD
helm.go:84: [debug] template: harbor/templates/trivy/trivy-sts.yaml:26:28: executing "harbor/templates/trivy/trivy-sts.yaml" at <include (print $.Template.BasePath "/trivy/trivy-secret.yaml") .>: error calling include: template: harbor/templates/trivy/trivy-secret.yaml:10:15: executing "harbor/templates/trivy/trivy-secret.yaml" at <include "harbor.redis.urlForTrivy" .>: error calling include: template: harbor/templates/_helpers.tpl:198:48: executing "harbor.redis.urlForTrivy" at <include "harbor.redis.url" $>: error calling include: template: harbor/templates/_helpers.tpl:166:64: executing "harbor.redis.url" at <include "harbor.redis.cred" $>: error calling include: template: harbor/templates/_helpers.tpl:155:25: executing "harbor.redis.cred" at <include "harbor.redis.pwdfromsecret" $>: error calling include: template: harbor/templates/_helpers.tpl:149:56: executing "harbor.redis.pwdfromsecret" at <.Values.redis.external.existingSecret>: nil pointer evaluating interface {}.REDIS_PASSWORD

This issue is not present in version 1.13.0 or any earlier release.

Step to Reproduce

  1. Install Harbor Helm Chart version 1.13.1.
  2. Attempt to provide an existingSecret for an external Redis database.
  3. Observe the error mentioned above while trying to deployed.

Expected Behavior

Providing an existingSecret for an external Redis database should work without errors, as it did in version 1.13.0 and earlier.

Environment

  • Helm Chart Version: 1.13.1
  • Kubernetes Version: v1.27.7
  • Helm CLI Version: v3.11.3

Additional Information

My exact values.yaml file:

database:
  type: external
  external:
    host: "<PG_IP>"
    port: 5432
    existingSecret: "harbor"
redis:
  type: external
  external:
    addr: 'redis-node-0.redis-headless.redis.svc.cluster.local:26379,redis-node-1.redis-headless.redis.svc.cluster.local:26379,redis-node-2.redis-headless.redis.svc.cluster.local:26379'
    sentinelMasterSet: 'redis-node-0.redis-headless.redis.svc.cluster.local:26379'
    existingSecret: "harbor-redis"
persistence:
  persistentVolumeClaim:
    registry:
      storageClass: harbor-storage
      accessMode: ReadWriteMany
    trivy:
      storageClass: harbor-storage
      accessMode: ReadWriteMany
    jobservice:
      jobLog:
        storageClass: harbor-storage
        accessMode: ReadWriteMany

Error Source

Maybe coming from commit 7b6501a from branch 1.13.0 especially from file templates/_helpers.tpl (https://github.com/goharbor/harbor-helm/commit/7b6501aea90ef3eb96f2f5819bb9f1b9703180a2)

Thank you for your attention to this matter! Let me know if you need any further information.

pierreblais avatar Nov 20 '23 14:11 pierreblais

I understand what's going on !

As we can see in the lines added in that commit: https://github.com/goharbor/harbor-helm/commit/7b6501aea90ef3eb96f2f5819bb9f1b9703180a2 the lookup function is used to discover the . Values.redis.external.existingSecret secret.

In my context, I'm using ArgoCD to deployed the Harbor Helm Chart. Here is how ArgoCD deployed a Helm Chart : helm template ... | kubectl apply -f - for security purpose the lookup function is disabled with helm template or helm install --dry-run, more information here also here is an interesting discussion with Helm community about that subject here. If you following well, you understand that deploying Harbor with the Helm Chart using existingSecret and ArgoCD to deployed aren't compatible.

I suggest to enhance source code and to make it Harbor Helm reliable with ArgoCD an harmonisation of the way to reach existingSecret and using the model used for external PostgreSQL. The reference to the secret should be inside a ConfigMap and not with the fancy lookup function. But This solution is quite hard to develop because the redis password is filled inside the addr string that is send to the Core ConfigMap, and this action is done but the tpl file. A solution could be to give directly the addr string from a secret with an option like existingAddr !

pierreblais avatar Nov 27 '23 18:11 pierreblais

it seem that you can specifiy username and password in values.yaml as .Values.redis.external.existingSecret does work with helm template ... | kubectl apply -f -. Alternatively, we are curious why you do not use helm install in the ArgoCD.

zyyw avatar Nov 30 '23 08:11 zyyw

besides,it is a limitation from helm template when you are using existing secret. I am afraid we can not do nay enhancement for this currently

Keep in mind that Helm is not supposed to contact the Kubernetes API Server during a helm template or a helm install|upgrade|delete|rollback --dry-run, so the lookup function will return an empty list (i.e. dict) in such a case.

https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-lookup-function

MinerYang avatar Nov 30 '23 08:11 MinerYang

it seem that you can specifiy username and password in values.yaml as .Values.redis.external.existingSecret does work with helm template ... | kubectl apply -f -. Alternatively, we are curious why you do not use helm install in the ArgoCD.

The things is I don't want to pass secrets in the values file because I'm using GitOps way with ArgoCD so I'm using the field .Values.redis.external.existingSecret. But we can see that this field consists of a call to lookup function in file templates/_helpers.tpl line 149. About ArgoCD, I can't choice which command I want to use behind. The way of working of ArgoCD is helm template ... | kubectl apply -f -.

besides,it is a limitation from helm template when you are using existing secret. I am afraid we can not do nay enhancement for this currently

Keep in mind that Helm is not supposed to contact the Kubernetes API Server during a helm template or a helm install|upgrade|delete|rollback --dry-run, so the lookup function will return an empty list (i.e. dict) in such a case.

https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-lookup-function

I think they are one possible enhancement as I say, using a ref to secret to pass the full Redis Addr, and this secret is mounted as env var in the ConfigMap core (exactly like .Values.databse. existingSecret). Something like:

values.yaml

redis:
  external:
    # If using addrFromSecret, the key must be REDIS_ADDR
    addrFromSecret: ""

I'm not even sure that is possible because the templating add a url path for both _REDIS_URL_CORE and _REDIS_URL_REG.

I'm not a huge fan of this solution, for me the problem come from both:

  • Helm because there are way to test the lookup function without deploying, there should be an option to debug with only read call to the API
  • ArgoCD should be compatible with helm templating function or Cleary indicate that this function isn't supported. I've seen an issue about that on the ArgoCD project but I can't find it anymore.

pierreblais avatar Nov 30 '23 09:11 pierreblais

The issue is that lookup function will

  • Return the object during install/upgrade
  • Return empty object if object does not exist OR either helm template or --dry-run option is used

The solution is that the chart should be prepared for the possibility that the return of the lookup may be empty (simply password should be treated as "").

I can provide a PR with a fix, but I would like to ask one of the contributors for the greenlight ;)

Kajot-dev avatar Dec 22 '23 00:12 Kajot-dev

It looks like the chart will create it's own secret, even when an existing one is passed in. Only for this new secret to end up in an envFrom/secretKeyRef. The obvious solution for me would be to pass the name of the existing secret to the secretKeyRef directly.

BlackCetha avatar Jan 11 '24 07:01 BlackCetha

+1 I've faced the same issue using Rancher Fleet.

janosmiko avatar Feb 22 '24 19:02 janosmiko

Facing the same issue as well.

nmcostello avatar Feb 22 '24 19:02 nmcostello

Couple things:

  • If you use helm template, lookup does not work
  • as of now chart does not create any secrets by itself for redis internal or external
  • Currently harbor requires full URL for redis for most of its components (so it's not possible to specify password in a separate env for most components, registry component being the exception here)

Technically we could add a check for empty value, but helm will not be able to template the url with value from existing secret, and your harbor will crash, having no password in the templated url

So, deploying with helm template and existingSecret for external redis is currently not supported

To do this properly two things would need to happen

  1. All harbor components would need to have an ability to use the password from the separate env variable (just like registry component does) - and this would have to be in harbor itself, not the helm chart (you would need to create Issue in the main harbor repo)
  2. Then Helm chart would need to adapt to that change and provide url without password, and separate env with password referencing existing secret

Kajot-dev avatar Feb 22 '24 21:02 Kajot-dev

Couple things:

  • If you use helm template, lookup does not work
  • as of now chart does not create any secrets by itself for redis internal or external
  • Currently harbor requires full URL for redis for most of its components (so it's not possible to specify password in a separate env for most components, registry component being the exception here)

Technically we could add a check for empty value, but helm will not be able to template the url with value from existing secret, and your harbor will crash, having no password in the templated url

So, deploying with helm template and existingSecret for external redis is currently not supported

To do this properly two things would need to happen

  1. All harbor components would need to have an ability to use the password from the separate env variable (just like registry component does) - and this would have to be in harbor itself, not the helm chart (you would need to create Issue in the main harbor repo)
  2. Then Helm chart would need to adapt to that change and provide url without password, and separate env with password referencing existing secret

Absolutely agree ! I think it could be a good idea to specify in the helm chart doc that specific behavior, and this issue may be closed

pierreblais avatar Feb 22 '24 22:02 pierreblais

Also running into the same roadblock as others - needing to leverage an existing secret for credentials and we’re using ArgoCD. Any chance there’s a dev taking a look or a timeline for this to get resolved?

brandonw62 avatar Mar 03 '24 15:03 brandonw62

I was able to render the template locally with the command helm template --dry-run=server using helm 3.14.3. The problem now is that my ArgoCd is running helm version 3.10.x which doesn't support that exact syntax. I'm working on getting this working in my cluster and I can post here when I figure it out.

nmcostello avatar Apr 01 '24 13:04 nmcostello