Add IPv6 support for the destination controller

[alpeb]

Services in dual-stack mode result in the creation of two EndpointSlices, one for each IP family. Before this change, the Get Destination API would nondeterministically return the address for any of those ES, depending on which one was processed last by the controller because they would overwrite each other.

As part of the ongoing effort to support IPv6/dual-stack networks, this change fixes that behavior giving preference to IPv6 addresses whenever a service exposes both families.

We introduce a new field ipv6Only for the servicePublisher struct that is set to true for any service that exposes an IPv6 address. Whenever an ES is added or updated, if ipv6Only is true, it's ignored if its addressType is IPv4.

Also the server unit tests were updated to segregate the tests and resources dealing with the IPv4/IPv6/dual-stack cases.

[adleong]

Are the address families of a service mutable? Is there an ordering concern here where we get an ipv4 endpoint slice update first, then the ipv6 endpoint slice (but the entires from the prior endpoint slice are preferred) and then we get the svc update that tells us that the service is ipv6?

Ideally we could make this more robust to update ordering and eventually consistent. Could we store two address maps in an AddressSet (one for ipv4 addresses and one for ipv6 addresses) and then have the Translator choose between the two similar to how we do for local traffic policy?

[alpeb]

Yes, a service's .spec.ipFamilies field is mutable. An update to it would trigger the associated EndpointSlices to be updated or deleted as well. In theory an ES event should come after the creation/update event for the service it refers to in its ownerReference, but I see your concern, there's nothing guaranteeing us that.

I like your suggestion about having the endpoints watcher index all the IPs and have the translator filter the appropriate ones. I think we'd also have to track the service ipFamilies entry into the AddressSet. So we'd have to pull the ES' associated service from the lister for that, assuming it's already there, which is a softer assumption than the ordering one mentioned above, but a similar one we already make when we pull the associated pod object and that doesn't seem to have caused any issues. And so the translator would have all the information to do the filtering. Does that make sense?

[adleong]

Yeah I agree we probably need to track AddressFamilyMode or something on AddressSet to track if we're in ipv4 or ipv6 mode. We're already watching Services so I don't think we need dedicated lookups for this. In other words:

• when we get an ES update, add the endpoints to either the ipv4 or ipv6 map depending on what kind of address it is
• when we get a Service update, set the AddressFamilyMode depending on the ipFamilies

This way, even if the address mode of a service toggles back and forth, we'll always have the full set of addresses for both address families and we can just swap between them.

[alpeb]

Turns out adding a separate map for IPv6 addresses implies a lot of changes and added complexity. I went for an alternative approach, expanding the key in the Addresses map to include the IP family.

So the translator keeps both IPv4 and IPv6 addresses in availableEndpoints and additional logic in diffEndpoints determines what adds/removes to consider. This logic accounts for the presence/absence of addresses for different families in availableEndpoints, without needing to have to also keep track of the services ipFamilies entry. This also avoids having to add logic handling service object updates (that don't imply ES changes) into the translator. This approach tested well (manually), but I'm not sure how readable that is, so I'm open to suggestions.

In the latest change I'm also consuming the disableIPv6 helm value in the destination controller, that we need to account for as well.

[adleong]

Is this block saying that if the current snapshot contains both IPV6 and IPV4 versions of an address, we should add the IPV4? I don't understand this.

[alpeb]

It means that if we're removing an IPv6 and we already know about its IPv4 counterpart, send an Add event for the IPv4. This would happen for example when a dual-stack service is updated to be single-stack IPv4; the event for deletion of the IPv6 ES is received and we'd like clients to fall back to the IPv4 ES.

[adleong]

Does this say that if the snapshot contains both ipv6 and ipv6 versions of an address then don't remove that address? Shouldn't this depend on what's in filtered?

[alpeb]

In this piece we say that we shouldn't remove an IPv4 address if there's no IPv6 alternative available. You're right we should depend on filtered instead.

[adleong]

Can we also add a test for the interaction between zone filtering and dual-stack?

[alpeb]

Will certainly do.

[adleong]

This updated diffing logic is complex and difficult to think through all the cases and convince ourselves that we didn't miss any. I think this could be simplified by breaking it into two discrete steps:

1. A func selectAddressFamily(addresses watcher.AddressSet) watcher.AddressSet which filters the address set based on address family:

• if enable-ipv6 is disabled, drop all ipv6 addresses
• if the address set contains both versions of an address, drop the ipv4 one

2. This will result in the filtered address set being the final desired state, which means we can just let diffEndpoints do it's thing and diff against the snapshot without being specifically aware of address family. it should generate the diff necessary to bring the filtered snapshot to the desired state.

[alpeb]

The expanded diffing logic was also trying to avoid generating spurious messages, like a Remove that didn't correspond to any previous Add, _but I may be over complicating things. Let me experiment with your proposed approach and see how it goes. I should also include the additional unit tests first to be more confident about these changes.

[alpeb]

Ok I've replaced prefer-ipv6 with enable-ipv6, and added a new selectAddressFamily() filtering function triggered after filterAddresses() but before diffEndpoints() (which remains the same).

Also there are a new set of unit tests in server_ipv6_test.go, and in the TestEndpointTranslatorForPods tests there's a couple of new cases to test the interaction with zone filtering.

[adleong]

Nice!