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: Restore namespace field in templates #9351

Merged
merged 3 commits into from
Sep 10, 2022
Merged

helm: Restore namespace field in templates #9351

merged 3 commits into from
Sep 10, 2022

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Sep 8, 2022

In #6635 (f9f3ebe), we removed the Namespace resources from the linkerd Helm charts. But this change also removed the namespace field from all generated metadata, adding conditional logic to only include it when being installed via the CLI.

This conditional logic currently causes spurious whitespace in output YAML. This doesn't cause problems but is aesthetically inconsistent/distracting.

This change removes the partials.namespace helper and instead inlines the value in our templates. This makes our CLI- and Helm-generated manifests slightly more consistent and removes needless indirection.

In #6635 (f9f3ebe), we removed the `Namespace` resources from the
linkerd Helm charts. But this change also removed the `namespace` field
from all generated metadata, adding conditional logic to only include it
when being installed via the CLI.

This conditional logic currently causes spurious whitespace in output
YAML. This doesn't cause problems but is aesthetically
inconsistent/distracting.

This change removes the `partials.namespace` helper and instead inlines
the value in our templates. This makes our CLI- and Helm-generated
manifests slightly more consistent and removes needless indirection.

Signed-off-by: Oliver Gould <ver@buoyant.io>
@olix0r olix0r requested a review from a team as a code owner September 8, 2022 03:11
@olix0r olix0r requested a review from alpeb September 8, 2022 03:11
@olix0r
Copy link
Member Author

olix0r commented Sep 8, 2022

@alpeb Is there any problem with removing the conditional logic?

@alpeb
Copy link
Member

alpeb commented Sep 8, 2022

Namespaces were left out of manifests in the helm case just to follow their recommendation, as posted here (unfortunately missing from their docs).
The main argument being to be able to support the pattern

helm template linkerd/linkerd-control-plane | kubectl apply -n linkerd -f -

My understanding is that there are helm plugins in CD tools rely on helm template and we might break things if they attempt to declare the namespace only until the manifests get persisted, but I have no evidence to back that claim...

@olix0r
Copy link
Member Author

olix0r commented Sep 8, 2022

@alpeb the problem, as i see it, is that we have to encode the namespace in many, many places:

:; rg Release.Namespace
templates/psp.yaml
9:  name: linkerd-{{.Release.Namespace}}-control-plane
11:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
71:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
77:  - linkerd-{{.Release.Namespace}}-control-plane
85:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
93:  namespace: {{.Release.Namespace}}
97:  namespace: {{.Release.Namespace}}
101:  namespace: {{.Release.Namespace}}
104:  namespace: {{.Release.Namespace}}

templates/identity.yaml
15:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
31:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
45:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
64:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
84:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
107:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
115:      linkerd.io/control-plane-ns: {{.Release.Namespace}}
130:        linkerd.io/control-plane-ns: {{.Release.Namespace}}
131:        linkerd.io/workload-ns: {{.Release.Namespace}}
149:        - -controller-namespace={{.Release.Namespace}}

templates/proxy-injector.yaml
18:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
43:        linkerd.io/control-plane-ns: {{.Release.Namespace}}
44:        linkerd.io/workload-ns: {{.Release.Namespace}}
75:        - -linkerd-namespace={{.Release.Namespace}}
142:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
163:    linkerd.io/control-plane-ns: {{.Release.Namespace}}

templates/destination.yaml
12:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
31:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
50:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
69:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
88:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
108:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
131:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
139:      linkerd.io/control-plane-ns: {{.Release.Namespace}}
157:        linkerd.io/control-plane-ns: {{.Release.Namespace}}
158:        linkerd.io/workload-ns: {{.Release.Namespace}}
188:        - -controller-namespace={{.Release.Namespace}}
257:        - --control-plane-namespace={{.Release.Namespace}}

templates/namespace.yaml
9:  name: {{ .Release.Namespace }}
15:    linkerd.io/control-plane-ns: {{.Release.Namespace}}

templates/identity-rbac.yaml
8:  name: linkerd-{{.Release.Namespace}}-identity
11:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
26:  name: linkerd-{{.Release.Namespace}}-identity
29:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
33:  name: linkerd-{{.Release.Namespace}}-identity
37:  namespace: {{.Release.Namespace}}
46:    linkerd.io/control-plane-ns: {{.Release.Namespace}}

templates/config.yaml
9:    linkerd.io/control-plane-ns: {{.Release.Namespace}}

templates/destination-rbac.yaml
8:  name: linkerd-{{.Release.Namespace}}-destination
11:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
34:  name: linkerd-{{.Release.Namespace}}-destination
37:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
41:  name: linkerd-{{.Release.Namespace}}-destination
45:  namespace: {{.Release.Namespace}}
54:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
57:{{- $host := printf "linkerd-sp-validator.%s.svc" .Release.Namespace }}
67:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
92:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
100:      namespace: {{ .Release.Namespace }}
114:{{- $host := printf "linkerd-policy-validator.%s.svc" .Release.Namespace }}
124:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
149:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
157:      namespace: {{ .Release.Namespace }}
184:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
215:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
223:    namespace: {{.Release.Namespace}}

templates/heartbeat-rbac.yaml
12:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
25:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
33:  namespace: {{.Release.Namespace}}
40:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
54:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
62:  namespace: {{.Release.Namespace}}
71:    linkerd.io/control-plane-ns: {{.Release.Namespace}}

templates/heartbeat.yaml
16:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
33:            linkerd.io/workload-ns: {{.Release.Namespace}}
60:            - "-controller-namespace={{.Release.Namespace}}"

templates/proxy-injector-rbac.yaml
8:  name: linkerd-{{.Release.Namespace}}-proxy-injector
11:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
32:  name: linkerd-{{.Release.Namespace}}-proxy-injector
35:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
39:  namespace: {{.Release.Namespace}}
43:  name: linkerd-{{.Release.Namespace}}-proxy-injector
53:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
56:{{- $host := printf "linkerd-proxy-injector.%s.svc" .Release.Namespace }}
66:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
91:    linkerd.io/control-plane-ns: {{.Release.Namespace}}
101:      namespace: {{ .Release.Namespace }}

I can't see the benefit from omitting from the metadata if it has to be kept in sync in all of these other places.

Specifically, the command you shared will not currently produce a working Linkerd:

helm template linkerd/linkerd-control-plane | kubectl apply -n linkerd -f -

This would install the resources in the linkerd namespace, but we'd refer to the default namespace throughout the configuration.

By pinning the namespace metadata in the manifest, we can at least ensure consistency.

@alpeb
Copy link
Member

alpeb commented Sep 8, 2022

Ah you're totally right!
OTOH I see partials.namespace still being used in multicluster/charts/linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml. If you refactor that as well, we can also get rid of that partial definition altogether.

Signed-off-by: Oliver Gould <ver@buoyant.io>
Signed-off-by: Oliver Gould <ver@buoyant.io>
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@olix0r olix0r merged commit c8348b3 into main Sep 10, 2022
@olix0r olix0r deleted the ver/namespace branch September 10, 2022 17:18
hawkw pushed a commit that referenced this pull request Apr 10, 2023
#9351 added a namespace field onto some ClusterRole and
ClusterRoleBinding resources in the linkerd-multicluster extension.
This was causing the resources in the templates to not match the
resources installed in the cluster when compared by name/namespace and
causing `linkerd mc prune` to flag those resources for deletion even
though they exist in the template.

We remove the namespace field from cluster scoped resources.

Signed-off-by: Alex Leong <alex@buoyant.io>
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