Remove namespace from charts and split them into `linkerd-crds` and `linkerd-control-plane` #6635

alpeb · 2021-08-09T18:18:00Z

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:

$ 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

$ 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 Helm charts versioning scheme #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.

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.

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.

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.

adleong

Nice. What will the upgrade procedure be for Helm users? I take it they have to do a uninstall and then reinstall of the helm chart? Assuming they use the same issuer certificates, can they keep the data plane running the entire time? Will there be any interruption to data plane traffic?

adleong · 2021-08-10T23:13:53Z

Just to make sure I understand: when installing with Helm, is the expectation is that namespace will already exist because either

a) the user created the linkerd namespace ahead of time or
b) the user passed the --create-namepsace flag to helm install

is that right?

Pothulapati

Changes LGTM but I would love to know more about the upgrade procedure. Especially if we can figure out a no down-time way of upgrading existing linkerd Helm installs.

From what I understand, The real pain-point with Helm is its secret which is being stored into the default namespace. (irrespective of which namespace the control-plane is being installed). Can we define some manual steps through which we can move that secret into the namespace in which linkerd is installed into, and from there on a normal upgrade would be sufficient? I guess we would be left with some stale namespace values when we are moving the secret but this should still work because we would be passing the right --linkerd-namespace flag with the upgrade which should correctly set all the values as expected🤔

Pothulapati · 2021-08-11T05:11:44Z

cli/cmd/install.go

-	if err != nil {
-		return nil, err
-	}
-	namespace, err := valuesTree.GetString("namespace")


Hmm, This is removed because. Now we don't need to find if namespace has been overriden through helm values, etc, and there's now only a single way to override namespace which is through --namespace flag propogated into controlPlaneNamespace, right?

That's correct.

Pothulapati · 2021-08-11T06:55:21Z

test/integration/install_test.go

@@ -509,7 +508,6 @@ func helmOverridesEdge(root *tls.CA) ([]string, []string) {
 	}
 	vizArgs := []string{
 		"--set", "linkerdVersion=" + TestHelper.GetVersion(),
-		"--set", "namespace=" + TestHelper.GetVizNamespace(),


These are vizArgs right? and we aren't removing that in this PR right? 🤔

Correct, that's addressed in #6638

alpeb · 2021-08-11T12:37:44Z

Thanks gentlemen for the reviews 👍

What will the upgrade procedure be for Helm users? I take it they have to do a uninstall and then reinstall of the helm chart? Assuming they use the same issuer certificates, can they keep the data plane running the entire time? Will there be any interruption to data plane traffic?

Yeah, they'll have to uninstall the chart and install the new chart again. The proxies in the data plane will log errors while the control plane gets reinstalled, but from my testing, traffic continues to flow between data plane pods. And after the control plane is available again, the proxies stop logging errors.

when installing with Helm, is the expectation is that namespace will already exist because either

a) the user created the linkerd namespace ahead of time or
b) the user passed the --create-namepsace flag to helm install

is that right?

That's correct.

Can we define some manual steps through which we can move that secret into the namespace in which linkerd is installed into, and from there on a normal upgrade would be sufficient?

Sounds tricky, not sure if a simple move will suffice. Also note there's also an upcoming PR that will split the core Helm chart into two (#6620), so we'll end up with totally different charts, which means we'll have to uninstall the old one anyways.

alpeb · 2021-08-11T12:41:56Z

Note: please don't merge this just yet, as I want to have all the Helm charts changes (including #6620) to be included in the same edge 😉

Pothulapati · 2021-08-12T07:49:27Z

Sounds tricky, not sure if a simple move will suffice. Also note there's also an upcoming PR that will split the core Helm chart into two (#6620), so we'll end up with totally different charts, which means we'll have to uninstall the old one anyways.

Hmm, Makes sense!

mateiidavid

Tested this out and it works really well. Confirmed the changes by installing with helm (helm install --create-namespace --namespace linkerd) and also with (helm install --generate-namespace --namespace dev). Both worked well to install the control plane.

