Support ip_families field in service #1161
Conversation
howardjohn
added
the
release-notes-none
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
|
/test all |
istio-testing
removed
the
do-not-merge/work-in-progress
src/dns/server.rs
Outdated
| service | ||
| .endpoints | ||
| .iter() | ||
| .filter_map(|(_, ep)| match &ep.address { | ||
| Some(addr) => { | ||
| if let Some(false) = family.map(|f| f.accepts_ip(addr.address)) { |
There was a problem hiding this comment.
@howardjohn I am not quite understanding this, checking record_type has already filtered unmatched ipfamily.
There was a problem hiding this comment.
example is a user queries A for a service with IP family Ipv6. We need to move sure we do NOT return ipv4 addresses
There was a problem hiding this comment.
If this is a clientside defense against a server bug, it needs an error log.
(And a test, this is basically covering a scenario where a v4 is returned for an AAAA record, which is violently against the DNS spec)
There was a problem hiding this comment.
This is not protection against a bug. Its perfectly valid for a client to send a A request for a service that is specified as IPv6 only, we just need to return no IPs.
Additionally ,there is a test that covers this exact scenario that I added alongside the PR.
(And a test, this is basically covering a scenario where a v4 is returned for an AAAA record, which is violently against the DNS spec)
No, I think there is a confusion. This is:
- Client sends A request for Service X
- Service X is IPv6 only, selects pods that are IPv4 AND IPv6
- Before: we would return IPv4 address. Expected/with this PR: we do not respond
In no scenario are were (with or without this PR) sending IPv4 responses to AAAA or IPv6 responses to A
There was a problem hiding this comment.
That being said I think I can short-circuit this check to be at the service level instead of the pod level to give the same behavior.
There was a problem hiding this comment.
Ok, changed to a service level check. Should be more clear now hopefully
There was a problem hiding this comment.
LMK if that addresses the concerns
Istio side istio/istio#51606
This implements the new ip_families field for services:
Add a new IPFamilies field to service. This is used for two things:
Fixes #1152