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

connectivity: Add node-local-dns match labels #995

Conversation

aditighag
Copy link
Member

When node-local dns is deployed along with local
redirect policy [1], the DNS traffic is redirected
to node-local dns cache pods. Hence, allow such DNS
traffic in the connectivity test policy yamls.

[1] https://docs.cilium.io/en/latest/gettingstarted/local-redirect-policy/#node-local-dns-cache

Reported-by: Julien Boulanger julien.boulanger@fr.clara.net
Signed-off-by: Aditi Ghag aditi@cilium.io

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Is there a reason we don't need the matchLabels for node-local DNS in the client-egress-to-entities-world policy?

When node-local dns is deployed along with local
redirect policy [1], the DNS traffic is redirected
to node-local dns cache pods. Hence, allow such DNS
traffic in the connectivity test policy yamls.

[1] https://docs.cilium.io/en/latest/gettingstarted/local-redirect-policy/#node-local-dns-cache

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/connectivity-allow-node-local-dns branch from 821cb66 to 31452e4 Compare July 28, 2022 14:03
@aditighag aditighag temporarily deployed to ci July 28, 2022 14:03 Inactive
@aditighag
Copy link
Member Author

Is there a reason we don't need the matchLabels for node-local DNS in the client-egress-to-entities-world policy?

Good catch, thanks! Fixed it. PTAL.

@aditighag aditighag requested a review from tklauser July 28, 2022 14:03
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks

@aditighag
Copy link
Member Author

aditighag commented Jul 28, 2022

The sole failure in multicluster is a flake, previous failures - https://github.com/cilium/cilium-cli/runs/7496646101?check_suite_focus=true. The PR already have a successful run - https://github.com/cilium/cilium-cli/actions/runs/2750016556. As the changes are purely in the connectivity tests, marking it as ready to merge.

@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 28, 2022
@tklauser tklauser merged commit cc2739a into cilium:master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants