Better ExternalName support (istio#46332)

* Better ExternalName support * Off by default * fixes * fix tests * Address Keith's comments * fix mc * fix mc * format * Add comment * Fix merge * Add benchmark
WillardHu · Oct 27, 2023 · 65d15b6 · 65d15b6
1 parent 823b826
commit 65d15b6
Show file tree

Hide file tree

Showing 25 changed files with 812 additions and 56 deletions.
diff --git a/pilot/pkg/features/pilot.go b/pilot/pkg/features/pilot.go
@@ -654,6 +654,14 @@ var (
 	EnableNativeSidecars = env.Register("ENABLE_NATIVE_SIDECARS", false,
 		"If set, used Kubernetes native Sidecar container support. Requires SidecarContainer feature flag.")
 
+	EnableExternalNameAlias = env.Register("ENABLE_EXTERNAL_NAME_ALIAS", false,
+		"If enabled, ExternalName Services will be treated as simple aliases: anywhere where we would match the concrete service, "+
+			"we also match the ExternalName. In general, this mirrors Kubernetes behavior more closely. However, it means that policies (routes and DestinationRule) "+
+			"cannot be applied to the ExternalName service. "+
+			"If disabled, ExternalName behaves in fairly unexpected manner. Port matters, while it does not in Kubernetes. If it is a TCP port, "+
+			"all traffic on that port will be matched, which can have disastrous consequences. Additionally, the destination is seen as an opaque destination; "+
+			"even if it is another service in the mesh, policies such as mTLS and load balancing will not be used when connecting to it.").Get()
+
 	// This is an experimental feature flag, can be removed once it became stable, and should introduced to Telemetry API.
 	MetricRotationInterval = env.Register("METRIC_ROTATION_INTERVAL", 0*time.Second,
 		"Metric scope rotation interval, set to 0 to disable the metric scope rotation").Get()

diff --git a/pilot/pkg/model/push_context.go b/pilot/pkg/model/push_context.go
@@ -15,6 +15,7 @@
 package model
 
 import (
+	"cmp"
 	"encoding/json"
 	"fmt"
 	"math"
@@ -1244,7 +1245,7 @@ func (ps *PushContext) InitContext(env *Environment, oldPushContext *PushContext
 }
 
 func (ps *PushContext) createNewContext(env *Environment) error {
-	ps.initServiceRegistry(env)
+	ps.initServiceRegistry(env, nil)
 
 	if err := ps.initKubernetesGateways(env); err != nil {
 		return err
@@ -1317,7 +1318,7 @@ func (ps *PushContext) updateContext(
 
 	if servicesChanged {
 		// Services have changed. initialize service registry
-		ps.initServiceRegistry(env)
+		ps.initServiceRegistry(env, pushReq.ConfigsUpdated)
 	} else {
 		// make sure we copy over things that would be generated in initServiceRegistry
 		ps.ServiceIndex = oldPushContext.ServiceIndex
@@ -1403,9 +1404,13 @@ func (ps *PushContext) updateContext(
 
 // Caches list of services in the registry, and creates a map
 // of hostname to service
-func (ps *PushContext) initServiceRegistry(env *Environment) {
+func (ps *PushContext) initServiceRegistry(env *Environment, configsUpdate sets.Set[ConfigKey]) {
 	// Sort the services in order of creation.
 	allServices := SortServicesByCreationTime(env.Services())
+	if features.EnableExternalNameAlias {
+		resolveServiceAliases(allServices, configsUpdate)
+	}
+
 	for _, s := range allServices {
 		portMap := map[string]int{}
 		for _, port := range s.Ports {
@@ -1458,6 +1463,108 @@ func (ps *PushContext) initServiceRegistry(env *Environment) {
 	ps.initServiceAccounts(env, allServices)
 }
 
+// resolveServiceAliases sets the Aliases attributes on all services. The incoming Service's will just have AliasFor set,
+// but in our usage we often need the opposite: for a given service, what are all the aliases?
+// resolveServiceAliases walks this 'graph' of services and updates the Alias field in-place.
+func resolveServiceAliases(allServices []*Service, configsUpdated sets.Set[ConfigKey]) {
+	// rawAlias builds a map of Service -> AliasFor. So this will be ExternalName -> Service.
+	// In an edge case, we can have ExternalName -> ExternalName; we resolve that below.
+	rawAlias := map[NamespacedHostname]host.Name{}
+	for _, s := range allServices {
+		if s.Resolution != Alias {
+			continue
+		}
+		nh := NamespacedHostname{
+			Hostname:  s.Hostname,
+			Namespace: s.Attributes.Namespace,
+		}
+		rawAlias[nh] = host.Name(s.Attributes.K8sAttributes.ExternalName)
+	}
+
+	// unnamespacedRawAlias is like rawAlias but without namespaces.
+	// This is because an `ExternalName` isn't namespaced. If there is a conflict, the behavior is undefined.
+	// This is split from above as a minor optimization to right-size the map
+	unnamespacedRawAlias := make(map[host.Name]host.Name, len(rawAlias))
+	for k, v := range rawAlias {
+		unnamespacedRawAlias[k.Hostname] = v
+	}
+
+	// resolvedAliases builds a map of Alias -> Concrete, fully resolving through multiple hops.
+	// Ex: Alias1 -> Alias2 -> Concrete will flatten to Alias1 -> Concrete.
+	resolvedAliases := make(map[NamespacedHostname]host.Name, len(rawAlias))
+	for alias, referencedService := range rawAlias {
+		// referencedService may be another alias or a concrete service.
+		if _, f := unnamespacedRawAlias[referencedService]; !f {
+			// Common case: alias pointing to a concrete service
+			resolvedAliases[alias] = referencedService
+			continue
+		}
+		// Otherwise, we need to traverse the alias "graph".
+		// In an obscure edge case, a user could make a loop, so we will need to handle that.
+		seen := sets.New(alias.Hostname, referencedService)
+		for {
+			n, f := unnamespacedRawAlias[referencedService]
+			if !f {
+				// The destination we are pointing to is not an alias, so this is the terminal step
+				resolvedAliases[alias] = referencedService
+				break
+			}
+			if seen.InsertContains(n) {
+				// We did a loop!
+				// Kubernetes will make these NXDomain, so we can just treat it like it doesn't exist at all
+				break
+			}
+			referencedService = n
+		}
+	}
+
+	// aliasesForService builds a map of Concrete -> []Aliases
+	// This basically reverses our resolvedAliased map, which is Alias -> Concrete,
+	aliasesForService := map[host.Name][]NamespacedHostname{}
+	for alias, concrete := range resolvedAliases {
+		aliasesForService[concrete] = append(aliasesForService[concrete], alias)
+
+		// We also need to update configsUpdated, such that any "alias" updated also marks the concrete service as updated.
+		aliasKey := ConfigKey{
+			Kind:      kind.ServiceEntry,
+			Name:      alias.Hostname.String(),
+			Namespace: alias.Namespace,
+		}
+		// Alias. We should mark all the concrete services as updated as well.
+		if configsUpdated.Contains(aliasKey) {
+			// We only have the hostname, but we need the namespace...
+			for _, svc := range allServices {
+				if svc.Hostname == concrete {
+					configsUpdated.Insert(ConfigKey{
+						Kind:      kind.ServiceEntry,
+						Name:      concrete.String(),
+						Namespace: svc.Attributes.Namespace,
+					})
+				}
+			}
+		}
+	}
+	// Sort aliases so order is deterministic.
+	for _, v := range aliasesForService {
+		slices.SortFunc(v, func(a, b NamespacedHostname) int {
+			if r := cmp.Compare(a.Namespace, b.Namespace); r != 0 {
+				return r
+			}
+			return cmp.Compare(a.Hostname, b.Hostname)
+		})
+	}
+
+	// Finally, we can traverse all services and update the ones that have aliases
+	for i, s := range allServices {
+		if aliases, f := aliasesForService[s.Hostname]; f {
+			// This service has an alias; set it. We need to make a copy since the underlying Service is shared
+			s = s.DeepCopy()
+			s.Attributes.Aliases = aliases
+			allServices[i] = s
+		}
+	}
+}
+
 // SortServicesByCreationTime sorts the list of services in ascending order by their creation time (if available).
 func SortServicesByCreationTime(services []*Service) []*Service {
 	sort.SliceStable(services, func(i, j int) bool {

diff --git a/pilot/pkg/model/push_context_test.go b/pilot/pkg/model/push_context_test.go
@@ -2805,7 +2805,7 @@ func TestServiceWithExportTo(t *testing.T) {
 		services: []*Service{svc1, svc2, svc3, svc4},
 	}
 	ps.initDefaultExportMaps()
-	ps.initServiceRegistry(env)
+	ps.initServiceRegistry(env, nil)
 
 	cases := []struct {
 		proxyNs   string
@@ -3012,3 +3012,98 @@ func (l *localServiceDiscovery) NetworkGateways() []NetworkGateway {
 func (l *localServiceDiscovery) MCSServices() []MCSServiceInfo {
 	return nil
 }
+
+func TestResolveServiceAliases(t *testing.T) {
+	type service struct {
+		Name         host.Name
+		Aliases      host.Names
+		ExternalName string
+	}
+	tests := []struct {
+		name   string
+		input  []service
+		output []service
+	}{
+		{
+			name:   "no aliases",
+			input:  []service{{Name: "test"}},
+			output: []service{{Name: "test"}},
+		},
+		{
+			name: "simple alias",
+			input: []service{
+				{Name: "concrete"},
+				{Name: "alias", ExternalName: "concrete"},
+			},
+			output: []service{
+				{Name: "concrete", Aliases: host.Names{"alias"}},
+				{Name: "alias", ExternalName: "concrete"},
+			},
+		},
+		{
+			name: "multiple alias",
+			input: []service{
+				{Name: "concrete"},
+				{Name: "alias1", ExternalName: "concrete"},
+				{Name: "alias2", ExternalName: "concrete"},
+			},
+			output: []service{
+				{Name: "concrete", Aliases: host.Names{"alias1", "alias2"}},
+				{Name: "alias1", ExternalName: "concrete"},
+				{Name: "alias2", ExternalName: "concrete"},
+			},
+		},
+		{
+			name: "chained alias",
+			input: []service{
+				{Name: "concrete"},
+				{Name: "alias1", ExternalName: "alias2"},
+				{Name: "alias2", ExternalName: "concrete"},
+			},
+			output: []service{
+				{Name: "concrete", Aliases: host.Names{"alias1", "alias2"}},
+				{Name: "alias1", ExternalName: "alias2"},
+				{Name: "alias2", ExternalName: "concrete"},
+			},
+		},
+		{
+			name: "looping alias",
+			input: []service{
+				{Name: "alias1", ExternalName: "alias2"},
+				{Name: "alias2", ExternalName: "alias1"},
+			},
+			output: []service{
+				{Name: "alias1", ExternalName: "alias2"},
+				{Name: "alias2", ExternalName: "alias1"},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			inps := slices.Map(tt.input, func(e service) *Service {
+				resolution := ClientSideLB
+				if e.ExternalName != "" {
+					resolution = Alias
+				}
+				return &Service{
+					Resolution: resolution,
+					Attributes: ServiceAttributes{
+						K8sAttributes: K8sAttributes{ExternalName: e.ExternalName},
+					},
+					Hostname: e.Name,
+				}
+			})
+			resolveServiceAliases(inps, nil)
+			out := slices.Map(inps, func(e *Service) service {
+				return service{
+					Name: e.Hostname,
+					Aliases: slices.Map(e.Attributes.Aliases, func(e NamespacedHostname) host.Name {
+						return e.Hostname
+					}),
+					ExternalName: e.Attributes.K8sAttributes.ExternalName,
+				}
+			})
+			assert.Equal(t, tt.output, out)
+		})
+	}
+}
diff --git a/pilot/pkg/model/service.go b/pilot/pkg/model/service.go
@@ -145,6 +145,8 @@ const (
 	Passthrough
 	// DNSRoundRobinLB implies that the proxy will resolve a DNS address and forward to the resolved address
 	DNSRoundRobinLB
+	// Alias defines a Service that is an alias for another.
+	Alias
 )
 
 // String converts Resolution in to String.
@@ -679,6 +681,11 @@ type ServiceAttributes struct {
 	// Applicable to both Kubernetes and ServiceEntries.
 	LabelSelectors map[string]string
 
+	// Aliases is the resolved set of aliases for this service. This is computed based on a global view of all Service's `AliasFor`
+	// fields.
+	// For example, if I had two Services with `externalName: foo`, "a" and "b", then the "foo" service would have Aliases=[a,b].
+	Aliases []NamespacedHostname
+
 	// For Kubernetes platform
 
 	// ClusterExternalAddresses is a mapping between a cluster name and the external
@@ -697,6 +704,11 @@ type ServiceAttributes struct {
 	K8sAttributes
 }
 
+type NamespacedHostname struct {
+	Hostname  host.Name
+	Namespace string
+}
+
 type K8sAttributes struct {
 	// Type holds the value of the corev1.Type of the Kubernetes service
 	// spec.Type
@@ -751,6 +763,8 @@ func (s *ServiceAttributes) DeepCopy() ServiceAttributes {
 		}
 	}
 
+	out.Aliases = slices.Clone(s.Aliases)
+
 	// AddressMap contains a mutex, which is safe to return a copy in this case.
 	// nolint: govet
 	return out
@@ -777,6 +791,10 @@ func (s *ServiceAttributes) Equals(other *ServiceAttributes) bool {
 		return false
 	}
 
+	if !slices.Equal(s.Aliases, other.Aliases) {
+		return false
+	}
+
 	if s.ClusterExternalAddresses.Len() != other.ClusterExternalAddresses.Len() {
 		return false
 	}