Linkerd chart anti-patterns around namespace

[darkn3rd]

If the namespace linkerd already exists, the chart fails. Thus the helm chart is not idempotent, which makes it difficult to manage changes on an existing infrastructure. It is true that this can be overridden (e.g. installNamespace: false), but the helm chart by default is not idempotent.

Embedded namespaces in a helm chart is an anti-pattern as it removes the ability to manage the kubernetes cluster away from the Kubernetes operator, and makes the chart not idempotent.

Further, explicit definition of namespace: linkerd is also an anti-pattern, as this value is set during deployment with .Release.Namespace. Templates can use .Release.Namespace to set values like linkerd.io/control-plane-ns for example.

Steps to Reproduce

LINKERD_EXP=$(date -v+8760H +"%Y-%m-%dT%H:%M:%SZ" 2> /dev/null) || \
 LINKERD_EXP=$(date -d '+8760 hour' +"%Y-%m-%dT%H:%M:%SZ")
export LINKERD_EXP
step certificate create root.linkerd.cluster.local ca.crt ca.key
step certificate create identity.linkerd.cluster.local issuer.crt issuer.key

helm install linkerd \
  --create-namespace --namespace linkerd \
  --set-file identityTrustAnchorsPEM=certs/ca.crt \
  --set-file identity.issuer.tls.crtPEM=certs/issuer.crt \
  --set-file identity.issuer.tls.keyPEM=certs/issuer.key \
  --set identity.issuer.crtExpiry=$LINKERD_EXP \
  linkerd/linkerd2

Additionally, when using downstream tools helmfile, this also reproduces:

## helmfile.yaml - helmfile  apply
repositories:
  - name: linkerd
    url: https://helm.linkerd.io/stable

releases:
  - name: linkerd
    namespace: linkerd
    chart: linkerd/linkerd2
    version: 2.10.2
    values:
      - identityTrustAnchorsPEM: |
{{ readFile "certs/ca.crt" | indent 10 }}
        identity:
          issuer:
            crtExpiry: {{ requiredEnv "LINKERD_EXP" }}
            tls:
              crtPEM: |
{{ readFile "certs/issuer.crt" | indent 16 }}
              keyPEM: |
{{ readFile "certs/issuer.key" | indent 16 }}

Actual Results

The deployment will fail with the following error.

Error: namespaces "linkerd" already exists

Expected Results

I expected this chart to be idempotent. If the namespace linkerd already exists, the chart should not error out. Forcing namespace is an anti-pattern, this should be under the control of the operator.

[alpeb]

That's a great point. The reason we set the namespace by default is because it requires metadata that Helm has no way of declaring. We could have gone the other way, and not provide the namespace by default and give instructions about adding that metadata. But that's an additional step that would make things a little bit more complicated for users. Instead, we opted for making things simpler and working out of the box without extra config, while leaving the door open to operators adopting a more "correct" Helm pattern via those params you pointed out.

Also, all this was designed under Helm v2's paradigm (see #3211), so it might be time to take another look and better align with v3's way of doing things. For starters, we should think about ways of no longer depending on that metadata.

There's a section in our Helm docs about Customizing the Namespace. One first thing we could do is better explain how that is a preferred approach, in order to better integrate with other Helm tools.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[adleong]

I believe this is in progress at #6635

[adleong]

@alpeb this is done, right?

[alpeb]

Yep, addressed in #6635