FQDN L3 aware DNS proxy #9587
FQDN L3 aware DNS proxy #9587
Conversation
raybejjani
added
kind/bug
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
26603cc
to
afad032
Compare
There was a problem hiding this comment.
For a bugfix, I'm curious about whether you consider a full LPM to be a requirement or whether it's reasonable to get away with only iterating the set of prefixes currently in use (would handle most use cases like only FQDN policies, no CIDR policies).
A few other minor comments/suggestions below. In particular it would be good to see some form of reproducer in unit-test form.
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
2 times, most recently
from
c2bc661
to
bb3c293
Compare
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
bb3c293
to
8f569df
Compare
joestringer
added
the
release-note/bug
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
8f569df
to
f55468d
Compare
|
test-me-please |
There was a problem hiding this comment.
Nice work. There's just a couple of inconsistencies in the unit testing that are confusing, could you take a fresh look at those?
I left some other minor comments as well but I don't think any of them are consequential. Assuming you fix up the unit tests (or describe what I missed), LGTM.
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
f55468d
to
30e53b7
Compare
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
30e53b7
to
7532c6d
Compare
|
test-me-please |
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
7532c6d
to
b0b9fc2
Compare
|
test-me-please (flakes) |
|
test-me-please (passed) |
|
test-me-please |
|
Oops, I didn't meant to ask for another review, joestringer. Ignore me! |
There was a problem hiding this comment.
One suggestion for a follow-up: Maybe compute selector-based rules when the policy is first injected, so that the rules would be natively shared by all the Endpoints applying the selector policy in question?
The other comment is just a nit about unused return error value, removal of which would also remove some dead code for checking the error return.
|
|
||
| // rejectReply is the OPCode send from the DNS-proxy to the endpoint if the | ||
| // DNS request is invalid | ||
| rejectReply int | ||
| } | ||
|
|
||
| // perEPAllow maps EndpointIDs to ports + selectors + rules | ||
| type perEPAllow map[uint64]portToSelectorAllow |
There was a problem hiding this comment.
Maybe consider tracking ports, selectors, and rules by a SecurityID instead of by EndpointID? Seems we could share the state between endpoints that have the same SecurityID here. There would be some additional complexity though due to policies being created per EP. policy.L4Filter.L7RulesPerEp might be a proper place to keep this information. Note that there is an another (wip) PR (#9486) that generalizes L7RulesPerEp a little, allowing for computed state being stored there, not just the api.L7Rules.
Visibility policies are generated specifically for the DNS proxy as allow-all policies. The L3 selector is also setup to be a select-all, but would crash when .Selects was invoked on it. The selector is now safer to use, particularly as it self-identities as a wildcard selector and skips the panicking line. Signed-off-by: Ray Bejjani <ray@isovalent.com>
The DNS proxy now accounts for the target L3 selector and destination port, only allowing requests whilelisted specifically for that L3/L4 pair. ipcache lookups iterate over in-use prefix lengths, which will likely be bounded in most scenarios. Regexpmap is also removed as the proxy was the final user. Signed-off-by: Ray Bejjani <ray@isovalent.com>
raybejjani
force-pushed
the
fqdn-l3-aware-proxy
branch
from
b0b9fc2
to
09b6426
Compare
|
test-me-please |
The DNS proxy now accounts for the target L3 selector, only allowing
requests whilelisted for that L3 selector. L4 dependencies are still
left to BPF redirection.
Backporters
I have this backported in #9616.
This change is