Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pierreblais opened this issue Nov 20, 2023 · 12 comments
Open
Assignees

Comments

@pierreblais
Copy link

pierreblais commented Nov 20, 2023

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 (7b6501a)

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

@pierreblais pierreblais changed the title Regression in Bitnami Redis Helm Chart 1.13.1: Error with 'existingSecret' for External Redis Database Regression Helm Chart 1.13.1: Error with 'existingSecret' for External Redis Database Nov 20, 2023
@pierreblais
Copy link
Author

pierreblais commented Nov 27, 2023

I understand what's going on !

As we can see in the lines added in that commit: 7b6501a 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 pierreblais changed the title Regression Helm Chart 1.13.1: Error with 'existingSecret' for External Redis Database Helm Chart 1.13.1: Error with 'existingSecret' for External Redis Database Nov 29, 2023
@zyyw
Copy link
Collaborator

zyyw commented Nov 30, 2023

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
Copy link
Collaborator

MinerYang commented Nov 30, 2023

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
Copy link
Author

pierreblais commented Nov 30, 2023

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
Copy link
Contributor

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
Copy link

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
Copy link

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

@nmcostello
Copy link

Facing the same issue as well.

@Kajot-dev
Copy link
Contributor

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

@pierreblais
Copy link
Author

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

@brandonw62
Copy link

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
Copy link

nmcostello commented Apr 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants