Skip to content

feat(chart): Ensure helper secret stability across upgrades - #313

Open
tidusete wants to merge 1 commit into
Sentinel-One:masterfrom
tidusete:fix/preserve-helper-secrets
Open

feat(chart): Ensure helper secret stability across upgrades#313
tidusete wants to merge 1 commit into
Sentinel-One:masterfrom
tidusete:fix/preserve-helper-secrets

Conversation

@tidusete

Copy link
Copy Markdown

Description

This PR addresses an idempotency issue where the helper's TLS certificate (sentinelone-helper) and server-token (sentinelone-helper-token) secrets were regenerated on every helm upgrade, Flux reconciliation, or terraform apply.

Problem

Constant rotation of these secrets causes several operational issues:

  • Service Disruption: Regenerating the TLS certificate can interrupt communication between agents and the helper service.
  • GitOps Issue: It creates unnecessary noise in GitOps diffs (e.g., in ArgoCD or Flux), making it difficult to identify meaningful changes.
  • Unpredictable State: The cluster's state changes with every reconciliation, even when no configuration values have been modified.

Solution

This change introduces the use of the Helm lookup() function to check if the helper secrets already exist in the cluster before rendering them.

  • On first installation: The secrets are generated and created as before.
  • On subsequent upgrades/reconciliations: If the secrets are found, their existing data is reused, preventing rotation.

The helm.sh/resource-policy: keep annotation remains on the secrets to prevent accidental deletion during a helm uninstall, preserving their state for future installations if desired.

ArgoCD Considerations

Due to the timing of how ArgoCD renders templates, lookup() may not find the secret during the sync planning phase and will attempt to regenerate it. The recommended workaround for ArgoCD users is to add ignoreDifferences rules for the /data field on both secrets within the Application resource specification.

Example ArgoCD ignoreDifferences:

spec:
  # ...
  ignoreDifferences:
  - group: ""
    kind: Secret
    name: sentinelone-helper
    jsonPointers:
    - /data
  - group: ""
    kind: Secret
    name: sentinelone-helper-token
    jsonPointers:
    - /data

Use lookup() to check whether the helper TLS certificate secret and the
helper server-token secret already exist in the cluster before rendering
new values. If they do, their existing data is reused verbatim so that
helm upgrade, Flux reconciliations and Terraform applies no longer
rotate certificates or tokens on every run.

On a first install the secrets are generated as before. The
helm.sh/resource-policy: keep annotation prevents accidental deletion
on helm uninstall.

The same lookup-based preservation is applied to the helper secret in
webhookconfiguration.yaml (webhooks path), including the caBundle used
by the MutatingWebhookConfiguration and ValidatingWebhookConfiguration.

For ArgoCD deployments, lookup() returns nil during helm-template
rendering so certs are still regenerated each sync. The recommended
mitigation is to add ignoreDifferences for /data on both secrets in the
ArgoCD Application spec.
@oded-s1

oded-s1 commented Feb 26, 2026

Copy link
Copy Markdown
Collaborator

@tidusete thanks for the PR.
Can you elaborate more on the motivation for this change?
Currently the helper certificate expiration duration is 365 days (the default value and can be configured). The reason we regenerate it on helm upgrade is to reset the expiration so it will not expire (assuming customers will upgrade the agent at lease once a year).
On helm upgrade we redeploy the agent and helper so the certificate should remain valid.
For argoCD our recommendation is to add the ignoreDifferences as you mentioned to avoid unnecessary noise in GitOps diffs.

@tidusete

Copy link
Copy Markdown
Author

Hey @oded-s1, I understand the intent behind regenerating certs on upgrade, but the problem in practice is that this happens on every helm upgrade not just cert-related ones. Because Helm applies the new secret and webhook configuration before the StatefulSet and DaemonSet rollouts complete, there is always a window where part of the pods are serving with the old certificate and part with the new one. This is an avoidable source of errors that makes troubleshooting harder, not easier.

I think this is also the same root issue that #242 tried to address.

From a GitOps and Helm maintainability perspective, secrets should be stable and changes should be explicit and intentional. What this PR achieves is exactly that: the certificate is preserved across upgrades, so routine changes don't cause unnecessary disruption. When you actually need to rotate (whether for expiry or any other reason) you delete the secret and it will be regenerated on the next helm upgrade or ArgoCD sync. The rotation becomes a deliberate, traceable operation rather than a side effect of every deployment.

@oded-s1

oded-s1 commented Mar 5, 2026

Copy link
Copy Markdown
Collaborator

@tidusete thanks for your explanation.
For this scenario I think you can use our option to use a pre-created certificate and pass it (secrets.helper_certificate/secrets.helper_token option).
What do you think?

@tidusete

tidusete commented Mar 5, 2026

Copy link
Copy Markdown
Author

Hi @oded-s1, thanks for following up on this.

Using the secrets.helper_certificate and secrets.helper_token options to pass pre-created certificates is definitely a valid workaround for users who already have cert-manager or a robust internal PKI setup.

However, relying on that as the standard fix presents a few issues for the broader user base:

  • It degrades the out-of-the-box experience: It shifts the operational burden onto the user. Users who just want a simple, zero-friction installation shouldn't be forced to manually manage certificates or set up external tools just to prevent their CI/CD pipelines from breaking the helper service on every run.

  • The default behavior remains destructive: In modern GitOps environments (Flux, ArgoCD) or Terraform setups, reconciliations and helm upgrade commands happen frequently. Having the default chart behavior regenerate the certificate on every single run causes those transient "split-brain" outages during pod rollouts unnecessarily.

  • It violates idempotency: Running a deployment tool twice with the exact same configuration should result in no cluster changes. The current default violates this core infrastructure-as-code principle.

@mardonner

mardonner commented Aug 14, 2026

Copy link
Copy Markdown

I have 3 issues with this situation as well.

  1. Having to supply my own helper token and certificate causes overhead and requires me to have cert-manager and external-secrets installed on the cluster with something like this to set it up in a somewhat sensible way.

I think it is unnessecary to make users jump through those hoops.

for the helper cert

# (namespaced and not cluster scoped resources should be used here, actually)
# assume release name and namespace to be 'sentinelone'
---
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: selfsigned-issuer
  namespace: cert-manager
spec:
  selfSigned: {}

---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: selfsigned-ca
  namespace: cert-manager
spec:
  isCA: true
  commonName: selfsigned-ca
  secretName: selfsigned-ca-root-secret
  privateKey:
    algorithm: ECDSA
    size: 256
  issuerRef:
    name: selfsigned-issuer
    kind: ClusterIssuer
    group: cert-manager.io

---
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: selfsigned-ca-issuer
  namespace: cert-manager
spec:
  ca:
    secretName: selfsigned-ca-root-secret

---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: selfsigned-ca
  namespace: cert-manager
spec:
  isCA: true
  commonName: selfsigned-ca
  secretName: selfsigned-ca-root-secret
  privateKey:
    algorithm: ECDSA
    size: 256
  issuerRef:
    name: selfsigned-issuer
    kind: ClusterIssuer
    group: cert-manager.io

---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: sentinelone-helper
  namespace: sentinelone
spec:
  secretName: sentinelone-helper-cert
  issuerRef:
    name: selfsigned-ca-issuer
    kind: ClusterIssuer
  duration: 8760h0m0s
  dnsNames:
    - "sentinelone-helper"
    - "sentinelone-helper.sentinelone"
    - "sentinelone-helper.sentinelone.svc"
    - "localhost"

for the helper token

# (namespaced and not cluster scoped resources should be used here, actually)
# assume release name and namespace to be 'sentinelone'
---
apiVersion: generators.external-secrets.io/v1alpha1
kind: ClusterGenerator
metadata:
  name: cluster-generator-password
spec:
  kind: Password
  generator:
    passwordSpec:
      allowRepeat: false
      digits: 6
      encoding: raw
      length: 24
      noUpper: false
      secretKeys:
      - password
      symbolCharacters: ~!@#$%^&*()_+-={}|[]:<>?,./
      symbols: 6

---
apiVersion: external-secrets.io/v1
kind: ExternalSecret
metadata:
  name: sentinelone-helper-token
  namespace: sentinelone
spec:
  refreshInterval: "24h"
  target:
    name: sentinelone-helper-token
    creationPolicy: Owner
    deletionPolicy: Merge
    template:
      data:
        server-token: '{{ .password }}'
  dataFrom:
  - sourceRef:
      generatorRef:
        apiVersion: generators.external-secrets.io/v1alpha1
        kind: ClusterGenerator
        name: cluster-generator-password
  1. chart version 26.1.3 creates a helper cert, even if a self-supplied one is specified with e.g.:
# values.yaml
# assume release name and namespace to be 'sentinelone'
configuration:
  deployment_type: "argocd"
  cluster:
    name: test
secrets:
  helper_certificate: sentinelone-helper-cert
  helper_token: sentinelone-helper-token
  site_key:
    name: sentinelone-site-key

due to this check in s1-agent/templates/_helpers.tpl

{{- define "helper.secret.create" -}}
{{- or (empty .Values.secrets.helper_certificate) (eq .Values.configuration.deployment_type "argocd") | ternary "true" "" }}
{{- end -}}

Is there a reason for this?

  1. The timestamp on the annotation in the helper statefulset (s1-agent/templates/helper/statefulset.yaml) and agent daemonset (s1-agent/templates/agent/daemonset.yaml) causes a redeployment of all pods.
    For people with gitOps engines that regularly re-render the chart this will cause frequent restarts of the daemonset
      annotations:
        timestamp: {{ now | quote }}

Is there a reason for this? Redeployment of all s1 pods is unnessecary IMO. Can this not be moved out of the template and put into the parent object metadata?

In argo, I can at least add this to my application specification as a workaround:

# assume release name and namespace to be 'sentinelone'
  ignoreDifferences:
    - group: apps
      kind: DaemonSet
      name: sentinelone-agent
      jsonPointers:
        - /spec/template/metadata/annotations/timestamp
    - group: apps
      kind: StatefulSet
      name: sentinelone-helper
      jsonPointers:
        - /spec/template/metadata/annotations/timestamp

However, in my case, this will still cause unnecessary PRs in my gitops repo.


The burden of providing workarounds for this is put on the enduser, which I'm frustrated by.

The proposed solution with lookup is better but has downsides too. E.g. when the rendering of the helm chart happens outside of the cluster, where the helm template command has no kubeconfig available to perform the lookup.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants