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

[v1.14] fqdn: Add Protocol to DNS Proxy Cache #31801

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.

[ upstream commit 1941679 ]

DNS Proxy needs to account for protocol when indexing
L7 DNS rules that it needs to adhere to, otherwise
L7 rules with differing port-protocols can override
each other (nondeterministically) and create overly
restrictive, and incorrect DNS rules. The problem with
accounting for protocol is that Endpoint restoration
logic uses DNS rules that index to port-only as JSON
saved to disk. Adding an additional protocol index to
a map structure changes the JSON structure and breaks
restoration logic between Cilium versions.

This change makes the map index backwards compatible,
since it changes the index from a uint16 to a uint32,
both of which marshal the same into a JSON structure.
The endpoint restoration logic will succeed between
versions, because the older version will be
automatically differentiated with a lack of a 1-bit
at bit position 24. Version 2 will save a 1 bit at the
24th bit going forward to differentiate when protocol
is indexed or not present.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet nathanjsweet added release-note/bug This PR fixes an issue in a previous release of Cilium. kind/backports This PR provides functionality previously merged into master. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. labels Apr 5, 2024
@nathanjsweet nathanjsweet requested a review from a team as a code owner April 5, 2024 19:42
@nathanjsweet
Copy link
Member Author

This is a backport of #31328

@nathanjsweet
Copy link
Member Author

/test-backport-1.14

@julianwiedmann
Copy link
Member

@nathanjsweet looks like the integration tests caught a persistent fail: https://github.com/cilium/cilium/actions/runs/8574930543/job/23517128951. I'll switch to draft for now, could you please have a look?

@julianwiedmann julianwiedmann marked this pull request as draft April 8, 2024 02:04
[ upstream commit bc7fbf3 ]

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).

All callers 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.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit 6baab36 ]

DNSRulesV2 accounts for protocol and DNSRules does not.
DNSProxy needs to account for both, and endpoint needs
to be able to restore from a downgrade. DNSRulesV2 is used
by default now, but DNSRules is maintained in case of a
downgrade.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit abd7c6e ]

In cases where a port-protocol is not present
in an restored port protocol, look up
up the Version 1 version of the PortoProto
in case a Version 1 PortProto was restored.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
[ upstream commit 5185252 ]

The Sort methods are updated to take an unused
testing.T structure to indicate to all callers
that they are only for testing purposes.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/backport-to-1-14-fix-dns-rules-policy-lockdown-bug branch from 09d5f12 to 3546693 Compare April 8, 2024 17:03
@nathanjsweet
Copy link
Member Author

/test-backport-1.14

@nathanjsweet nathanjsweet marked this pull request as ready for review April 8, 2024 17:41
@nathanjsweet nathanjsweet added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Apr 8, 2024
@joestringer joestringer merged commit fcc1b41 into v1.14 Apr 8, 2024
223 checks passed
@joestringer joestringer deleted the pr/nathanjsweet/backport-to-1-14-fix-dns-rules-policy-lockdown-bug branch April 8, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants