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.

Sign up for GitHub

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

Initial ambient dual stack support #51606

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Initial ambient dual stack support #51606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df195d4
Initial ambient dual stack support
howardjohn Jun 14, 2024
4d66979
add service
howardjohn Jun 17, 2024
76ce3c1
wip
howardjohn Jun 18, 2024
cb7bb9f
rename
howardjohn Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions manifests/charts/istio-cni/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ defaults:
configDir: ""
# If enabled, and ambient is enabled, DNS redirection will be enabled
dnsCapture: false
# UNSTABLE: If enabled, and ambient is enabled, enables ipv6 support
ipv6: false
# If enabled, and ambient is enabled, enables ipv6 support
ipv6: true


repair:
Expand Down
22 changes: 20 additions & 2 deletions pilot/pkg/serviceregistry/kube/controller/ambient/services.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,17 @@ func (a *index) constructService(svc *v1.Service, w *Waypoint) *workloadapi.Serv
Mode: workloadapi.LoadBalancing_STRICT,
}
}
ipFamily := workloadapi.IPFamilies_AUTOMATIC
Copy link
Contributor

@bleggett bleggett Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if len(svc.Spec.IPFamilies) == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ipFamily = workloadapi.IPFamilies_DUAL
} else if len(svc.Spec.IPFamilies) == 1 {
family := svc.Spec.IPFamilies[0]
if family == v1.IPv4Protocol {
ipFamily = workloadapi.IPFamilies_IPV4_ONLY
} else {
ipFamily = workloadapi.IPFamilies_IPV6_ONLY
}
}
// TODO this is only checking one controller - we may be missing service vips for instances in another cluster
return &workloadapi.Service{
Name: svc.Name,
Expand All @@ -202,13 +213,20 @@ func (a *index) constructService(svc *v1.Service, w *Waypoint) *workloadapi.Serv
Ports: ports,
Waypoint: waypointAddress,
LoadBalancing: lb,
IpFamilies: ipFamily,
}
}

func getVIPs(svc *v1.Service) []string {
res := []string{}
if svc.Spec.ClusterIP != "" && svc.Spec.ClusterIP != v1.ClusterIPNone {
res = append(res, svc.Spec.ClusterIP)
cips := svc.Spec.ClusterIPs
if len(cips) == 0 {
cips = []string{svc.Spec.ClusterIP}
}
for _, cip := range cips {
if cip != "" && cip != v1.ClusterIPNone {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add ut for this function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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

res = append(res, cip)
}
}
return res
}
18 changes: 16 additions & 2 deletions pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bleggett bleggett Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

k8sPodIPs = []v1.PodIP{{IP: p.Status.PodIP}}
}
if len(k8sPodIPs) == 0 {
return nil
}
podIPs, err := slices.MapErr(k8sPodIPs, func(e v1.PodIP) ([]byte, error) {
n, err := netip.ParseAddr(e.IP)
if err != nil {
return nil, err
}
return n.AsSlice(), nil
})
if err != nil {
// Is this possible? Probably not in typical case, but anyone could put garbage there.
return nil
Expand All @@ -174,6 +187,7 @@ func (a *index) podWorkloadBuilder(
status = workloadapi.WorkloadStatus_UNHEALTHY
}
a.networkUpdateTrigger.MarkDependant(ctx) // Mark we depend on out of band a.Network
// We only check the network of the first IP. This should be fine; it is not supported for a single pod to span multiple networks
network := a.Network(p.Status.PodIP, p.Labels).String()

var appTunnel *workloadapi.ApplicationTunnel
Expand All @@ -198,7 +212,7 @@ func (a *index) podWorkloadBuilder(
Namespace: p.Namespace,
Network: network,
ClusterId: string(a.ClusterID),
Addresses: [][]byte{podIP.AsSlice()},
Addresses: podIPs,
ServiceAccount: p.Spec.ServiceAccountName,
Waypoint: a.getWaypointAddress(targetWaypoint),
Node: p.Spec.NodeName,
Expand Down
Loading