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

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

Closed
joebowbeer opened this issue Dec 22, 2022 · 3 comments · Fixed by #10044
Closed

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

joebowbeer opened this issue Dec 22, 2022 · 3 comments · Fixed by #10044
Labels
bug

Comments

@joebowbeer
Copy link
Contributor

joebowbeer commented Dec 22, 2022 •
edited
Loading

What is the issue?

1️⃣ The jaeger helm chart creates four resources without a namespace:

https://github.com/linkerd/linkerd2/blob/main/jaeger/charts/linkerd-jaeger/templates/namespace-metadata-rbac.yaml

Resources:

  • ServiceAccount namespace-metadata
  • Role namespace-metadata
  • RoleBinding namespace-metadata

https://github.com/linkerd/linkerd2/blob/main/jaeger/charts/linkerd-jaeger/templates/namespace-metadata.yaml

Resource:

  • Job namespace-metadata

2️⃣ The viz helm chart creates four resources without a namespace:

https://github.com/linkerd/linkerd2/blob/main/viz/charts/linkerd-jaeger/templates/namespace-metadata-rbac.yaml

Resources:

  • ServiceAccount namespace-metadata
  • Role namespace-metadata
  • RoleBinding namespace-metadata

https://github.com/linkerd/linkerd2/blob/main/viz/charts/linkerd-jaeger/templates/namespace-metadata.yaml

Resource:

  • Job namespace-metadata

3️⃣ ditto for multicluster charts

How can it be reproduced?

It's obvious from looking at the template source.

I discovered this by trying to install linkerd-jaeger and having my no-default-namespace admission policy deny my request.

Logs, error output, etc

policy disallow-default-namespace -> resource default/ServiceAccount/namespace-metadata failed:

  1. validate-namespace: validation error: Using 'default' namespace is not allowed. rule validate-namespace failed at path /metadata/namespace/

policy disallow-default-namespace -> resource default/Job/namespace-metadata failed:

  1. validate-podcontroller-namespace: validation error: Using 'default' namespace is not allowed for pod controllers. rule validate-podcontroller-namespace failed at path /metadata/namespace/

Note: The Role and RoleBinding are also missing a namespace

output of linkerd check -o short

N/A

Environment

N/A

Possible solution

Add namespace: {{ .Release.Namespace }} to the four resources where it is missing.

Additional context

I think this was a regression that happened in #6635

Would you like to work on fixing this bug?

yes
@joebowbeer joebowbeer added the bug label Dec 22, 2022
@joebowbeer joebowbeer changed the title Regression: linkerd-jaeger helm chart installs 4 resources in default namespace linkerd-jaeger helm chart installs 4 resources in default namespace Dec 22, 2022
joebowbeer referenced this issue Dec 22, 2022
@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.
joebowbeer added a commit to joebowbeer/linkerd2 that referenced this issue Dec 22, 2022
@joebowbeer
Add ns to namespace-metadata resources (linkerd#10043)
980178b 
Signed-off-by: Joe Bowbeer <joe.bowbeer@gmail.com>
@joebowbeer joebowbeer mentioned this issue Dec 22, 2022
Merged
@joebowbeer joebowbeer changed the title linkerd-jaeger helm chart installs 4 resources in default namespace jaeger and viz helm charts install resources in default namespace Dec 22, 2022
joebowbeer added a commit to joebowbeer/linkerd2 that referenced this issue Dec 22, 2022
@joebowbeer
Add missing ns to viz helm charts (linkerd#10043)
a9622e4 
Signed-off-by: Joe Bowbeer <joe.bowbeer@gmail.com>
@alpeb
Copy link
Member

alpeb commented Dec 22, 2022

It's ok to add the namespace like you did in #10044 via .Release.Namespace, but it's not really necessary. When you install with helm install --namespace, those resources will be put under the specified namespace. How are you installing linkerd, that is resulting in a different outcome?

@joebowbeer
Copy link
Contributor Author

joebowbeer commented Dec 22, 2022 •
edited
Loading

@alpeb I am templating the chart (v30.4.5) so that it can be kustomized and installed by ArgoCD:

helm template linkerd ./v30.4.5 -n linkerd-jaeger

What I see in the jaeger and viz charts source, is that every resource is namespaced except these four, and helm template -n is not supplying a namespace for those four resources. (helm v3.9.4)

I didn't realize there might be a difference between install and template with respect to namespace injection, but given that ever other resource in the charts is namespaced, I suggest adding it - both for consistency and for template correctness.

@alpeb
Copy link
Member

alpeb commented Dec 22, 2022

Good point 💯 , I had forgotten about that linkerd template gotcha!

joebowbeer added a commit to joebowbeer/linkerd2 that referenced this issue Dec 23, 2022
@joebowbeer
Add ns to multicluster helm charts (linkerd#10043)
6bbb8eb 
Signed-off-by: Joe Bowbeer <joe.bowbeer@gmail.com>
kleimkuhler pushed a commit that referenced this issue Jan 3, 2023
@joebowbeer
Add ns to namespace-metadata resources (#10043) (#10044)
b8ee97e 
Closes #10043 

Signed-off-by: Joe Bowbeer <joe.bowbeer@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2023
hawkw pushed a commit that referenced this issue Feb 3, 2023
@joebowbeer @hawkw
Add ns to namespace-metadata resources (#10043) (#10044)
f0db6ae 
Closes #10043 

Signed-off-by: Joe Bowbeer <joe.bowbeer@gmail.com>
hawkw pushed a commit that referenced this issue Feb 6, 2023
@joebowbeer @hawkw
Add ns to namespace-metadata resources (#10043) (#10044)
5d277b1 
Closes #10043 

Signed-off-by: Joe Bowbeer <joe.bowbeer@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add ns to namespace-metadata resources (#10043) joebowbeer/linkerd2
2 participants
@joebowbeer @alpeb