istio · istio-testing · Jul 17, 2024 · Jul 9, 2024 · Jul 9, 2024 · Jul 9, 2024
@@ -1261,22  1261,26 @@ 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 {
-	if (features.EnableDualStack || features.EnableAmbient) && node.Metadata != nil && node.Metadata.ClusterID != "" {
 	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
 		}
@@ -1298,6  1302,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

@@ -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,118 @@ func TestParseSubsetKeyHostname(t *testing.T) {
 		})
 	}
 }
 
 func TestGetAllAddresses(t *testing.T) {
 	tests := []struct {
 		name                   string
 		service                *Service
 		ipMode                 IPMode
 		dualStackEnabled       bool
 		ambientEnabled         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"},
 		},
 	}
 	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}
 			addresses := tc.service.GetAllAddressesForProxy(proxy)
 			assert.Equal(t, addresses, tc.expectedAddresses)
 			extraAddresses := tc.service.GetExtraAddressesForProxy(proxy)
 			assert.Equal(t, extraAddresses, tc.expectedExtraAddresses)
 		})
 	}
 }
@@ -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 {
@@ -817,9  817,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...)
 				}
 			}
 		}
@@ -1058,7  1057,7 @@ type outboundListenerOpts struct {
 	proxy *model.Proxy
 
 	bind listenerBinding
-	cidr string
 	cidr []string
 
 	port    *model.Port
 	service *model.Service

@@ -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, "",

@@ -47,8  47,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
@@ -182,21  182,23 @@ func convertServices(cfg config.Config, clusterID cluster.ID) []*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}})
 		}
 	}
 
@@ -210,7  212,7 @@ func convertServices(cfg config.Config, clusterID cluster.ID) []*model.Service {
 			CreationTime:   creationTime,
 			MeshExternal:   serviceEntry.Location == networking.ServiceEntry_MESH_EXTERNAL,
 			Hostname:       host.Name(ha.host),
-			DefaultAddress: ha.address,
 			DefaultAddress: ha.addresses[0],
 			Ports:          svcPorts,
 			Resolution:     resolution,
 			Attributes: model.ServiceAttributes{
@@ -223,20  225,10 @@ func convertServices(cfg config.Config, clusterID cluster.ID) []*model.Service {
 				LabelSelectors:         labelSelectors,
 			},
 			ServiceAccounts: serviceEntry.SubjectAltNames,
-		}
-		addresses := serviceEntry.Addresses
-		if len(addresses) == 0 {
-			addresses = []string{constants.UnspecifiedIP}
-		}
-		// This logic ensures backward compatibility for non-ambient proxies.
-		// It makes sure that the default address is always the first VIP in the list, so there is no difference
-		// between using DefaultAddress or ClusterVIPs[0] to create a listener.
-		notDefaultAddresses := sets.New[string](addresses...).Delete(ha.address)
-		addresses = []string{ha.address}
-		addresses = append(addresses, sets.SortedList(notDefaultAddresses)...)
-		svc.ClusterVIPs = model.AddressMap{
-			Addresses: map[cluster.ID][]string{
-				clusterID: addresses,
 			ClusterVIPs: model.AddressMap{
 				Addresses: map[cluster.ID][]string{
 					clusterID: ha.addresses,
 				},
 			},
 		}
 		out = append(out, svc)
@@ -436,7  428,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)