Remove namespace from multicluster charts #6665
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" ```
Codecov Report
@@ Coverage Diff @@
## alpeb/no-ns-helm-core #6665 +/- ##
=========================================================
- Coverage 45.38% 45.33% -0.05%
=========================================================
Files 182 182
Lines 24440 24266 -174
Branches 290 290
=========================================================
- Hits 11091 11001 -90
+ Misses 12560 12490 -70
+ Partials 789 775 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
adleong
approved these changes
Aug 13, 2021
Pothulapati
approved these changes
Aug 16, 2021
@Pothulapati I've just created #6693 to track that effort. |
alpeb
added a commit
that referenced
this pull request
Dec 10, 2021
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another part of #6584, branched off of #6635 (alpeb/no-ns-helm-core).
Stop rendering the namespace in the
linkerd-multiclusterchart as wedid in #6635.
Also:
namespacevalues.yaml entry in thelinkerd-multiclusterandlinkerd-multicluster-linkcharts, andalso the
installNamespaceentry in the former.partialschart to thelinkerd-multicluster-linkchart, so that we can tap on thepartials.namespacehelper.linkerd-multiclusterchart.
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
allowsubcommand was fixed; it has beenbroken for a while apparently: