Remove namespace from linkerd2-viz chart #6638
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
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.
Codecov Report
@@ Coverage Diff @@
## alpeb/no-ns-helm-core #6638 +/- ##
=========================================================
- Coverage 45.38% 45.35% -0.03%
=========================================================
Files 182 182
Lines 24440 24273 -167
Branches 290 290
=========================================================
- Hits 11091 11010 -81
+ Misses 12560 12488 -72
+ 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.
|
alpeb
changed the title
mateiidavid
approved these changes
Aug 12, 2021
There was a problem hiding this comment.
Tested and it all works well. No comments on the actual manifest changes.
$ helm install linkerd-viz \
--create-namespace --namespace vizz \
-f charts/linkerd-viz/values.yaml charts/linkerd-viz
# installed successfully
# job successfully annotated namespace
#
$ k get ns vizz -o yaml
apiVersion: v1
kind: Namespace
metadata:
annotations:
config.linkerd.io/proxy-await: enabled
creationTimestamp: "2021-08-12T15:24:36Z"
labels:
kubernetes.io/metadata.name: vizz
linkerd.io/extension: viz
name: vizz
resourceVersion: "10420"
uid: c21ee6de-b7b5-44c1-bb20-68cece106cd3
spec:
finalizers:
- kubernetes
status:
phase: Active
adleong
approved these changes
Aug 13, 2021
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.
Second part of #6584, followup of #6635 (and based off of alpeb/no-ns-helm-core)
Stop rendering
namespace.yamlin thelinkerd-vizchart, same as we did in #6635.The additional change here is the addition of the
namespace-metadata.yamltemplate (and its RBAC), not rendered in CLI installs, which is a Helmpost-installhook, 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/curlimage, 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. Thecurlimages/curlimage is based on Alpine; it won't have as much 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.