CiscoDevNet / appdynamics-charts

Helm charts for AppDynamics
https://appdynamics.github.io/appdynamics-charts/
Apache License 2.0
21 stars 38 forks source link

Remove "lookup" logic from the cluster-agent secret #23

Open AndresPineros opened 3 years ago

AndresPineros commented 3 years ago

The Problem

Right now the cluster-agent has this logic in the "secret-cluster-agent.yaml":

{{ $secret := (lookup "v1" "Secret" .Release.Namespace "cluster-agent-secret") }}

This is a query to the Kubernetes API that checks if the "cluster-agent-secret" exists or not. This is an anti-pattern for Helm, and it should be avoided because it creates unnecessary racing conditions. The only way that this would work is if I guarantee that the secret exists before the chart is rendered and executed, but this approach is bad because Helm is not a good tool to define order in which resources are deployed. Basically my deployer now needs logic to say "first deploy the secret, and then deploy the cluster agent". This type of logic is not supported by Helm or other Kubernetes deployers like ArgoCD, Kustomize or Kaptain. So... basically, we're stuck with a manual step or we have to code custom logic just to make this automatic.

Also, our Helm CI is breaking because to validate the configurations we do Helm Template. But the template will always break because it has no access to the cluster API, so the lookup function always returns an empty map.

The solution

  1. Simply ask for a variable in the values.yaml called "customSecretName", so I can say something like:

    customSecretName: my-secret

    I can create this secret in the same automation, in another helm chart dependency, as another ArgoCD resource... Many options. The point is that I'm creating my own secret and letting the chart know the secret name.

  2. Then in the "secret-cluster-agent.yaml" just add the logic saying "if .Values.customSecretName is defined, do not render the cluster-agent-secret"

  3. In the deployment add the logic "If .Values.customSecretName is defined, use that secret to inject secrets in the container. Otherwise, use the cluster-agent-secret"

Why is this a solution?

Because now the racing condition will be solved by the deployment. The container won't be deployed until the secretMount exists, so it doesn't matter if the custom secret is created before or after the deployment, the deployment will wait until it exists.

Look at the way other Charts do this: https://artifacthub.io/packages/helm/wso2/mysql That is the official Mysql Chart, look at the variable existingSecret. https://artifacthub.io/packages/helm/wso2/mysql?modal=template&template=secrets.yaml

gingerwizard commented 3 years ago

Aside from the race condition, asking the chart to create the secret means a sensitive key must be passed to the chart. In reality, we most orgs manage their secrets using separate automation and infrastructure. Having to pass these to the chart on automation exposes risks and makes automating the deployment of this chart challenging. As @AndresPineros points out, leave secret creation to others.

EvertonSA commented 2 years ago

Having to pass these to the chart on automation exposes risks and makes automating the deployment of this chart challenging.

agreed. contradictory to that, the current chart does that here:

https://github.com/CiscoDevNet/appdynamics-charts/blob/75a2a9f2734186497aa2e2951201c9a4a6feaa95/cluster-agent/values.yaml#L22