diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index b171f6239524..7019c22ad94b 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -1,6 +1,6 @@ { "name": "istio build-tools", - "image": "gcr.io/istio-testing/build-tools:release-1.22-46fce460ef8547fb88a20de8494683bfb3bfa8e5", + "image": "gcr.io/istio-testing/build-tools:release-1.22-02098ccc0766fde1c577cf9f9258fb43a08ec8c8", "privileged": true, "remoteEnv": { "USE_GKE_GCLOUD_AUTH_PLUGIN": "True", diff --git a/Makefile.core.mk b/Makefile.core.mk index da2603e0e1e7..62228b1455d8 100644 --- a/Makefile.core.mk +++ b/Makefile.core.mk @@ -49,7 +49,7 @@ endif export VERSION # Base version of Istio image to use -BASE_VERSION ?= 1.22-2024-06-27T19-01-41 +BASE_VERSION ?= 1.22-2024-08-08T19-01-18 ISTIO_BASE_REGISTRY ?= gcr.io/istio-release export GO111MODULE ?= on @@ -380,7 +380,10 @@ copy-templates: for profile in manifests/helm-profiles/*.yaml ; do \ sed "1s|^|$${warning}\n\n|" $$profile > manifests/charts/$$chart/files/profile-$$(basename $$profile) ; \ done; \ - cp manifests/zzz_profile.yaml manifests/charts/$$chart/templates ; \ + [[ "$$chart" == "ztunnel" ]] && flatten="true" || flatten="false" ; \ + cat manifests/zzz_profile.yaml | \ + sed "s/FLATTEN_GLOBALS_REPLACEMENT/$${flatten}/g" \ + > manifests/charts/$$chart/templates/zzz_profile.yaml ; \ done #----------------------------------------------------------------------------- diff --git a/cni/pkg/ipset/nldeps_linux.go b/cni/pkg/ipset/nldeps_linux.go index 11278acd7c6b..11a37f3ce5b8 100644 --- a/cni/pkg/ipset/nldeps_linux.go +++ b/cni/pkg/ipset/nldeps_linux.go @@ -19,9 +19,9 @@ import ( "fmt" "net" "net/netip" + "strings" "github.com/vishvananda/netlink" - "github.com/vishvananda/netlink/nl" ) func RealNlDeps() NetlinkIpsetDeps { @@ -32,7 +32,16 @@ type realDeps struct{} func (m *realDeps) ipsetIPPortCreate(name string) error { err := netlink.IpsetCreate(name, "hash:ip", netlink.IpsetCreateOptions{Comments: true, Replace: true}) - if ipsetErr, ok := err.(nl.IPSetError); ok && ipsetErr == nl.IPSET_ERR_EXIST { + // Note there appears to be a bug in vishvananda/netlink here: + // https://github.com/vishvananda/netlink/issues/992 + // + // The "right" way to do this is: + // if err == nl.IPSetError(nl.IPSET_ERR_EXIST) { + // log.Debugf("ignoring ipset err") + // return nil + // } + // but that doesn't actually work, so strings.Contains the error. + if err != nil && strings.Contains(err.Error(), "exists") { return nil } return err diff --git a/cni/pkg/nodeagent/cni-watcher_test.go b/cni/pkg/nodeagent/cni-watcher_test.go index f567ea96092d..d5cec0f7c8a3 100644 --- a/cni/pkg/nodeagent/cni-watcher_test.go +++ b/cni/pkg/nodeagent/cni-watcher_test.go @@ -107,10 +107,7 @@ func TestCNIPluginServer(t *testing.T) { valid.Netns, ).Return(nil) - dpServer := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + dpServer := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, dpServer, "istio-system") @@ -183,10 +180,7 @@ func TestGetPodWithRetry(t *testing.T) { wg, _ := NewWaitForNCalls(t, 1) fs := &fakeServer{testWG: wg} - dpServer := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + dpServer := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, dpServer, "istio-system") @@ -261,10 +255,7 @@ func TestCNIPluginServerPrefersCNIProvidedPodIP(t *testing.T) { valid.Netns, ).Return(nil) - dpServer := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + dpServer := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, dpServer, "istio-system") diff --git a/cni/pkg/nodeagent/informers_test.go b/cni/pkg/nodeagent/informers_test.go index e949cdd00190..d3564be7c7c0 100644 --- a/cni/pkg/nodeagent/informers_test.go +++ b/cni/pkg/nodeagent/informers_test.go @@ -65,10 +65,7 @@ func TestExistingPodAddedWhenNsLabeled(t *testing.T) { "", ).Return(nil) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -127,10 +124,7 @@ func TestExistingPodAddedWhenDualStack(t *testing.T) { "", ).Return(nil) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) fs.Start(ctx) handlers := setupHandlers(ctx, client, server, "istio-system") @@ -179,10 +173,7 @@ func TestExistingPodNotAddedIfNoIPInAnyStatusField(t *testing.T) { fs := &fakeServer{} - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -244,10 +235,7 @@ func TestExistingPodRemovedWhenNsUnlabeled(t *testing.T) { "", ).Return(nil) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -337,10 +325,7 @@ func TestExistingPodRemovedWhenPodLabelRemoved(t *testing.T) { "", ).Return(nil) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -440,10 +425,7 @@ func TestJobPodRemovedWhenPodTerminates(t *testing.T) { "", ).Return(nil) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -556,10 +538,8 @@ func TestGetActiveAmbientPodSnapshotOnlyReturnsActivePods(t *testing.T) { client := kube.NewFakeClient(ns, enrolledNotRedirected, redirectedNotEnrolled) fs := &fakeServer{} fs.Start(ctx) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -618,10 +598,8 @@ func TestGetActiveAmbientPodSnapshotSkipsTerminatedJobPods(t *testing.T) { client := kube.NewFakeClient(ns, enrolledNotRedirected, enrolledButTerminated) fs := &fakeServer{} fs.Start(ctx) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -661,10 +639,8 @@ func TestAmbientEnabledReturnsPodIfEnabled(t *testing.T) { client := kube.NewFakeClient(ns, pod) fs := &fakeServer{} fs.Start(ctx) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -704,10 +680,8 @@ func TestAmbientEnabledReturnsNoPodIfNotEnabled(t *testing.T) { client := kube.NewFakeClient(ns, pod) fs := &fakeServer{} fs.Start(ctx) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -748,10 +722,8 @@ func TestAmbientEnabledReturnsErrorIfBogusNS(t *testing.T) { client := kube.NewFakeClient(ns, pod) fs := &fakeServer{} fs.Start(ctx) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) @@ -802,10 +774,7 @@ func TestExistingPodAddedWhenItPreExists(t *testing.T) { "", ).Return(nil) - server := &meshDataplane{ - kubeClient: client.Kube(), - netServer: fs, - } + server := getFakeDP(fs, client.Kube()) handlers := setupHandlers(ctx, client, server, "istio-system") client.RunAndWait(ctx.Done()) diff --git a/cni/pkg/nodeagent/net.go b/cni/pkg/nodeagent/net.go index 0182d2204ab9..706f48ab672f 100644 --- a/cni/pkg/nodeagent/net.go +++ b/cni/pkg/nodeagent/net.go @@ -20,15 +20,11 @@ import ( "fmt" "net/netip" - "golang.org/x/sys/unix" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - "istio.io/istio/cni/pkg/ipset" "istio.io/istio/cni/pkg/iptables" - "istio.io/istio/cni/pkg/util" "istio.io/istio/pkg/slices" - "istio.io/istio/pkg/util/sets" dep "istio.io/istio/tools/istio-iptables/pkg/dependencies" ) @@ -39,15 +35,13 @@ type NetServer struct { iptablesConfigurator *iptables.IptablesConfigurator podNs PodNetnsFinder // allow overriding for tests - netnsRunner func(fdable NetnsFd, toRun func() error) error - hostsideProbeIPSet ipset.IPSet + netnsRunner func(fdable NetnsFd, toRun func() error) error } var _ MeshDataplane = &NetServer{} func newNetServer(ztunnelServer ZtunnelServer, podNsMap *podNetnsCache, iptablesConfigurator *iptables.IptablesConfigurator, podNs PodNetnsFinder, - probeSet ipset.IPSet, ) *NetServer { return &NetServer{ ztunnelServer: ztunnelServer, @@ -55,7 +49,6 @@ func newNetServer(ztunnelServer ZtunnelServer, podNsMap *podNetnsCache, podNs: podNs, iptablesConfigurator: iptablesConfigurator, netnsRunner: NetnsDo, - hostsideProbeIPSet: probeSet, } } @@ -68,11 +61,6 @@ func (s *NetServer) Stop() { log.Debug("removing host iptables rules") s.iptablesConfigurator.DeleteHostRules() - log.Debug("destroying host ipset") - s.hostsideProbeIPSet.Flush() - if err := s.hostsideProbeIPSet.DestroySet(); err != nil { - log.Warnf("could not destroy host ipset on shutdown") - } log.Debug("stopping ztunnel server") s.ztunnelServer.Close() } @@ -142,13 +130,6 @@ func (s *NetServer) AddPodToMesh(ctx context.Context, pod *corev1.Pod, podIPs [] return err } - // Handle node healthcheck probe rewrites - _, err = addPodToHostNSIpset(pod, podIPs, &s.hostsideProbeIPSet) - if err != nil { - log.Errorf("failed to add pod to ipset: %s/%s %v", pod.Namespace, pod.Name, err) - return err - } - log.Debug("calling CreateInpodRules") if err := s.netnsRunner(openNetns, func() error { return s.iptablesConfigurator.CreateInpodRules(&HostProbeSNATIP) @@ -181,11 +162,6 @@ func (s *NetServer) sendPodToZtunnelAndWaitForAck(ctx context.Context, pod *core func (s *NetServer) ConstructInitialSnapshot(ambientPods []*corev1.Pod) error { var consErr []error - if err := s.syncHostIPSets(ambientPods); err != nil { - log.Warnf("failed to sync host IPset: %v", err) - consErr = append(consErr, err) - } - podsByUID := slices.GroupUnique(ambientPods, (*corev1.Pod).GetUID) if err := s.buildZtunnelSnapshot(podsByUID); err != nil { log.Warnf("failed to construct initial ztunnel snapshot: %v", err) @@ -233,13 +209,6 @@ func (s *NetServer) RemovePodFromMesh(ctx context.Context, pod *corev1.Pod, isDe // Aggregate errors together, so that if part of the cleanup fails we still proceed with other steps. var errs []error - // Remove the hostside ipset entry first, and unconditionally - if later failures happen, we never - // want to leave stale entries - if err := removePodFromHostNSIpset(pod, &s.hostsideProbeIPSet); err != nil { - log.Errorf("failed to remove pod %s from host ipset, error was: %v", pod.Name, err) - errs = append(errs, err) - } - // Whether pod is already deleted or not, we need to let go of our netns ref. openNetns := s.currentPodSnapshot.Take(string(pod.UID)) if openNetns == nil { @@ -266,98 +235,3 @@ func (s *NetServer) RemovePodFromMesh(ctx context.Context, pod *corev1.Pod, isDe } return errors.Join(errs...) } - -// syncHostIPSets is called after the host node ipset has been created (or found + flushed) -// during initial snapshot creation, it will insert every snapshotted pod's IP into the set. -// -// The set does not allow dupes (obviously, that would be undefined) - but in the real world due to misconfigured -// IPAM or other things, we may see two pods with the same IP on the same node - we will skip the dupes, -// which is all we can do - these pods will fail healthcheck until the IPAM issue is resolved (which seems reasonable) -func (s *NetServer) syncHostIPSets(ambientPods []*corev1.Pod) error { - var addedIPSnapshot []netip.Addr - - for _, pod := range ambientPods { - podIPs := util.GetPodIPsIfPresent(pod) - if len(podIPs) == 0 { - log.Warnf("pod %s does not appear to have any assigned IPs, not syncing with ipset", pod.Name) - } else { - addedIps, err := addPodToHostNSIpset(pod, podIPs, &s.hostsideProbeIPSet) - if err != nil { - log.Errorf("pod %s has IP collision, pod will be skipped and will fail healthchecks", pod.Name, podIPs) - } - addedIPSnapshot = append(addedIPSnapshot, addedIps...) - } - - } - return pruneHostIPset(sets.New(addedIPSnapshot...), &s.hostsideProbeIPSet) -} - -// addPodToHostNSIpset: -// 1. get pod manifest -// 2. Get all pod ips (might be several, v6/v4) -// 3. update ipsets accordingly -// 4. return the ones we added successfully, and errors for any we couldn't (dupes) -// -// Dupe IPs should be considered an IPAM error and should never happen. -func addPodToHostNSIpset(pod *corev1.Pod, podIPs []netip.Addr, hostsideProbeSet *ipset.IPSet) ([]netip.Addr, error) { - // Add the pod UID as an ipset entry comment, so we can (more) easily find and delete - // all relevant entries for a pod later. - podUID := string(pod.ObjectMeta.UID) - ipProto := uint8(unix.IPPROTO_TCP) - - var ipsetAddrErrs []error - var addedIps []netip.Addr - - // For each pod IP - for _, pip := range podIPs { - // Add to host ipset - log.Debugf("adding pod %s probe to ipset %s with ip %s", pod.Name, hostsideProbeSet.Name, pip) - // Add IP/port combo to set. Note that we set Replace to false here - we _did_ previously - // set it to true, but in theory that could mask weird scenarios where K8S triggers events out of order -> - // an add(sameIPreused) then delete(originalIP). - // Which will result in the new pod starting to fail healthchecks. - // - // Since we purge on restart of CNI, and remove pod IPs from the set on every pod removal/deletion, - // we _shouldn't_ get any overwrite/overlap, unless something is wrong and we are asked to add - // a pod by an IP we already have in the set (which will give an error, which we want). - if err := hostsideProbeSet.AddIP(pip, ipProto, podUID, false); err != nil { - ipsetAddrErrs = append(ipsetAddrErrs, err) - log.Errorf("failed adding pod %s to ipset %s with ip %s, error was %s", - pod.Name, hostsideProbeSet.Name, pip, err) - } else { - addedIps = append(addedIps, pip) - } - } - - return addedIps, errors.Join(ipsetAddrErrs...) -} - -func removePodFromHostNSIpset(pod *corev1.Pod, hostsideProbeSet *ipset.IPSet) error { - podIPs := util.GetPodIPsIfPresent(pod) - for _, pip := range podIPs { - if err := hostsideProbeSet.ClearEntriesWithIP(pip); err != nil { - return err - } - log.Debugf("removed pod name %s with UID %s from host ipset %s by ip %s", pod.Name, pod.UID, hostsideProbeSet.Name, pip) - } - - return nil -} - -func pruneHostIPset(expected sets.Set[netip.Addr], hostsideProbeSet *ipset.IPSet) error { - actualIPSetContents, err := hostsideProbeSet.ListEntriesByIP() - if err != nil { - log.Warnf("unable to list IPSet: %v", err) - return err - } - actual := sets.New[netip.Addr](actualIPSetContents...) - stales := actual.DifferenceInPlace(expected) - - for staleIP := range stales { - if err := hostsideProbeSet.ClearEntriesWithIP(staleIP); err != nil { - return err - } - log.Debugf("removed stale ip %s from host ipset %s", staleIP, hostsideProbeSet.Name) - } - return nil -} diff --git a/cni/pkg/nodeagent/net_test.go b/cni/pkg/nodeagent/net_test.go index 0244123dad76..fb08c0274907 100644 --- a/cni/pkg/nodeagent/net_test.go +++ b/cni/pkg/nodeagent/net_test.go @@ -23,13 +23,11 @@ import ( "testing" "time" - "golang.org/x/sys/unix" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" - "istio.io/istio/cni/pkg/ipset" "istio.io/istio/cni/pkg/iptables" istiolog "istio.io/istio/pkg/log" "istio.io/istio/pkg/test/util/assert" @@ -51,7 +49,6 @@ type netTestFixture struct { ztunnelServer *fakeZtunnel iptablesConfigurator *iptables.IptablesConfigurator nlDeps *fakeIptablesDeps - ipsetDeps *ipset.MockedIpsetDeps } func getTestFixure(ctx context.Context) netTestFixture { @@ -61,9 +58,7 @@ func getTestFixure(ctx context.Context) netTestFixture { ztunnelServer := &fakeZtunnel{} - fakeIPSetDeps := ipset.FakeNLDeps() - set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} - netServer := newNetServer(ztunnelServer, podNsMap, iptablesConfigurator, NewPodNetnsProcFinder(fakeFs()), set) + netServer := newNetServer(ztunnelServer, podNsMap, iptablesConfigurator, NewPodNetnsProcFinder(fakeFs())) netServer.netnsRunner = func(fdable NetnsFd, toRun func() error) error { return toRun() @@ -75,7 +70,6 @@ func getTestFixure(ctx context.Context) netTestFixture { ztunnelServer: ztunnelServer, iptablesConfigurator: iptablesConfigurator, nlDeps: nlDeps, - ipsetDeps: fakeIPSetDeps, } } @@ -141,14 +135,6 @@ func TestServerAddPod(t *testing.T) { podIP := netip.MustParseAddr("99.9.9.9") podIPs := []netip.Addr{podIP} - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("99.9.9.9"), - uint8(unix.IPPROTO_TCP), - string(podMeta.UID), - false, - ).Return(nil) - err := netServer.AddPodToMesh(ctx, &corev1.Pod{ObjectMeta: podMeta}, podIPs, "fakenetns") assert.NoError(t, err) assert.Equal(t, 1, ztunnelServer.addedPods.Load()) @@ -171,7 +157,6 @@ func TestServerRemovePod(t *testing.T) { Netns: fakens, } - expectPodRemovedFromIPSet(fixture.ipsetDeps, pod.Status.PodIPs) fixture.podNsMap.UpsertPodCacheWithNetns(string(pod.UID), workload) err := netServer.RemovePodFromMesh(ctx, pod, false) assert.NoError(t, err) @@ -181,7 +166,6 @@ func TestServerRemovePod(t *testing.T) { // make sure the uid was taken from cache and netns closed netns := fixture.podNsMap.Take(string(pod.UID)) assert.Equal(t, nil, netns) - fixture.ipsetDeps.AssertExpectations(t) // run gc to clean up ns: @@ -215,7 +199,6 @@ func TestServerRemovePodAlwaysRemovesIPSetEntryEvenOnFail(t *testing.T) { Workload: podToWorkload(pod), Netns: fakens, } - expectPodRemovedFromIPSet(fixture.ipsetDeps, pod.Status.PodIPs) fixture.podNsMap.UpsertPodCacheWithNetns(string(pod.UID), workload) err := netServer.RemovePodFromMesh(ctx, pod, false) assert.Error(t, err) @@ -225,7 +208,6 @@ func TestServerRemovePodAlwaysRemovesIPSetEntryEvenOnFail(t *testing.T) { // make sure the uid was taken from cache and netns closed netns := fixture.podNsMap.Take(string(pod.UID)) assert.Equal(t, nil, netns) - fixture.ipsetDeps.AssertExpectations(t) // run gc to clean up ns: @@ -259,7 +241,6 @@ func TestServerDeletePod(t *testing.T) { Netns: fakens, } fixture.podNsMap.UpsertPodCacheWithNetns(string(pod.UID), workload) - expectPodRemovedFromIPSet(fixture.ipsetDeps, pod.Status.PodIPs) err := netServer.RemovePodFromMesh(ctx, pod, true) assert.NoError(t, err) assert.Equal(t, ztunnelServer.deletedPods.Load(), 1) @@ -270,7 +251,6 @@ func TestServerDeletePod(t *testing.T) { // make sure the uid was taken from cache and netns closed netns := fixture.podNsMap.Take(string(pod.UID)) assert.Equal(t, nil, netns) - fixture.ipsetDeps.AssertExpectations(t) // run gc to clean up ns: //revive:disable-next-line:call-to-gc Just a test that we are cleaning up the netns @@ -278,25 +258,6 @@ func TestServerDeletePod(t *testing.T) { assertNSClosed(t, closed) } -func expectPodAddedToIPSet(ipsetDeps *ipset.MockedIpsetDeps, podIP netip.Addr, podMeta metav1.ObjectMeta) { - ipsetDeps.On("addIP", - "foo", - podIP, - uint8(unix.IPPROTO_TCP), - string(podMeta.UID), - false, - ).Return(nil) -} - -func expectPodRemovedFromIPSet(ipsetDeps *ipset.MockedIpsetDeps, podIPs []corev1.PodIP) { - for _, ip := range podIPs { - ipsetDeps.On("clearEntriesWithIP", - "foo", - netip.MustParseAddr(ip.IP), - ).Return(nil) - } -} - func TestServerAddPodWithNoNetns(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -312,7 +273,6 @@ func TestServerAddPodWithNoNetns(t *testing.T) { } podIP := netip.MustParseAddr("99.9.9.9") podIPs := []netip.Addr{podIP} - expectPodAddedToIPSet(fixture.ipsetDeps, podIP, podMeta) err := netServer.AddPodToMesh(ctx, &corev1.Pod{ObjectMeta: podMeta}, podIPs, "") assert.NoError(t, err) @@ -336,7 +296,6 @@ func TestReturnsPartialErrorOnZtunnelFail(t *testing.T) { podIP := netip.MustParseAddr("99.9.9.9") podIPs := []netip.Addr{podIP} - expectPodAddedToIPSet(fixture.ipsetDeps, podIP, podMeta) err := netServer.AddPodToMesh(ctx, &corev1.Pod{ObjectMeta: podMeta}, podIPs, "faksens") assert.Equal(t, ztunnelServer.addedPods.Load(), 1) if !errors.Is(err, ErrPartialAdd) { @@ -362,7 +321,6 @@ func TestDoesntReturnPartialErrorOnIptablesFail(t *testing.T) { podIP := netip.MustParseAddr("99.9.9.9") podIPs := []netip.Addr{podIP} - expectPodAddedToIPSet(fixture.ipsetDeps, podIP, podMeta) err := netServer.AddPodToMesh(ctx, &corev1.Pod{ObjectMeta: podMeta}, podIPs, "faksens") // no calls to ztunnel if iptables failed assert.Equal(t, ztunnelServer.addedPods.Load(), 0) @@ -387,10 +345,6 @@ func TestConstructInitialSnap(t *testing.T) { } pod := &corev1.Pod{ObjectMeta: podMeta} - fixture.ipsetDeps.On("listEntriesByIP", - "foo", - ).Return([]netip.Addr{}, nil) - err := netServer.ConstructInitialSnapshot([]*corev1.Pod{pod}) assert.NoError(t, err) if fixture.podNsMap.Get("863b91d4-4b68-4efa-917f-4b560e3e86aa") == nil { @@ -398,261 +352,6 @@ func TestConstructInitialSnap(t *testing.T) { } } -func TestAddPodToHostNSIPSets(t *testing.T) { - pod := buildConvincingPod() - - var podUID string = string(pod.ObjectMeta.UID) - fakeIPSetDeps := ipset.FakeNLDeps() - set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} - ipProto := uint8(unix.IPPROTO_TCP) - - fakeIPSetDeps.On("addIP", - "foo", - netip.MustParseAddr("99.9.9.9"), - ipProto, - podUID, - false, - ).Return(nil) - - fakeIPSetDeps.On("addIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ipProto, - podUID, - false, - ).Return(nil) - - podIPs := []netip.Addr{netip.MustParseAddr("99.9.9.9"), netip.MustParseAddr("2.2.2.2")} - _, err := addPodToHostNSIpset(pod, podIPs, &set) - assert.NoError(t, err) - - fakeIPSetDeps.AssertExpectations(t) -} - -func TestAddPodIPToHostNSIPSetsReturnsErrorIfOneFails(t *testing.T) { - pod := buildConvincingPod() - - var podUID string = string(pod.ObjectMeta.UID) - fakeIPSetDeps := ipset.FakeNLDeps() - set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} - ipProto := uint8(unix.IPPROTO_TCP) - - fakeIPSetDeps.On("addIP", - "foo", - netip.MustParseAddr("99.9.9.9"), - ipProto, - podUID, - false, - ).Return(nil) - - fakeIPSetDeps.On("addIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ipProto, - podUID, - false, - ).Return(errors.New("bwoah")) - - podIPs := []netip.Addr{netip.MustParseAddr("99.9.9.9"), netip.MustParseAddr("2.2.2.2")} - - addedPIPs, err := addPodToHostNSIpset(pod, podIPs, &set) - assert.Error(t, err) - assert.Equal(t, 1, len(addedPIPs), "only expected one IP to be added") - - fakeIPSetDeps.AssertExpectations(t) -} - -func TestRemovePodProbePortsFromHostNSIPSets(t *testing.T) { - pod := buildConvincingPod() - - fakeIPSetDeps := ipset.FakeNLDeps() - set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} - - fakeIPSetDeps.On("clearEntriesWithIP", - "foo", - netip.MustParseAddr("3.3.3.3"), - ).Return(nil) - - fakeIPSetDeps.On("clearEntriesWithIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ).Return(nil) - - err := removePodFromHostNSIpset(pod, &set) - assert.NoError(t, err) - fakeIPSetDeps.AssertExpectations(t) -} - -func TestSyncHostIPSetsPrunesNothingIfNoExtras(t *testing.T) { - pod := buildConvincingPod() - - fakeIPSetDeps := ipset.FakeNLDeps() - - var podUID string = string(pod.ObjectMeta.UID) - ipProto := uint8(unix.IPPROTO_TCP) - ctx, cancel := context.WithCancel(context.Background()) - fixture := getTestFixure(ctx) - defer cancel() - setupLogging() - - // expectations - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("3.3.3.3"), - ipProto, - podUID, - false, - ).Return(nil) - - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ipProto, - podUID, - false, - ).Return(nil) - - fixture.ipsetDeps.On("listEntriesByIP", - "foo", - ).Return([]netip.Addr{}, nil) - - netServer := fixture.netServer - err := netServer.syncHostIPSets([]*corev1.Pod{pod}) - assert.NoError(t, err) - fakeIPSetDeps.AssertExpectations(t) -} - -func TestSyncHostIPSetsIgnoresPodIPAddErrorAndContinues(t *testing.T) { - pod1 := buildConvincingPod() - pod2 := buildConvincingPod() - - pod2.ObjectMeta.SetUID("4455") - - fakeIPSetDeps := ipset.FakeNLDeps() - - var pod1UID string = string(pod1.ObjectMeta.UID) - var pod2UID string = string(pod2.ObjectMeta.UID) - ipProto := uint8(unix.IPPROTO_TCP) - ctx, cancel := context.WithCancel(context.Background()) - fixture := getTestFixure(ctx) - defer cancel() - setupLogging() - - // First IP of first pod should error, but we should add the rest - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("3.3.3.3"), - ipProto, - pod1UID, - false, - ).Return(errors.New("CANNOT ADD")) - - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ipProto, - pod1UID, - false, - ).Return(nil) - - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("3.3.3.3"), - ipProto, - pod2UID, - false, - ).Return(errors.New("CANNOT ADD")) - - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ipProto, - pod2UID, - false, - ).Return(nil) - - fixture.ipsetDeps.On("listEntriesByIP", - "foo", - ).Return([]netip.Addr{}, nil) - - netServer := fixture.netServer - err := netServer.syncHostIPSets([]*corev1.Pod{pod1, pod2}) - assert.NoError(t, err) - fakeIPSetDeps.AssertExpectations(t) -} - -func TestSyncHostIPSetsAddsNothingIfPodHasNoIPs(t *testing.T) { - pod := buildConvincingPod() - - pod.Status.PodIP = "" - pod.Status.PodIPs = []corev1.PodIP{} - - fakeIPSetDeps := ipset.FakeNLDeps() - - ctx, cancel := context.WithCancel(context.Background()) - fixture := getTestFixure(ctx) - defer cancel() - setupLogging() - - fixture.ipsetDeps.On("listEntriesByIP", - "foo", - ).Return([]netip.Addr{}, nil) - - netServer := fixture.netServer - err := netServer.syncHostIPSets([]*corev1.Pod{pod}) - assert.NoError(t, err) - fakeIPSetDeps.AssertExpectations(t) -} - -func TestSyncHostIPSetsPrunesIfExtras(t *testing.T) { - pod := buildConvincingPod() - - fakeIPSetDeps := ipset.FakeNLDeps() - - var podUID string = string(pod.ObjectMeta.UID) - ipProto := uint8(unix.IPPROTO_TCP) - ctx, cancel := context.WithCancel(context.Background()) - fixture := getTestFixure(ctx) - defer cancel() - setupLogging() - - // expectations - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("3.3.3.3"), - ipProto, - podUID, - false, - ).Return(nil) - - fixture.ipsetDeps.On("addIP", - "foo", - netip.MustParseAddr("2.2.2.2"), - ipProto, - podUID, - false, - ).Return(nil) - - // List should return one IP not in our "pod snapshot", which means we prune - fixture.ipsetDeps.On("listEntriesByIP", - "foo", - ).Return([]netip.Addr{ - netip.MustParseAddr("2.2.2.2"), - netip.MustParseAddr("6.6.6.6"), - netip.MustParseAddr("3.3.3.3"), - }, nil) - - fixture.ipsetDeps.On("clearEntriesWithIP", - "foo", - netip.MustParseAddr("6.6.6.6"), - ).Return(nil) - - netServer := fixture.netServer - err := netServer.syncHostIPSets([]*corev1.Pod{pod}) - assert.NoError(t, err) - fakeIPSetDeps.AssertExpectations(t) -} - // for tests that call `runtime.GC()` - we have no control over when the GC is actually scheduled, // and it is flake-prone to check for closure after calling it, this retries for a bit to make // sure the netns is closed eventually. diff --git a/cni/pkg/nodeagent/server.go b/cni/pkg/nodeagent/server.go index d1a594740e87..bb0854e524b6 100644 --- a/cni/pkg/nodeagent/server.go +++ b/cni/pkg/nodeagent/server.go @@ -23,6 +23,7 @@ import ( "path/filepath" "sync/atomic" + "golang.org/x/sys/unix" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -33,6 +34,7 @@ import ( "istio.io/istio/cni/pkg/util" "istio.io/istio/pkg/kube" istiolog "istio.io/istio/pkg/log" + "istio.io/istio/pkg/util/sets" ) var log = istiolog.RegisterScope("cni-agent", "ambient node agent server").WithLabels("server") @@ -98,7 +100,7 @@ func NewServer(ctx context.Context, ready *atomic.Value, pluginSocket string, ar } podNetns := NewPodNetnsProcFinder(os.DirFS(filepath.Join(pconstants.HostMountsPath, "proc"))) - netServer := newNetServer(ztunnelServer, podNsMap, iptablesConfigurator, podNetns, set) + netServer := newNetServer(ztunnelServer, podNsMap, iptablesConfigurator, podNetns) // Set some defaults s := &Server{ @@ -106,8 +108,9 @@ func NewServer(ctx context.Context, ready *atomic.Value, pluginSocket string, ar kubeClient: client, isReady: ready, dataplane: &meshDataplane{ - kubeClient: client.Kube(), - netServer: netServer, + kubeClient: client.Kube(), + netServer: netServer, + hostsideProbeIPSet: set, }, } s.NotReady() @@ -188,8 +191,9 @@ func (s *Server) Stop() { } type meshDataplane struct { - kubeClient kubernetes.Interface - netServer MeshDataplane + kubeClient kubernetes.Interface + netServer MeshDataplane + hostsideProbeIPSet ipset.IPSet } func (s *meshDataplane) Start(ctx context.Context) { @@ -197,10 +201,23 @@ func (s *meshDataplane) Start(ctx context.Context) { } func (s *meshDataplane) Stop() { + log.Info("CNI ambient server terminating, cleaning up node net rules") + + log.Debug("destroying host ipset") + s.hostsideProbeIPSet.Flush() + if err := s.hostsideProbeIPSet.DestroySet(); err != nil { + log.Warnf("could not destroy host ipset on shutdown") + } + s.netServer.Stop() } func (s *meshDataplane) ConstructInitialSnapshot(ambientPods []*corev1.Pod) error { + if err := s.syncHostIPSets(ambientPods); err != nil { + log.Errorf("failed to sync host IPset: %v", err) + return err + } + return s.netServer.ConstructInitialSnapshot(ambientPods) } @@ -218,13 +235,48 @@ func (s *meshDataplane) AddPodToMesh(ctx context.Context, pod *corev1.Pod, podIP log.Debugf("annotating pod %s", pod.Name) if err := util.AnnotateEnrolledPod(s.kubeClient, &pod.ObjectMeta); err != nil { log.Errorf("failed to annotate pod enrollment: %v", err) - // don't return error here, as this is purely informational. + retErr = err + } + + // ipset is only relevant for pod healthchecks. + // therefore, if we had *any* error adding the pod to the mesh + // do not add the pod to the ipset, so that it will definitely *not* pass healthchecks, + // and the operator can investigate. + // + // This is also important to avoid ipset sync issues if we add the pod ip to the ipset, but + // enrolling fails because ztunnel (or the pod netns, or whatever) isn't ready yet, + // and the pod is rescheduled with a new IP. In that case we don't get + // a removal event, and so would never clean up the old IP that we eagerly-added. + // + // TODO one place this *can* fail is + // - if a CNI plugin after us in the chain fails (currently, we are explicitly the last in the chain by design) + // - the CmdAdd comes back thru here with a new IP + // - we will never clean up that old IP that we "lost" + // To fix this we probably need to impl CmdDel + manage our own podUID/IP mapping. + if retErr == nil { + // Handle node healthcheck probe rewrites + _, err = s.addPodToHostNSIpset(pod, podIPs) + if err != nil { + log.Errorf("failed to add pod to ipset: %s/%s %v", pod.Namespace, pod.Name, err) + return err + } + } else { + log.Errorf("pod: %s/%s was not enrolled and is unhealthy: %v", pod.Namespace, pod.Name, retErr) } + return retErr } func (s *meshDataplane) RemovePodFromMesh(ctx context.Context, pod *corev1.Pod, isDelete bool) error { log := log.WithLabels("ns", pod.Namespace, "name", pod.Name) + + // Remove the hostside ipset entry first, and unconditionally - if later failures happen, we never + // want to leave stale entries + if err := removePodFromHostNSIpset(pod, &s.hostsideProbeIPSet); err != nil { + log.Errorf("failed to remove pod %s from host ipset, error was: %v", pod.Name, err) + return err + } + err := s.netServer.RemovePodFromMesh(ctx, pod, isDelete) if err != nil { log.Errorf("failed to remove pod from mesh: %v", err) @@ -237,3 +289,98 @@ func (s *meshDataplane) RemovePodFromMesh(ctx context.Context, pod *corev1.Pod, } return err } + +// syncHostIPSets is called after the host node ipset has been created (or found + flushed) +// during initial snapshot creation, it will insert every snapshotted pod's IP into the set. +// +// The set does not allow dupes (obviously, that would be undefined) - but in the real world due to misconfigured +// IPAM or other things, we may see two pods with the same IP on the same node - we will skip the dupes, +// which is all we can do - these pods will fail healthcheck until the IPAM issue is resolved (which seems reasonable) +func (s *meshDataplane) syncHostIPSets(ambientPods []*corev1.Pod) error { + var addedIPSnapshot []netip.Addr + + for _, pod := range ambientPods { + podIPs := util.GetPodIPsIfPresent(pod) + if len(podIPs) == 0 { + log.Warnf("pod %s does not appear to have any assigned IPs, not syncing with ipset", pod.Name) + } else { + addedIps, err := s.addPodToHostNSIpset(pod, podIPs) + if err != nil { + log.Errorf("pod %s has IP collision, pod will be skipped and will fail healthchecks", pod.Name, podIPs) + } + addedIPSnapshot = append(addedIPSnapshot, addedIps...) + } + + } + return pruneHostIPset(sets.New(addedIPSnapshot...), &s.hostsideProbeIPSet) +} + +// addPodToHostNSIpset: +// 1. get pod manifest +// 2. Get all pod ips (might be several, v6/v4) +// 3. update ipsets accordingly +// 4. return the ones we added successfully, and errors for any we couldn't (dupes) +// +// Dupe IPs should be considered an IPAM error and should never happen. +func (s *meshDataplane) addPodToHostNSIpset(pod *corev1.Pod, podIPs []netip.Addr) ([]netip.Addr, error) { + // Add the pod UID as an ipset entry comment, so we can (more) easily find and delete + // all relevant entries for a pod later. + podUID := string(pod.ObjectMeta.UID) + ipProto := uint8(unix.IPPROTO_TCP) + + var ipsetAddrErrs []error + var addedIps []netip.Addr + + // For each pod IP + for _, pip := range podIPs { + // Add to host ipset + log.Debugf("adding pod %s probe to ipset %s with ip %s", pod.Name, s.hostsideProbeIPSet, pip) + // Add IP/port combo to set. Note that we set Replace to false here - we _did_ previously + // set it to true, but in theory that could mask weird scenarios where K8S triggers events out of order -> + // an add(sameIPreused) then delete(originalIP). + // Which will result in the new pod starting to fail healthchecks. + // + // Since we purge on restart of CNI, and remove pod IPs from the set on every pod removal/deletion, + // we _shouldn't_ get any overwrite/overlap, unless something is wrong and we are asked to add + // a pod by an IP we already have in the set (which will give an error, which we want). + if err := s.hostsideProbeIPSet.AddIP(pip, ipProto, podUID, false); err != nil { + ipsetAddrErrs = append(ipsetAddrErrs, err) + log.Errorf("failed adding pod %s to ipset %s with ip %s, error was %s", + pod.Name, &s.hostsideProbeIPSet, pip, err) + } else { + addedIps = append(addedIps, pip) + } + } + + return addedIps, errors.Join(ipsetAddrErrs...) +} + +func removePodFromHostNSIpset(pod *corev1.Pod, hostsideProbeSet *ipset.IPSet) error { + podIPs := util.GetPodIPsIfPresent(pod) + for _, pip := range podIPs { + if err := hostsideProbeSet.ClearEntriesWithIP(pip); err != nil { + return err + } + log.Debugf("removed pod name %s with UID %s from host ipset %s by ip %s", pod.Name, pod.UID, hostsideProbeSet, pip) + } + + return nil +} + +func pruneHostIPset(expected sets.Set[netip.Addr], hostsideProbeSet *ipset.IPSet) error { + actualIPSetContents, err := hostsideProbeSet.ListEntriesByIP() + if err != nil { + log.Warnf("unable to list IPSet: %v", err) + return err + } + actual := sets.New[netip.Addr](actualIPSetContents...) + stales := actual.DifferenceInPlace(expected) + + for staleIP := range stales { + if err := hostsideProbeSet.ClearEntriesWithIP(staleIP); err != nil { + return err + } + log.Debugf("removed stale ip %s from host ipset %s", staleIP, hostsideProbeSet) + } + return nil +} diff --git a/cni/pkg/nodeagent/server_test.go b/cni/pkg/nodeagent/server_test.go index 7ccc2a3c4a04..0382e0bde86f 100644 --- a/cni/pkg/nodeagent/server_test.go +++ b/cni/pkg/nodeagent/server_test.go @@ -23,11 +23,14 @@ import ( "time" "github.com/stretchr/testify/mock" + "golang.org/x/sys/unix" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + "istio.io/istio/cni/pkg/ipset" "istio.io/istio/pkg/config/constants" "istio.io/istio/pkg/test/util/assert" ) @@ -56,14 +59,18 @@ func TestMeshDataplaneAddsAnnotationOnAdd(t *testing.T) { ).Return(nil) server.Start(fakeCtx) - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + m := getFakeDPWithIPSet(server, fakeClientSet, set) + expectPodAddedToIPSet(fakeIPSetDeps, podIP, pod.ObjectMeta) err := m.AddPodToMesh(fakeCtx, pod, podIPs, "") assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) + pod, err = fakeClientSet.CoreV1().Pods("test").Get(fakeCtx, "test", metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, len(pod.Annotations), 1) @@ -93,14 +100,18 @@ func TestMeshDataplaneAddsAnnotationOnAddWithPartialError(t *testing.T) { server.Start(fakeCtx) fakeClientSet := fake.NewSimpleClientset(pod) - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + m := getFakeDPWithIPSet(server, fakeClientSet, set) err := m.AddPodToMesh(fakeCtx, pod, podIPs, "") assert.Error(t, err) + // as this is a partial add error we should NOT have added to the ipset + fakeIPSetDeps.AssertExpectations(t) + pod, err = fakeClientSet.CoreV1().Pods("test").Get(fakeCtx, "test", metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, len(pod.Annotations), 1) @@ -130,14 +141,18 @@ func TestMeshDataplaneDoesntAnnotateOnAddWithRealError(t *testing.T) { server.Start(fakeCtx) fakeClientSet := fake.NewSimpleClientset(pod) - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + m := getFakeDPWithIPSet(server, fakeClientSet, set) err := m.AddPodToMesh(fakeCtx, pod, podIPs, "") assert.Error(t, err) + // as this is a partial add error we should NOT have added to the ipset + fakeIPSetDeps.AssertExpectations(t) + pod, err = fakeClientSet.CoreV1().Pods("test").Get(fakeCtx, "test", metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, len(pod.Annotations), 0) @@ -157,14 +172,18 @@ func TestMeshDataplaneRemovePodRemovesAnnotation(t *testing.T) { ).Return(nil) fakeClientSet := fake.NewSimpleClientset(pod) - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + m := getFakeDPWithIPSet(server, fakeClientSet, set) + expectPodRemovedFromIPSet(fakeIPSetDeps, pod.Status.PodIPs) err := m.RemovePodFromMesh(fakeCtx, pod, false) assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) + pod, err = fakeClientSet.CoreV1().Pods("test").Get(fakeCtx, "test", metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, len(pod.Annotations), 0) @@ -183,14 +202,18 @@ func TestMeshDataplaneRemovePodErrorDoesntRemoveAnnotation(t *testing.T) { ).Return(errors.New("fake error")) fakeClientSet := fake.NewSimpleClientset(pod) - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + m := getFakeDPWithIPSet(server, fakeClientSet, set) + expectPodRemovedFromIPSet(fakeIPSetDeps, pod.Status.PodIPs) err := m.RemovePodFromMesh(fakeCtx, pod, false) assert.Error(t, err) + fakeIPSetDeps.AssertExpectations(t) + pod, err = fakeClientSet.CoreV1().Pods("test").Get(fakeCtx, "test", metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, pod.Annotations[constants.AmbientRedirection], constants.AmbientRedirectionEnabled) @@ -210,13 +233,17 @@ func TestMeshDataplaneDelPod(t *testing.T) { ).Return(nil) fakeClientSet := fake.NewSimpleClientset() - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + expectPodRemovedFromIPSet(fakeIPSetDeps, pod.Status.PodIPs) // pod is not in fake client, so if this will try to remove annotation, it will fail. err := m.RemovePodFromMesh(fakeCtx, pod, true) + + fakeIPSetDeps.AssertExpectations(t) + assert.NoError(t, err) } @@ -234,16 +261,297 @@ func TestMeshDataplaneDelPodErrorDoesntPatchPod(t *testing.T) { ).Return(errors.New("fake error")) fakeClientSet := fake.NewSimpleClientset() - m := meshDataplane{ - kubeClient: fakeClientSet, - netServer: server, - } + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + m := getFakeDPWithIPSet(server, fakeClientSet, set) + expectPodRemovedFromIPSet(fakeIPSetDeps, pod.Status.PodIPs) // pod is not in fake client, so if this will try to remove annotation, it will fail. err := m.RemovePodFromMesh(fakeCtx, pod, true) + + fakeIPSetDeps.AssertExpectations(t) + assert.Error(t, err) } +func TestMeshDataplaneAddPodToHostNSIPSets(t *testing.T) { + pod := buildConvincingPod() + + fakeCtx := context.Background() + server := &fakeServer{} + server.Start(fakeCtx) + fakeClientSet := fake.NewSimpleClientset() + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + + var podUID string = string(pod.ObjectMeta.UID) + ipProto := uint8(unix.IPPROTO_TCP) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("99.9.9.9"), + ipProto, + podUID, + false, + ).Return(nil) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ipProto, + podUID, + false, + ).Return(nil) + + podIPs := []netip.Addr{netip.MustParseAddr("99.9.9.9"), netip.MustParseAddr("2.2.2.2")} + _, err := m.addPodToHostNSIpset(pod, podIPs) + assert.NoError(t, err) + + fakeIPSetDeps.AssertExpectations(t) +} + +func TestMeshDataplaneAddPodIPToHostNSIPSetsReturnsErrorIfOneFails(t *testing.T) { + pod := buildConvincingPod() + + fakeCtx := context.Background() + server := &fakeServer{} + server.Start(fakeCtx) + fakeClientSet := fake.NewSimpleClientset() + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + + var podUID string = string(pod.ObjectMeta.UID) + ipProto := uint8(unix.IPPROTO_TCP) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("99.9.9.9"), + ipProto, + podUID, + false, + ).Return(nil) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ipProto, + podUID, + false, + ).Return(errors.New("bwoah")) + + podIPs := []netip.Addr{netip.MustParseAddr("99.9.9.9"), netip.MustParseAddr("2.2.2.2")} + addedPIPs, err := m.addPodToHostNSIpset(pod, podIPs) + assert.Error(t, err) + assert.Equal(t, 1, len(addedPIPs), "only expected one IP to be added") + + fakeIPSetDeps.AssertExpectations(t) +} + +func TestMeshDataplaneRemovePodIPFromHostNSIPSets(t *testing.T) { + pod := buildConvincingPod() + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + fakeIPSetDeps.On("clearEntriesWithIP", + "foo", + netip.MustParseAddr("3.3.3.3"), + ).Return(nil) + + fakeIPSetDeps.On("clearEntriesWithIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ).Return(nil) + + err := removePodFromHostNSIpset(pod, &set) + assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) +} + +func TestMeshDataplaneSyncHostIPSetsPrunesNothingIfNoExtras(t *testing.T) { + pod := buildConvincingPod() + + fakeCtx := context.Background() + server := &fakeServer{} + server.Start(fakeCtx) + fakeClientSet := fake.NewSimpleClientset() + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + + var podUID string = string(pod.ObjectMeta.UID) + ipProto := uint8(unix.IPPROTO_TCP) + + // expectations + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("3.3.3.3"), + ipProto, + podUID, + false, + ).Return(nil) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ipProto, + podUID, + false, + ).Return(nil) + + fakeIPSetDeps.On("listEntriesByIP", + "foo", + ).Return([]netip.Addr{}, nil) + + err := m.syncHostIPSets([]*corev1.Pod{pod}) + assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) +} + +func TestMeshDataplaneSyncHostIPSetsIgnoresPodIPAddErrorAndContinues(t *testing.T) { + pod1 := buildConvincingPod() + pod2 := buildConvincingPod() + + pod2.ObjectMeta.SetUID("4455") + + fakeClientSet := fake.NewSimpleClientset() + + fakeCtx := context.Background() + server := &fakeServer{} + server.Start(fakeCtx) + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + + var pod1UID string = string(pod1.ObjectMeta.UID) + var pod2UID string = string(pod2.ObjectMeta.UID) + ipProto := uint8(unix.IPPROTO_TCP) + + // First IP of first pod should error, but we should add the rest + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("3.3.3.3"), + ipProto, + pod1UID, + false, + ).Return(errors.New("CANNOT ADD")) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ipProto, + pod1UID, + false, + ).Return(nil) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("3.3.3.3"), + ipProto, + pod2UID, + false, + ).Return(errors.New("CANNOT ADD")) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ipProto, + pod2UID, + false, + ).Return(nil) + + fakeIPSetDeps.On("listEntriesByIP", + "foo", + ).Return([]netip.Addr{}, nil) + + err := m.syncHostIPSets([]*corev1.Pod{pod1, pod2}) + assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) +} + +func TestMeshDataplaneSyncHostIPSetsAddsNothingIfPodHasNoIPs(t *testing.T) { + pod := buildConvincingPod() + + pod.Status.PodIP = "" + pod.Status.PodIPs = []corev1.PodIP{} + + fakeCtx := context.Background() + server := &fakeServer{} + server.Start(fakeCtx) + fakeClientSet := fake.NewSimpleClientset() + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + + fakeIPSetDeps.On("listEntriesByIP", + "foo", + ).Return([]netip.Addr{}, nil) + + err := m.syncHostIPSets([]*corev1.Pod{pod}) + assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) +} + +func TestMeshDataplaneSyncHostIPSetsPrunesIfExtras(t *testing.T) { + pod := buildConvincingPod() + + var podUID string = string(pod.ObjectMeta.UID) + ipProto := uint8(unix.IPPROTO_TCP) + + fakeCtx := context.Background() + server := &fakeServer{} + server.Start(fakeCtx) + fakeClientSet := fake.NewSimpleClientset() + + fakeIPSetDeps := ipset.FakeNLDeps() + set := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + m := getFakeDPWithIPSet(server, fakeClientSet, set) + + // expectations + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("3.3.3.3"), + ipProto, + podUID, + false, + ).Return(nil) + + fakeIPSetDeps.On("addIP", + "foo", + netip.MustParseAddr("2.2.2.2"), + ipProto, + podUID, + false, + ).Return(nil) + + // List should return one IP not in our "pod snapshot", which means we prune + fakeIPSetDeps.On("listEntriesByIP", + "foo", + ).Return([]netip.Addr{ + netip.MustParseAddr("2.2.2.2"), + netip.MustParseAddr("6.6.6.6"), + netip.MustParseAddr("3.3.3.3"), + }, nil) + + fakeIPSetDeps.On("clearEntriesWithIP", + "foo", + netip.MustParseAddr("6.6.6.6"), + ).Return(nil) + + err := m.syncHostIPSets([]*corev1.Pod{pod}) + assert.NoError(t, err) + fakeIPSetDeps.AssertExpectations(t) +} + func podWithAnnotation() *corev1.Pod { return &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -339,3 +647,47 @@ func (wg *WaitGroup) Done() { func (wg *WaitGroup) C() <-chan struct{} { return wg.done } + +func expectPodAddedToIPSet(ipsetDeps *ipset.MockedIpsetDeps, podIP netip.Addr, podMeta metav1.ObjectMeta) { + ipsetDeps.On("addIP", + "foo", + podIP, + uint8(unix.IPPROTO_TCP), + string(podMeta.UID), + false, + ).Return(nil) +} + +func expectPodRemovedFromIPSet(ipsetDeps *ipset.MockedIpsetDeps, podIPs []corev1.PodIP) { + for _, ip := range podIPs { + ipsetDeps.On("clearEntriesWithIP", + "foo", + netip.MustParseAddr(ip.IP), + ).Return(nil) + } +} + +func getFakeDPWithIPSet(fs *fakeServer, fakeClient kubernetes.Interface, fakeSet ipset.IPSet) *meshDataplane { + return &meshDataplane{ + kubeClient: fakeClient, + netServer: fs, + hostsideProbeIPSet: fakeSet, + } +} + +func getFakeDP(fs *fakeServer, fakeClient kubernetes.Interface) *meshDataplane { + fakeIPSetDeps := ipset.FakeNLDeps() + + fakeIPSetDeps.On("addIP", + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(nil).Maybe() + + fakeIPSetDeps.On("clearEntriesWithIP", mock.Anything, mock.Anything).Return(nil).Maybe() + fakeSet := ipset.IPSet{Name: "foo", Deps: fakeIPSetDeps} + + return getFakeDPWithIPSet(fs, fakeClient, fakeSet) +} diff --git a/common/.commonfiles.sha b/common/.commonfiles.sha index 21c0083e876d..4a1565379696 100644 --- a/common/.commonfiles.sha +++ b/common/.commonfiles.sha @@ -1 +1 @@ -bd056af56a018c5245e8657b6cc3fed1c82b12c0 +2f988bb7f975a3426624f4d9e92ea26d542b1b6f diff --git a/common/scripts/setup_env.sh b/common/scripts/setup_env.sh index 9deb1497a91f..604671c1b134 100755 --- a/common/scripts/setup_env.sh +++ b/common/scripts/setup_env.sh @@ -75,7 +75,7 @@ fi TOOLS_REGISTRY_PROVIDER=${TOOLS_REGISTRY_PROVIDER:-gcr.io} PROJECT_ID=${PROJECT_ID:-istio-testing} if [[ "${IMAGE_VERSION:-}" == "" ]]; then - IMAGE_VERSION=release-1.22-46fce460ef8547fb88a20de8494683bfb3bfa8e5 + IMAGE_VERSION=release-1.22-02098ccc0766fde1c577cf9f9258fb43a08ec8c8 fi if [[ "${IMAGE_NAME:-}" == "" ]]; then IMAGE_NAME=build-tools diff --git a/go.mod b/go.mod index f390fd77d887..832949058d6a 100644 --- a/go.mod +++ b/go.mod @@ -106,8 +106,8 @@ require ( gopkg.in/yaml.v2 v2.4.0 gopkg.in/yaml.v3 v3.0.1 helm.sh/helm/v3 v3.14.3 - istio.io/api v1.22.3-0.20240703105953-437a88321a16 - istio.io/client-go v1.22.3-0.20240703110620-5f69a1e4c030 + istio.io/api v1.22.4-0.20240808015337-e0ff1ca45c33 + istio.io/client-go v1.22.4-0.20240808020015-3d90011dbcfe k8s.io/api v0.30.0 k8s.io/apiextensions-apiserver v0.30.0 k8s.io/apimachinery v0.30.0 @@ -146,7 +146,7 @@ require ( github.com/cyphar/filepath-securejoin v0.2.4 // indirect github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect github.com/docker/distribution v2.8.3+incompatible // indirect - github.com/docker/docker v25.0.5+incompatible // indirect + github.com/docker/docker v25.0.6+incompatible // indirect github.com/docker/docker-credential-helpers v0.8.1 // indirect github.com/emicklei/go-restful/v3 v3.12.0 // indirect github.com/envoyproxy/protoc-gen-validate v1.0.4 // indirect diff --git a/go.sum b/go.sum index 39b369acc9cb..6587288280b4 100644 --- a/go.sum +++ b/go.sum @@ -162,8 +162,8 @@ github.com/docker/cli v26.0.0+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvM github.com/docker/distribution v2.8.3+incompatible h1:AtKxIZ36LoNK51+Z6RpzLpddBirtxJnzDrHLEKxTAYk= github.com/docker/distribution v2.8.3+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w= github.com/docker/docker v0.7.3-0.20190327010347-be7ac8be2ae0/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= -github.com/docker/docker v25.0.5+incompatible h1:UmQydMduGkrD5nQde1mecF/YnSbTOaPeFIeP5C4W+DE= -github.com/docker/docker v25.0.5+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= +github.com/docker/docker v25.0.6+incompatible h1:5cPwbwriIcsua2REJe8HqQV+6WlWc1byg2QSXzBxBGg= +github.com/docker/docker v25.0.6+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= github.com/docker/docker-credential-helpers v0.8.1 h1:j/eKUktUltBtMzKqmfLB0PAgqYyMHOp5vfsD1807oKo= github.com/docker/docker-credential-helpers v0.8.1/go.mod h1:P3ci7E3lwkZg6XiHdRKft1KckHiO9a2rNtyFbZ/ry9M= github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= @@ -1102,10 +1102,10 @@ helm.sh/helm/v3 v3.14.3/go.mod h1:v6myVbyseSBJTzhmeE39UcPLNv6cQK6qss3dvgAySaE= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= -istio.io/api v1.22.3-0.20240703105953-437a88321a16 h1:0XRnzLqAZ1BYX1jBzBG1slXytUkdhRoVwC23upTyFl4= -istio.io/api v1.22.3-0.20240703105953-437a88321a16/go.mod h1:S3l8LWqNYS9yT+d4bH+jqzH2lMencPkW7SKM1Cu9EyM= -istio.io/client-go v1.22.3-0.20240703110620-5f69a1e4c030 h1:K/G2h2qeso2/xuNQFDNiJqcVNvOlyjsdACDN6Db+zkI= -istio.io/client-go v1.22.3-0.20240703110620-5f69a1e4c030/go.mod h1:D/vNne1n5586423NgGXMnPgshE/99mQgnjnxK/Vw2yM= +istio.io/api v1.22.4-0.20240808015337-e0ff1ca45c33 h1:/IeYCiL05FL8ZxndwibKznhLsrZRDH0xaHwsk/roU7I= +istio.io/api v1.22.4-0.20240808015337-e0ff1ca45c33/go.mod h1:S3l8LWqNYS9yT+d4bH+jqzH2lMencPkW7SKM1Cu9EyM= +istio.io/client-go v1.22.4-0.20240808020015-3d90011dbcfe h1:8E+07PR3a1LypFLxNksDYcTwyMPB06797cavwK5zWds= +istio.io/client-go v1.22.4-0.20240808020015-3d90011dbcfe/go.mod h1:pCCBfkXZVAxptGlL5gdGIonPxFsNQZ+iBxvYIUF9z7c= k8s.io/api v0.18.2/go.mod h1:SJCWI7OLzhZSvbY7U8zwNl9UA4o1fizoug34OV/2r78= k8s.io/api v0.18.4/go.mod h1:lOIQAKYgai1+vz9J7YcDZwC26Z0zQewYOGWdyIPUUQ4= k8s.io/api v0.30.0 h1:siWhRq7cNjy2iHssOB9SCGNCl2spiF1dO3dABqZ8niA= diff --git a/istio.deps b/istio.deps index 3cb1118e2b03..09115cce8e96 100644 --- a/istio.deps +++ b/istio.deps @@ -4,13 +4,13 @@ "name": "PROXY_REPO_SHA", "repoName": "proxy", "file": "", - "lastStableSHA": "fbc0329f0bf8ab3c22108a3552cf567358244494" + "lastStableSHA": "9603bcb6fd358cb0affbb662630751d77ab66efb" }, { "_comment": "", "name": "ZTUNNEL_REPO_SHA", "repoName": "ztunnel", "file": "", - "lastStableSHA": "dfb420fda59271c23ab7abb132bb63f756639f6f" + "lastStableSHA": "2eaa669fecf8505c9abb9676b64ff6a50f124a37" } ] diff --git a/manifests/charts/base/files/profile-compatibility-version-1.21.yaml b/manifests/charts/base/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/base/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/base/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/base/templates/zzz_profile.yaml b/manifests/charts/base/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/base/templates/zzz_profile.yaml +++ b/manifests/charts/base/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/default/files/profile-compatibility-version-1.21.yaml b/manifests/charts/default/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/default/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/default/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/default/templates/zzz_profile.yaml b/manifests/charts/default/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/default/templates/zzz_profile.yaml +++ b/manifests/charts/default/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/gateway/files/profile-compatibility-version-1.21.yaml b/manifests/charts/gateway/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/gateway/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/gateway/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/gateway/templates/zzz_profile.yaml b/manifests/charts/gateway/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/gateway/templates/zzz_profile.yaml +++ b/manifests/charts/gateway/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/gateway/values.schema.json b/manifests/charts/gateway/values.schema.json index c97d84c1e551..4c4f0836d7e0 100644 --- a/manifests/charts/gateway/values.schema.json +++ b/manifests/charts/gateway/values.schema.json @@ -99,10 +99,10 @@ "type": "object", "properties": { "cpu": { - "type": "string" + "type": ["string", "null"] }, "memory": { - "type": "string" + "type": ["string", "null"] } } }, @@ -110,10 +110,10 @@ "type": "object", "properties": { "cpu": { - "type": "string" + "type": ["string", "null"] }, "memory": { - "type": "string" + "type": ["string", "null"] } } } diff --git a/manifests/charts/gateways/istio-egress/files/profile-compatibility-version-1.21.yaml b/manifests/charts/gateways/istio-egress/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/gateways/istio-egress/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/gateways/istio-egress/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/gateways/istio-egress/templates/zzz_profile.yaml b/manifests/charts/gateways/istio-egress/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/gateways/istio-egress/templates/zzz_profile.yaml +++ b/manifests/charts/gateways/istio-egress/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/gateways/istio-ingress/files/profile-compatibility-version-1.21.yaml b/manifests/charts/gateways/istio-ingress/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/gateways/istio-ingress/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/gateways/istio-ingress/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/gateways/istio-ingress/templates/zzz_profile.yaml b/manifests/charts/gateways/istio-ingress/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/gateways/istio-ingress/templates/zzz_profile.yaml +++ b/manifests/charts/gateways/istio-ingress/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/istio-cni/files/profile-compatibility-version-1.21.yaml b/manifests/charts/istio-cni/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/istio-cni/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/istio-cni/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/istio-cni/templates/zzz_profile.yaml b/manifests/charts/istio-cni/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/istio-cni/templates/zzz_profile.yaml +++ b/manifests/charts/istio-cni/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/istio-control/istio-discovery/files/profile-compatibility-version-1.21.yaml b/manifests/charts/istio-control/istio-discovery/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/istio-control/istio-discovery/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/istio-control/istio-discovery/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/istio-control/istio-discovery/templates/NOTES.txt b/manifests/charts/istio-control/istio-discovery/templates/NOTES.txt index 0771b919d315..1acb4dbf90e7 100644 --- a/manifests/charts/istio-control/istio-discovery/templates/NOTES.txt +++ b/manifests/charts/istio-control/istio-discovery/templates/NOTES.txt @@ -5,7 +5,8 @@ To learn more about the release, try: $ helm get all {{ .Release.Name }} -n {{ .Release.Namespace }} Next steps: -{{- if (eq .Values.profile "ambient") }} +{{- $profile := default "" .Values.profile }} +{{- if (eq $profile "ambient") }} * Get started with ambient: https://istio.io/latest/docs/ops/ambient/getting-started/ * Review ambient's architecture: https://istio.io/latest/docs/ops/ambient/architecture/ {{- else }} diff --git a/manifests/charts/istio-control/istio-discovery/templates/zzz_profile.yaml b/manifests/charts/istio-control/istio-discovery/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/istio-control/istio-discovery/templates/zzz_profile.yaml +++ b/manifests/charts/istio-control/istio-discovery/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/istio-operator/files/profile-compatibility-version-1.21.yaml b/manifests/charts/istio-operator/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/istio-operator/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/istio-operator/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/istio-operator/templates/zzz_profile.yaml b/manifests/charts/istio-operator/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/istio-operator/templates/zzz_profile.yaml +++ b/manifests/charts/istio-operator/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/istiod-remote/files/profile-compatibility-version-1.21.yaml b/manifests/charts/istiod-remote/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/istiod-remote/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/istiod-remote/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/istiod-remote/templates/zzz_profile.yaml b/manifests/charts/istiod-remote/templates/zzz_profile.yaml index 6359d435a74e..2d0bd4af7a35 100644 --- a/manifests/charts/istiod-remote/templates/zzz_profile.yaml +++ b/manifests/charts/istiod-remote/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if false }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/charts/ztunnel/files/profile-compatibility-version-1.21.yaml b/manifests/charts/ztunnel/files/profile-compatibility-version-1.21.yaml index 808d224ed8d2..a204a7ad4e28 100644 --- a/manifests/charts/ztunnel/files/profile-compatibility-version-1.21.yaml +++ b/manifests/charts/ztunnel/files/profile-compatibility-version-1.21.yaml @@ -9,9 +9,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/charts/ztunnel/templates/_helpers.tpl b/manifests/charts/ztunnel/templates/_helpers.tpl new file mode 100644 index 000000000000..5ef0d0e40730 --- /dev/null +++ b/manifests/charts/ztunnel/templates/_helpers.tpl @@ -0,0 +1,10 @@ +{{- define "istio-labels" }} + app.kubernetes.io/name: ztunnel + {{- if .Release.Service }} + app.kubernetes.io/managed-by: {{ .Release.Service }} + {{- end }} + {{- if .Release.Name}} + app.kubernetes.io/instance: {{ .Release.Name }} + {{- end }} + app.kubernetes.io/part-of: istio +{{- end }} diff --git a/manifests/charts/ztunnel/templates/daemonset.yaml b/manifests/charts/ztunnel/templates/daemonset.yaml index 5d600d3cc5b1..f83c36dfa252 100644 --- a/manifests/charts/ztunnel/templates/daemonset.yaml +++ b/manifests/charts/ztunnel/templates/daemonset.yaml @@ -4,7 +4,8 @@ metadata: name: ztunnel namespace: {{ .Release.Namespace }} labels: - {{- .Values.labels | toYaml | nindent 4}} +{{- template "istio-labels" -}} +{{ with .Values.labels -}}{{ toYaml . | nindent 4}}{{ end }} annotations: {{- .Values.annotations | toYaml | nindent 4 }} spec: diff --git a/manifests/charts/ztunnel/templates/rbac.yaml b/manifests/charts/ztunnel/templates/rbac.yaml index 9583b200ed17..f332898e1859 100644 --- a/manifests/charts/ztunnel/templates/rbac.yaml +++ b/manifests/charts/ztunnel/templates/rbac.yaml @@ -10,7 +10,8 @@ metadata: name: ztunnel namespace: {{ .Release.Namespace }} labels: - {{- .Values.labels | toYaml | nindent 4}} +{{- template "istio-labels" -}} +{{ with .Values.labels -}}{{ toYaml . | nindent 4}}{{ end }} annotations: {{- .Values.annotations | toYaml | nindent 4 }} --- diff --git a/manifests/charts/ztunnel/templates/zzz_profile.yaml b/manifests/charts/ztunnel/templates/zzz_profile.yaml index 6359d435a74e..752a7f3756f1 100644 --- a/manifests/charts/ztunnel/templates/zzz_profile.yaml +++ b/manifests/charts/ztunnel/templates/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if true }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/manifests/helm-profiles/compatibility-version-1.21.yaml b/manifests/helm-profiles/compatibility-version-1.21.yaml index 9d8618aa9264..327278a209c5 100644 --- a/manifests/helm-profiles/compatibility-version-1.21.yaml +++ b/manifests/helm-profiles/compatibility-version-1.21.yaml @@ -5,9 +5,9 @@ pilot: ENABLE_RESOLUTION_NONE_TARGET_PORT: "false" meshConfig: # 1.22 behavioral changes - proxyMetadata: - ISTIO_DELTA_XDS: "false" defaultConfig: + proxyMetadata: + ISTIO_DELTA_XDS: "false" tracing: zipkin: address: zipkin.istio-system:9411 diff --git a/manifests/zzz_profile.yaml b/manifests/zzz_profile.yaml index 6359d435a74e..21600dc8cd1c 100644 --- a/manifests/zzz_profile.yaml +++ b/manifests/zzz_profile.yaml @@ -15,6 +15,7 @@ However, we can workaround this by placing all of (1) under a specific key (.Val We can then merge the profile onto the defaults, then the user settings onto that. Finally, we can set all of that under .Values so the chart behaves without awareness. */}} +{{- $globals := $.Values.global | default dict | deepCopy }} {{- $defaults := $.Values.defaults }} {{- $_ := unset $.Values "defaults" }} {{- $profile := dict }} @@ -35,4 +36,8 @@ Finally, we can set all of that under .Values so the chart behaves without aware {{- if $profile }} {{- $a := mustMergeOverwrite $defaults $profile }} {{- end }} +# Flatten globals, if defined on a per-chart basis +{{- if FLATTEN_GLOBALS_REPLACEMENT }} +{{- $a := mustMergeOverwrite $defaults $globals }} +{{- end }} {{- $b := set $ "Values" (mustMergeOverwrite $defaults $.Values) }} diff --git a/operator/cmd/mesh/manifest-generate_test.go b/operator/cmd/mesh/manifest-generate_test.go index 420e7488793e..744907dbef6e 100644 --- a/operator/cmd/mesh/manifest-generate_test.go +++ b/operator/cmd/mesh/manifest-generate_test.go @@ -189,35 +189,47 @@ func TestMain(m *testing.M) { func TestManifestGenerateComponentHubTag(t *testing.T) { g := NewWithT(t) - objs, err := runManifestCommands("component_hub_tag", "", liveCharts, []string{"templates/deployment.yaml"}) + objs, err := runManifestCommands("component_hub_tag", "", liveCharts, []string{"templates/deployment.yaml", "templates/daemonset.yaml"}) if err != nil { t.Fatal(err) } tests := []struct { deploymentName string + daemonsetName string containerName string want string }{ { deploymentName: "istio-ingressgateway", containerName: "istio-proxy", - want: "istio-spec.hub/proxyv2:istio-spec.tag", + want: "istio-spec.hub/proxyv2:istio-spec.tag-global.variant", }, { deploymentName: "istiod", containerName: "discovery", - want: "component.pilot.hub/pilot:2", + want: "component.pilot.hub/pilot:2-global.variant", + }, + { + daemonsetName: "istio-cni-node", + containerName: "install-cni", + want: "component.cni.hub/install-cni:v3.3.3-global.variant", + }, + { + daemonsetName: "ztunnel", + containerName: "istio-proxy", + want: "component.ztunnel.hub/ztunnel:4-global.variant", }, } for _, tt := range tests { for _, os := range objs { - containerName := tt.deploymentName - if tt.containerName != "" { - containerName = tt.containerName + var container map[string]any + if tt.deploymentName != "" { + container = mustGetContainer(g, os, tt.deploymentName, tt.containerName) + } else { + container = mustGetContainerFromDaemonset(g, os, tt.daemonsetName, tt.containerName) } - container := mustGetContainer(g, os, tt.deploymentName, containerName) g.Expect(container).Should(HavePathValueEqual(PathValue{"image", tt.want})) } } diff --git a/operator/cmd/mesh/test-util_test.go b/operator/cmd/mesh/test-util_test.go index 2d1282d732e7..c2548f5784d8 100644 --- a/operator/cmd/mesh/test-util_test.go +++ b/operator/cmd/mesh/test-util_test.go @@ -170,6 +170,13 @@ func mustGetDeployment(g *WithT, objs *ObjectSet, deploymentName string) *object return obj } +// mustGetDaemonset returns the DaemonSet with the given name or fails if it's not found in objs. +func mustGetDaemonset(g *WithT, objs *ObjectSet, daemonSetName string) *object.K8sObject { + obj := objs.kind(name2.DaemonSetStr).nameEquals(daemonSetName) + g.Expect(obj).Should(Not(BeNil())) + return obj +} + // mustGetClusterRole returns the clusterRole with the given name or fails if it's not found in objs. func mustGetClusterRole(g *WithT, objs *ObjectSet, name string) *object.K8sObject { obj := objs.kind(name2.ClusterRoleStr).nameEquals(name) @@ -192,6 +199,14 @@ func mustGetContainer(g *WithT, objs *ObjectSet, deploymentName, containerName s return container } +// mustGetContainer returns the container tree with the given name in the deployment with the given name. +func mustGetContainerFromDaemonset(g *WithT, objs *ObjectSet, daemonSetName, containerName string) map[string]any { + obj := mustGetDaemonset(g, objs, daemonSetName) + container := obj.Container(containerName) + g.Expect(container).Should(Not(BeNil()), fmt.Sprintf("Expected to get container %s in daemonset %s", containerName, daemonSetName)) + return container +} + // mustGetEndpoint returns the endpoint tree with the given name in the deployment with the given name. func mustGetEndpoint(g *WithT, objs *ObjectSet, endpointName string) *object.K8sObject { obj := objs.kind(name2.EndpointStr).nameEquals(endpointName) diff --git a/operator/cmd/mesh/testdata/manifest-generate/input/component_hub_tag.yaml b/operator/cmd/mesh/testdata/manifest-generate/input/component_hub_tag.yaml index 971e8a180ef0..a228c211dbe4 100644 --- a/operator/cmd/mesh/testdata/manifest-generate/input/component_hub_tag.yaml +++ b/operator/cmd/mesh/testdata/manifest-generate/input/component_hub_tag.yaml @@ -3,6 +3,9 @@ kind: IstioOperator spec: hub: istio-spec.hub tag: istio-spec.tag + values: + global: + variant: global.variant components: pilot: enabled: true @@ -12,4 +15,8 @@ spec: enabled: true hub: component.cni.hub tag: v3.3.3 + ztunnel: + enabled: true + hub: component.ztunnel.hub + tag: 4 diff --git a/operator/pkg/translate/translate.go b/operator/pkg/translate/translate.go index b16156006417..8808299bcc0a 100644 --- a/operator/pkg/translate/translate.go +++ b/operator/pkg/translate/translate.go @@ -531,15 +531,13 @@ func (t *Translator) TranslateHelmValues(iop *v1alpha1.IstioOperatorSpec, compon } c, f := t.ComponentMaps[componentName] if f && c.FlattenValues { - globals, ok := mergedVals["global"].(map[string]any) - if !ok { - return "", fmt.Errorf("global value isn't a map") - } components, ok := mergedVals[c.ToHelmValuesTreeRoot].(map[string]any) if !ok { return "", fmt.Errorf("component value isn't a map") } finalVals := map[string]any{} + // These get access to globals + finalVals["global"] = mergedVals["global"] // strip out anything from the original apiVals which are a map[string]any but populate other top-level fields for k, v := range apiVals { _, isMap := v.(map[string]any) @@ -552,9 +550,6 @@ func (t *Translator) TranslateHelmValues(iop *v1alpha1.IstioOperatorSpec, compon finalVals[k] = v } } - for k, v := range globals { - finalVals[k] = v - } for k, v := range components { finalVals[k] = v } diff --git a/pilot/pkg/autoregistration/controller.go b/pilot/pkg/autoregistration/controller.go index 1a7041031972..35747afbd817 100644 --- a/pilot/pkg/autoregistration/controller.go +++ b/pilot/pkg/autoregistration/controller.go @@ -702,8 +702,12 @@ func workloadEntryFromGroup(name string, proxy *model.Proxy, groupCfg *config.Co if proxy.Metadata.Network != "" { entry.Network = string(proxy.Metadata.Network) } - if proxy.Locality != nil { - entry.Locality = util.LocalityToString(proxy.Locality) + // proxy.Locality can be unset when auto registration takes place, because + // its state is not fully initialized. Therefore, we check the bootstrap + // node. + if proxy.XdsNode != nil && proxy.XdsNode.Locality != nil { + entry.Locality = util.LocalityToString(proxy.XdsNode.Locality) + log.Infof("Setting Locality: %s for WLE: %s via XdsNode", entry.Locality, name) } if proxy.Metadata.ProxyConfig != nil && proxy.Metadata.ProxyConfig.ReadinessProbe != nil { annotations[status.WorkloadEntryHealthCheckAnnotation] = "true" diff --git a/pilot/pkg/autoregistration/controller_test.go b/pilot/pkg/autoregistration/controller_test.go index 63bce1447bb6..db8da54a60f4 100644 --- a/pilot/pkg/autoregistration/controller_test.go +++ b/pilot/pkg/autoregistration/controller_test.go @@ -209,15 +209,15 @@ func TestAutoregistrationLifecycle(t *testing.T) { var p1conn1, p1conn2 *fakeConn p := fakeProxy("1.2.3.4", wgA, "nw1", "sa-a") - p.Locality = n.Locality + p.XdsNode = n var p2conn1 *fakeConn p2 := fakeProxy("1.2.3.4", wgA, "nw2", "sa-a") - p2.Locality = n.Locality + p2.XdsNode = n var p3conn1 *fakeConn p3 := fakeProxy("1.2.3.5", wgA, "nw1", "sa-a") - p3.Locality = n.Locality + p3.XdsNode = n t.Run("initial registration", func(t *testing.T) { // simply make sure the entry exists after connecting @@ -432,7 +432,6 @@ func TestWorkloadEntryFromGroup(t *testing.T) { proxy := fakeProxy("10.0.0.1", group, "nw1", "sa") proxy.Labels[model.LocalityLabel] = "rgn2/zone2/subzone2" proxy.XdsNode = fakeNode("rgn2", "zone2", "subzone2") - proxy.Locality = proxy.XdsNode.Locality wantLabels := map[string]string{ "app": "a", // from WorkloadEntry template diff --git a/pilot/pkg/config/kube/crdclient/client.go b/pilot/pkg/config/kube/crdclient/client.go index 8ef615774526..8af2bf2e0890 100644 --- a/pilot/pkg/config/kube/crdclient/client.go +++ b/pilot/pkg/config/kube/crdclient/client.go @@ -435,7 +435,11 @@ func composeFilters(filter kubetypes.DynamicObjectFilter, extra ...func(obj any) } func (cl *Client) inRevision(obj any) bool { - return config.LabelsInRevision(obj.(controllers.Object).GetLabels(), cl.revision) + object := controllers.ExtractObject(obj) + if object == nil { + return false + } + return config.LabelsInRevision(object.GetLabels(), cl.revision) } func (cl *Client) onEvent(resourceGVK config.GroupVersionKind, old controllers.Object, curr controllers.Object, event model.Event) { diff --git a/pilot/pkg/config/kube/gateway/conversion.go b/pilot/pkg/config/kube/gateway/conversion.go index a10cde941798..df3fedbe7bc1 100644 --- a/pilot/pkg/config/kube/gateway/conversion.go +++ b/pilot/pkg/config/kube/gateway/conversion.go @@ -1862,7 +1862,7 @@ func createGRPCURIMatch(match k8s.GRPCRouteMatch) (*istio.StringMatch, *ConfigEr // getGatewayClass finds all gateway class that are owned by Istio // Response is ClassName -> Controller type -func getGatewayClasses(r GatewayResources, supportedFeatures []k8s.SupportedFeature) map[string]k8s.GatewayController { +func getGatewayClasses(r GatewayResources) map[string]k8s.GatewayController { res := map[string]k8s.GatewayController{} // Setup builtin ones - these can be overridden possibly for name, controller := range builtinClasses { @@ -1880,7 +1880,6 @@ func getGatewayClasses(r GatewayResources, supportedFeatures []k8s.SupportedFeat obj.Status.(*kstatus.WrappedStatus).Mutate(func(s config.Status) config.Status { gcs := s.(*k8s.GatewayClassStatus) *gcs = GetClassStatus(gcs, obj.Generation) - gcs.SupportedFeatures = supportedFeatures return gcs }) } @@ -2029,7 +2028,7 @@ func convertGateways(r configContext) ([]config.Config, map[parentKey][]*parentI // namespaceLabelReferences keeps track of all namespace label keys referenced by Gateways. This is // used to ensure we handle namespace updates for those keys. namespaceLabelReferences := sets.New[string]() - classes := getGatewayClasses(r.GatewayResources, gatewaySupportedFeatures) + classes := getGatewayClasses(r.GatewayResources) for _, obj := range r.Gateway { obj := obj kgw := obj.Spec.(*k8s.GatewaySpec) diff --git a/pilot/pkg/config/kube/gateway/supported_features.go b/pilot/pkg/config/kube/gateway/supported_features.go index ca1934a94cab..e71988cacb80 100644 --- a/pilot/pkg/config/kube/gateway/supported_features.go +++ b/pilot/pkg/config/kube/gateway/supported_features.go @@ -15,20 +15,7 @@ package gateway import ( - k8sv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/pkg/features" - - "istio.io/istio/pkg/util/sets" ) var SupportedFeatures = features.AllFeatures - -var gatewaySupportedFeatures = getSupportedFeatures() - -func getSupportedFeatures() []k8sv1.SupportedFeature { - ret := sets.New[k8sv1.SupportedFeature]() - for _, feature := range SupportedFeatures.UnsortedList() { - ret.Insert(k8sv1.SupportedFeature(feature)) - } - return sets.SortedList(ret) -} diff --git a/pilot/pkg/model/endpointshards.go b/pilot/pkg/model/endpointshards.go index 327990aafb8c..7c8bcf097cfe 100644 --- a/pilot/pkg/model/endpointshards.go +++ b/pilot/pkg/model/endpointshards.go @@ -296,60 +296,8 @@ func (e *EndpointIndex) UpdateServiceEndpoints( ep.Lock() defer ep.Unlock() - newIstioEndpoints := istioEndpoints - oldIstioEndpoints := ep.Shards[shard] - needPush := false - if oldIstioEndpoints == nil { - // If there are no old endpoints, we should push with incoming endpoints as there is nothing to compare. - needPush = true - } else { - newIstioEndpoints = make([]*IstioEndpoint, 0, len(istioEndpoints)) - // Check if new Endpoints are ready to be pushed. This check - // will ensure that if a new pod comes with a non ready endpoint, - // we do not unnecessarily push that config to Envoy. - // Please note that address is not a unique key. So this may not accurately - // identify based on health status and push too many times - which is ok since its an optimization. - omap := make(map[string]*IstioEndpoint, len(oldIstioEndpoints)) - nmap := make(map[string]*IstioEndpoint, len(newIstioEndpoints)) - // Add new endpoints only if they are ever ready once to shards - // so that full push does not send them from shards. - for _, oie := range oldIstioEndpoints { - omap[oie.Address] = oie - } - for _, nie := range istioEndpoints { - nmap[nie.Address] = nie - } - for _, nie := range istioEndpoints { - if oie, exists := omap[nie.Address]; exists { - // If endpoint exists already, we should push if it's changed. - // Skip this check if we already decide we need to push to avoid expensive checks - if !needPush && !oie.Equals(nie) { - needPush = true - } - newIstioEndpoints = append(newIstioEndpoints, nie) - } else { - // If the endpoint does not exist in shards that means it is a - // new endpoint. Always send new healthy endpoints. - // Also send new unhealthy endpoints when SendUnhealthyEndpoints is enabled. - // This is OK since we disable panic threshold when SendUnhealthyEndpoints is enabled. - if nie.HealthStatus != UnHealthy || features.SendUnhealthyEndpoints.Load() { - needPush = true - } - newIstioEndpoints = append(newIstioEndpoints, nie) - } - } - // Next, check for endpoints that were in old but no longer exist. If there are any, there is a - // removal so we need to push an update. - if !needPush { - for _, oie := range oldIstioEndpoints { - if _, f := nmap[oie.Address]; !f { - needPush = true - break - } - } - } - } + newIstioEndpoints, needPush := endpointUpdateRequiresPush(oldIstioEndpoints, istioEndpoints) if pushType != FullPush && !needPush { log.Debugf("No push, either old endpoint health status did not change or new endpoint came with unhealthy status, %v", hostname) @@ -380,6 +328,60 @@ func (e *EndpointIndex) UpdateServiceEndpoints( return pushType } +// endpointUpdateRequiresPush determines if an endpoint update is required. +func endpointUpdateRequiresPush(oldIstioEndpoints []*IstioEndpoint, incomingEndpoints []*IstioEndpoint) ([]*IstioEndpoint, bool) { + if oldIstioEndpoints == nil { + // If there are no old endpoints, we should push with incoming endpoints as there is nothing to compare. + return incomingEndpoints, true + } + needPush := false + newIstioEndpoints := make([]*IstioEndpoint, 0, len(incomingEndpoints)) + // Check if new Endpoints are ready to be pushed. This check + // will ensure that if a new pod comes with a non ready endpoint, + // we do not unnecessarily push that config to Envoy. + omap := make(map[string]*IstioEndpoint, len(oldIstioEndpoints)) + nmap := make(map[string]*IstioEndpoint, len(newIstioEndpoints)) + // Add new endpoints only if they are ever ready once to shards + // so that full push does not send them from shards. + for _, oie := range oldIstioEndpoints { + omap[oie.Key()] = oie + } + for _, nie := range incomingEndpoints { + nmap[nie.Key()] = nie + } + for _, nie := range incomingEndpoints { + if oie, exists := omap[nie.Key()]; exists { + // If endpoint exists already, we should push if it's changed. + // Skip this check if we already decide we need to push to avoid expensive checks + if !needPush && !oie.Equals(nie) { + needPush = true + } + newIstioEndpoints = append(newIstioEndpoints, nie) + } else { + // If the endpoint does not exist in shards that means it is a + // new endpoint. Always send new healthy endpoints. + // Also send new unhealthy endpoints when SendUnhealthyEndpoints is enabled. + // This is OK since we disable panic threshold when SendUnhealthyEndpoints is enabled. + if nie.HealthStatus != UnHealthy || features.SendUnhealthyEndpoints.Load() { + needPush = true + } + newIstioEndpoints = append(newIstioEndpoints, nie) + } + } + // Next, check for endpoints that were in old but no longer exist. If there are any, there is a + // removal so we need to push an update. + if !needPush { + for _, oie := range oldIstioEndpoints { + if _, f := nmap[oie.Key()]; !f { + needPush = true + break + } + } + } + + return newIstioEndpoints, needPush +} + // updateShardServiceAccount updates the service endpoints' sa when service/endpoint event happens. // Note: it is not concurrent safe. func updateShardServiceAccount(shards *EndpointShards, serviceName string) bool { diff --git a/pilot/pkg/model/service.go b/pilot/pkg/model/service.go index 866a991e8094..3adf30a37095 100644 --- a/pilot/pkg/model/service.go +++ b/pilot/pkg/model/service.go @@ -585,6 +585,11 @@ func (ep *IstioEndpoint) CmpOpts() []cmp.Option { return istioEndpointCmpOpts } +// Key returns a function suitable for usage to distinguish this IstioEndpoint from another +func (ep *IstioEndpoint) Key() string { + return ep.Address + "/" + ep.WorkloadName +} + // EndpointMetadata represents metadata set on Envoy LbEndpoint used for telemetry purposes. type EndpointMetadata struct { // Network holds the network where this endpoint is present @@ -1265,13 +1270,32 @@ func (s *Service) GetAddressForProxy(node *Proxy) string { // GetExtraAddressesForProxy returns a k8s service's extra addresses to the cluster where the node resides. // Especially for dual stack k8s service to get other IP family addresses. func (s *Service) GetExtraAddressesForProxy(node *Proxy) []string { - if features.EnableDualStack && node.Metadata != nil { - if node.Metadata.ClusterID != "" { - addresses := s.ClusterVIPs.GetAddressesFor(node.Metadata.ClusterID) - if len(addresses) > 1 { - return addresses[1:] - } + addresses := s.getAllAddressesForProxy(node) + if len(addresses) > 1 { + return addresses[1:] + } + return nil +} + +// GetAllAddressesForProxy returns a k8s service's extra addresses to the cluster where the node resides. +// Especially for dual stack k8s service to get other IP family addresses. +func (s *Service) GetAllAddressesForProxy(node *Proxy) []string { + return s.getAllAddressesForProxy(node) +} + +func (s *Service) getAllAddressesForProxy(node *Proxy) []string { + if node.Metadata != nil && node.Metadata.ClusterID != "" { + addresses := s.ClusterVIPs.GetAddressesFor(node.Metadata.ClusterID) + if (features.EnableDualStack || features.EnableAmbient) && len(addresses) > 0 { + return addresses } + addresses = filterAddresses(addresses, node.SupportsIPv4(), node.SupportsIPv6()) + if len(addresses) > 0 { + return addresses + } + } + if a := s.GetAddressForProxy(node); a != "" { + return []string{a} } return nil } @@ -1287,6 +1311,37 @@ func (s *Service) getAllAddresses() []string { return addresses } +func filterAddresses(addresses []string, supportsV4, supportsV6 bool) []string { + var ipv4Addresses []string + var ipv6Addresses []string + for _, addr := range addresses { + // check if an address is a CIDR range + if strings.Contains(addr, "/") { + if prefix, err := netip.ParsePrefix(addr); err != nil { + log.Warnf("failed to parse prefix address '%s': %s", addr, err) + continue + } else if supportsV4 && prefix.Addr().Is4() { + ipv4Addresses = append(ipv4Addresses, addr) + } else if supportsV6 && prefix.Addr().Is6() { + ipv6Addresses = append(ipv6Addresses, addr) + } + } else { + if ipAddr, err := netip.ParseAddr(addr); err != nil { + log.Warnf("failed to parse address '%s': %s", addr, err) + continue + } else if supportsV4 && ipAddr.Is4() { + ipv4Addresses = append(ipv4Addresses, addr) + } else if supportsV6 && ipAddr.Is6() { + ipv6Addresses = append(ipv6Addresses, addr) + } + } + } + if len(ipv4Addresses) > 0 { + return ipv4Addresses + } + return ipv6Addresses +} + // GetTLSModeFromEndpointLabels returns the value of the label // security.istio.io/tlsMode if set. Do not return Enums or constants // from this function as users could provide values other than istio/disabled diff --git a/pilot/pkg/model/service_test.go b/pilot/pkg/model/service_test.go index e174d9f85e67..31e3f38e6708 100644 --- a/pilot/pkg/model/service_test.go +++ b/pilot/pkg/model/service_test.go @@ -21,11 +21,13 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" fuzz "github.com/google/gofuzz" + "istio.io/istio/pilot/pkg/features" "istio.io/istio/pkg/cluster" "istio.io/istio/pkg/config/constants" "istio.io/istio/pkg/config/host" "istio.io/istio/pkg/config/labels" "istio.io/istio/pkg/config/visibility" + "istio.io/istio/pkg/test" "istio.io/istio/pkg/test/util/assert" ) @@ -586,3 +588,145 @@ func TestParseSubsetKeyHostname(t *testing.T) { }) } } + +func TestGetAllAddresses(t *testing.T) { + tests := []struct { + name string + service *Service + ipMode IPMode + dualStackEnabled bool + ambientEnabled bool + autoAllocationEnabled bool + expectedAddresses []string + expectedExtraAddresses []string + }{ + { + name: "IPv4 mode, IPv4 and IPv6 CIDR addresses, expected to return only IPv4 addresses", + service: &Service{ + DefaultAddress: "10.0.0.0/28", + ClusterVIPs: AddressMap{ + Addresses: map[cluster.ID][]string{ + "id": {"10.0.0.0/28", "10.0.0.16/28", "::ffff:10.0.0.32/96", "::ffff:10.0.0.48/96"}, + }, + }, + }, + ipMode: IPv4, + expectedAddresses: []string{"10.0.0.0/28", "10.0.0.16/28"}, + expectedExtraAddresses: []string{"10.0.0.16/28"}, + }, + { + name: "IPv6 mode, IPv4 and IPv6 CIDR addresses, expected to return only IPv6 addresses", + service: &Service{ + DefaultAddress: "10.0.0.0/28", + ClusterVIPs: AddressMap{ + Addresses: map[cluster.ID][]string{ + "id": {"10.0.0.0/28", "10.0.0.16/28", "::ffff:10.0.0.32/96", "::ffff:10.0.0.48/96"}, + }, + }, + }, + ipMode: IPv6, + expectedAddresses: []string{"::ffff:10.0.0.32/96", "::ffff:10.0.0.48/96"}, + expectedExtraAddresses: []string{"::ffff:10.0.0.48/96"}, + }, + { + name: "dual mode, ISTIO_DUAL_STACK disabled, IPv4 and IPv6 addresses, expected to return only IPv4 addresses", + service: &Service{ + DefaultAddress: "10.0.0.0", + ClusterVIPs: AddressMap{ + Addresses: map[cluster.ID][]string{ + "id": {"10.0.0.0", "10.0.0.16", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + }, + }, + ipMode: Dual, + expectedAddresses: []string{"10.0.0.0", "10.0.0.16"}, + expectedExtraAddresses: []string{"10.0.0.16"}, + }, + { + name: "dual mode, ISTIO_DUAL_STACK enabled, IPv4 and IPv6 addresses, expected to return only IPv4 addresses", + service: &Service{ + DefaultAddress: "10.0.0.0", + ClusterVIPs: AddressMap{ + Addresses: map[cluster.ID][]string{ + "id": {"10.0.0.0", "10.0.0.16", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + }, + }, + ipMode: Dual, + dualStackEnabled: true, + expectedAddresses: []string{"10.0.0.0", "10.0.0.16", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + expectedExtraAddresses: []string{"10.0.0.16", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + { + name: "IPv4 mode, ISTIO_DUAL_STACK disabled, ambient enabled, IPv4 and IPv6 addresses, expected to return all addresses", + service: &Service{ + DefaultAddress: "10.0.0.0/28", + ClusterVIPs: AddressMap{ + Addresses: map[cluster.ID][]string{ + "id": {"10.0.0.0/28", "10.0.0.16/28", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + }, + }, + ipMode: IPv4, + ambientEnabled: true, + expectedAddresses: []string{"10.0.0.0/28", "10.0.0.16/28", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + expectedExtraAddresses: []string{"10.0.0.16/28", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + { + name: "IPv6 mode, ISTIO_DUAL_STACK disabled, ambient enabled, IPv4 and IPv6 addresses, expected to return all addresses", + service: &Service{ + DefaultAddress: "10.0.0.0/28", + ClusterVIPs: AddressMap{ + Addresses: map[cluster.ID][]string{ + "id": {"10.0.0.0/28", "10.0.0.16/28", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + }, + }, + ipMode: IPv6, + ambientEnabled: true, + expectedAddresses: []string{"10.0.0.0/28", "10.0.0.16/28", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + expectedExtraAddresses: []string{"10.0.0.16/28", "::ffff:10.0.0.32", "::ffff:10.0.0.48"}, + }, + { + name: "IPv4 mode, auto-allocation enabled, expected auto-allocated address", + service: &Service{ + DefaultAddress: "0.0.0.0", + AutoAllocatedIPv4Address: "240.240.0.1", + }, + ipMode: IPv4, + autoAllocationEnabled: true, + expectedAddresses: []string{"240.240.0.1"}, + expectedExtraAddresses: []string{}, + }, + { + name: "IPv6 mode, auto-allocation enabled, expected auto-allocated address", + service: &Service{ + DefaultAddress: "0.0.0.0", + AutoAllocatedIPv6Address: "2001:2::f0f0:e351", + }, + ipMode: IPv6, + autoAllocationEnabled: true, + expectedAddresses: []string{"2001:2::f0f0:e351"}, + expectedExtraAddresses: []string{}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if tc.dualStackEnabled { + test.SetForTest(t, &features.EnableDualStack, true) + } + if tc.ambientEnabled { + test.SetForTest(t, &features.EnableAmbient, true) + } + proxy := &Proxy{Metadata: &NodeMetadata{ClusterID: "id"}, ipMode: tc.ipMode} + if tc.autoAllocationEnabled { + proxy.Metadata.DNSCapture = true + proxy.Metadata.DNSAutoAllocate = true + } + addresses := tc.service.GetAllAddressesForProxy(proxy) + assert.Equal(t, addresses, tc.expectedAddresses) + extraAddresses := tc.service.GetExtraAddressesForProxy(proxy) + assert.Equal(t, extraAddresses, tc.expectedExtraAddresses) + }) + } +} diff --git a/pilot/pkg/networking/core/listener.go b/pilot/pkg/networking/core/listener.go index cdd71bdb6852..f0fdc4307097 100644 --- a/pilot/pkg/networking/core/listener.go +++ b/pilot/pkg/networking/core/listener.go @@ -264,10 +264,10 @@ func (l listenerBinding) Primary() string { // Extra returns any additional bindings. This is always empty if dual stack is disabled func (l listenerBinding) Extra() []string { - if !features.EnableDualStack || len(l.binds) == 1 { - return nil + if len(l.binds) > 1 { + return l.binds[1:] } - return l.binds[1:] + return nil } type outboundListenerEntry struct { @@ -820,9 +820,8 @@ func (lb *ListenerBuilder) buildSidecarOutboundListener(listenerOpts outboundLis } else { // Address is a CIDR. Fall back to 0.0.0.0 and // filter chain match - // TODO: this probably needs to handle dual stack better listenerOpts.bind.binds = actualWildcards - listenerOpts.cidr = svcListenAddress + listenerOpts.cidr = append([]string{svcListenAddress}, svcExtraListenAddresses...) } } } @@ -1061,7 +1060,7 @@ type outboundListenerOpts struct { proxy *model.Proxy bind listenerBinding - cidr string + cidr []string port *model.Port service *model.Service diff --git a/pilot/pkg/networking/core/tls.go b/pilot/pkg/networking/core/tls.go index 501ded47bc38..c1b44bcafaf1 100644 --- a/pilot/pkg/networking/core/tls.go +++ b/pilot/pkg/networking/core/tls.go @@ -100,7 +100,7 @@ func hashRuntimeTLSMatchPredicates(match *v1alpha3.TLSMatchAttributes) string { return strings.Join(match.SniHosts, ",") + "|" + strings.Join(match.DestinationSubnets, ",") } -func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushContext, destinationCIDR string, +func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushContext, destinationCIDRs []string, service *model.Service, bind string, listenPort *model.Port, gateways sets.String, configs []config.Config, ) []*filterChainOpts { @@ -142,11 +142,7 @@ func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushC // Use the service's CIDRs. // But if a virtual service overrides it with its own destination subnet match // give preference to the user provided one - // destinationCIDR will be empty for services with VIPs - var destinationCIDRs []string - if destinationCIDR != "" { - destinationCIDRs = []string{destinationCIDR} - } + // destinationCIDRs will be empty for services with VIPs // Only set CIDR match if the listener is bound to an IP. // If its bound to a unix domain socket, then ignore the CIDR matches // Unix domain socket bound ports have Port value set to 0 @@ -209,7 +205,7 @@ func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushC svcListenAddress = constants.UnspecifiedIPv6 } - if len(destinationCIDR) > 0 || len(svcListenAddress) == 0 || (svcListenAddress == actualWildcard && bind == actualWildcard) { + if len(destinationCIDRs) > 0 || len(svcListenAddress) == 0 || (svcListenAddress == actualWildcard && bind == actualWildcard) { sniHosts = []string{string(service.Hostname)} for _, a := range service.Attributes.Aliases { alt := GenerateAltVirtualHosts(a.Hostname.String(), 0, node.DNSDomain) @@ -219,10 +215,6 @@ func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushC } destinationRule := CastDestinationRule(node.SidecarScope.DestinationRule( model.TrafficDirectionOutbound, node, service.Hostname).GetRule()) - var destinationCIDRs []string - if destinationCIDR != "" { - destinationCIDRs = []string{destinationCIDR} - } out = append(out, &filterChainOpts{ sniHosts: sniHosts, destinationCIDRs: destinationCIDRs, @@ -234,7 +226,7 @@ func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushC return out } -func buildSidecarOutboundTCPFilterChainOpts(node *model.Proxy, push *model.PushContext, destinationCIDR string, +func buildSidecarOutboundTCPFilterChainOpts(node *model.Proxy, push *model.PushContext, destinationCIDRs []string, service *model.Service, listenPort *model.Port, gateways sets.String, configs []config.Config, ) []*filterChainOpts { @@ -253,10 +245,6 @@ TcpLoop: for _, cfg := range configs { virtualService := cfg.Spec.(*v1alpha3.VirtualService) for _, tcp := range virtualService.Tcp { - var destinationCIDRs []string - if destinationCIDR != "" { - destinationCIDRs = []string{destinationCIDR} - } if len(tcp.Match) == 0 { // implicit match out = append(out, &filterChainOpts{ @@ -333,10 +321,6 @@ TcpLoop: if len(push.Mesh.OutboundClusterStatName) != 0 { statPrefix = telemetry.BuildStatPrefix(push.Mesh.OutboundClusterStatName, string(service.Hostname), "", &model.Port{Port: port}, 0, &service.Attributes) } - var destinationCIDRs []string - if destinationCIDR != "" { - destinationCIDRs = []string{destinationCIDR} - } out = append(out, &filterChainOpts{ destinationCIDRs: destinationCIDRs, networkFilters: lb.buildOutboundNetworkFiltersWithSingleDestination(statPrefix, clusterName, "", diff --git a/pilot/pkg/serviceregistry/serviceentry/controller.go b/pilot/pkg/serviceregistry/serviceentry/controller.go index 932d0fbb071f..364a2aa13585 100644 --- a/pilot/pkg/serviceregistry/serviceentry/controller.go +++ b/pilot/pkg/serviceregistry/serviceentry/controller.go @@ -237,9 +237,7 @@ func (s *Controller) workloadEntryHandler(old, curr config.Config, event model.E s.NotifyWorkloadInstanceHandlers(wi, event) } - // includes instances new updated or unchanged, in other word it is the current state. - instancesUpdated := []*model.ServiceInstance{} - instancesDeleted := []*model.ServiceInstance{} + allInstances := []*model.ServiceInstance{} fullPush := false configsUpdated := sets.New[model.ConfigKey]() @@ -278,16 +276,16 @@ func (s *Controller) workloadEntryHandler(old, curr config.Config, event model.E continue } instance := s.convertWorkloadEntryToServiceInstances(wle, services, se, &key, s.Cluster()) - instancesUpdated = append(instancesUpdated, instance...) + allInstances = append(allInstances, instance...) parentKey := configKeyWithParent{ configKey: key, parent: namespacedName, } if event == model.EventDelete { s.serviceInstances.deleteServiceEntryInstances(namespacedName, key) - s.serviceInstances.deleteInstanceKeys(parentKey, instancesUpdated) + s.serviceInstances.deleteInstanceKeys(parentKey, instance) } else { - s.serviceInstances.updateInstances(parentKey, instancesUpdated) + s.serviceInstances.updateInstances(parentKey, instance) s.serviceInstances.updateServiceEntryInstancesPerConfig(namespacedName, key, instance) } addConfigs(se, services) @@ -307,7 +305,7 @@ func (s *Controller) workloadEntryHandler(old, curr config.Config, event model.E configKey: key, parent: namespacedName, } - instancesDeleted = append(instancesDeleted, instance...) + allInstances = append(allInstances, instance...) s.serviceInstances.deleteServiceEntryInstances(namespacedName, key) s.serviceInstances.deleteInstanceKeys(parentKey, instance) addConfigs(se, services) @@ -320,7 +318,6 @@ func (s *Controller) workloadEntryHandler(old, curr config.Config, event model.E } s.mutex.Unlock() - allInstances := append(instancesUpdated, instancesDeleted...) if !fullPush { // trigger full xds push to the related sidecar proxy if event == model.EventAdd { @@ -365,7 +362,7 @@ func getUpdatedConfigs(services []*model.Service) sets.Set[model.ConfigKey] { func (s *Controller) serviceEntryHandler(old, curr config.Config, event model.Event) { log.Debugf("Handle event %s for service entry %s/%s", event, curr.Namespace, curr.Name) currentServiceEntry := curr.Spec.(*networking.ServiceEntry) - cs := convertServices(curr) + cs := convertServices(curr, s.clusterID) configsUpdated := sets.New[model.ConfigKey]() key := curr.NamespacedName() @@ -748,14 +745,14 @@ func (s *Controller) queueEdsEvent(keys sets.Set[instancesKey], edsFn func(keys func (s *Controller) doEdsCacheUpdate(keys sets.Set[instancesKey]) { endpoints := s.buildEndpoints(keys) shard := model.ShardKeyFromRegistry(s) - // This is delete. - if len(endpoints) == 0 { - for k := range keys { - s.XdsUpdater.EDSCacheUpdate(shard, string(k.hostname), k.namespace, nil) - } - } else { - for k, eps := range endpoints { + + for k := range keys { + if eps, ok := endpoints[k]; ok { + // Update the cache with the generated endpoints. s.XdsUpdater.EDSCacheUpdate(shard, string(k.hostname), k.namespace, eps) + } else { + // Handle deletions by sending a nil endpoints update. + s.XdsUpdater.EDSCacheUpdate(shard, string(k.hostname), k.namespace, nil) } } } @@ -764,14 +761,14 @@ func (s *Controller) doEdsCacheUpdate(keys sets.Set[instancesKey]) { func (s *Controller) doEdsUpdate(keys sets.Set[instancesKey]) { endpoints := s.buildEndpoints(keys) shard := model.ShardKeyFromRegistry(s) - // This is delete. - if len(endpoints) == 0 { - for k := range keys { - s.XdsUpdater.EDSUpdate(shard, string(k.hostname), k.namespace, nil) - } - } else { - for k, eps := range endpoints { + + for k := range keys { + if eps, ok := endpoints[k]; ok { + // Update with the generated endpoints. s.XdsUpdater.EDSUpdate(shard, string(k.hostname), k.namespace, eps) + } else { + // Handle deletions by sending a nil endpoints update. + s.XdsUpdater.EDSUpdate(shard, string(k.hostname), k.namespace, nil) } } } diff --git a/pilot/pkg/serviceregistry/serviceentry/controller_test.go b/pilot/pkg/serviceregistry/serviceentry/controller_test.go index 701f7dc41d3d..8cd125d30cf3 100644 --- a/pilot/pkg/serviceregistry/serviceentry/controller_test.go +++ b/pilot/pkg/serviceregistry/serviceentry/controller_test.go @@ -129,9 +129,10 @@ func initServiceDiscoveryWithOpts(t test.Failer, workloadOnly bool, opts ...Opti func TestServiceDiscoveryServices(t *testing.T) { store, sd, fx := initServiceDiscovery(t) expectedServices := []*model.Service{ - makeService("*.istio.io", "httpDNSRR", constants.UnspecifiedIP, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSRoundRobinLB), - makeService("*.google.com", "httpDNS", constants.UnspecifiedIP, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB), - makeService("tcpstatic.com", "tcpStatic", "172.217.0.1", map[string]int{"tcp-444": 444}, true, model.ClientSideLB), + makeService( + "*.istio.io", "httpDNSRR", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSRoundRobinLB), + makeService("*.google.com", "httpDNS", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB), + makeService("tcpstatic.com", "tcpStatic", []string{"172.217.0.1"}, map[string]int{"tcp-444": 444}, true, model.ClientSideLB), } createConfigs([]*config.Config{httpDNS, httpDNSRR, tcpStatic}, store, t) @@ -554,6 +555,7 @@ func TestServiceDiscoveryServiceUpdate(t *testing.T) { Event{Type: "service", ID: "selector1.com", Namespace: httpStaticOverlay.Namespace}, Event{Type: "service", ID: "*.google.com", Namespace: httpStaticOverlay.Namespace}, Event{Type: "eds cache", ID: "*.google.com", Namespace: httpStaticOverlay.Namespace}, + Event{Type: "eds cache", ID: "selector1.com", Namespace: httpStaticOverlay.Namespace}, Event{Type: "xds full", ID: "*.google.com,selector1.com"}) // service added selector1Updated := func() *config.Config { @@ -569,6 +571,7 @@ func TestServiceDiscoveryServiceUpdate(t *testing.T) { Event{Type: "service", ID: "*.google.com", Namespace: httpStaticOverlay.Namespace}, Event{Type: "service", ID: "selector1.com", Namespace: httpStaticOverlay.Namespace}, Event{Type: "eds cache", ID: "*.google.com", Namespace: httpStaticOverlay.Namespace}, + Event{Type: "eds cache", ID: "selector1.com", Namespace: httpStaticOverlay.Namespace}, Event{Type: "xds full", ID: "*.google.com,selector1.com"}) // service updated }) } @@ -1490,7 +1493,7 @@ func expectEvents(t testing.TB, ch *xdsfake.Updater, events ...Event) { func expectServiceInstances(t testing.TB, sd *Controller, cfg *config.Config, port int, expected ...[]*model.ServiceInstance) { t.Helper() - svcs := convertServices(*cfg) + svcs := convertServices(*cfg, "") if len(svcs) != len(expected) { t.Fatalf("got more services than expected: %v vs %v", len(svcs), len(expected)) } @@ -1724,8 +1727,8 @@ func TestServicesDiff(t *testing.T) { for _, tt := range cases { t.Run(tt.name, func(t *testing.T) { - as := convertServices(*tt.current) - bs := convertServices(*tt.new) + as := convertServices(*tt.current, "") + bs := convertServices(*tt.new, "") added, deleted, updated, unchanged := servicesDiff(as, bs) for i, item := range []struct { hostnames []host.Name diff --git a/pilot/pkg/serviceregistry/serviceentry/conversion.go b/pilot/pkg/serviceregistry/serviceentry/conversion.go index 43793940455c..5fa2e71fe007 100644 --- a/pilot/pkg/serviceregistry/serviceentry/conversion.go +++ b/pilot/pkg/serviceregistry/serviceentry/conversion.go @@ -17,7 +17,6 @@ package serviceentry import ( "net/netip" "strings" - "time" "istio.io/api/label" networking "istio.io/api/networking/v1alpha3" @@ -34,6 +33,7 @@ import ( "istio.io/istio/pkg/config/visibility" "istio.io/istio/pkg/kube/labels" "istio.io/istio/pkg/network" + "istio.io/istio/pkg/slices" "istio.io/istio/pkg/spiffe" netutil "istio.io/istio/pkg/util/net" "istio.io/istio/pkg/util/sets" @@ -48,8 +48,8 @@ func convertPort(port *networking.ServicePort) *model.Port { } type HostAddress struct { - host string - address string + host string + addresses []string } // ServiceToServiceEntry converts from internal Service representation to ServiceEntry @@ -139,7 +139,7 @@ func ServiceToServiceEntry(svc *model.Service, proxy *model.Proxy) *config.Confi } // convertServices transforms a ServiceEntry config to a list of internal Service objects. -func convertServices(cfg config.Config) []*model.Service { +func convertServices(cfg config.Config, clusterID cluster.ID) []*model.Service { serviceEntry := cfg.Spec.(*networking.ServiceEntry) creationTime := cfg.CreationTimestamp @@ -182,56 +182,58 @@ func convertServices(cfg config.Config) []*model.Service { hostAddresses := []*HostAddress{} for _, hostname := range serviceEntry.Hosts { if len(serviceEntry.Addresses) > 0 { + ha := &HostAddress{hostname, []string{}} for _, address := range serviceEntry.Addresses { - // Check if address is an IP first because that is the most common case. + // Check if addresses is an IP first because that is the most common case. if netutil.IsValidIPAddress(address) { - hostAddresses = append(hostAddresses, &HostAddress{hostname, address}) + ha.addresses = append(ha.addresses, address) } else if cidr, cidrErr := netip.ParsePrefix(address); cidrErr == nil { newAddress := address if cidr.Bits() == cidr.Addr().BitLen() { - // /32 mask. Remove the /32 and make it a normal IP address + // /32 mask. Remove the /32 and make it a normal IP addresses newAddress = cidr.Addr().String() } - hostAddresses = append(hostAddresses, &HostAddress{hostname, newAddress}) + ha.addresses = append(ha.addresses, newAddress) } } + hostAddresses = append(hostAddresses, ha) } else { - hostAddresses = append(hostAddresses, &HostAddress{hostname, constants.UnspecifiedIP}) + hostAddresses = append(hostAddresses, &HostAddress{hostname, []string{constants.UnspecifiedIP}}) } } - return buildServices(hostAddresses, cfg.Name, cfg.Namespace, svcPorts, serviceEntry.Location, resolution, - exportTo, labelSelectors, serviceEntry.SubjectAltNames, creationTime, cfg.Labels, portOverrides) -} - -func buildServices(hostAddresses []*HostAddress, name, namespace string, ports model.PortList, location networking.ServiceEntry_Location, - resolution model.Resolution, exportTo sets.Set[visibility.Instance], selectors map[string]string, saccounts []string, - ctime time.Time, labels map[string]string, overrides map[uint32]uint32, -) []*model.Service { out := make([]*model.Service, 0, len(hostAddresses)) - lbls := labels - if features.CanonicalServiceForMeshExternalServiceEntry && location == networking.ServiceEntry_MESH_EXTERNAL { - lbls = ensureCanonicalServiceLabels(name, labels) + lbls := cfg.Labels + if features.CanonicalServiceForMeshExternalServiceEntry && serviceEntry.Location == networking.ServiceEntry_MESH_EXTERNAL { + lbls = ensureCanonicalServiceLabels(cfg.Name, cfg.Labels) } for _, ha := range hostAddresses { - out = append(out, &model.Service{ - CreationTime: ctime, - MeshExternal: location == networking.ServiceEntry_MESH_EXTERNAL, + svc := &model.Service{ + CreationTime: creationTime, + MeshExternal: serviceEntry.Location == networking.ServiceEntry_MESH_EXTERNAL, Hostname: host.Name(ha.host), - DefaultAddress: ha.address, - Ports: ports, + DefaultAddress: ha.addresses[0], + Ports: svcPorts, Resolution: resolution, Attributes: model.ServiceAttributes{ ServiceRegistry: provider.External, - PassthroughTargetPorts: overrides, + PassthroughTargetPorts: portOverrides, Name: ha.host, - Namespace: namespace, + Namespace: cfg.Namespace, Labels: lbls, ExportTo: exportTo, - LabelSelectors: selectors, + LabelSelectors: labelSelectors, }, - ServiceAccounts: saccounts, - }) + ServiceAccounts: serviceEntry.SubjectAltNames, + } + if !slices.Equal(ha.addresses, []string{constants.UnspecifiedIP}) { + svc.ClusterVIPs = model.AddressMap{ + Addresses: map[cluster.ID][]string{ + clusterID: ha.addresses, + }, + } + } + out = append(out, svc) } return out } @@ -326,7 +328,7 @@ func (s *Controller) convertServiceEntryToInstances(cfg config.Config, services return nil } if services == nil { - services = convertServices(cfg) + services = convertServices(cfg, s.clusterID) } for _, service := range services { for _, serviceEntryPort := range serviceEntry.Ports { @@ -422,7 +424,7 @@ func (s *Controller) convertWorkloadEntryToWorkloadInstance(cfg config.Config, c dnsServiceEntryOnly = true } if addr != "" && !netutil.IsValidIPAddress(addr) { - // k8s can't use workloads with hostnames in the address field. + // k8s can't use workloads with hostnames in the addresses field. dnsServiceEntryOnly = true } tlsMode := getTLSModeFromWorkloadEntry(we) diff --git a/pilot/pkg/serviceregistry/serviceentry/conversion_test.go b/pilot/pkg/serviceregistry/serviceentry/conversion_test.go index 317562a1c882..d7b8b75590ec 100644 --- a/pilot/pkg/serviceregistry/serviceentry/conversion_test.go +++ b/pilot/pkg/serviceregistry/serviceentry/conversion_test.go @@ -35,6 +35,7 @@ import ( "istio.io/istio/pkg/config/protocol" "istio.io/istio/pkg/config/schema/gvk" "istio.io/istio/pkg/network" + "istio.io/istio/pkg/slices" "istio.io/istio/pkg/spiffe" "istio.io/istio/pkg/test" ) @@ -502,13 +503,13 @@ func convertPortNameToProtocol(name string) protocol.Instance { return protocol.Parse(prefix) } -func makeService(hostname host.Name, configNamespace, address string, ports map[string]int, +func makeService(hostname host.Name, configNamespace string, addresses []string, ports map[string]int, external bool, resolution model.Resolution, serviceAccounts ...string, ) *model.Service { svc := &model.Service{ CreationTime: GlobalTime, Hostname: hostname, - DefaultAddress: address, + DefaultAddress: addresses[0], MeshExternal: external, Resolution: resolution, ServiceAccounts: serviceAccounts, @@ -518,6 +519,11 @@ func makeService(hostname host.Name, configNamespace, address string, ports map[ Namespace: configNamespace, }, } + if !slices.Equal(addresses, []string{constants.UnspecifiedIP}) { + svc.ClusterVIPs = model.AddressMap{ + Addresses: map[cluster.ID][]string{"": addresses}, + } + } if external && features.CanonicalServiceForMeshExternalServiceEntry { if svc.Attributes.Labels == nil { @@ -565,7 +571,7 @@ func makeInstanceWithServiceAccount(cfg *config.Config, address string, port int func makeTarget(cfg *config.Config, address string, port int, svcPort *networking.ServicePort, svcLabels map[string]string, mtlsMode MTLSMode, ) model.ServiceTarget { - services := convertServices(*cfg) + services := convertServices(*cfg, "") svc := services[0] // default for _, s := range services { if string(s.Hostname) == address { @@ -596,7 +602,7 @@ func makeTarget(cfg *config.Config, address string, port int, func makeInstance(cfg *config.Config, address string, port int, svcPort *networking.ServicePort, svcLabels map[string]string, mtlsMode MTLSMode, ) *model.ServiceInstance { - services := convertServices(*cfg) + services := convertServices(*cfg, "") svc := services[0] // default for _, s := range services { if string(s.Hostname) == address { @@ -649,7 +655,7 @@ func testConvertServiceBody(t *testing.T) { // service entry http externalSvc: httpNone, services: []*model.Service{ - makeService("*.google.com", "httpNone", constants.UnspecifiedIP, + makeService("*.google.com", "httpNone", []string{constants.UnspecifiedIP}, map[string]int{"http-number": 80, "http2-number": 8080}, true, model.Passthrough), }, }, @@ -657,7 +663,7 @@ func testConvertServiceBody(t *testing.T) { // service entry tcp externalSvc: tcpNone, services: []*model.Service{ - makeService("tcpnone.com", "tcpNone", "172.217.0.0/16", + makeService("tcpnone.com", "tcpNone", []string{"172.217.0.0/16"}, map[string]int{"tcp-444": 444}, true, model.Passthrough), }, }, @@ -665,7 +671,7 @@ func testConvertServiceBody(t *testing.T) { // service entry http static externalSvc: httpStatic, services: []*model.Service{ - makeService("*.google.com", "httpStatic", constants.UnspecifiedIP, + makeService("*.google.com", "httpStatic", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.ClientSideLB), }, }, @@ -673,9 +679,9 @@ func testConvertServiceBody(t *testing.T) { // service entry DNS with no endpoints externalSvc: httpDNSnoEndpoints, services: []*model.Service{ - makeService("google.com", "httpDNSnoEndpoints", constants.UnspecifiedIP, + makeService("google.com", "httpDNSnoEndpoints", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB, "google.com"), - makeService("www.wikipedia.org", "httpDNSnoEndpoints", constants.UnspecifiedIP, + makeService("www.wikipedia.org", "httpDNSnoEndpoints", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB, "google.com"), }, }, @@ -683,7 +689,7 @@ func testConvertServiceBody(t *testing.T) { // service entry dns externalSvc: httpDNS, services: []*model.Service{ - makeService("*.google.com", "httpDNS", constants.UnspecifiedIP, + makeService("*.google.com", "httpDNS", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB), }, }, @@ -691,7 +697,7 @@ func testConvertServiceBody(t *testing.T) { // service entry dns with target port externalSvc: dnsTargetPort, services: []*model.Service{ - makeService("google.com", "dnsTargetPort", constants.UnspecifiedIP, + makeService("google.com", "dnsTargetPort", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80}, true, model.DNSLB), }, }, @@ -699,7 +705,7 @@ func testConvertServiceBody(t *testing.T) { // service entry tcp DNS externalSvc: tcpDNS, services: []*model.Service{ - makeService("tcpdns.com", "tcpDNS", constants.UnspecifiedIP, + makeService("tcpdns.com", "tcpDNS", []string{constants.UnspecifiedIP}, map[string]int{"tcp-444": 444}, true, model.DNSLB), }, }, @@ -707,7 +713,7 @@ func testConvertServiceBody(t *testing.T) { // service entry tcp static externalSvc: tcpStatic, services: []*model.Service{ - makeService("tcpstatic.com", "tcpStatic", "172.217.0.1", + makeService("tcpstatic.com", "tcpStatic", []string{"172.217.0.1"}, map[string]int{"tcp-444": 444}, true, model.ClientSideLB), }, }, @@ -715,7 +721,7 @@ func testConvertServiceBody(t *testing.T) { // service entry http internal externalSvc: httpNoneInternal, services: []*model.Service{ - makeService("*.google.com", "httpNoneInternal", constants.UnspecifiedIP, + makeService("*.google.com", "httpNoneInternal", []string{constants.UnspecifiedIP}, map[string]int{"http-number": 80, "http2-number": 8080}, false, model.Passthrough), }, }, @@ -723,7 +729,7 @@ func testConvertServiceBody(t *testing.T) { // service entry tcp internal externalSvc: tcpNoneInternal, services: []*model.Service{ - makeService("tcpinternal.com", "tcpNoneInternal", "172.217.0.0/16", + makeService("tcpinternal.com", "tcpNoneInternal", []string{"172.217.0.0/16"}, map[string]int{"tcp-444": 444}, false, model.Passthrough), }, }, @@ -731,19 +737,15 @@ func testConvertServiceBody(t *testing.T) { // service entry multiAddrInternal externalSvc: multiAddrInternal, services: []*model.Service{ - makeService("tcp1.com", "multiAddrInternal", "1.1.1.0/16", - map[string]int{"tcp-444": 444}, false, model.Passthrough), - makeService("tcp1.com", "multiAddrInternal", "2.2.2.0/16", - map[string]int{"tcp-444": 444}, false, model.Passthrough), - makeService("tcp2.com", "multiAddrInternal", "1.1.1.0/16", + makeService("tcp1.com", "multiAddrInternal", []string{"1.1.1.0/16", "2.2.2.0/16"}, map[string]int{"tcp-444": 444}, false, model.Passthrough), - makeService("tcp2.com", "multiAddrInternal", "2.2.2.0/16", + makeService("tcp2.com", "multiAddrInternal", []string{"1.1.1.0/16", "2.2.2.0/16"}, map[string]int{"tcp-444": 444}, false, model.Passthrough), }, }, } - selectorSvc := makeService("selector.com", "selector", "0.0.0.0", + selectorSvc := makeService("selector.com", "selector", []string{constants.UnspecifiedIP}, map[string]int{"tcp-444": 444, "http-445": 445}, true, model.ClientSideLB) selectorSvc.Attributes.LabelSelectors = map[string]string{"app": "wle"} @@ -756,7 +758,7 @@ func testConvertServiceBody(t *testing.T) { }) for _, tt := range serviceTests { - services := convertServices(*tt.externalSvc) + services := convertServices(*tt.externalSvc, "") if err := compare(t, services, tt.services); err != nil { t.Errorf("testcase: %v\n%v ", tt.externalSvc.Name, err) } @@ -943,7 +945,7 @@ func TestConvertWorkloadEntryToServiceInstances(t *testing.T) { for _, tt := range serviceInstanceTests { t.Run(tt.name, func(t *testing.T) { - services := convertServices(*tt.se) + services := convertServices(*tt.se, "") s := &Controller{} instances := s.convertWorkloadEntryToServiceInstances(tt.wle, services, tt.se.Spec.(*networking.ServiceEntry), &configKey{}, tt.clusterID) sortServiceInstances(instances) diff --git a/pilot/pkg/serviceregistry/serviceentry/store_test.go b/pilot/pkg/serviceregistry/serviceentry/store_test.go index 850242ddf120..1ab17b833c48 100644 --- a/pilot/pkg/serviceregistry/serviceentry/store_test.go +++ b/pilot/pkg/serviceregistry/serviceentry/store_test.go @@ -114,8 +114,9 @@ func TestServiceStore(t *testing.T) { } expectedServices := []*model.Service{ - makeService("*.istio.io", "httpDNSRR", constants.UnspecifiedIP, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSRoundRobinLB), - makeService("*.istio.io", "httpDNSRR", constants.UnspecifiedIP, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB), + makeService( + "*.istio.io", "httpDNSRR", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSRoundRobinLB), + makeService("*.istio.io", "httpDNSRR", []string{constants.UnspecifiedIP}, map[string]int{"http-port": 80, "http-alt-port": 8080}, true, model.DNSLB), } store.updateServices(httpDNSRR.NamespacedName(), expectedServices) diff --git a/pilot/pkg/serviceregistry/serviceregistry_test.go b/pilot/pkg/serviceregistry/serviceregistry_test.go index f5013cc19b01..750345639310 100644 --- a/pilot/pkg/serviceregistry/serviceregistry_test.go +++ b/pilot/pkg/serviceregistry/serviceregistry_test.go @@ -519,6 +519,203 @@ func TestWorkloadInstances(t *testing.T) { expectServiceEndpoints(t, fx, expectedSvc, 80, instances) }) + t.Run("External only: workloadEntry with overlapping IPs", func(t *testing.T) { + store, _, fx := setupTest(t) + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "service-entry", + Namespace: namespace, + GroupVersionKind: gvk.ServiceEntry, + }, + Spec: &networking.ServiceEntry{ + Hosts: []string{"example.com"}, + Ports: []*networking.ServicePort{ + {Number: 445, Name: "http-445", Protocol: "http"}, + }, + WorkloadSelector: &networking.WorkloadSelector{ + Labels: labels, + }, + }, + }) + expectedSvc := &model.Service{ + Hostname: "example.com", + Ports: []*model.Port{{ + Name: "http-445", + Port: 445, + Protocol: "http", + }}, + Attributes: model.ServiceAttributes{ + Namespace: namespace, + Name: "service", + LabelSelectors: labels, + }, + } + we1 := config.Config{ + Meta: config.Meta{ + Name: "we1", + Namespace: namespace, + GroupVersionKind: gvk.WorkloadEntry, + Domain: "cluster.local", + }, + Spec: &networking.WorkloadEntry{ + Address: "2.3.4.5", + Labels: labels, + Ports: map[string]uint32{ + "http-445": 1234, + }, + }, + } + we2 := config.Config{ + Meta: config.Meta{ + Name: "we2", + Namespace: namespace, + GroupVersionKind: gvk.WorkloadEntry, + Domain: "cluster.local", + }, + Spec: &networking.WorkloadEntry{ + Address: "2.3.4.5", + Labels: labels, + Ports: map[string]uint32{ + "http-445": 5678, + }, + }, + } + makeIstioObject(t, store, we1) + makeIstioObject(t, store, we2) + fx.WaitOrFail(t, "xds full") + + instances := []EndpointResponse{{ + Address: workloadEntry.Spec.(*networking.WorkloadEntry).Address, + Port: 1234, + }, { + Address: workloadEntry.Spec.(*networking.WorkloadEntry).Address, + Port: 5678, + }} + expectServiceEndpoints(t, fx, expectedSvc, 445, instances) + + fx.Clear() + + // Delete one of the WE + _ = store.Delete(gvk.WorkloadEntry, we2.Name, we2.Namespace, nil) + + fx.WaitOrFail(t, "xds") + instances = []EndpointResponse{ + { + Address: workloadEntry.Spec.(*networking.WorkloadEntry).Address, + Port: 1234, + }, + } + expectServiceEndpoints(t, fx, expectedSvc, 445, instances) + }) + + t.Run("External only: workloadEntry with overlapping IPs and multiple SE", func(t *testing.T) { + store, _, fx := setupTest(t) + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "service-entry1", + Namespace: namespace, + GroupVersionKind: gvk.ServiceEntry, + }, + Spec: &networking.ServiceEntry{ + Hosts: []string{"1.example.com"}, + Ports: []*networking.ServicePort{ + {Number: 445, Name: "http-445", Protocol: "http"}, + }, + WorkloadSelector: &networking.WorkloadSelector{ + Labels: labels, + }, + }, + }) + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "service-entry2", + Namespace: namespace, + GroupVersionKind: gvk.ServiceEntry, + }, + Spec: &networking.ServiceEntry{ + Hosts: []string{"2.example.com"}, + Ports: []*networking.ServicePort{ + {Number: 445, Name: "http-445", Protocol: "http"}, + }, + WorkloadSelector: &networking.WorkloadSelector{ + Labels: labels, + }, + }, + }) + expectedSvc := &model.Service{ + Hostname: "1.example.com", + Ports: []*model.Port{{ + Name: "http-445", + Port: 445, + Protocol: "http", + }}, + Attributes: model.ServiceAttributes{ + Namespace: namespace, + Name: "service", + LabelSelectors: labels, + }, + } + we1 := config.Config{ + Meta: config.Meta{ + Name: "we1", + Namespace: namespace, + GroupVersionKind: gvk.WorkloadEntry, + Domain: "cluster.local", + }, + Spec: &networking.WorkloadEntry{ + Address: "2.3.4.5", + Labels: labels, + Ports: map[string]uint32{ + "http-445": 1234, + }, + }, + } + we2 := config.Config{ + Meta: config.Meta{ + Name: "we2", + Namespace: namespace, + GroupVersionKind: gvk.WorkloadEntry, + Domain: "cluster.local", + }, + Spec: &networking.WorkloadEntry{ + Address: "2.3.4.5", + Labels: labels, + Ports: map[string]uint32{ + "http-445": 5678, + }, + }, + } + makeIstioObject(t, store, we1) + makeIstioObject(t, store, we2) + fx.WaitOrFail(t, "xds full") + + instances := []EndpointResponse{{ + Address: workloadEntry.Spec.(*networking.WorkloadEntry).Address, + Port: 1234, + }, { + Address: workloadEntry.Spec.(*networking.WorkloadEntry).Address, + Port: 5678, + }} + expectServiceEndpoints(t, fx, expectedSvc, 445, instances) + + fx.Clear() + + // Delete one of the WE + _ = store.Delete(gvk.WorkloadEntry, we2.Name, we2.Namespace, nil) + + fx.WaitOrFail(t, "xds") + newInstances := []EndpointResponse{ + { + Address: workloadEntry.Spec.(*networking.WorkloadEntry).Address, + Port: 1234, + }, + } + expectServiceEndpoints(t, fx, expectedSvc, 445, newInstances) + + makeIstioObject(t, store, we2) + expectServiceEndpoints(t, fx, expectedSvc, 445, instances) + }) + t.Run("Service selects WorkloadEntry", func(t *testing.T) { store, kube, fx := setupTest(t) makeService(t, kube, service) @@ -815,6 +1012,119 @@ func TestWorkloadInstances(t *testing.T) { expectServiceEndpoints(t, fx, expectedSvc, 80, instances) }) + t.Run("ServiceEntry selects WorkloadEntry", func(t *testing.T) { + store, _, fx := setupTest(t) + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "service-entry-1", + Namespace: namespace, + GroupVersionKind: gvk.ServiceEntry, + Domain: "cluster.local", + }, + Spec: &networking.ServiceEntry{ + Hosts: []string{"service-1.namespace.svc.cluster.local"}, + Ports: []*networking.ServicePort{port}, + WorkloadSelector: &networking.WorkloadSelector{ + Labels: map[string]string{ + "app.foo": "true", + }, + }, + Resolution: networking.ServiceEntry_STATIC, + }, + }) + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "service-entry-2", + Namespace: namespace, + GroupVersionKind: gvk.ServiceEntry, + Domain: "cluster.local", + }, + Spec: &networking.ServiceEntry{ + Hosts: []string{"service-2.namespace.svc.cluster.local"}, + Ports: []*networking.ServicePort{port}, + WorkloadSelector: &networking.WorkloadSelector{ + Labels: map[string]string{ + "app.bar": "true", + }, + }, + Resolution: networking.ServiceEntry_STATIC, + }, + }) + // Both service entries share a common workload entry + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "workloadentry", + Namespace: namespace, + GroupVersionKind: gvk.WorkloadEntry, + Domain: "cluster.local", + }, + Spec: &networking.WorkloadEntry{ + Address: "2.3.4.5", + Labels: map[string]string{ + "app.foo": "true", + "app.bar": "true", + }, + }, + }) + + fx.WaitOrFail(t, "xds full") + instances := []EndpointResponse{{ + Address: "2.3.4.5", + Port: 80, + }} + expectedSvc1 := &model.Service{ + Hostname: "service-1.namespace.svc.cluster.local", + Ports: []*model.Port{{ + Name: "http", + Port: 80, + Protocol: "http", + }}, + Attributes: model.ServiceAttributes{ + Namespace: namespace, + Name: "service-entry-1", + LabelSelectors: map[string]string{ + "app.foo": "true", + }, + }, + } + expectedSvc2 := &model.Service{ + Hostname: "service-2.namespace.svc.cluster.local", + Ports: []*model.Port{{ + Name: "http", + Port: 80, + Protocol: "http", + }}, + Attributes: model.ServiceAttributes{ + Namespace: namespace, + Name: "service-entry-2", + LabelSelectors: map[string]string{ + "app.bar": "true", + }, + }, + } + expectServiceEndpoints(t, fx, expectedSvc1, 80, instances) + expectServiceEndpoints(t, fx, expectedSvc2, 80, instances) + fx.Clear() + // Delete the `app.foo` label to unselect service entry + makeIstioObject(t, store, config.Config{ + Meta: config.Meta{ + Name: "workloadentry", + Namespace: namespace, + GroupVersionKind: gvk.WorkloadEntry, + Domain: "cluster.local", + }, + Spec: &networking.WorkloadEntry{ + Address: "2.3.4.5", + Labels: map[string]string{ + "app.bar": "true", + }, + }, + }) + fx.WaitOrFail(t, "xds") + expectServiceEndpoints(t, fx, expectedSvc1, 80, []EndpointResponse{}) + expectServiceEndpoints(t, fx, expectedSvc2, 80, instances) + }) + t.Run("All directions", func(t *testing.T) { store, kube, fx := setupTest(t) makeService(t, kube, service) @@ -1258,7 +1568,7 @@ func expectServiceEndpointsFromIndex(t *testing.T, ei *model.EndpointIndex, svc } }) slices.SortBy(got, func(a EndpointResponse) string { - return a.Address + return fmt.Sprintf("%v:%d", a.Address, a.Port) }) return assert.Compare(got, expected) }, retry.Converge(2), retry.Timeout(time.Second*2), retry.Delay(time.Millisecond*10)) diff --git a/pilot/pkg/xds/delta.go b/pilot/pkg/xds/delta.go index 0632f8c98ec1..9c8441a1b3f1 100644 --- a/pilot/pkg/xds/delta.go +++ b/pilot/pkg/xds/delta.go @@ -406,21 +406,30 @@ func (s *DiscoveryServer) shouldRespondDelta(con *Connection, request *discovery // If there is mismatch in the nonce, that is a case of expired/stale nonce. // A nonce becomes stale following a newer nonce being sent to Envoy. - // TODO: due to concurrent unsubscribe, this probably doesn't make sense. Do we need any logic here? if request.ResponseNonce != "" && request.ResponseNonce != previousInfo.NonceSent { deltaLog.Debugf("ADS:%s: REQ %s Expired nonce received %s, sent %s", stype, con.conID, request.ResponseNonce, previousInfo.NonceSent) xdsExpiredNonce.With(typeTag.Value(v3.GetMetricType(request.TypeUrl))).Increment() return false } - // If it comes here, that means nonce match. This an ACK. We should record - // the ack details and respond if there is a change in resource names. + + // Spontaneous DeltaDiscoveryRequests from the client. + // This can be done to dynamically add or remove elements from the tracked resource_names set. + // In this case response_nonce is empty. + spontaneousReq := request.ResponseNonce == "" + var previousResources, currentResources []string var alwaysRespond bool + + // Update resource names, and record ACK if required. con.proxy.UpdateWatchedResource(request.TypeUrl, func(wr *model.WatchedResource) *model.WatchedResource { previousResources = wr.ResourceNames currentResources, _ = deltaWatchedResources(previousResources, request) - wr.NonceAcked = request.ResponseNonce + if !spontaneousReq { + // We got an ACK, record it. + // Otherwise, this is just a change in resource subscription, so leave the last ACK info in place. + wr.NonceAcked = request.ResponseNonce + } wr.ResourceNames = currentResources alwaysRespond = wr.AlwaysRespond wr.AlwaysRespond = false @@ -428,10 +437,6 @@ func (s *DiscoveryServer) shouldRespondDelta(con *Connection, request *discovery }) subChanged := !slices.EqualUnordered(previousResources, currentResources) - // Spontaneous DeltaDiscoveryRequests from the client. - // This can be done to dynamically add or remove elements from the tracked resource_names set. - // In this case response_nonce is empty. - spontaneousReq := request.ResponseNonce == "" // It is invalid in the below two cases: // 1. no subscribed resources change from spontaneous delta request. // 2. subscribed resources changes from ACK. diff --git a/pilot/pkg/xds/delta_test.go b/pilot/pkg/xds/delta_test.go index 3bb132eb34d6..ed5aea092fa0 100644 --- a/pilot/pkg/xds/delta_test.go +++ b/pilot/pkg/xds/delta_test.go @@ -31,6 +31,7 @@ import ( "istio.io/istio/pkg/slices" "istio.io/istio/pkg/test" "istio.io/istio/pkg/test/util/assert" + "istio.io/istio/pkg/test/util/retry" "istio.io/istio/pkg/util/sets" "istio.io/istio/pkg/workloadapi" ) @@ -438,3 +439,35 @@ func TestDeltaWDS(t *testing.T) { t.Fatalf("received unexpected eds resource %v", resp.Resources) } } + +func TestDeltaUnsub(t *testing.T) { + s := xds.NewFakeDiscoveryServer(t, xds.FakeOptions{}) + + ads := s.ConnectDeltaADS().WithID("sidecar~127.0.0.1~test.default~default.svc.cluster.local") + + runAssert := func(nonce string) { + t.Helper() + retry.UntilSuccessOrFail(t, func() error { + sync := getSyncStatus(t, s.Discovery) + if len(sync) != 1 { + return fmt.Errorf("got %v sync status", len(sync)) + } + if sync[0].ClusterSent != nonce { + return fmt.Errorf("want %q, got %q for send", nonce, sync[0].ClusterSent) + } + if sync[0].ClusterAcked != nonce { + return fmt.Errorf("want %q, got %q for ack", nonce, sync[0].ClusterAcked) + } + return nil + }) + } + // Initially we get everything + resp := ads.RequestResponseAck(&discovery.DeltaDiscoveryRequest{ + ResourceNamesSubscribe: []string{}, + }) + runAssert(resp.Nonce) + ads.Request(&discovery.DeltaDiscoveryRequest{ + ResourceNamesUnsubscribe: []string{"something"}, + }) + runAssert(resp.Nonce) +} diff --git a/pilot/pkg/xds/eds_test.go b/pilot/pkg/xds/eds_test.go index 595cc2e17cef..a8b30d2c4d7e 100644 --- a/pilot/pkg/xds/eds_test.go +++ b/pilot/pkg/xds/eds_test.go @@ -42,6 +42,7 @@ import ( xdsfake "istio.io/istio/pilot/test/xds" "istio.io/istio/pilot/test/xdstest" "istio.io/istio/pkg/adsc" + "istio.io/istio/pkg/config/constants" "istio.io/istio/pkg/config/host" "istio.io/istio/pkg/config/protocol" "istio.io/istio/pkg/config/schema/kind" @@ -1230,7 +1231,8 @@ func updateServiceResolution(s *xdsfake.FakeDiscoveryServer, resolution model.Re func addOverlappingEndpoints(s *xdsfake.FakeDiscoveryServer) { svc := &model.Service{ - Hostname: "overlapping.cluster.local", + DefaultAddress: constants.UnspecifiedIP, + Hostname: "overlapping.cluster.local", Ports: model.PortList{ { Name: "dns", @@ -1263,7 +1265,8 @@ func addOverlappingEndpoints(s *xdsfake.FakeDiscoveryServer) { func addUnhealthyCluster(s *xdsfake.FakeDiscoveryServer) { svc := &model.Service{ - Hostname: "unhealthy.svc.cluster.local", + DefaultAddress: constants.UnspecifiedIP, + Hostname: "unhealthy.svc.cluster.local", Ports: model.PortList{ { Name: "tcp-dns", diff --git a/pilot/pkg/xds/workload.go b/pilot/pkg/xds/workload.go index d8a329980db3..005ec591c50d 100644 --- a/pilot/pkg/xds/workload.go +++ b/pilot/pkg/xds/workload.go @@ -165,21 +165,9 @@ func (e WorkloadRBACGenerator) GenerateDeltas( ) (model.Resources, model.DeletedResources, model.XdsLogDetails, bool, error) { var updatedPolicies sets.Set[model.ConfigKey] if len(req.ConfigsUpdated) != 0 { + // The ambient store will send all of these as kind.AuthorizationPolicy, even if generated from PeerAuthentication, + // so we can only fetch these ones. updatedPolicies = model.ConfigsOfKind(req.ConfigsUpdated, kind.AuthorizationPolicy) - // Convert the actual Kubernetes PeerAuthentication policies to the synthetic ones - // by adding the prefix - // - // This is needed because the handler that produces the ConfigUpdate blindly sends - // the Kubernetes resource names without context of the synthetic Ambient policies - // TODO: Split out PeerAuthentication into a separate handler in - // https://github.com/istio/istio/blob/master/pilot/pkg/bootstrap/server.go#L882 - for p := range model.ConfigsOfKind(req.ConfigsUpdated, kind.PeerAuthentication) { - updatedPolicies.Insert(model.ConfigKey{ - Name: model.GetAmbientPolicyConfigName(p), - Namespace: p.Namespace, - Kind: p.Kind, - }) - } } if len(req.ConfigsUpdated) != 0 && len(updatedPolicies) == 0 { // This was a incremental push for a resource we don't watch... skip diff --git a/pilot/pkg/xds/workload_test.go b/pilot/pkg/xds/workload_test.go index 5fe2f50214aa..27eb75ff03e6 100644 --- a/pilot/pkg/xds/workload_test.go +++ b/pilot/pkg/xds/workload_test.go @@ -31,7 +31,9 @@ import ( "istio.io/istio/pilot/pkg/model" v3 "istio.io/istio/pilot/pkg/xds/v3" "istio.io/istio/pilot/test/xds" + "istio.io/istio/pkg/config" "istio.io/istio/pkg/config/constants" + "istio.io/istio/pkg/config/schema/gvk" "istio.io/istio/pkg/kube/kclient/clienttest" "istio.io/istio/pkg/test" "istio.io/istio/pkg/test/util/assert" @@ -81,8 +83,8 @@ func buildExpectAddedAndRemoved(t *testing.T) func(resp *discovery.DeltaDiscover for _, r := range resp.RemovedResources { haveRemoved.Insert(r) } - assert.Equal(t, sets.SortedList(have), sets.SortedList(wantAdded)) - assert.Equal(t, sets.SortedList(haveRemoved), sets.SortedList(wantRemoved)) + assert.Equal(t, sets.SortedList(have), sets.SortedList(wantAdded), "updated") + assert.Equal(t, sets.SortedList(haveRemoved), sets.SortedList(wantRemoved), "removed") } } @@ -291,6 +293,7 @@ func deletePeerAuthentication(s *xds.FakeDiscoveryServer, name string, ns string clienttest.NewWriter[*securityclient.PeerAuthentication](s.T(), s.KubeClient()).Delete(name, ns) } +// nolint: unparam func createPeerAuthentication(s *xds.FakeDiscoveryServer, name string, ns string, spec *v1beta1.PeerAuthentication) { c := &securityclient.PeerAuthentication{ ObjectMeta: metav1.ObjectMeta{ @@ -442,3 +445,52 @@ func TestWorkloadPeerAuthentication(t *testing.T) { createPod(s, "pod", "sa", "127.0.0.1", "node") ads.ExpectNoResponse() } + +// Regression tests for NOP PeerAuthentication triggering a removal +func TestPeerAuthenticationUpdate(t *testing.T) { + test.SetForTest(t, &features.EnableAmbient, true) + expectAddedAndRemoved := buildExpectAddedAndRemoved(t) + s := xds.NewFakeDiscoveryServer(t, xds.FakeOptions{}) + ads := s.ConnectDeltaADS().WithType(v3.WorkloadAuthorizationType).WithTimeout(time.Second * 10).WithNodeType(model.Ztunnel) + + ads.Request(&discovery.DeltaDiscoveryRequest{ + ResourceNamesSubscribe: []string{"*"}, + }) + ads.ExpectEmptyResponse() + + // One PA will create 2 resources, but they may not come in one push. Workaround this by forcing them to come one-by-one + createPeerAuthentication(s, "policy1", "ns", &v1beta1.PeerAuthentication{}) + expectAddedAndRemoved(ads.ExpectResponse(), []string{"istio-system/istio_converted_static_strict"}, nil) + + // Now create our real policy + spec := &v1beta1.PeerAuthentication{ + Selector: &metav1beta1.WorkloadSelector{ + MatchLabels: map[string]string{ + "app": "sa", + }, + }, + Mtls: &v1beta1.PeerAuthentication_MutualTLS{ + Mode: v1beta1.PeerAuthentication_MutualTLS_STRICT, + }, + PortLevelMtls: map[uint32]*v1beta1.PeerAuthentication_MutualTLS{ + 8080: { + Mode: v1beta1.PeerAuthentication_MutualTLS_DISABLE, + }, + }, + } + createPeerAuthentication(s, "policy1", "ns", spec) + expectAddedAndRemoved(ads.ExpectResponse(), []string{"ns/converted_peer_authentication_policy1"}, nil) + + // Create in the Istio config store. This is important, since this will trigger an update on PeerAuthentication which + // is what caused the issue. + _, err := s.Discovery.Env.Create(config.Config{ + Meta: config.Meta{ + GroupVersionKind: gvk.PeerAuthentication, + Name: "policy1", + Namespace: "ns", + }, + Spec: spec, + }) + assert.NoError(t, err) + ads.ExpectNoResponse() +} diff --git a/pkg/config/validation/agent/validation.go b/pkg/config/validation/agent/validation.go index 4684d440bb7d..cb06803aa9f6 100644 --- a/pkg/config/validation/agent/validation.go +++ b/pkg/config/validation/agent/validation.go @@ -19,6 +19,7 @@ import ( "fmt" "net" "net/netip" + "regexp" "strconv" "strings" "time" @@ -43,7 +44,9 @@ const ( // nolint: revive connectTimeoutMax = time.Hour // nolint: revive - connectTimeoutMin = time.Millisecond + connectTimeoutMin = time.Millisecond + outboundHostPrefix = "outbound" + outboundHostNameFormat = "outbound_._._." ) var scope = log.RegisterScope("validation", "CRD validation debugging") @@ -673,6 +676,50 @@ func ValidateWildcardDomain(domain string) error { return nil } +// ValidateWildcardDomainForVirtualServiceBoundToGateway checks that a domain is a valid FQDN, but also allows wildcard prefixes. +// If it is an SNI domain, then it does a special validation where it allows a +// which matches outbound_._._. format +func ValidateWildcardDomainForVirtualServiceBoundToGateway(sni bool, domain string) error { + if err := CheckDNS1123Preconditions(domain); err != nil { + return err + } + + // check if its an auto generated domain, with outbound_ as a prefix. + if sni && strings.HasPrefix(domain, outboundHostPrefix+"_") { + // example of a valid domain: outbound_.80_._.e2e.foobar.mesh + trafficDirectionSuffix, port, hostname, err := parseAutoGeneratedSNIDomain(domain) + if err != nil { + return err + } + if trafficDirectionSuffix != outboundHostPrefix { + return fmt.Errorf("domain name %q invalid (label %q invalid)", domain, trafficDirectionSuffix) + } + match, _ := regexp.MatchString("([0-9].*)", port) + if !match { + return fmt.Errorf("domain name %q invalid (label %q invalid). should follow %s format", domain, port, outboundHostNameFormat) + } + match, _ = regexp.MatchString("([a-zA-Z].*)", hostname) + if !match { + return fmt.Errorf("domain name %q invalid (label %q invalid). should follow %s format", domain, hostname, outboundHostNameFormat) + } + return nil + } + return ValidateWildcardDomain(domain) +} + +// parseAutoGeneratedSNIDomain parses auto generated sni domains +// which are generated when using AUTO_PASSTHROUGH mode in envoy +func parseAutoGeneratedSNIDomain(domain string) (string, string, string, error) { + parts := strings.Split(domain, "_") + if len(parts) < 4 { + return "", "", "", fmt.Errorf("domain name %s invalid, should follow '%s' format", domain, outboundHostNameFormat) + } + trafficDirectionPrefix := parts[0] + port := strings.Trim(parts[1], ".") + hostname := strings.Trim(parts[3], ".") + return trafficDirectionPrefix, port, hostname, nil +} + // validate the trust domain format func ValidateTrustDomain(domain string) error { if len(domain) == 0 { diff --git a/pkg/config/validation/agent/validation_test.go b/pkg/config/validation/agent/validation_test.go index cee31bd50507..6c29432012d8 100644 --- a/pkg/config/validation/agent/validation_test.go +++ b/pkg/config/validation/agent/validation_test.go @@ -876,6 +876,79 @@ func TestValidateWildcardDomain(t *testing.T) { } } +func TestValidateWildcardDomainForVirtualServiceBoundToGateway(t *testing.T) { + tests := []struct { + name string + in string + sni bool + out string + }{ + {"empty", "", false, "empty"}, + {"too long", strings.Repeat("x", 256), false, "too long"}, + {"happy", strings.Repeat("x", 63), false, ""}, + {"wildcard", "*", false, ""}, + {"wildcard multi-segment", "*.bar.com", false, ""}, + {"wildcard single segment", "*foo", false, ""}, + {"wildcard prefix", "*foo.bar.com", false, ""}, + {"wildcard prefix dash", "*-foo.bar.com", false, ""}, + {"bad wildcard", "foo.*.com", false, "invalid"}, + {"bad wildcard", "foo*.bar.com", false, "invalid"}, + {"IP address", "1.1.1.1", false, "invalid"}, + {"SNI domain", "outbound_.80_._.e2e.foobar.mesh", true, ""}, + {"happy", "outbound.com", true, ""}, + {"invalid SNI domain", "neverbound_.80_._.e2e.foobar.mesh", true, "invalid"}, + {"invalid SNI domain", "outbound_.thisIsNotAPort_._.e2e.foobar.mesh", true, "invalid"}, + {"invalid SNI domain", "outbound_.80_._", true, "invalid"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateWildcardDomainForVirtualServiceBoundToGateway(tt.sni, tt.in) + if err == nil && tt.out != "" { + t.Fatalf("ValidateWildcardDomain(%v) = nil, wanted %q", tt.in, tt.out) + } else if err != nil && tt.out == "" { + t.Fatalf("ValidateWildcardDomain(%v) = %v, wanted nil", tt.in, err) + } else if err != nil && !strings.Contains(err.Error(), tt.out) { + t.Fatalf("ValidateWildcardDomain(%v) = %v, wanted %q", tt.in, err, tt.out) + } + }) + } +} + +func TestParseAutoGeneratedSNIDomain(t *testing.T) { + tests := []struct { + name string + in string + trafficDirectionSuffix string + port string + hostname string + err string + }{ + {"invalid", "foobar.com", "", "", "", "domain name foobar.com invalid, should follow '" + outboundHostNameFormat + "' format"}, + {"valid", "outbound_.80_._.e2e.foobar.mesh", "outbound", "80", "e2e.foobar.mesh", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + trafficDirectionSuffix, port, hostname, err := parseAutoGeneratedSNIDomain(tt.in) + if err == nil && tt.err != "" { + t.Fatalf("parseOutboundSNIDomain(%v) = nil, wanted %q", tt.in, tt.err) + } else if err != nil && tt.err == "" { + t.Fatalf("parseOutboundSNIDomain(%v) = %v, wanted nil", tt.in, err) + } else if err != nil && !strings.Contains(err.Error(), tt.err) { + t.Fatalf("parseOutboundSNIDomain(%v) = %v, wanted %q", tt.in, err, tt.err) + } + if trafficDirectionSuffix != tt.trafficDirectionSuffix { + t.Fatalf("parseOutboundSNIDomain(%v) = %v, wanted %q", tt.in, trafficDirectionSuffix, tt.trafficDirectionSuffix) + } + if port != tt.port { + t.Fatalf("parseOutboundSNIDomain(%v) = %v, wanted %q", tt.in, port, tt.port) + } + if hostname != tt.hostname { + t.Fatalf("parseOutboundSNIDomain(%v) = %v, wanted %q", tt.in, hostname, tt.hostname) + } + }) + } +} + func TestValidateTrustDomain(t *testing.T) { tests := []struct { name string diff --git a/pkg/config/validation/validation.go b/pkg/config/validation/validation.go index ba04a757ee61..99af62499ef6 100644 --- a/pkg/config/validation/validation.go +++ b/pkg/config/validation/validation.go @@ -1552,13 +1552,8 @@ var ValidateVirtualService = RegisterValidateFunc("ValidateVirtualService", appliesToMesh = true } else { errs = AppendValidation(errs, validateGatewayNames(virtualService.Gateways, gatewaySemantics)) - for _, gatewayName := range virtualService.Gateways { - if gatewayName == constants.IstioMeshGateway { - appliesToMesh = true - } else { - appliesToGateway = true - } - } + appliesToGateway = isGateway(virtualService) + appliesToMesh = !appliesToGateway } if !appliesToGateway { @@ -1580,7 +1575,13 @@ var ValidateVirtualService = RegisterValidateFunc("ValidateVirtualService", allHostsValid := true for _, virtualHost := range virtualService.Hosts { - if err := agent.ValidateWildcardDomain(virtualHost); err != nil { + var err error + if appliesToGateway { + err = agent.ValidateWildcardDomainForVirtualServiceBoundToGateway(isSniHost(virtualService), virtualHost) + } else { + err = agent.ValidateWildcardDomain(virtualHost) + } + if err != nil { if !netutil.IsValidIPAddress(virtualHost) { errs = AppendValidation(errs, err) allHostsValid = false @@ -1657,6 +1658,26 @@ func assignExactOrPrefix(exact, prefix string) string { return "" } +func isSniHost(context *networking.VirtualService) bool { + for _, tls := range context.Tls { + for _, match := range tls.Match { + if len(match.SniHosts) > 0 { + return true + } + } + } + return false +} + +func isGateway(context *networking.VirtualService) bool { + for _, gatewayName := range context.Gateways { + if gatewayName == constants.IstioMeshGateway { + return false + } + } + return true +} + // genMatchHTTPRoutes build the match rules into struct OverlappingMatchValidationForHTTPRoute // based on particular HTTPMatchRequest, according to comments on https://github.com/istio/istio/pull/32701 // only support Match's port, method, authority, headers, query params and nonheaders for now. @@ -2025,7 +2046,13 @@ func validateTLSMatch(match *networking.TLSMatchAttributes, context *networking. } func validateSniHost(sniHost string, context *networking.VirtualService) (errs Validation) { - if err := agent.ValidateWildcardDomain(sniHost); err != nil { + var err error + if isGateway(context) { + err = agent.ValidateWildcardDomainForVirtualServiceBoundToGateway(true, sniHost) + } else { + err = agent.ValidateWildcardDomain(sniHost) + } + if err != nil { // Could also be an IP if netutil.IsValidIPAddress(sniHost) { errs = AppendValidation(errs, WrapWarning(fmt.Errorf("using an IP address (%q) goes against SNI spec and most clients do not support this", sniHost))) diff --git a/pkg/config/validation/validation_test.go b/pkg/config/validation/validation_test.go index 2da62a28a840..d06b2cae05ee 100644 --- a/pkg/config/validation/validation_test.go +++ b/pkg/config/validation/validation_test.go @@ -2023,6 +2023,26 @@ func TestValidateVirtualService(t *testing.T) { }}, }}, }, valid: true}, + {name: "allow sni based domains", in: &networking.VirtualService{ + Hosts: []string{"outbound_.15010_._.istiod.istio-system.svc.cluster.local"}, + Gateways: []string{"ns1/gateway"}, + Tls: []*networking.TLSRoute{ + { + Match: []*networking.TLSMatchAttributes{ + { + SniHosts: []string{"outbound_.15010_._.istiod.istio-system.svc.cluster.local"}, + }, + }, + Route: []*networking.RouteDestination{ + { + Destination: &networking.Destination{ + Host: "istio.istio-system.svc.cluster.local", + }, + }, + }, + }, + }, + }, valid: true}, {name: "duplicate hosts", in: &networking.VirtualService{ Hosts: []string{"*.foo.bar", "*.bar"}, Http: []*networking.HTTPRoute{{ diff --git a/pkg/kube/kclient/client_test.go b/pkg/kube/kclient/client_test.go index 4596d28badde..c810d8731c5b 100644 --- a/pkg/kube/kclient/client_test.go +++ b/pkg/kube/kclient/client_test.go @@ -35,6 +35,7 @@ import ( istioclient "istio.io/client-go/pkg/apis/extensions/v1alpha1" istionetclient "istio.io/client-go/pkg/apis/networking/v1alpha3" "istio.io/istio/pilot/pkg/features" + "istio.io/istio/pkg/config" "istio.io/istio/pkg/config/mesh" "istio.io/istio/pkg/config/schema/gvr" "istio.io/istio/pkg/kube" @@ -479,3 +480,40 @@ func TestFilterClusterScoped(t *testing.T) { tester.Create(obj1) tracker.WaitOrdered("add/1") } + +func TestFilterDeadlock(t *testing.T) { + // This is a regression test for an issue causing a deadlock when using the filter + tracker := assert.NewTracker[string](t) + + c := kube.NewFakeClient(&appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "random", Namespace: "test"}, + }) + meshWatcher := mesh.NewTestWatcher(&meshconfig.MeshConfig{DiscoverySelectors: []*metav1.LabelSelector{{ + MatchLabels: map[string]string{"selected": "yes"}, + }}}) + stop := test.NewStop(t) + testns := clienttest.NewWriter[*corev1.Namespace](t, c) + testns.Create(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test", Labels: map[string]string{"selected": "no"}}}) + namespaces := kclient.New[*corev1.Namespace](c) + discoveryNamespacesFilter := filter.NewDiscoveryNamespacesFilter( + namespaces, + meshWatcher, + stop, + ) + + // Create some random client + deployments := kclient.NewFiltered[*appsv1.Deployment](c, kubetypes.Filter{ + ObjectFilter: discoveryNamespacesFilter, + }) + // The client calls .List() in the handler -- this was the cause of the deadlock. + deployments.AddEventHandler(controllers.ObjectHandler(func(o controllers.Object) { + for _, obj := range deployments.List(o.GetNamespace(), klabels.Everything()) { + tracker.Record(config.NamespacedName(obj).String()) + } + })) + c.RunAndWait(stop) + c.WaitForCacheSync("test", stop, deployments.HasSynced) + + testns.Update(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test", Labels: map[string]string{"selected": "yes"}}}) + tracker.WaitOrdered("test/random") +} diff --git a/pkg/kube/namespace/filter.go b/pkg/kube/namespace/filter.go index 507853f813f2..ba18600cbf47 100644 --- a/pkg/kube/namespace/filter.go +++ b/pkg/kube/namespace/filter.go @@ -27,6 +27,7 @@ import ( "istio.io/istio/pkg/kube/kclient" "istio.io/istio/pkg/kube/kubetypes" "istio.io/istio/pkg/log" + "istio.io/istio/pkg/slices" "istio.io/istio/pkg/util/sets" ) @@ -57,17 +58,18 @@ func NewDiscoveryNamespacesFilter( namespaces.AddEventHandler(controllers.EventHandler[*corev1.Namespace]{ AddFunc: func(ns *corev1.Namespace) { f.lock.Lock() - defer f.lock.Unlock() + created := f.namespaceCreatedLocked(ns.ObjectMeta) + f.lock.Unlock() // In rare cases, a namespace may be created after objects in the namespace, because there is no synchronization between watches // So we need to notify if we started selecting namespace - if f.namespaceCreatedLocked(ns.ObjectMeta) { - f.notifyHandlersLocked(sets.New(ns.Name), nil) + if created { + f.notifyHandlers(sets.New(ns.Name), nil) } }, UpdateFunc: func(old, new *corev1.Namespace) { f.lock.Lock() - defer f.lock.Unlock() membershipChanged, namespaceAdded := f.namespaceUpdatedLocked(old.ObjectMeta, new.ObjectMeta) + f.lock.Unlock() if membershipChanged { added := sets.New(new.Name) var removed sets.String @@ -75,7 +77,7 @@ func NewDiscoveryNamespacesFilter( removed = added added = nil } - f.notifyHandlersLocked(added, removed) + f.notifyHandlers(added, removed) } }, DeleteFunc: func(ns *corev1.Namespace) { @@ -95,8 +97,14 @@ func NewDiscoveryNamespacesFilter( return f } -func (d *discoveryNamespacesFilter) notifyHandlersLocked(added sets.Set[string], removed sets.String) { - for _, h := range d.handlers { +func (d *discoveryNamespacesFilter) notifyHandlers(added sets.Set[string], removed sets.String) { + // Clone handlers; we handle dynamic handlers so they can change after the filter has started. + // Important: handlers are not called under the lock. If they are, then handlers which eventually call discoveryNamespacesFilter.Filter + // (as some do in the codebase currently, via kclient.List), will deadlock. + d.lock.RLock() + handlers := slices.Clone(d.handlers) + d.lock.RUnlock() + for _, h := range handlers { h(added, removed) } } @@ -143,49 +151,54 @@ func (d *discoveryNamespacesFilter) selectorsChanged( discoverySelectors []*metav1.LabelSelector, notify bool, ) { - d.lock.Lock() - defer d.lock.Unlock() - var selectors []labels.Selector - newDiscoveryNamespaces := sets.New[string]() - - namespaceList := d.namespaces.List("", labels.Everything()) - - // convert LabelSelectors to Selectors - for _, selector := range discoverySelectors { - ls, err := metav1.LabelSelectorAsSelector(selector) - if err != nil { - log.Errorf("error initializing discovery namespaces filter, invalid discovery selector: %v", err) - return + // Call closure to allow safe defer lock handling + selectedNamespaces, deselectedNamespaces := func() (sets.String, sets.String) { + d.lock.Lock() + defer d.lock.Unlock() + var selectors []labels.Selector + newDiscoveryNamespaces := sets.New[string]() + + namespaceList := d.namespaces.List("", labels.Everything()) + + // convert LabelSelectors to Selectors + for _, selector := range discoverySelectors { + ls, err := metav1.LabelSelectorAsSelector(selector) + if err != nil { + log.Errorf("error initializing discovery namespaces filter, invalid discovery selector: %v", err) + return nil, nil + } + selectors = append(selectors, ls) } - selectors = append(selectors, ls) - } - // range over all namespaces to get discovery namespaces - for _, ns := range namespaceList { - for _, selector := range selectors { - if selector.Matches(labels.Set(ns.Labels)) { - newDiscoveryNamespaces.Insert(ns.Name) + // range over all namespaces to get discovery namespaces + for _, ns := range namespaceList { + for _, selector := range selectors { + if selector.Matches(labels.Set(ns.Labels)) { + newDiscoveryNamespaces.Insert(ns.Name) + } } - } - // omitting discoverySelectors indicates discovering all namespaces - if len(selectors) == 0 { - for _, ns := range namespaceList { - newDiscoveryNamespaces.Insert(ns.Name) + // omitting discoverySelectors indicates discovering all namespaces + if len(selectors) == 0 { + for _, ns := range namespaceList { + newDiscoveryNamespaces.Insert(ns.Name) + } } } - } - if notify { + // update filter state oldDiscoveryNamespaces := d.discoveryNamespaces - selectedNamespaces := newDiscoveryNamespaces.Difference(oldDiscoveryNamespaces) - deselectedNamespaces := oldDiscoveryNamespaces.Difference(newDiscoveryNamespaces) - // Important: keep the lock while we call handlers. This allows handlers to ensure they do not miss events - // if they are processing the change and new events come in. - d.notifyHandlersLocked(selectedNamespaces, deselectedNamespaces) + d.discoveryNamespaces = newDiscoveryNamespaces + d.discoverySelectors = selectors + if notify { + selectedNamespaces := newDiscoveryNamespaces.Difference(oldDiscoveryNamespaces) + deselectedNamespaces := oldDiscoveryNamespaces.Difference(newDiscoveryNamespaces) + return selectedNamespaces, deselectedNamespaces + } + return nil, nil + }() + if notify { + d.notifyHandlers(selectedNamespaces, deselectedNamespaces) } - // update filter state - d.discoveryNamespaces = newDiscoveryNamespaces - d.discoverySelectors = selectors } // namespaceCreated: if newly created namespace is selected, update namespace membership diff --git a/pkg/monitoring/monitortest/test.go b/pkg/monitoring/monitortest/test.go index fc3d628c9359..fe433d6e4ffa 100644 --- a/pkg/monitoring/monitortest/test.go +++ b/pkg/monitoring/monitortest/test.go @@ -112,6 +112,15 @@ func Exactly(v float64) func(any) error { } } +func LessThan(v float64) func(any) error { + return func(f any) error { + if v <= toFloat(f) { + return fmt.Errorf("want <= %v (got %v)", v, toFloat(f)) + } + return nil + } +} + func Distribution(count uint64, sum float64) func(any) error { return func(f any) error { d := f.(*dto.Histogram) diff --git a/pkg/revisions/tag_watcher.go b/pkg/revisions/tag_watcher.go index da95e7c3669e..5d2e2fbe8549 100644 --- a/pkg/revisions/tag_watcher.go +++ b/pkg/revisions/tag_watcher.go @@ -104,11 +104,11 @@ func (p *tagWatcher) notifyHandlers() { } func isTagWebhook(uobj any) bool { - obj, ok := uobj.(controllers.Object) - if !ok { + obj := controllers.ExtractObject(uobj) + if obj == nil { return false } - _, ok = obj.GetLabels()[IstioTagLabel] + _, ok := obj.GetLabels()[IstioTagLabel] return ok } diff --git a/prow/lib.sh b/prow/lib.sh index 307c8ac7ca77..e2969b984532 100755 --- a/prow/lib.sh +++ b/prow/lib.sh @@ -128,7 +128,7 @@ function build_images() { if [[ "${SELECT_TEST}" == "test.integration.operator.kube" || "${SELECT_TEST}" == "test.integration.kube" || "${JOB_TYPE:-postsubmit}" == "postsubmit" ]]; then targets+="docker.operator " fi - if [[ "${SELECT_TEST}" == "test.integration.ambient.kube" || "${SELECT_TEST}" == "test.integration.kube" || "${JOB_TYPE:-postsubmit}" == "postsubmit" ]]; then + if [[ "${SELECT_TEST}" == "test.integration.ambient.kube" || "${SELECT_TEST}" == "test.integration.kube" || "${SELECT_TEST}" == "test.integration.helm.kube" || "${JOB_TYPE:-postsubmit}" == "postsubmit" ]]; then targets+="docker.ztunnel " fi targets+="docker.install-cni " diff --git a/releasenotes/notes/51800.yaml b/releasenotes/notes/51800.yaml new file mode 100644 index 000000000000..7511bc64e96b --- /dev/null +++ b/releasenotes/notes/51800.yaml @@ -0,0 +1,10 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management + +issue: + - 51800 + +releaseNotes: + - | + **Fixed** VirtualMachine WorkloadEntry locality label missing during autoregistration. diff --git a/releasenotes/notes/51967.yaml b/releasenotes/notes/51967.yaml new file mode 100644 index 000000000000..0ccbf6ecc2d5 --- /dev/null +++ b/releasenotes/notes/51967.yaml @@ -0,0 +1,8 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management +issue: + - 51747 +releaseNotes: + - | + **Fixed** matching multiple service VIPs in ServiceEntry. diff --git a/releasenotes/notes/52005.yaml b/releasenotes/notes/52005.yaml new file mode 100644 index 000000000000..e00af474524b --- /dev/null +++ b/releasenotes/notes/52005.yaml @@ -0,0 +1,6 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: telemetry +releaseNotes: + - | + **Fixed** inconsistent behavior with the `istio_agent_cert_expiry_seconds` metric. diff --git a/releasenotes/notes/52017.yaml b/releasenotes/notes/52017.yaml new file mode 100644 index 000000000000..15b38935a891 --- /dev/null +++ b/releasenotes/notes/52017.yaml @@ -0,0 +1,8 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: installation +issue: + - 52016 +releaseNotes: +- | + **Fixed** the istiod chart installation for older Helm versions (v3.6 and v3.7) by ensuring that `.Values.profile` is set to a string. diff --git a/releasenotes/notes/52033.yaml b/releasenotes/notes/52033.yaml new file mode 100644 index 000000000000..bfbc12826e7a --- /dev/null +++ b/releasenotes/notes/52033.yaml @@ -0,0 +1,6 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: installation +releaseNotes: + - | + **Fixed** an omission in ztunnel helm charts which resulted in some Kubernetes resources being created without labels diff --git a/releasenotes/notes/52320.yaml b/releasenotes/notes/52320.yaml new file mode 100644 index 000000000000..ff4b75b676cf --- /dev/null +++ b/releasenotes/notes/52320.yaml @@ -0,0 +1,8 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management +issue: + - 52218 +releaseNotes: +- | + **Fixed** Do not add pod to ipset if we have a partial failure adding to the dataplane. diff --git a/releasenotes/notes/delta-xds-stale.yaml b/releasenotes/notes/delta-xds-stale.yaml new file mode 100644 index 000000000000..fe7677eca2c8 --- /dev/null +++ b/releasenotes/notes/delta-xds-stale.yaml @@ -0,0 +1,9 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management +issue: + - 51612 + +releaseNotes: + - | + **Fixed** an issue causing resources to incorrectly be reported by `istioctl proxy-status` as `STALE`. diff --git a/releasenotes/notes/namespace-filter-deadlock.yaml b/releasenotes/notes/namespace-filter-deadlock.yaml new file mode 100644 index 000000000000..c2400a20828e --- /dev/null +++ b/releasenotes/notes/namespace-filter-deadlock.yaml @@ -0,0 +1,7 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management +releaseNotes: + - | + **Fixed** an issue that can trigger a deadlock when `discoverySelectors` (configured in `MeshConfig`) and a namespace, + which has an `Ingress` object or a `gateway.networking.k8s.io` `Gateway` object, moves from being selected to unselected. diff --git a/releasenotes/notes/pilot-dupe-ip.yaml b/releasenotes/notes/pilot-dupe-ip.yaml new file mode 100644 index 000000000000..20fdbfb7c537 --- /dev/null +++ b/releasenotes/notes/pilot-dupe-ip.yaml @@ -0,0 +1,6 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management +releaseNotes: + - | + **Fixed** an issue causing stale endpoints when the same IP address was present in multiple `WorkloadEntries`. diff --git a/releasenotes/notes/support-features.yaml b/releasenotes/notes/support-features.yaml new file mode 100644 index 000000000000..ecf3a9746d43 --- /dev/null +++ b/releasenotes/notes/support-features.yaml @@ -0,0 +1,6 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: traffic-management +releaseNotes: + - | + **Removed** writing the experimental field `GatewayClass.status.supportedFeatures`, as it is unstable in the API. diff --git a/security/pkg/nodeagent/cache/secretcache.go b/security/pkg/nodeagent/cache/secretcache.go index 67cea67f1991..697fa0a63f25 100644 --- a/security/pkg/nodeagent/cache/secretcache.go +++ b/security/pkg/nodeagent/cache/secretcache.go @@ -584,7 +584,7 @@ func (sc *SecretManagerClient) generateNewSecret(resourceName string) (*security ServiceAccount: sc.configOptions.ServiceAccount, } - cacheLog.Debugf("constructed host name for CSR: %s", csrHostName.String()) + cacheLog.Debugf("%s constructed host name for CSR: %s", logPrefix, csrHostName.String()) options := pkiutil.CertOptions{ Host: csrHostName.String(), RSAKeySize: sc.configOptions.WorkloadRSAKeySize, @@ -626,7 +626,10 @@ func (sc *SecretManagerClient) generateNewSecret(resourceName string) (*security return nil, fmt.Errorf("failed to extract expire time from server certificate in CSR response: %v", err) } - cacheLog.WithLabels("latency", time.Since(t0), "ttl", time.Until(expireTime)).Info("generated new workload certificate") + cacheLog.WithLabels("resourceName", resourceName, + "latency", time.Since(t0), + "ttl", time.Until(expireTime)). + Info("generated new workload certificate") if len(trustBundlePEM) > 0 { rootCertPEM = concatCerts(trustBundlePEM) @@ -657,7 +660,6 @@ var rotateTime = func(secret security.SecretItem, graceRatio float64) time.Durat func (sc *SecretManagerClient) registerSecret(item security.SecretItem) { delay := rotateTime(item, sc.configOptions.SecretRotationGracePeriodRatio) - certExpirySeconds.ValueFrom(func() float64 { return time.Until(item.ExpireTime).Seconds() }, ResourceName.Value(item.ResourceName)) item.ResourceName = security.WorkloadKeyCertResourceName // In case there are two calls to GenerateSecret at once, we don't want both to be concurrently registered if sc.cache.GetWorkload() != nil { @@ -666,6 +668,7 @@ func (sc *SecretManagerClient) registerSecret(item security.SecretItem) { } sc.cache.SetWorkload(&item) resourceLog(item.ResourceName).Debugf("scheduled certificate for rotation in %v", delay) + certExpirySeconds.ValueFrom(func() float64 { return time.Until(item.ExpireTime).Seconds() }, ResourceName.Value(item.ResourceName)) sc.queue.PushDelayed(func() error { // In case `UpdateConfigTrustBundle` called, it will resign workload cert. // Check if this is a stale scheduled rotating task. diff --git a/security/pkg/nodeagent/cache/secretcache_test.go b/security/pkg/nodeagent/cache/secretcache_test.go index 9a86ad0f75f5..0fd1bbc94bb6 100644 --- a/security/pkg/nodeagent/cache/secretcache_test.go +++ b/security/pkg/nodeagent/cache/secretcache_test.go @@ -28,6 +28,7 @@ import ( "istio.io/istio/pkg/file" "istio.io/istio/pkg/log" + "istio.io/istio/pkg/monitoring/monitortest" "istio.io/istio/pkg/security" "istio.io/istio/pkg/test/util/assert" "istio.io/istio/pkg/test/util/retry" @@ -58,6 +59,7 @@ func createCache(t *testing.T, caClient security.Client, notifyCb func(resourceN } func testWorkloadAgentGenerateSecret(t *testing.T, isUsingPluginProvider bool) { + mt := monitortest.New(t) fakeCACli, err := mock.NewMockCAClient(time.Hour, true) var got, want []byte if err != nil { @@ -105,6 +107,9 @@ func testWorkloadAgentGenerateSecret(t *testing.T, isUsingPluginProvider bool) { if got := sc.cache.GetRoot(); !bytes.Equal(got, want) { t.Errorf("Got unexpected root certificate. Got: %v\n want: %v", string(got), string(want)) } + + certDefaultTTL := time.Hour.Seconds() + mt.Assert(certExpirySeconds.Name(), map[string]string{"resource_name": "default"}, monitortest.LessThan(certDefaultTTL)) } type UpdateTracker struct { diff --git a/tests/integration/pilot/common/routing.go b/tests/integration/pilot/common/routing.go index ac70fffec317..22aa1389caac 100644 --- a/tests/integration/pilot/common/routing.go +++ b/tests/integration/pilot/common/routing.go @@ -18,6 +18,7 @@ package common import ( + "context" "fmt" "net/http" "net/netip" @@ -29,7 +30,12 @@ import ( "google.golang.org/grpc/codes" wrappers "google.golang.org/protobuf/types/known/wrapperspb" + corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "istio.io/api/annotation" "istio.io/istio/pilot/pkg/model" "istio.io/istio/pkg/config/host" "istio.io/istio/pkg/config/protocol" @@ -47,6 +53,7 @@ import ( "istio.io/istio/pkg/test/framework/components/istio/ingress" "istio.io/istio/pkg/test/framework/label" "istio.io/istio/pkg/test/scopes" + "istio.io/istio/pkg/test/util/retry" "istio.io/istio/pkg/test/util/tmpl" "istio.io/istio/pkg/util/sets" "istio.io/istio/tests/common/jwt" @@ -3853,6 +3860,67 @@ spec: } } +func testServiceEntryWithMultipleVIPsAndResolutionNone(t TrafficContext) { + if len(t.Apps.External.All) == 0 { + t.Skip("no external service instances") + } + + clusterIPs := createService(t, "external", t.Apps.External.All.NamespaceName(), "external", 2) + if len(clusterIPs) < 2 { + t.Errorf("failed to get 2 cluster IPs, got %v", clusterIPs) + } + + // Create CIDRs from IPs. We want to check if CIDR matching in a wildcard listener works. + var cidrs []string + for _, clusterIP := range clusterIPs { + if ip, err := netip.ParseAddr(clusterIP); err != nil { + t.Errorf("failed to parse cluster IP address '%s': %s", err) + } else if ip.Is4() { + cidrs = append(cidrs, fmt.Sprintf("%s/24", clusterIP)) + } else if ip.Is6() { + cidrs = append(cidrs, clusterIP) + } + } + + serviceEntry := tmpl.MustEvaluate(` +apiVersion: networking.istio.io/v1alpha3 +kind: ServiceEntry +metadata: + name: server-default-svc + namespace: {{.Namespace}} +spec: + exportTo: + - "." + hosts: + - server.default.svc + addresses: +{{ range $ip := .IPs }} + - "{{$ip}}" +{{ end }} + location: MESH_EXTERNAL + ports: + - number: 443 + name: tcp + protocol: TCP + resolution: NONE +`, map[string]any{"Namespace": t.Apps.A.NamespaceName(), "IPs": cidrs}) + + t.RunTraffic(TrafficTestCase{ + name: "send a request to one of the service entry VIPs", + config: serviceEntry, + opts: echo.CallOptions{ + // Use second IP address to make sure that Istio matches all CIDRs, not only the first one. + Address: clusterIPs[1], + Port: echo.Port{ + Protocol: protocol.HTTPS, + ServicePort: 443, + }, + Check: check.Status(http.StatusOK), + }, + call: t.Apps.A[0].CallOrFail, + }) +} + func destinationRule(app, mode string) string { return fmt.Sprintf(`apiVersion: networking.istio.io/v1beta1 kind: DestinationRule @@ -5022,3 +5090,59 @@ func LocationHeader(expected string) echo.Checker { "Location") }) } + +// createService creates additional Services for a given app to obtain routable VIPs inside the cluster. +func createService(t TrafficContext, name, ns, appLabelValue string, instances int) []string { + var clusterIPs []string + for i := 0; i < instances; i++ { + svcName := fmt.Sprintf("%s-vip-%d", name, i) + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: svcName, + Namespace: ns, + Annotations: map[string]string{ + // Export the service nowhere, so that no proxy will receive it or its VIP. + annotation.NetworkingExportTo.Name: "~", + }, + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "app": appLabelValue, + }, + Type: corev1.ServiceTypeClusterIP, + }, + } + for _, p := range ports.All() { + if p.ServicePort == echo.NoServicePort { + continue + } + svc.Spec.Ports = append(svc.Spec.Ports, corev1.ServicePort{ + Name: p.Name, + Protocol: corev1.ProtocolTCP, + Port: int32(p.ServicePort), + TargetPort: intstr.IntOrString{IntVal: int32(ports.HTTPS.WorkloadPort)}, + }) + } + _, err := t.Clusters().Default().Kube().CoreV1().Services(ns).Create(context.TODO(), svc, metav1.CreateOptions{}) + if err != nil && !kerrors.IsAlreadyExists(err) { + t.Errorf("failed to create service %s: %s", svc, err) + } + + // Wait until a ClusterIP has been assigned. + err = retry.UntilSuccess(func() error { + svc, err := t.Clusters().Default().Kube().CoreV1().Services(ns).Get(context.TODO(), svcName, metav1.GetOptions{}) + if err != nil { + return err + } + if len(svc.Spec.ClusterIPs) == 0 { + return fmt.Errorf("service VIP not set for service") + } + clusterIPs = append(clusterIPs, svc.Spec.ClusterIPs...) + return nil + }, retry.Timeout(10*time.Second)) + if err != nil { + t.Errorf("failed to assign ClusterIP for %s service: %s", svcName, err) + } + } + return clusterIPs +} diff --git a/tests/integration/pilot/common/traffic.go b/tests/integration/pilot/common/traffic.go index 5d5601409c33..e47919f1c425 100644 --- a/tests/integration/pilot/common/traffic.go +++ b/tests/integration/pilot/common/traffic.go @@ -297,6 +297,7 @@ func RunAllTrafficTests(t framework.TestContext, i istio.Instance, apps deployme RunCase("vm", VMTestCases(apps.VM)) RunSkipAmbient("dns", DNSTestCases, "https://github.com/istio/istio/issues/48614") RunCase("externalservice", TestExternalService) + RunSkipAmbient("service-entry-vips-resolution-none", testServiceEntryWithMultipleVIPsAndResolutionNone, "") } func ExpectString(got, expected, help string) error { diff --git a/tests/integration/pilot/dns_auto_allocation_test.go b/tests/integration/pilot/dns_auto_allocation_test.go new file mode 100644 index 000000000000..568b94ed4319 --- /dev/null +++ b/tests/integration/pilot/dns_auto_allocation_test.go @@ -0,0 +1,104 @@ +//go:build integ +// +build integ + +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pilot + +import ( + "fmt" + "testing" + "time" + + "istio.io/istio/pkg/config/protocol" + "istio.io/istio/pkg/test/framework" + "istio.io/istio/pkg/test/framework/components/echo" + "istio.io/istio/pkg/test/framework/components/echo/check" + "istio.io/istio/pkg/test/framework/components/echo/deployment" + "istio.io/istio/pkg/test/framework/components/namespace" + "istio.io/istio/pkg/test/util/retry" +) + +func TestDNSAutoAllocation(t *testing.T) { + framework. + NewTest(t). + Run(func(t framework.TestContext) { + ns := namespace.NewOrFail(t, t, namespace.Config{ + Prefix: "dns-auto-allocation", + Inject: true, + }) + cfg := `apiVersion: networking.istio.io/v1beta1 +kind: ProxyConfig +metadata: + name: enable-dns-auto-allocation +spec: + selector: + matchLabels: + app: a + environmentVariables: + ISTIO_META_DNS_CAPTURE: "true" + ISTIO_META_DNS_AUTO_ALLOCATE: "true" +--- +apiVersion: networking.istio.io/v1alpha3 +kind: ServiceEntry +metadata: + name: fake-local +spec: + hosts: + - "fake.local" + resolution: DNS + ports: + - number: 80 + name: http + protocol: HTTP +--- +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: route-to-b +spec: + hosts: + - fake.local + http: + - route: + - destination: + host: b.{{.echoNamespace}}.svc.cluster.local +` + t.ConfigIstio().Eval(ns.Name(), map[string]string{"echoNamespace": apps.Namespace.Name()}, cfg).ApplyOrFail(t) + instances := deployment.New(t, t.AllClusters().Configs()...).WithConfig(echo.Config{Namespace: ns, Service: "a"}).BuildOrFail(t) + + retry.UntilSuccessOrFail(t, func() error { + _, err := instances[0].Call(echo.CallOptions{ + Address: "fake.local", + Port: echo.Port{ + Name: "http", + ServicePort: 80, + Protocol: protocol.HTTP, + }, + Check: check.OK(), + }) + if err == nil { + return nil + } + // trigger injection in case of delay of ProxyConfig propagation + for _, i := range instances { + if err := i.Restart(); err != nil { + return fmt.Errorf("failed to restart echo instance: %v", err) + } + } + return nil + }, retry.Timeout(time.Second*30)) + }) +}