Also looked in the configmap to see if we are missing the namespace field (since that's no longer persisted in the cm), and also had a look at the injector manifest to double check the flag is in. Can confirm everything worked well and control plane was up and running.

The only thing here is that when we create a namespace that doesn't have the default name (e.g dev), checks and injections fail unless we explicitly pass in --linkerd-namespace. I guess this is by design though since we have historically supported installing in a separate namespace for a while -- I've never personally done it in tests until now so thought I'd mention it.

I'll hold off on approval until the rest of the changes are merged in, will want to re-test it everything then but so far everything seems to be 👌🏻 👌🏻 .

alpeb · 2021-08-12T13:17:29Z

Thanks @mateiidavid ,

The only thing here is that when we create a namespace that doesn't have the default name (e.g dev), checks and injections fail unless we explicitly pass in --linkerd-namespace. I guess this is by design though since we have historically supported installing in a separate namespace for a while -- I've never personally done it in tests until now so thought I'd mention it.

Yep, no change in functionality there, check and inject have required expliciting the namespace for a while, when not using the default one.

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" ```

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.

* 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 · 2021-08-13T19:44:53Z

I've just merged #6638 here and updated this PR's description accordingly.

* 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.

mateiidavid · 2021-12-02T15:43:16Z

@kleimkuhler ah thanks, yeah that makes a lot of sense. I failed to consider this might be turned off. Disregard what I said then this is definitely good to 🚢 from my side!

Fixes #6620, branched off of #6635 (remove namespace from charts) This removes the `linkerd2` chart, and replaces it with the `linkerd-base` and `linkerd-control-plane` charts. Note that the viz and other extension charts are not concerned by this change. `linkerd-base` and `linkerd-control-plane` mimic what the CLI commands `linkerd install config` (cluster-level and RBAC resources) and `linkerd install control-plane` (everything else, namespace-scoped resources) do, respectively. Please see #6620 for more details. Also note the original `values.yaml` file has been split into both charts accordingly. ### UX ```console $ helm install linkerd-base --namespace linkerd --create-namespace linkerd/linkerd-base ... # 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 - 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 - Update the `release.yaml` workflow to push these new charts. The old one probably needs a deprecation tag or something like that. - Now it's possible to add a `ServiceProfile` instance for Destination in the `linkerd-control-plane` chart.

alpeb · 2021-12-03T14:21:00Z

I merged #6691 here and updated the description accordingly.

alpeb · 2021-12-09T19:34:13Z

After some feedback we decided to rename linkerd-base to linkerd-crds, which now only holds the CRDs.
Also please check #7448 which takes care of the versioning changes for both charts.

Still looks good. Left some comments on #7448 which once merged, I can approve this again.

* Set up helm charts versioning Implements part of #7405 Separates `version` from `appVersion` in both `linkerd-crds` and `linkerd-control-plane` charts. In `linkerd-crds` embeds `version` as a label in the CRDs. In `linkerd-control-plane`, embeds `linkerd-crds`'s `version` as a new entry in the `linkerd-config` ConfigMap, to use in a new `linkerd check` that will be implemented in a separate PR.

* Helm migration instructions for 2.12 This updates the helm install instructions to account for the new charts as per linkerd/linkerd2#6635 and linkerd/linkerd2#6691 Also this adds a new section for 2.12 in `upgrade.md`, detailing the migration steps needed for the new charts. Co-authored-by: Matei David <matei@buoyant.io>

Followup to #6635, where we missed removing the no longer used `namespace` entry.

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>

alpeb added the area/helm label Aug 9, 2021

alpeb requested a review from a team as a code owner August 9, 2021 18:18

Fix helm-deep test, and bypass helm upgrade test for now

9f6ee44

alpeb mentioned this pull request Aug 9, 2021

Remove namespace from linkerd2-viz chart #6638

Merged

alpeb changed the title ~~Remove namespace for linkerd2 chart~~ Remove namespace from linkerd2 chart Aug 10, 2021

alpeb force-pushed the alpeb/no-ns-helm-core branch from 13ab43b to 9f6ee44 Compare August 10, 2021 19:38

alpeb mentioned this pull request Aug 10, 2021

Remove namespace from linkerd2-cni chart #6643

Merged

adleong approved these changes Aug 10, 2021

View reviewed changes

Pothulapati reviewed Aug 11, 2021

View reviewed changes

Delete ns in bin/test-cleanup

898e722

mateiidavid reviewed Aug 12, 2021

View reviewed changes

alpeb mentioned this pull request Aug 12, 2021

Remove namespace from multicluster charts #6665

Merged

alpeb mentioned this pull request Aug 12, 2021

Remove namespace from linkerd-jaeger chart #6669

Merged

alpeb added 3 commits August 13, 2021 15:09

Merge branch 'main' into alpeb/no-ns-helm-core

872634d

Update policy resources

c9cf389

alpeb changed the title ~~Remove namespace from charts~~ Remove namespace from charts and split them into linkerd-base and linkerd-control-plane Dec 3, 2021

alpeb added 2 commits December 3, 2021 08:26

Merge branch 'main' into alpeb/no-ns-helm-core

2717703

Remove trafficsplit CRD

cda5102

linkerd deleted a comment from codecov-commenter Dec 3, 2021

alpeb added 3 commits December 7, 2021 09:41

Update helm-build, enableEndpointSlices, kubeVersion, helm READMEs

c7a0b19

use linkerd-crds instead of linkerd-base

5d3eb2f

Merge branch 'main' into alpeb/no-ns-helm-core

09e2923

alpeb changed the title ~~Remove namespace from charts and split them into linkerd-base and linkerd-control-plane~~ Remove namespace from charts and split them into linkerd-crd and linkerd-control-plane Dec 9, 2021

kleimkuhler approved these changes Dec 10, 2021

View reviewed changes

alpeb merged commit f9f3ebe into main Dec 10, 2021

alpeb deleted the alpeb/no-ns-helm-core branch December 10, 2021 20:53

alpeb changed the title ~~Remove namespace from charts and split them into linkerd-crd and linkerd-control-plane~~ Remove namespace from charts and split them into linkerd-crds and linkerd-control-plane Dec 13, 2021

This was referenced Jan 10, 2022

Branch off 2.12 docs linkerd/website#1244

Closed

Helm charts versioning scheme #7405

Closed

alpeb added a commit that referenced this pull request Mar 11, 2022

Remove namespace entry in linkerd-control-plane values.yaml

dbfbf42

Followup to #6635, where we missed removing the no longer used `namespace` entry.

alpeb mentioned this pull request Mar 11, 2022

Remove namespace entry in linkerd-control-plane values.yaml #8049

Merged

alpeb added a commit that referenced this pull request Mar 15, 2022

Remove namespace entry in linkerd-control-plane values.yaml (#8049)

e3623c1

Followup to #6635, where we missed removing the no longer used `namespace` entry.

kleimkuhler mentioned this pull request Jun 27, 2022

Render partials in linkerd multicluster allow #8756

Merged

olix0r mentioned this pull request Sep 8, 2022

helm: Restore namespace field in templates #9351

Merged

philomory mentioned this pull request Sep 11, 2022

Incorrect reference to non-existent linkerd-base chart in docs linkerd/website#1491

Closed

joebowbeer mentioned this pull request Dec 22, 2022

jaeger and viz helm charts install resources in default namespace #10043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove namespace from charts and split them into `linkerd-crds` and `linkerd-control-plane` #6635

Remove namespace from charts and split them into `linkerd-crds` and `linkerd-control-plane` #6635

alpeb commented Aug 9, 2021 •

edited

Loading

adleong left a comment

adleong commented Aug 10, 2021

Pothulapati left a comment •

edited

Loading

Pothulapati Aug 11, 2021

alpeb Aug 11, 2021

Pothulapati Aug 11, 2021

alpeb Aug 11, 2021

alpeb commented Aug 11, 2021

alpeb commented Aug 11, 2021

Pothulapati commented Aug 12, 2021

mateiidavid left a comment

alpeb commented Aug 12, 2021

alpeb commented Aug 13, 2021

mateiidavid commented Dec 2, 2021

alpeb commented Dec 3, 2021

alpeb commented Dec 9, 2021 •

edited

Loading

Remove namespace from charts and split them into linkerd-crds and linkerd-control-plane #6635

Remove namespace from charts and split them into linkerd-crds and linkerd-control-plane #6635

Conversation

alpeb commented Aug 9, 2021 • edited Loading

Namespace Removal

Core Helm Charts Split

UX

Upgrade

CLI

Other changes

Followups

adleong left a comment

Choose a reason for hiding this comment

adleong commented Aug 10, 2021

Pothulapati left a comment • edited Loading

Choose a reason for hiding this comment

Pothulapati Aug 11, 2021

Choose a reason for hiding this comment

alpeb Aug 11, 2021

Choose a reason for hiding this comment

Pothulapati Aug 11, 2021

Choose a reason for hiding this comment

alpeb Aug 11, 2021

Choose a reason for hiding this comment

alpeb commented Aug 11, 2021

alpeb commented Aug 11, 2021

Pothulapati commented Aug 12, 2021

mateiidavid left a comment

Choose a reason for hiding this comment

alpeb commented Aug 12, 2021

alpeb commented Aug 13, 2021

mateiidavid commented Dec 2, 2021

alpeb commented Dec 3, 2021

alpeb commented Dec 9, 2021 • edited Loading

Remove namespace from charts and split them into `linkerd-crds` and `linkerd-control-plane` #6635

Remove namespace from charts and split them into `linkerd-crds` and `linkerd-control-plane` #6635

alpeb commented Aug 9, 2021 •

edited

Loading

Pothulapati left a comment •

edited

Loading

alpeb commented Dec 9, 2021 •

edited

Loading