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

Split core Helm charts #6620

Closed
alpeb opened this issue Aug 5, 2021 · 2 comments
Closed

Split core Helm charts #6620

alpeb opened this issue Aug 5, 2021 · 2 comments

Comments

@alpeb
Copy link
Member

alpeb commented Aug 5, 2021

Background

It's not possible to have CRD definitions and instances in the same chart under the templates directory, because it takes time for the new CRD APIs to become available, and attempting to persist instances prematurely will result in an error.

The options

Helm v3 documents two methods for handling CRDs:

  1. Putting CRDs definitions under the templates/crds directory. Helm will first persist what's in there and wait for its availability before processing the other resources under templates/.
  2. Use separate charts. A first one containing the CRD definitions, a second one their instances.

The first option is a no-go because it implies CRDs will get installed and Helm won't ever touch them again. helm upgrade nor helm delete will touch them, so the anytime these require changes, instructions would need to be provided for the user to do that manually. Helm's reasons are documented here.

Our strategy

So we'd go with the second option. Similar to the staged installs available for the CLI, we'd have one first chart ("linkerd-base" or "linkerd-root"?) containing all the CRD definitions along with all the other cluster-level resources, and a second chart ("linkerd-core"?) with the rest.

Like with the staged CLI install, a nice additional benefit is that the first chart could be handed to admin-level folks and the former to operators with only namespace-level access. Note though that, from what I've seen, the staged CLI hasn't seen much adoption. Given the Helm method is more prod oriented, maybe it'd be more relevant in that case?

Are the motivations strong enough?

In order of importance, these are the motivations:

  • Allow us to define instances for the new auth CRDs, for the control plane pods. proxy: Support configurable default inbound policies #6606 added support setting default policies declaratively in the injected proxy manifest. So do we really need auth CRD instances at install-time, or do we just want to leave the door open for adding that in the future?
  • Remove the namespace resource from the Helm charts #6584 proposes to remove namespaces from the charts, which is a breaking change; users will need to uninstall the control plane and reinstall it using the new chart version. This proposal is also breaking. So club them together and call it a day?
  • Very minor point: We'll be able to add a ServiceProfile instance for Destination, which was dropped a couple of releases ago.
alpeb added a commit that referenced this issue Aug 17, 2021
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` 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 README templates and NOTES.txt files explaining what each chart does.
- Now it's possible to add a `ServiceProfile` instance for Destination in the `linkerd-control-plane` chart.
- In the proxy-injector deployment, the `checksum/config` annotation was reverted back to `linkerd.io/helm-release-version`. This was used to redeploy the injector only when the injector RBAC template changed, but we can no longer refer to the RBAC file that lives in a different chart, so the injector will simply be rolled-out on every upgrade.
@stale
Copy link

stale bot commented Nov 3, 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 3, 2021
@adleong adleong removed the wontfix label Nov 9, 2021
@adleong adleong added this to the stable-2.12.0 milestone Nov 9, 2021
alpeb added a commit that referenced this issue Dec 3, 2021
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 added a commit that referenced this issue 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.
@adleong adleong closed this as completed Jan 4, 2022
@kleimkuhler
Copy link
Contributor

Closed by #6691

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants