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

Add support for Linkerd 2.13 #1417

Merged
merged 2 commits into from
May 8, 2023
Merged

Add support for Linkerd 2.13 #1417

merged 2 commits into from
May 8, 2023

Conversation

alpeb
Copy link
Contributor

@alpeb alpeb commented Apr 24, 2023

In Linkerd 2.13 the Prometheus instance in the linkerd-viz namespace is now locked behind an
AuthorizationPolicy that only allows access to the metrics-api ServiceAccount.

This adds an extra AuthorizationPolicy to authorize the flagger ServiceAccount. It's created by default when using Kustomize, but needs to be opted-in when using Helm via the new linkerdAuthPolicy.create value.
This also implies that the Flagger workload has to be injected by the Linkerd proxy, and that can't happen in the same linkerd namespace where the control plane lives, so we're moving Flagger into the new injected flagger-system namespace.

The namespace field in kustomization.yml was resetting the namespace for the new AuthorizationPolicy resource, so that gets restored back to linkerd-viz using a patchesJson6902 entry. A better way to do this would have been to use the unsetOnly field in a NamespaceTransformer (see kubernetes-sigs/kustomize#4708) but for the life of me I couldn't make that work...

@aryan9600
Copy link
Member

hey @alpeb, thanks for opening this PR! could you also bump the linkerd version used in the e2e tests (see test/linkerd/install.sh)

test/linkerd/install.sh Outdated Show resolved Hide resolved
@aryan9600
Copy link
Member

aryan9600 commented Apr 26, 2023

I am thinking about how this change will impact users on Linkerd <= 2.11.x, since it looks like AuthorizationPolicy was introduced in 2.12 (please correct me if I'm wrong). I think it should be the same except for the fact that Flagger will be installed in the linkerd-flagger ns instead of the linkerd ns. But the issue is that kustomize will report an error about the required CRD not being installed. Maybe we can get away with a note about this in the docs, but I'm not 100% sure so asking @stefanprodan to weigh in here.

@stefanprodan
Copy link
Member

I would prefer to use the flagger-system namespace which is part of the default installation. To @aryan9600 point changing the namespace and adding a custom resource is major breaking change. Let's add the AuthorizationPolicy as an opt-in to Flagger Helm chart too.

@alpeb
Copy link
Contributor Author

alpeb commented May 3, 2023

Thanks for all your feedback 👍

In my latest push I've changed the namespace to flagger-system and updated the Helm chart with a couple of additional entries in the values.yaml file.
I also reorganized the commits a little bit.

@aryan9600
Copy link
Member

aryan9600 commented May 5, 2023

okay so tests are finally passing :)))
since there are so many Deployments that linkerd installs, CPU requests were getting exhausted, so I had to scale down a few unnecessary Deployments in the linkerd-viz ns and delete the podinfo Daemonset since we don't need it for linkerd e2e anyway.
@alpeb it'd be great if you could modify the docs to reflect the caveats of installing Flagger with Linkerd on <=v2.11 vs >=2.12 and using Helm vs Kustomize. After which, if you could squash these commits into 1-2 meaningful commits, then this should be ready to merge.

@alpeb
Copy link
Contributor Author

alpeb commented May 6, 2023

Thanks for having taken care of that test! 😅
I've updated the linkerd tutorial and squashed the commits 👍

alpeb added 2 commits May 8, 2023 06:33
In Linkerd 2.13 the Prometheus instance in
the `linkerd-viz` namespace is now locked behind an
[_AuthorizationPolicy_](https://github.com/linkerd/linkerd2/blob/stable-2.13.1/viz/charts/linkerd-viz/templates/prometheus-policy.yaml)
that only allows access to the `metrics-api` _ServiceAccount_.

This adds an extra _AuthorizationPolicy_ to authorize the `flagger`
_ServiceAccount_. It's created by default when using Kustomize, but
needs to be opted-in when using Helm via the new
`linkerdAuthPolicy.create` value. This also implies that the Flagger
workload has to be injected by the Linkerd proxy, and that can't happen
in the same `linkerd` namespace where the control plane lives, so we're
moving Flagger into the new injected `flagger-system` namespace.

The `namespace` field in `kustomization.yml` was resetting the namespace
for the new _AuthorizationPolicy_ resource, so that gets restored back
  to `linkerd-viz` using a `patchesJson6902` entry. A better way to do
  this would have been to use the `unsetOnly` field in a
  _NamespaceTransformer_ (see kubernetes-sigs/kustomize#4708) but for
  the life of me I couldn't make that work...

Signed-off-by: Alejandro Pedraza <[email protected]>
Signed-off-by: Alejandro Pedraza <[email protected]>
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks @alpeb 🙇

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @alpeb

PS. @aryan9600 please make sure we document the breaking changes in the changelog, so people know they must delete the Flagger deployment from the linkerd namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants