deep clone service instance when sidecar has ingress listeners (istio…

…#19871) * deep clone service instance Signed-off-by: Rama Chavali <[email protected]> * refactor findServiceInstances method Signed-off-by: Rama Chavali <[email protected]>
howardjohn · Jan 1, 2020 · ef68aa5 · ef68aa5
1 parent ad4516a
commit ef68aa5
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 52 deletions.
diff --git a/pilot/pkg/model/service.go b/pilot/pkg/model/service.go
@@ -249,6 +249,19 @@ func (instance *ServiceInstance) GetLocality() string {
 	return GetLocalityOrDefault(instance.Endpoint.Labels[LocalityLabel], instance.Endpoint.Locality)
 }
 
+// DeepCopy creates a copy of ServiceInstance.
+func (instance *ServiceInstance) DeepCopy() *ServiceInstance {
+	return &ServiceInstance{
+		Service:  instance.Service.DeepCopy(),
+		Endpoint: instance.Endpoint.DeepCopy(),
+		ServicePort: &Port{
+			Name:     instance.ServicePort.Name,
+			Port:     instance.ServicePort.Port,
+			Protocol: instance.ServicePort.Protocol,
+		},
+	}
+}
+
 // GetLocalityOrDefault returns the locality from the supplied label, or falls back to
 // the supplied default locality if the supplied label is empty. Because Kubernetes
 // labels don't support `/`, we replace "." with "/" in the supplied label as a workaround.
@@ -665,6 +678,11 @@ func (s *Service) DeepCopy() *Service {
 	}
 }
 
+// DeepCopy creates a clone of IstioEndpoint.
+func (ep *IstioEndpoint) DeepCopy() *IstioEndpoint {
+	return copyInternal(ep).(*IstioEndpoint)
+}
+
 func copyInternal(v interface{}) interface{} {
 	copied, err := copystructure.Copy(v)
 	if err != nil {

diff --git a/pilot/pkg/networking/core/v1alpha3/cluster.go b/pilot/pkg/networking/core/v1alpha3/cluster.go
@@ -571,7 +571,6 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(proxy *model.Proxy,
 		}
 	} else {
 		rule := sidecarScope.Config.Spec.(*networking.Sidecar)
-		sidecarScopeID := sidecarScope.Config.Name + "." + sidecarScope.Config.Namespace
 		for _, ingressListener := range rule.Ingress {
 			// LDS would have setup the inbound clusters
 			// as inbound|portNumber|portName|Hostname[or]SidecarScopeID
@@ -604,29 +603,9 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(proxy *model.Proxy,
 			}
 
 			// Find the service instance that corresponds to this ingress listener by looking
-			// for a service instance that either matches this ingress port as this will allow us
+			// for a service instance that matches this ingress port as this will allow us
 			// to generate the right cluster name that LDS expects inbound|portNumber|portName|Hostname
-			instance := configgen.findServiceInstanceForIngressListener(instances, ingressListener)
-
-			if instance == nil {
-				// We didn't find a matching instance. Create a dummy one because we need the right
-				// params to generate the right cluster name. LDS would have setup the cluster as
-				// as inbound|portNumber|portName|SidecarScopeID
-				attrs := model.ServiceAttributes{
-					Name:      sidecarScope.Config.Name,
-					Namespace: sidecarScope.Config.Namespace,
-				}
-				instance = &model.ServiceInstance{
-					Service: &model.Service{
-						Hostname:   host.Name(sidecarScopeID),
-						Attributes: attrs,
-					},
-					Endpoint: &model.IstioEndpoint{
-						Attributes: attrs,
-					},
-				}
-			}
-
+			instance := configgen.findOrCreateServiceInstance(instances, ingressListener, sidecarScope.Config.Name, sidecarScope.Config.Namespace)
 			instance.Endpoint.Family = endpointFamily
 			instance.Endpoint.Address = endpointAddress
 			instance.ServicePort = listenPort
@@ -648,17 +627,30 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(proxy *model.Proxy,
 	return clusters
 }
 
-func (configgen *ConfigGeneratorImpl) findServiceInstanceForIngressListener(instances []*model.ServiceInstance,
-	ingressListener *networking.IstioIngressListener) *model.ServiceInstance {
-	// Search by port
+func (configgen *ConfigGeneratorImpl) findOrCreateServiceInstance(instances []*model.ServiceInstance,
+	ingressListener *networking.IstioIngressListener, sidecar string, sidecarns string) *model.ServiceInstance {
 	for _, realInstance := range instances {
 		if realInstance.Endpoint.EndpointPort == ingressListener.Port.Number {
-			instanceCopy := *realInstance
-			return &instanceCopy
+			// We need to create a copy of the instance, as it is modified later while building clusters/listeners.
+			return realInstance.DeepCopy()
 		}
 	}
-
-	return nil
+	// We didn't find a matching instance. Create a dummy one because we need the right
+	// params to generate the right cluster name i.e. inbound|portNumber|portName|SidecarScopeID - which is uniformly generated by LDS/CDS.
+	attrs := model.ServiceAttributes{
+		Name: sidecar,
+		// This will ensure that the right AuthN policies are selected
+		Namespace: sidecarns,
+	}
+	return &model.ServiceInstance{
+		Service: &model.Service{
+			Hostname:   host.Name(sidecar + "." + sidecarns),
+			Attributes: attrs,
+		},
+		Endpoint: &model.IstioEndpoint{
+			Attributes: attrs,
+		},
+	}
 }
 
 func (configgen *ConfigGeneratorImpl) buildInboundClusterForPortOrUDS(pluginParams *plugin.InputParams) *apiv2.Cluster {

diff --git a/pilot/pkg/networking/core/v1alpha3/cluster_test.go b/pilot/pkg/networking/core/v1alpha3/cluster_test.go
@@ -32,6 +32,7 @@ import (
 
 	authn "istio.io/api/authentication/v1alpha1"
 	meshconfig "istio.io/api/mesh/v1alpha1"
+	"istio.io/api/networking/v1alpha3"
 	networking "istio.io/api/networking/v1alpha3"
 
 	"istio.io/istio/pilot/pkg/features"
@@ -1338,6 +1339,55 @@ func TestBuildLocalityLbEndpoints(t *testing.T) {
 	}
 }
 
+func TestFindServiceInstanceForIngressListener(t *testing.T) {
+	servicePort := &model.Port{
+		Name:     "default",
+		Port:     7443,
+		Protocol: protocol.HTTP,
+	}
+	service := &model.Service{
+		Hostname:    host.Name("*.example.org"),
+		Address:     "1.1.1.1",
+		ClusterVIPs: make(map[string]string),
+		Ports:       model.PortList{servicePort},
+		Resolution:  model.ClientSideLB,
+	}
+
+	instances := []*model.ServiceInstance{
+		{
+			Service:     service,
+			ServicePort: servicePort,
+			Endpoint: &model.IstioEndpoint{
+				Address:      "192.168.1.1",
+				EndpointPort: 7443,
+				Locality:     "region1/zone1/subzone1",
+				LbWeight:     30,
+			},
+		},
+	}
+
+	ingress := &networking.IstioIngressListener{
+		CaptureMode:     v1alpha3.CaptureMode_NONE,
+		DefaultEndpoint: "127.0.0.1:7020",
+		Port: &v1alpha3.Port{
+			Number:   7443,
+			Name:     "grpc-core",
+			Protocol: "GRPC",
+		},
+	}
+	configgen := NewConfigGenerator([]plugin.Plugin{})
+	instance := configgen.findOrCreateServiceInstance(instances, ingress, "sidecar", "sidecarns")
+	if instance == nil || instance.Service.Hostname.Matches("sidecar.sidecarns") {
+		t.Fatal("Expected to return a valid instance, but got nil/default instance")
+	}
+	if instance == instances[0] {
+		t.Fatal("Expected to return a copy of instance, but got the same instance")
+	}
+	if !reflect.DeepEqual(instance, instances[0]) {
+		t.Fatal("Expected returned copy of instance to be equal, but they are different")
+	}
+}
+
 func TestClusterDiscoveryTypeAndLbPolicyRoundRobin(t *testing.T) {
 	g := NewGomegaWithT(t)
 

diff --git a/pilot/pkg/networking/core/v1alpha3/listener.go b/pilot/pkg/networking/core/v1alpha3/listener.go
@@ -491,7 +491,6 @@ func (configgen *ConfigGeneratorImpl) buildSidecarInboundListeners(
 
 	} else {
 		rule := sidecarScope.Config.Spec.(*networking.Sidecar)
-		sidecarScopeID := sidecarScope.Config.Name + "." + sidecarScope.Config.Namespace
 		for _, ingressListener := range rule.Ingress {
 			// determine the bindToPort setting for listeners. Validation guarantees that these are all IP listeners.
 			bindToPort := false
@@ -520,27 +519,8 @@ func (configgen *ConfigGeneratorImpl) buildSidecarInboundListeners(
 				bind = getSidecarInboundBindIP(node)
 			}
 
-			instance := configgen.findServiceInstanceForIngressListener(node.ServiceInstances, ingressListener)
-
-			if instance == nil {
-				// We didn't find a matching instance. Create a dummy one because we need the right
-				// params to generate the right cluster name. CDS would have setup the cluster as
-				// as inbound|portNumber|portName|SidecarScopeID
-				attrs := model.ServiceAttributes{
-					Name: sidecarScope.Config.Name,
-					// This will ensure that the right AuthN policies are selected
-					Namespace: sidecarScope.Config.Namespace,
-				}
-				instance = &model.ServiceInstance{
-					Service: &model.Service{
-						Hostname:   host.Name(sidecarScopeID),
-						Attributes: attrs,
-					},
-					Endpoint: &model.IstioEndpoint{
-						Attributes: attrs,
-					},
-				}
-			}
+			instance := configgen.findOrCreateServiceInstance(node.ServiceInstances, ingressListener,
+				sidecarScope.Config.Name, sidecarScope.Config.Namespace)
 
 			listenerOpts := buildListenerOpts{
 				push:           push,