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

fqdn: Add Protocol to DNS Proxy Cache #31328

Merged
merged 5 commits into from Mar 26, 2024
Merged

fqdn: Add Protocol to DNS Proxy Cache #31328

merged 5 commits into from Mar 26, 2024

Conversation

nathanjsweet
Copy link
Member

DNS Proxy indexes domain selectors by port
only. In cases where protocols collide on port
the DNS proxy may have a more restrictive selector than it should because it does not merge port
protocols for L7 policies (only ports).

Refactor all users of the DNS Proxy are updated
to add protocol to any DNS Proxy entries, and all
tests are updated to test for port-protocol
merge errors.

fqdn: Fixed bug that caused DNS Proxy to be overly restrictive on allowed DNS selectors.

@nathanjsweet nathanjsweet requested review from a team as code owners March 11, 2024 20:35
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 11, 2024
@nathanjsweet nathanjsweet added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.15.2 Mar 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.13 Mar 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.8 Mar 11, 2024
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/fix-dns-rules-policy-lockdown-bug branch from 837a2f4 to d27bad7 Compare March 12, 2024 01:59
pippolo84
pippolo84 requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Note: the precheck error is due to a missing update of MockFQDNProxy.UpdateAllowed signature in pkg/fqdn/proxy/proxy.go.

pkg/endpoint/policy.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/helpers.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/helpers.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/helpers.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/helpers_test.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved
pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved
pkg/fqdn/restore/restore.go Outdated Show resolved Hide resolved
@squeed
Copy link
Contributor

squeed commented Mar 12, 2024

Does it make sense to have a generic ProtoPort struct somewhere? That would make the signatures a lot simpler, as well as being a proper map key. It would also help prevent future errors where we talk about port without protocol.

@christarazi christarazi added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. area/fqdn Affects the FQDN policies feature labels Mar 12, 2024
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/fix-dns-rules-policy-lockdown-bug branch from d27bad7 to c2c1128 Compare March 12, 2024 20:03
@nathanjsweet nathanjsweet added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Mar 13, 2024
@nathanjsweet
Copy link
Member Author

nathanjsweet commented Mar 13, 2024 •
edited

This affects upgrades in a way that needs to be better fleshed out 😢.

Edit:
Figured it out! Thanks to @squeed and @christarazi for your help!

@jrajahalme jrajahalme removed this from Needs backport from main in 1.15.2 Mar 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.13.13 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.13.13 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.13.14 Mar 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.13.13 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.14.9 Mar 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.14.8 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.13.15 Mar 26, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.13.14 Mar 26, 2024
@jrajahalme jrajahalme added this to Needs backport from main in 1.14.10 Mar 26, 2024
@jrajahalme jrajahalme removed this from Needs backport from main in 1.14.9 Mar 26, 2024
@nathanjsweet
Copy link
Member Author

/ci-verifier

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 26, 2024
@nathanjsweet nathanjsweet added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 5185252 Mar 26, 2024
230 checks passed
@nathanjsweet nathanjsweet deleted the pr/nathanjsweet/fix-dns-rules-policy-lockdown-bug branch March 26, 2024 17:26
@nathanjsweet nathanjsweet added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 1, 2024
@nathanjsweet nathanjsweet mentioned this pull request Apr 1, 2024
Merged
This was referenced Apr 2, 2024
Merged
Merged
@joamaki joamaki added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Apr 2, 2024
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Apr 3, 2024
@nathanjsweet nathanjsweet mentioned this pull request Apr 5, 2024
Merged
@nathanjsweet nathanjsweet added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Apr 5, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Apr 8, 2024
@nathanjsweet nathanjsweet added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Apr 9, 2024
@asauber asauber moved this from Needs backport from main to Backport done to v1.13 in 1.13.15 Apr 11, 2024
@asauber asauber mentioned this pull request Apr 11, 2024
Merged
@asauber asauber moved this from Needs backport from main to Backport done to v1.14 in 1.14.10 Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi left review comments

@squeed squeed squeed approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

Assignees
No one assigned
Labels
area/fqdn Affects the FQDN policies feature backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
No open projects
1.13.15
Backport done to v1.13
1.14.10
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nathanjsweet @squeed @pippolo84 @christarazi @joamaki @julianwiedmann