Fix private destination rules in root namespace (#25962)

howardjohn · web-flow · commit a497e861b355 · 2020-08-14T08:57:41.000-07:00
* Fix private destination rules in root namespace Fixes #25253 * Move ordering * Fix inverted logic * Fix format
diff --git a/pilot/pkg/model/push_context.go b/pilot/pkg/model/push_context.go
@@ -100,6 +100,7 @@ type PushContext struct {
 	//  exportedDestRulesByNamespace: all dest rules pertaining to a service exported by a namespace
 	namespaceLocalDestRules      map[string]*processedDestRules
 	exportedDestRulesByNamespace map[string]*processedDestRules
+	rootNamespaceLocalDestRules  *processedDestRules
 
 	// clusterLocalHosts extracted from the MeshConfig
 	clusterLocalHosts host.Names
@@ -702,6 +703,14 @@ func (ps *PushContext) DestinationRule(proxy *Proxy, service *Service) *Config {
 				return ps.namespaceLocalDestRules[proxy.ConfigNamespace].destRule[hostname]
 			}
 		}
+	} else {
+		// If this is a namespace local DR in the same namespace, this must be meant for this proxy, so we do not
+		// need to worry about overriding other DRs with *.local type rules here. If we ignore this, then exportTo=. in
+		// root namespace would always be ignored
+		if hostname, ok := MostSpecificHostMatch(service.Hostname,
+			ps.rootNamespaceLocalDestRules.hosts); ok {
+			return ps.rootNamespaceLocalDestRules.destRule[hostname]
+		}
 	}
 
 	// 2. select destination rule from service namespace
@@ -729,7 +738,6 @@ func (ps *PushContext) DestinationRule(proxy *Proxy, service *Service) *Config {
 
 	// 4. if no public/private rule in calling proxy's namespace matched, and no public rule in the
 	// target service's namespace matched, search for any exported destination rule in the config root namespace
-	// NOTE: This does mean that we are effectively ignoring private dest rules in the config root namespace
 	if out := ps.getExportedDestinationRuleFromNamespace(ps.Mesh.RootNamespace, service.Hostname, proxy.ConfigNamespace); out != nil {
 		return out
 	}
@@ -927,6 +935,7 @@ func (ps *PushContext) updateContext(
 	} else {
 		ps.namespaceLocalDestRules = oldPushContext.namespaceLocalDestRules
 		ps.exportedDestRulesByNamespace = oldPushContext.exportedDestRulesByNamespace
+		ps.rootNamespaceLocalDestRules = oldPushContext.rootNamespaceLocalDestRules
 	}
 
 	if authnChanged {
@@ -1303,6 +1312,14 @@ func (ps *PushContext) initDestinationRules(env *Environment) error {
 	return nil
 }
 
+func newProcessedDestRules() *processedDestRules {
+	return &processedDestRules{
+		hosts:    make([]host.Name, 0),
+		exportTo: map[host.Name]map[visibility.Instance]bool{},
+		destRule: map[host.Name]*Config{},
+	}
+}
+
 // SetDestinationRules is updates internal structures using a set of configs.
 // Split out of DestinationRule expensive conversions, computed once per push.
 // This also allows tests to inject a config without having the mock.
@@ -1313,6 +1330,7 @@ func (ps *PushContext) SetDestinationRules(configs []Config) {
 	sortConfigByCreationTime(configs)
 	namespaceLocalDestRules := make(map[string]*processedDestRules)
 	exportedDestRulesByNamespace := make(map[string]*processedDestRules)
+	rootNamespaceLocalDestRules := newProcessedDestRules()
 
 	for i := range configs {
 		rule := configs[i].Spec.(*networking.DestinationRule)
@@ -1329,11 +1347,7 @@ func (ps *PushContext) SetDestinationRules(configs []Config) {
 			// a proxy from this namespace will first look here for the destination rule for a given service
 			// This pool consists of both public/private destination rules.
 			if _, exist := namespaceLocalDestRules[configs[i].Namespace]; !exist {
-				namespaceLocalDestRules[configs[i].Namespace] = &processedDestRules{
-					hosts:    make([]host.Name, 0),
-					exportTo: map[host.Name]map[visibility.Instance]bool{},
-					destRule: map[host.Name]*Config{},
-				}
+				namespaceLocalDestRules[configs[i].Namespace] = newProcessedDestRules()
 			}
 			// Merge this destination rule with any public/private dest rules for same host in the same namespace
 			// If there are no duplicates, the dest rule will be added to the list
@@ -1351,15 +1365,14 @@ func (ps *PushContext) SetDestinationRules(configs []Config) {
 
 		if !isPrivateOnly {
 			if _, exist := exportedDestRulesByNamespace[configs[i].Namespace]; !exist {
-				exportedDestRulesByNamespace[configs[i].Namespace] = &processedDestRules{
-					hosts:    make([]host.Name, 0),
-					exportTo: map[host.Name]map[visibility.Instance]bool{},
-					destRule: map[host.Name]*Config{},
-				}
+				exportedDestRulesByNamespace[configs[i].Namespace] = newProcessedDestRules()
 			}
 			// Merge this destination rule with any other exported dest rule for the same host in the same namespace
 			// If there are no duplicates, the dest rule will be added to the list
 			ps.mergeDestinationRule(exportedDestRulesByNamespace[configs[i].Namespace], configs[i], exportToMap)
+		} else if configs[i].Namespace == ps.Mesh.RootNamespace {
+			// Keep track of private root namespace destination rules
+			ps.mergeDestinationRule(rootNamespaceLocalDestRules, configs[i], exportToMap)
 		}
 	}
 
@@ -1374,6 +1387,7 @@ func (ps *PushContext) SetDestinationRules(configs []Config) {
 
 	ps.namespaceLocalDestRules = namespaceLocalDestRules
 	ps.exportedDestRulesByNamespace = exportedDestRulesByNamespace
+	ps.rootNamespaceLocalDestRules = rootNamespaceLocalDestRules
 }
 
 func (ps *PushContext) initAuthorizationPolicies(env *Environment) error {
diff --git a/pilot/pkg/model/push_context_test.go b/pilot/pkg/model/push_context_test.go
@@ -520,8 +520,26 @@ func TestSetDestinationRuleWithExportTo(t *testing.T) {
 			},
 		},
 	}
+	destinationRuleRootNamespaceLocal := Config{
+		ConfigMeta: ConfigMeta{
+			Name:      "rule1",
+			Namespace: "istio-system",
+		},
+		Spec: &networking.DestinationRule{
+			Host:     testhost,
+			ExportTo: []string{"."},
+			Subsets: []*networking.Subset{
+				{
+					Name: "subset9",
+				},
+				{
+					Name: "subset10",
+				},
+			},
+		},
+	}
 	ps.SetDestinationRules([]Config{destinationRuleNamespace1, destinationRuleNamespace2,
-		destinationRuleNamespace3, destinationRuleRootNamespace})
+		destinationRuleNamespace3, destinationRuleRootNamespace, destinationRuleRootNamespaceLocal})
 	cases := []struct {
 		proxyNs     string
 		serviceNs   string
@@ -567,21 +585,33 @@ func TestSetDestinationRuleWithExportTo(t *testing.T) {
 			serviceNs:   "random",
 			wantSubsets: []string{"subset5", "subset6"},
 		},
+		{
+			proxyNs:     "istio-system",
+			serviceNs:   "random",
+			wantSubsets: []string{"subset9", "subset10"},
+		},
+		{
+			proxyNs:     "istio-system",
+			serviceNs:   "istio-system",
+			wantSubsets: []string{"subset9", "subset10"},
+		},
 	}
 	for _, tt := range cases {
-		destRuleConfig := ps.DestinationRule(&Proxy{ConfigNamespace: tt.proxyNs},
-			&Service{Hostname: host.Name(testhost), Attributes: ServiceAttributes{Namespace: tt.serviceNs}})
-		if destRuleConfig == nil {
-			t.Fatalf("proxy in %s namespace: dest rule is nil, expected subsets %+v", tt.proxyNs, tt.wantSubsets)
-		}
-		destRule := destRuleConfig.Spec.(*networking.DestinationRule)
-		var gotSubsets []string
-		for _, ss := range destRule.Subsets {
-			gotSubsets = append(gotSubsets, ss.Name)
-		}
-		if !reflect.DeepEqual(gotSubsets, tt.wantSubsets) {
-			t.Fatalf("proxy in %s namespace: want %+v, got %+v", tt.proxyNs, tt.wantSubsets, gotSubsets)
-		}
+		t.Run(fmt.Sprintf("%s-%s", tt.proxyNs, tt.serviceNs), func(t *testing.T) {
+			destRuleConfig := ps.DestinationRule(&Proxy{ConfigNamespace: tt.proxyNs},
+				&Service{Hostname: host.Name(testhost), Attributes: ServiceAttributes{Namespace: tt.serviceNs}})
+			if destRuleConfig == nil {
+				t.Fatalf("proxy in %s namespace: dest rule is nil, expected subsets %+v", tt.proxyNs, tt.wantSubsets)
+			}
+			destRule := destRuleConfig.Spec.(*networking.DestinationRule)
+			var gotSubsets []string
+			for _, ss := range destRule.Subsets {
+				gotSubsets = append(gotSubsets, ss.Name)
+			}
+			if !reflect.DeepEqual(gotSubsets, tt.wantSubsets) {
+				t.Fatalf("want %+v, got %+v", tt.wantSubsets, gotSubsets)
+			}
+		})
 	}
 }
 
