Initial ambient dual stack support #51606

howardjohn · 2024-06-17T19:33:58Z

This has a few changes:

pass all pod IPs down WDS
Pass all service VIPs down WDS
Add a new IPFamilies field to service. This is used for two things:
1. DNS lookups for headless services MUST not return unsupported families. Without this field, it is impossible to know which workload IPs to filter
2. When we select a workload IP after resolving a service, we must pick a supported family. This is possible to do without for non-headless services (by inferring from the VIPs field), but easier to be explicit.
Enable IPv6 in the CNI by default. This should work in all cases, even on IPv4 only clusters. Note pure IPv4 nodes (no kernel support for ipv6 at all) is already broken on ambient (Issue trying ambient mode on an ipv4-only k8s cluster ztunnel#1131)

Corresponding ztunnel change: istio/ztunnel#1161

istio-testing · 2024-06-17T19:34:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

bleggett · 2024-06-17T21:24:21Z

Ah ok - single stack IPv6 worked in ambient (I think we got at least one successful test) but we still have gotchas/bugs around "things might have more than one IP".

(hopefully not too many of those)

bleggett · 2024-06-17T21:36:36Z

I would rather not use dualstack terminology in ambient tho - we shouldn't need to "support" dualstack. We should support V4 and V6 (together or independently, and probably tested and configured together and/or independently).

If we have bugs when support for both is enabled, that's probably just A Bug, and not really "support for a feature" we lack.

howardjohn · 2024-06-17T22:11:49Z

TBH I disagree. Its totally different to have 2 IPs vs 1.

But also this is a PR not user facing, so we can have the docs folks bikeshed it when the time comes 🙂

bleggett · 2024-06-17T23:14:11Z

TBH I disagree. Its totally different to have 2 IPs vs 1.

It is, my point is "the lack of support for more than one IP" is just a bug, especially in a K8S context.

But also this is a PR not user facing, so we can have the docs folks bikeshed it when the time comes 🙂

It's fine as long as we have tests for either/or/both - I'm less concerned with users here and more concerned with how the language we use affects how we view the problem.

howardjohn · 2024-06-18T20:57:21Z

/test all

louiscryan

This seems safe from an API/upgrade/back-compat perspective.

louiscryan · 2024-06-18T21:09:13Z

pilot/pkg/serviceregistry/kube/controller/ambient/services.go

@@ -193,6 +193,17 @@ func (a *index) constructService(svc *v1.Service, w *Waypoint) *workloadapi.Serv
 Mode: workloadapi.LoadBalancing_STRICT,
 }
 }
+ ipPolicy := workloadapi.IPFamilies_AUTOMATIC
+ if len(svc.Spec.IPFamilies) == 2 {


This seems potentially a little brittle, can users do silly things like put v1.IPv4Protocol in the list twice ?

No the validation is pretty strict. We could make it more robust anyways though

howardjohn · 2024-06-19T00:22:20Z

/retest

pilot/pkg/serviceregistry/kube/controller/ambient/services.go

hzxuzhonghu · 2024-06-19T03:43:40Z

pilot/pkg/serviceregistry/kube/controller/ambient/services.go

+ cips = []string{svc.Spec.ClusterIP}
+ }
+ for _, cip := range cips {
+ if cip != "" && cip != v1.ClusterIPNone {


add ut for this function

#51665 adds some basic ones, will make it easier to add new ones. if we can merge that it will help and Ill update this PR with a test

hzxuzhonghu · 2024-06-19T03:47:54Z

pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go

@@ -155,7 +155,20 @@ func (a *index) podWorkloadBuilder(
 if (!IsPodRunning(p) && !IsPodPending(p)) || p.Spec.HostNetwork {
 return nil
 }
- podIP, err := netip.ParseAddr(p.Status.PodIP)
+ k8sPodIPs := p.Status.PodIPs
+ if len(k8sPodIPs) == 0 && p.Status.PodIP != "" {


question, when Status.PodIPs can be empty? I guess we donot need to check it since have checked Running status before

This is mostly defense in depth against weird Pods, shouldn't really happen. In particular our tests often just set podIp and not PodIps.

But any Pod can be arbitrarily modified by user, so we cannot assume it's valid

Also IIRC the k8s docs say that if PodIPs is non-empty, the first entry in the list is PodIP - they don't declare any guarantees that PodIPs will be non-empty. I think this is mostly for back compat with old K8S versions though. So yeah better to check.

hzxuzhonghu · 2024-06-19T03:51:21Z

pkg/workloadapi/workload.proto

@@ -81,6 +79,20 @@ message Service {
 // Note: this applies only to connecting directly to the workload; when waypoints are used, the waypoint's load_balancing
 // configuration is used.
 LoadBalancing load_balancing = 8;
+
+ // IP families provides configuration about the IP families this service supports.
+ IPFamilies ip_families = 9;


Is this really needed, can infer from addresses

unfortunately for headless services there is no vip. If it wasn't for that I wouldn't have added it probably .

see istio/ztunnel#1152

How is headless processed in ztunnel now, it has no ip associated on service.

It should be matched by a workload ip. Since workload has the ip, can we infer from it. Anyway, i am not sure how you want ztunnel to do with dual stack

It is for DNS. The linked ztunnel PR has the ztunnel implementation and tests

bleggett · 2024-06-24T18:20:25Z

pilot/pkg/serviceregistry/kube/controller/ambient/services.go

@@ -193,6 +193,17 @@ func (a *index) constructService(svc *v1.Service, w *Waypoint) *workloadapi.Serv
 Mode: workloadapi.LoadBalancing_STRICT,
 }
 }
+ ipFamily := workloadapi.IPFamilies_AUTOMATIC


QQ - are we emulating the k8s ipfamily or ipfamilypolicy API here, or both?

It seems like mostly this is just emulating ipfamilypolicy semantics? Do we plan to hardcode the behavior, or additionally expose ipfamily style APIs for the relevant resources we have in Istio, to match k8s?

"dualstack" is just two problems AFAICT:

should you assign more than 1 IP to (some resource with IPs) -> (k8s ipfamilypolicy), I think you said we will probably always assign both if V6 enabled.

can you handle more than 1 IP assigned to (some resource with IPs).

if you can, how do you priority-order the list of IPs on that resource (k8sipfamily ordering semantics)

The only bit we really care about as A Problem is the ordering of that IP list for non-K8S/Istio-only resources, right? Doesn't matter whether that list contains 1 V4 and 1 V6, 2 V4s, or 5 V6s, right?

ipFamilyPolicy. This is actually mostly about joining two resources (Service and Pod), and which IPs you should join. Since k8s already pre-filters the IPs (and assigns the IPs), its not like we need to do "which IPs do we assign" , etc

Would we need to do that for stuff like SEs eventually, though?

We really only need this for headless services selecting pods, which there is no SE equivalent. So I don't think we really do

bleggett · 2024-06-24T18:21:59Z

pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go

@@ -155,7 +155,20 @@ func (a *index) podWorkloadBuilder(
 if (!IsPodRunning(p) && !IsPodPending(p)) || p.Spec.HostNetwork {
 return nil
 }
- podIP, err := netip.ParseAddr(p.Status.PodIP)
+ k8sPodIPs := p.Status.PodIPs
+ if len(k8sPodIPs) == 0 && p.Status.PodIP != "" {


Also IIRC the k8s docs say that if PodIPs is non-empty, the first entry in the list is PodIP - they don't declare any guarantees that PodIPs will be non-empty. I think this is mostly for back compat with old K8S versions though. So yeah better to check.

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 17, 2024

bleggett approved these changes Jun 17, 2024

View reviewed changes

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 18, 2024

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jun 18, 2024

howardjohn mentioned this pull request Jun 18, 2024

Support ip_families field in service istio/ztunnel#1161

Merged

louiscryan reviewed Jun 18, 2024

View reviewed changes

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Jun 18, 2024

howardjohn marked this pull request as ready for review June 19, 2024 00:22

howardjohn requested review from a team as code owners June 19, 2024 00:22

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 19, 2024

hzxuzhonghu reviewed Jun 19, 2024

View reviewed changes

howardjohn added 4 commits June 21, 2024 15:42

Initial ambient dual stack support

df195d4

add service

4d66979

wip

76ce3c1

rename

cb7bb9f

howardjohn force-pushed the ambient/dualstack branch from 169e878 to cb7bb9f Compare June 21, 2024 22:43

hzxuzhonghu approved these changes Jun 24, 2024

View reviewed changes

bleggett reviewed Jun 24, 2024

View reviewed changes

zirain approved these changes Jun 24, 2024

View reviewed changes

howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Jun 24, 2024

istio-testing merged commit dc132ea into istio:master Jun 24, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial ambient dual stack support #51606

Initial ambient dual stack support #51606

howardjohn commented Jun 17, 2024 •

edited

Loading

istio-testing commented Jun 17, 2024

bleggett commented Jun 17, 2024 •

edited

Loading

bleggett commented Jun 17, 2024 •

edited

Loading

howardjohn commented Jun 17, 2024

bleggett commented Jun 17, 2024

howardjohn commented Jun 18, 2024

louiscryan left a comment

louiscryan Jun 18, 2024

howardjohn Jun 19, 2024

howardjohn commented Jun 19, 2024

hzxuzhonghu Jun 19, 2024

howardjohn Jun 21, 2024

hzxuzhonghu Jun 19, 2024

howardjohn Jun 19, 2024

bleggett Jun 24, 2024 •

edited

Loading

hzxuzhonghu Jun 19, 2024

howardjohn Jun 19, 2024

hzxuzhonghu Jun 19, 2024

howardjohn Jun 19, 2024

bleggett Jun 24, 2024 •

edited

Loading

howardjohn Jun 24, 2024

bleggett Jun 24, 2024

howardjohn Jun 24, 2024

bleggett Jun 24, 2024 •

edited

Loading

Initial ambient dual stack support #51606

Initial ambient dual stack support #51606

Conversation

howardjohn commented Jun 17, 2024 • edited Loading

istio-testing commented Jun 17, 2024

bleggett commented Jun 17, 2024 • edited Loading

bleggett commented Jun 17, 2024 • edited Loading

howardjohn commented Jun 17, 2024

bleggett commented Jun 17, 2024

howardjohn commented Jun 18, 2024

louiscryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

howardjohn commented Jun 17, 2024 •

edited

Loading

bleggett commented Jun 17, 2024 •

edited

Loading

bleggett commented Jun 17, 2024 •

edited

Loading

bleggett Jun 24, 2024 •

edited

Loading

bleggett Jun 24, 2024 •

edited

Loading

bleggett Jun 24, 2024 •

edited

Loading