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

Don't render Token if it already exists #604

Open
DanielWozniak94 opened this issue Apr 21, 2022 · 7 comments
Open

Don't render Token if it already exists #604

DanielWozniak94 opened this issue Apr 21, 2022 · 7 comments
Labels
chart/datadog This issue or pull request is related to the datadog chart enhancement New feature or request

Comments

@DanielWozniak94
Copy link

Describe what happened:
When using ArgoCD together with this helm chart, whenever ArgoCD refreshes, it triggers the template to get rendered. This causes the cluster agent token secret to generate a new random token. This is fine as it doesn't break anything however it does show the Argo App to be out of sync. Would be nice to use the lookup function so that if the secret already exists it doesn't regenerate a new token.

Describe what you expected:
If the secret already exists there should be no changes to the token.

Steps to reproduce the issue:
Using the chart as is and not specifying a token in the values file.

Additional environment details (Operating System, Cloud provider, etc):

@clamoriniere clamoriniere added chart/datadog This issue or pull request is related to the datadog chart enhancement New feature or request labels Apr 21, 2022
@clamoriniere
Copy link
Collaborator

Hi @DanielWozniak94,

Thanks for opening this issue,

we have provided in #364 a workaround to avoid having this issue: #364 (comment)

We will evaluate the solution that you propose with the lookup function.

@DanielWozniak94
Copy link
Author

Thanks for the suggestion.

However using that workaround would require placing the token as plaintext in the values file, which we version control with Git.

@clamoriniere
Copy link
Collaborator

clamoriniere commented Apr 22, 2022

To avoid plain text secret, we propose a feature in the agent call secret-backend that can be use for the token too.

We provide a default secret-backend implementation that allow a reference to a token/password stored in a Kubernetes Secret. it will be something like ENC[k8s_secret@some_namespace/some_name/a_key] placeholder, see: https://docs.datadoghq.com/agent/guide/secrets-management/?tab=linux#script-for-reading-from-multiple-secret-providers

Also I have investigated how to use lookup function. Indeed we can check if the secret already exist, and we can trigger it if we see the token option empty. But if we don't create the secret, it will be delete by helm when it will do the release resources diff. We can prevent the deletion thanks to the annotation helm.sh/resource-policy: keep but it means that when the Datadog agent chart is uninstall by a user, we will leak the secret on the cluster.

Please let me know if you see another approach to use the lookup function.

@DanielWozniak94
Copy link
Author

Secret-backend looks promising. Our current version is 7.31.1 so I won't really be able to try the helper script until we get that upgraded.

As for lookup function, thanks for checking that out. I'll see if anything comes to mind of how to get it to work.

@vboulineau
Copy link
Contributor

Hello,

For reference, it's not possible to use lookup in Helm with ArgoCD, see argoproj/argo-cd#5202, so at the moment we can only stick to the previous recommendations: clusterAgent.tokenExistingSecret or secret backend.

@kayketeixeira
Copy link

kayketeixeira commented Apr 2, 2024

Hi @DanielWozniak94,

Thanks for opening this issue,

we have provided in #364 a workaround to avoid having this issue: #364 (comment).

We will evaluate the solution that you propose with the lookup function.

Helloo @DanielWozniak94, i tried to use this code below on a new secret file, but i got some errors described in this issue #1059. At first time it worked but after some deploys i got errors.

image

data:
{{- $secret := (lookup "v1" "Secret" "datadog" "datadog-cluster-agent-secret") | default (dict "data" (dict "token" "")) }}
  {{- $token := empty $secret.data.token | ternary ((default (randAlphaNum 32) .Values.clusterAgent.token) | b64enc | quote) $secret.data.token }}
  token: {{ $token }}

@sarcasticadmin
Copy link

sarcasticadmin commented Dec 11, 2024

The lookup function seems to work well from my testing. It requires the following helm flag: --dry-run=server to do a comparison against a running cluster so the lookup is something other than nil:

diff --git a/charts/datadog/templates/secret-cluster-agent-token.yaml b/charts/datadog/templates/secret-cluster-agent-token.yaml
index c1b64f8..eb30d91 100644
--- a/charts/datadog/templates/secret-cluster-agent-token.yaml
+++ b/charts/datadog/templates/secret-cluster-agent-token.yaml
@@ -8,14 +8,14 @@ metadata:
   labels:
 {{ include "datadog.labels" . | indent 4 }}
 {{- if .Values.datadog.secretAnnotations }}
   annotations: {{ toYaml .Values.datadog.secretAnnotations | nindent 4 }}
 {{- end }}
 type: Opaque
 data:
   {{ if .Values.clusterAgent.token -}}
   token: {{ .Values.clusterAgent.token | b64enc | quote }}
   {{ else -}}
-  token: {{ randAlphaNum 32 | b64enc | quote }}
+  token: {{ (dig "data" "token"  (randAlphaNum 32 | b64enc) (lookup "v1" "Secret" .Release.Namespace "datadog-cluster-agent")) | quote }}
   {{ end }}
 {{- end }}
 {{ end }}

Deploying the chart with the above patch:

$ cd charts
$ helm package -u datadog
$ helm upgrade --install datadog datadog-3.83.0.tgz --namespace datadog --version 3.83.0 -f values-common.yml --debug

And then running helm diff shows the token is now stable:

$ helm diff --suppress-secrets upgrade datadog datadog-3.83.0.tgz --namespace datadog --version 3.83.0 -f values-common.yml --dry-run=server

NOTE: Theres still a sha256sum diff of the annotation.checksum/clusteragent_token but this is a separate issue: #1632 It's due to multiple invocations of secret-cluster-agent-token.yaml template. Regardless, things stabilize after applying a single helm upgrade against the same chart version since the randAlphaNum function is no longer used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart/datadog This issue or pull request is related to the datadog chart enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants