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

pierreblais commented 9 months ago

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

Install Harbor Helm Chart version 1.13.1.
Attempt to provide an existingSecret for an external Redis database.
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 commented 9 months ago

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 !

zyyw commented 9 months ago

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.

MinerYang commented 9 months ago

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

pierreblais commented 9 months ago

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.

Kajot-dev commented 8 months ago

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 ;)

BlackCetha commented 8 months ago

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.

janosmiko commented 6 months ago

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

nmcostello commented 6 months ago

Facing the same issue as well.

Kajot-dev commented 6 months ago

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

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)
Then Helm chart would need to adapt to that change and provide url without password, and separate env with password referencing existing secret

pierreblais commented 6 months ago

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

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)

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

brandonw62 commented 6 months ago

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?

nmcostello commented 5 months ago

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.

github-actions[bot] commented 2 months ago

This issue is being marked stale due to a period of inactivity. If this issue is still relevant, please comment or remove the stale label. Otherwise, this issue will close in 30 days.

Aransh commented 2 months ago

Still relevant

github-actions[bot] commented 3 weeks ago

This issue is being marked stale due to a period of inactivity. If this issue is still relevant, please comment or remove the stale label. Otherwise, this issue will close in 30 days.

Kajot-dev commented 2 weeks ago

Not stale

goharbor / harbor-helm