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.

Sign up for GitHub

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

Jump to bottom

Remove the namespace resource from the Helm charts #6584

Closed
alpeb opened this issue Jul 30, 2021 · 2 comments · Fixed by #6635
Closed

Remove the namespace resource from the Helm charts #6584

alpeb opened this issue Jul 30, 2021 · 2 comments · Fixed by #6635
Assignees
@alpeb
Labels
area/helm
Milestone
stable-2.12.0

Comments

@alpeb
Copy link
Member

alpeb commented Jul 30, 2021

It is considered bad practice to have the charts create their namespace, specially since Helm v3. A few reasons for this:

  • Just like you would do with kubectl --namespace xxx apply, it's more flexible to leave helm install instruct in a flag the namespace it'd like the resources to be deployed into.
  • Now that Tiller is gone, the chart config is no longer stored in Tiller's namespace, but in the namespace passed to helm install --namespace. If nothing was passed, the config secret is stored in the default namespace regardless of the namespace used in the chart's templates. That secret can contain private data, so that's not secure.

Removing the namespace from the templates requires to take these points into account.

  • Have the CLI to manually generate the yaml for the namespace.
  • The namespace can no longer have this specific linkerd metadata:
    • The linkerd.io/inject: disabled annotation. No big deal, we'll just have to add that to each control plane workload.
    • The config.linkerd.io/admission-webhooks: disabled label. Without this the injector will get called for control plane pod restarts, but it shouldn't mutate the pod given the presence of the linkerd.io/inject: disabled annotation.
    • The linkerd.io/is-control-plane: "true" label. From what I could tell this is not relied on.
    • The linkerd.io/control-plane-ns label. Used for identifying all the resources belonging to linkerd, and used for kubectl apply --prune. Since the namespace isn't touched during upgrades, it seems this doesn't matter. Integrations tests though will need to manually remove namespaces for cleanup.
    • Linkerd extensions rely on the linkerd.io/extension label. If we can't find an easy workaround for this, we could add a simple post-install hook for just adding that label.
  • For folks having installed without using helm install --namespace, a simple upgrade to this new way won't likely be possible. Will have to figure if it's possible to do it without downtime, and document the procedure. Maybe we can consider still leaving the namespace template in the chart, but disabled by default, and just instruct to enable it in the upgrade docs for folks not wanting to migrate to the new way.

Ref #6463 #3211
@kleimkuhler
Copy link
Contributor

I don't think we need to worry about the linkerd.io/inject: disabled annotation on control plane workloads. When the webhook checks if workloads are injectable here, the function will see that it already has a sidecar here.

That should be enough for injection to be skipped.

@alpeb alpeb self-assigned this Aug 3, 2021
@alpeb alpeb mentioned this issue Aug 5, 2021
Closed
alpeb added a commit that referenced this issue Aug 9, 2021
@alpeb
Remove namespace for linkerd2 chart
4ab0a32 
First part of #6584

With this change, the `namespace.yaml` template is rendered only for CLI installs and not Helm, and likewise the `namespace:` entry in the namespace-level objects (using a new `partials.namespace` helper).

The `installNamespace` and `namespace` entries in `values.yaml` have been removed.

There in the templates where the namespace is required, we moved from `.Values.namespace` to `.Release.Namespace` which is filled-in automatically by Helm. For the CLI, `install.go` now explicitly defines the contents of the `Release` map alongside `Values`.

The proxy-injector has a new `linkerd-namespace` argument given the namespace is no longer persisted in the `linkerd-config` ConfigMap, so it has to be passed in. To pass it further down to `injector.Inject()` without modifying the `Handler` signature, a closure was used.

To land later in a followup: same deal for the other charts. Extensions will require usage of the post-install hook for adding metadata though.
@alpeb alpeb mentioned this issue Aug 9, 2021
Merged
alpeb added a commit that referenced this issue Aug 9, 2021
@alpeb
Remove namespace for linkerd2-viz chart
2076dac 
Second part of #6584, followup of #6635 (and based off of alpeb/no-ns-helm-core)

Stop rendering `namespace.yaml` in the `linkerd-viz` chart, same as we did in #6635.

The additional change here is the addition of the `namespace-metadata.yaml` template (and its RBAC), _not_ rendered in CLI installs, which is a Helm `post-install` hook, consisting on a Job that executes a script adding the required annotations and labels to the viz namespace using a PATCH request against kube-api. The script first checks if the namespace doesn't already have an annotations/labels entries, in which case it has to add extra ops in that patch.

The Job uses a multiarch `curlimages/curl` image, which is built by curl.haxx.se. I couldn't find a simple multiarch "kubectl" image that would have allowed us issuing simple kubectl commands. The `curlimages/curl` image is based on Alpine; it won't have as much as CVE-related reports noise as a Debian image, but it'd be better to have something slimmer still. Eventually we could build our own scratch-based binary if deemed worthy.

I'll post in followup PRs the same changes for the other extensions and the CNI chart.
@alpeb alpeb mentioned this issue Aug 9, 2021
Merged
alpeb added a commit that referenced this issue Aug 10, 2021
@alpeb
Remove namespace from linkerd2-cni chart
13ab43b 
Third part of #6584, followup of #6635 (and based off of alpeb/no-ns-helm-core)

Stop rendering the namespace in the `linkerd2-cni` chart, same as we did in #6635.

This also removes the `install-namespace` option from `linkerd install-cni`,
which isn't found in `linkerd install` nor `linkerd viz install` anyways, and
it would add some complexity to support.
alpeb added a commit that referenced this issue Aug 10, 2021
@alpeb
Remove namespace from linkerd2-cni chart
54d6e8d 
Third part of #6584, followup of #6635 (and based off of alpeb/no-ns-helm-core)

Stop rendering the namespace in the `linkerd2-cni` chart, same as we did in #6635.

This also removes the `install-namespace` option from `linkerd install-cni`,
which isn't found in `linkerd install` nor `linkerd viz install` anyways, and
it would add some complexity to support.
@alpeb alpeb mentioned this issue Aug 10, 2021
Merged
alpeb added a commit that referenced this issue Aug 12, 2021
@alpeb
Remove namespace from multicluster charts
7eac2de 
Another part of #6584, branched off of #6635 (alpeb/no-ns-helm-core).

Stop rendering the namespace in the `linkerd-multicluster` chart as we
did in #6635.

Also:
- Got rid of the `namespace` values.yaml entry in the
  `linkerd-multicluster` and `linkerd-multicluster-link` charts, and
  also the `installNamespace` entry in the former.
- Added a dependency on the `partials` chart to the
  `linkerd-multicluster-link` chart, so that we can tap on the
  `partials.namespace` helper.
- Added the post-install hook resources to the `linkerd-multicluster`
  chart.
- We don't have any more the restriction on having the muticluster
  objects live in a separate namespace than linkerd. It's still good
  practice, and that's the default for the CLI install, but I removed
  that validation.

Finally, as a side-effect, the `allow` subcommand was fixed; it has been
broken for a while apparently:

```console
$ linkerd mc allow --service-account-name foobar
Error: template: linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml:16:7: executing "linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml" at <include "partials.annotations.created-by" $>: error calling include: template: no template "partials.annotations.created-by" associated with template "gotpl"
```
@alpeb alpeb mentioned this issue Aug 12, 2021
Merged
alpeb added a commit that referenced this issue Aug 12, 2021
@alpeb
Remove namespace from linkerd-jaeger chart
b2e2e74 
Another part of #6584 based off of #6635 (alpeb/no-ns-helm-core)

Stop rendering `namespace.yaml` in the `linkerd-jaeger` chart, same as
we did in #6635.

This makes use of a Helm `post-install` hook to add the
`linkerd.io/extension: jaeger` label to the namespace created by Helm.

The `linkerd.io/inject: enabled` and `config.linkerd.io/proxy-await:
enabled` annotations have been moved down from the namespace and into
each workload.
@alpeb alpeb mentioned this issue Aug 12, 2021
Merged
alpeb added a commit that referenced this issue Aug 13, 2021
@alpeb
Remove namespace from linkerd2-viz chart (#6638)
57a83eb 
* Remove namespace for linkerd2-viz chart

Second part of #6584, followup of #6635 (and based off of alpeb/no-ns-helm-core)

Stop rendering `namespace.yaml` in the `linkerd-viz` chart, same as we did in #6635.

The additional change here is the addition of the `namespace-metadata.yaml` template (and its RBAC), _not_ rendered in CLI installs, which is a Helm `post-install` hook, consisting on a Job that executes a script adding the required annotations and labels to the viz namespace using a PATCH request against kube-api. The script first checks if the namespace doesn't already have an annotations/labels entries, in which case it has to add extra ops in that patch.

The Job uses a multiarch `curlimages/curl` image, which is built by curl.haxx.se. I couldn't find a simple multiarch "kubectl" image that would have allowed us issuing simple kubectl commands. The `curlimages/curl` image is based on Alpine; it won't have as much as CVE-related reports noise as a Debian image, but it'd be better to have something slimmer still. Eventually we could build our own scratch-based binary if deemed worthy.
alpeb added a commit that referenced this issue Aug 17, 2021
@alpeb
Remove namespace from linkerd2-cni chart (#6643)
22d4a86 
* Remove namespace from linkerd2-cni chart

Third part of #6584, followup of #6635 (and based off of alpeb/no-ns-helm-core)

Stop rendering the namespace in the `linkerd2-cni` chart, same as we did in #6635.

This also removes the `install-namespace` option from `linkerd install-cni`,
which isn't found in `linkerd install` nor `linkerd viz install` anyways, and
it would add some complexity to support.
alpeb added a commit that referenced this issue Aug 17, 2021
@alpeb
Remove namespace from multicluster charts (#6665)
11ff9db 
Another part of #6584, branched off of #6635 (alpeb/no-ns-helm-core).

Stop rendering the namespace in the `linkerd-multicluster` chart as we
did in #6635.

Also:
- Got rid of the `namespace` values.yaml entry in the
  `linkerd-multicluster` and `linkerd-multicluster-link` charts, and
  also the `installNamespace` entry in the former.
- Added a dependency on the `partials` chart to the
  `linkerd-multicluster-link` chart, so that we can tap on the
  `partials.namespace` helper.
- Added the post-install hook resources to the `linkerd-multicluster`
  chart.
- We don't have any more the restriction on having the muticluster
  objects live in a separate namespace than linkerd. It's still good
  practice, and that's the default for the CLI install, but I removed
  that validation.

Finally, as a side-effect, the `allow` subcommand was fixed; it has been
broken for a while apparently:

```console
$ linkerd mc allow --service-account-name foobar
Error: template: linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml:16:7: executing "linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml" at <include "partials.annotations.created-by" $>: error calling include: template: no template "partials.annotations.created-by" associated with template "gotpl"
```
alpeb added a commit that referenced this issue Aug 17, 2021
@alpeb
Remove namespace from linkerd-jaeger chart (#6669)
b298348 
Another part of #6584 based off of #6635 (alpeb/no-ns-helm-core)

Stop rendering `namespace.yaml` in the `linkerd-jaeger` chart, same as
we did in #6635.

This makes use of a Helm `post-install` hook to add the
`linkerd.io/extension: jaeger` label to the namespace created by Helm.

The `linkerd.io/inject: enabled` and `config.linkerd.io/proxy-await:
enabled` annotations have been moved down from the namespace and into
each workload.
@stale
Copy link

stale bot commented Nov 1, 2021

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.

@stale stale bot added the wontfix label Nov 1, 2021
@adleong adleong removed the wontfix label Nov 2, 2021
@adleong adleong added this to the stable-2.12.0 milestone Nov 9, 2021
alpeb added a commit that referenced this issue Dec 10, 2021
@alpeb
Remove namespace from charts and split them into linkerd-crd and `l…
f9f3ebe 
…inkerd-control-plane` (#6635)

Fixes #6584 #6620 #7405

# Namespace Removal

With this change, the `namespace.yaml` template is rendered only for CLI installs and not Helm, and likewise the `namespace:` entry in the namespace-level objects (using a new `partials.namespace` helper).

The `installNamespace` and `namespace` entries in `values.yaml` have been removed.

There in the templates where the namespace is required, we moved from `.Values.namespace` to `.Release.Namespace` which is filled-in automatically by Helm. For the CLI, `install.go` now explicitly defines the contents of the `Release` map alongside `Values`.

The proxy-injector has a new `linkerd-namespace` argument given the namespace is no longer persisted in the `linkerd-config` ConfigMap, so it has to be passed in. To pass it further down to `injector.Inject()` without modifying the `Handler` signature, a closure was used.

------------
Update: Merged-in #6638: Similar changes for the `linkerd-viz` chart:

Stop rendering `namespace.yaml` in the `linkerd-viz` chart.

The additional change here is the addition of the `namespace-metadata.yaml` template (and its RBAC), _not_ rendered in CLI installs, which is a Helm `post-install` hook, consisting on a Job that executes a script adding the required annotations and labels to the viz namespace using a PATCH request against kube-api. The script first checks if the namespace doesn't already have an annotations/labels entries, in which case it has to add extra ops in that patch.

---------
Update: Merged-in the approved #6643, #6665 and #6669 which address the `linkerd2-cni`, `linkerd-multicluster` and `linkerd-jaeger` charts. 

Additional changes from what's already mentioned above:
- Removes the install-namespace option from `linkerd install-cni`, which isn't found in `linkerd install` nor `linkerd viz install` anyways, and it would add some complexity to support.
- Added a dependency on the `partials` chart to the `linkerd-multicluster-link` chart, so that we can tap on the `partials.namespace` helper.
- We don't have any more the restriction on having the muticluster objects live in a separate namespace than linkerd. It's still good practice, and that's the default for the CLI install, but I removed that validation.


Finally, as a side-effect, the `linkerd mc allow` subcommand was fixed; it has been broken for a while apparently:

```console
$ linkerd mc allow --service-account-name foobar
Error: template: linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml:16:7: executing "linkerd-multicluster/templates/remote-access-service-mirror-rbac.yaml" at <include "partials.annotations.created-by" $>: error calling include: template: no template "partials.annotations.created-by" associated with template "gotpl"
```
---------
Update: see helm/helm#5465 describing the current best-practice

# Core Helm Charts Split

This removes the `linkerd2` chart, and replaces it with the `linkerd-crds` and `linkerd-control-plane` charts. Note that the viz and other extension charts are not concerned by this change.

Also note the original `values.yaml` file has been split into both charts accordingly.

### UX

```console
$ helm install linkerd-crds --namespace linkerd --create-namespace linkerd/linkerd-crds
...
# certs.yaml should contain identityTrustAnchorsPEM and the identity issuer values
$ helm install linkerd-control-plane --namespace linkerd -f certs.yaml linkerd/linkerd-control-plane
```

### Upgrade

As explained in #6635, this is a breaking change. Users will have to uninstall the `linkerd2` chart and install these two, and eventually rollout the proxies (they should continue to work during the transition anyway).

### CLI

The CLI install/upgrade code was updated to be able to pick the templates from these new charts, but the CLI UX remains identical as before.

### Other changes

- The `linkerd-crds` and `linkerd-control-plane` charts now carry a version scheme independent of linkerd's own versioning, as explained in #7405.
- These charts are Helm v3, which is reflected in the `Chart.yaml` entries and in the removal of the `requirements.yaml` files.
- In the integration tests, replaced the `helm-chart` arg with `helm-charts` containing the path `./charts`, used to build the paths for both charts.

### Followups

- Now it's possible to add a `ServiceProfile` instance for Destination in the `linkerd-control-plane` chart.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@alpeb alpeb

Labels
area/helm
Projects
None yet
Milestone
stable-2.12.0
3 participants
@alpeb @adleong @kleimkuhler