Skip to content

fix(cni-plugin): append inbound skip ports instead of replacing #518

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

Merged
merged 2 commits into from
May 17, 2025

Conversation

adleong
Copy link
Member

@adleong adleong commented May 5, 2025

Fixes linkerd/linkerd2#13976

When the Linkerd CNI-plugin acts on a pod with a config.linkerd.io/skip-inbound-ports annotation, it replaces the inbound skip ports that the CNI-plugin was configured with instead of adding to them. The CNI-plugin is configured with the Linkerd proxy's admin and tap ports as skip inbound ports. Therefore, on any pod with the config.linkerd.io/skip-inbound-ports annotation, the admin and tap ports will no longer be skipped by the iptables redirect rules, making these proxy ports inaccessible and causing tap and promethus scraping to no longer function.

Instead of replacing, we append the ports specified in config.linkerd.io/skip-inbound-ports to the inbound skip ports. This allows the admin and tap ports to continue to skip iptables redirection and reach the proxy as desired.

This matches the behavior of the linkerd-init container: https://github.com/linkerd/linkerd2/blob/main/charts/partials/templates/_proxy-init.tpl#L25

Tested manually on k3d with Calico and validating that tap works as expected on workloads with config.linkerd.io/skip-inbound-ports configured. Due to the architecture of the cni-plugin executable, it is not well set up to be tested automatically without further refactoring so we omit automated tests here.

@adleong adleong requested a review from a team as a code owner May 5, 2025 22:01
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 🚢

@@ -25,6 +25,9 @@ kubectl apply -f https://k3d.io/v5.1.0/usage/advanced/calico.yaml
kubectl --namespace=kube-system wait --for=condition=available --timeout=120s \
deploy/calico-kube-controllers

# Install gateway API
kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.2.1/experimental-install.yaml
Copy link
Member

Choose a reason for hiding this comment

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

🙇‍♂️

@cratelyn
Copy link

a pendatic question, but should this be fix(cni-plugin)!: labeled as a breaking change, since the behavior of this annotation has been changed?

i'm not strongly attached to either side of this choice, but want to raise the question for consideration.

@adleong
Copy link
Member Author

adleong commented May 12, 2025

@cratelyn I agree that this is a grey area but I lean toward not marking it as breaking because

  • the current behavior is clearly not intentional
  • it feels unlikely that anyone would be depending on the current behavior

@cratelyn
Copy link

@cratelyn I agree that this is a grey area but I lean toward not marking it as breaking because

* the current behavior is clearly not intentional

* it feels unlikely that anyone would be depending on the current behavior

makes sense to me 🙇‍♀️ thanks!

@adleong adleong merged commit 78a82d6 into main May 17, 2025
15 checks passed
@adleong adleong deleted the alex/inbound-skip branch May 17, 2025 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants