Merge pull request #376 from virajrk/master

MESH-6019 : Make ingress gateway selection consistent.
istio-ecosystem · Jan 22, 2025 · cfd939d · cfd939d
2 parents bff6695 + 6854f0a
commit cfd939d
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 101 deletions.
diff --git a/admiral/pkg/clusters/service_handler.go b/admiral/pkg/clusters/service_handler.go
@@ -102,7 +102,7 @@ func handleServiceEventForDeployment(
 		return fmt.Errorf(AlertLogMsg, ctx.Value(common.EventType))
 	}
 
-	if common.IsIstioIngressGatewayService(svc) {
+	if common.IsIstioIngressGatewayService(svc, common.GetAdmiralParams().LabelSet.GatewayApp) {
 		// The eventType is overridden to admiral.Update. This is mainly
 		// for admiral.Delete events sent for the ingress in the cluster
 		// else it would delete all the SEs in the source and dependent clusters
@@ -165,7 +165,7 @@ func handleServiceEventForRollout(
 		return fmt.Errorf(AlertLogMsg, ctx.Value(common.EventType))
 	}
 
-	if common.IsIstioIngressGatewayService(svc) {
+	if common.IsIstioIngressGatewayService(svc, common.GetAdmiralParams().LabelSet.GatewayApp) {
 		// The eventType is overridden to admiral.Update. This is mainly
 		// for admiral.Delete events sent for the ingress in the cluster
 		// else it would delete all the SEs in the source and dependent clusters

diff --git a/admiral/pkg/clusters/service_handler_test.go b/admiral/pkg/clusters/service_handler_test.go
@@ -128,7 +128,7 @@ func TestHandleServiceEventForDeployment(t *testing.T) {
 		deployment1InNamespace1        = newFakeDeployment(deploymentName1, namespace1, labels)
 		deployment2InNamespace1        = newFakeDeployment(deploymentName2, namespace1, labels)
 		deployment1InNamespace2        = newFakeDeployment(deploymentName1, namespace2, labels)
-		istioIngressGatewayService     = newFakeService(common.IstioIngressGatewayServiceName, common.NamespaceIstioSystem, labels)
+		istioIngressGatewayService     = newFakeService(common.IstioIngressGatewayLabelValue, common.NamespaceIstioSystem, labels)
 		applicationServiceInNamespace1 = newFakeService(serviceInNamespace1, namespace1, labels)
 
 		remoteControllers = map[string]*RemoteController{
@@ -325,7 +325,7 @@ func TestHandleServiceEventForRollout(t *testing.T) {
 		rollout1InNamespace1           = newFakeRollout(rolloutName1, namespace1, labels)
 		rollout2InNamespace1           = newFakeRollout(rolloutName2, namespace1, labels)
 		rollout1InNamespace2           = newFakeRollout(rolloutName1, namespace2, labels)
-		istioIngressGatewayService     = newFakeService(common.IstioIngressGatewayServiceName, common.NamespaceIstioSystem, labels)
+		istioIngressGatewayService     = newFakeService(common.IstioIngressGatewayLabelValue, common.NamespaceIstioSystem, labels)
 		applicationServiceInNamespace1 = newFakeService(serviceInNamespace1, namespace1, labels)
 		remoteControllers              = map[string]*RemoteController{
 			clusterName: &RemoteController{

diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go
@@ -213,7 +213,7 @@ func modifyServiceEntryForNewServiceOrPod(
 			ctxLogger.Infof(common.CtxLogFormat, "event", "", "", clusterId, "neither deployment nor rollouts found")
 			continue
 		}
-		ingressEndpoint, port := getIngressEndpointAndPort(rc)
+		ingressEndpoint, port := rc.ServiceController.Cache.GetLoadBalancer(common.GetAdmiralParams().LabelSet.GatewayApp, common.NamespaceIstioSystem)
 
 		registryConfig.Clusters[clusterId] = &registry.IdentityConfigCluster{
 			Name:            clusterId,

diff --git a/admiral/pkg/clusters/util.go b/admiral/pkg/clusters/util.go
@@ -310,15 +310,6 @@ func getLocality(rc *RemoteController) string {
 	return ""
 }
 
-func getIngressEndpointAndPort(rc *RemoteController) (string, int) {
-	return rc.ServiceController.Cache.
-		GetLoadBalancer(common.GetAdmiralParams().LabelSet.GatewayApp, common.NamespaceIstioSystem)
-}
-
-func getIngressPort(rc *RemoteController) string {
-	return ""
-}
-
 func getIngressPortName(meshPorts map[string]uint32) string {
 	var finalProtocol = commonUtil.Http
 	for protocol := range meshPorts {

diff --git a/admiral/pkg/controller/admiral/service.go b/admiral/pkg/controller/admiral/service.go
@@ -188,7 +188,7 @@ func (s *serviceCache) GetLoadBalancer(key string, namespace string) (string, in
 		return lb, 0
 	}
 	for _, service := range services {
-		if service.Labels["app"] == key {
+		if common.IsIstioIngressGatewayService(service, key) {
 			loadBalancerStatus := service.Status.LoadBalancer.Ingress
 			if len(loadBalancerStatus) > 0 {
 				if len(loadBalancerStatus[0].Hostname) > 0 {

diff --git a/admiral/pkg/controller/admiral/service_test.go b/admiral/pkg/controller/admiral/service_test.go
@@ -285,26 +285,30 @@ func TestServiceCache_GetLoadBalancer(t *testing.T) {
 	sc.cache = make(map[string]*ServiceClusterEntry)
 	sc.mutex = &sync.Mutex{}
 
+	sc_fallbackToIp := serviceCache{}
+	sc_fallbackToIp.cache = make(map[string]*ServiceClusterEntry)
+	sc_fallbackToIp.mutex = &sync.Mutex{}
+
 	service := &coreV1.Service{}
 	service.Name = "test-service"
-	service.Namespace = "ns"
+	service.Namespace = common.NamespaceIstioSystem
 	service.Status = coreV1.ServiceStatus{}
 	service.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	service.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
-	service.Labels = map[string]string{"app": "test-service"}
+	service.Labels = map[string]string{"app": common.IstioIngressGatewayLabelValue}
 
 	s2 := &coreV1.Service{}
 	s2.Name = "test-service-ip"
-	s2.Namespace = "ns"
+	s2.Namespace = common.NamespaceIstioSystem
 	s2.Status = coreV1.ServiceStatus{}
 	s2.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	s2.Status.LoadBalancer.Ingress = append(s2.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{IP: "1.2.3.4"})
-	s2.Labels = map[string]string{"app": "test-service-ip"}
+	s2.Labels = map[string]string{"app": common.IstioIngressGatewayLabelValue}
 
 	// The primary use case is to support ingress gateways for local development
 	externalIPService := &coreV1.Service{}
 	externalIPService.Name = "test-service-externalip"
-	externalIPService.Namespace = "ns"
+	externalIPService.Namespace = common.NamespaceIstioSystem
 	externalIPService.Spec = coreV1.ServiceSpec{}
 	externalIPService.Spec.ExternalIPs = []string{"1.2.3.4"}
 	externalIPService.Spec.Ports = []coreV1.ServicePort{
@@ -320,7 +324,7 @@ func TestServiceCache_GetLoadBalancer(t *testing.T) {
 
 	ignoreService := &coreV1.Service{}
 	ignoreService.Name = "test-service-ignored"
-	ignoreService.Namespace = "ns"
+	ignoreService.Namespace = common.NamespaceIstioSystem
 	ignoreService.Status = coreV1.ServiceStatus{}
 	ignoreService.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	ignoreService.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
@@ -329,24 +333,22 @@ func TestServiceCache_GetLoadBalancer(t *testing.T) {
 
 	ignoreService2 := &coreV1.Service{}
 	ignoreService2.Name = "test-service-ignored-later"
-	ignoreService2.Namespace = "ns"
+	ignoreService2.Namespace = common.NamespaceIstioSystem
 	ignoreService2.Status = coreV1.ServiceStatus{}
 	ignoreService2.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	ignoreService2.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
 	ignoreService2.Labels = map[string]string{"app": "test-service-ignored-later"}
 
 	ignoreService3 := &coreV1.Service{}
 	ignoreService3.Name = "test-service-unignored-later"
-	ignoreService3.Namespace = "ns"
+	ignoreService3.Namespace = common.NamespaceIstioSystem
 	ignoreService3.Status = coreV1.ServiceStatus{}
 	ignoreService3.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	ignoreService3.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
 	ignoreService3.Annotations = map[string]string{"admiral.io/ignore": "true"}
 	ignoreService3.Labels = map[string]string{"app": "test-service-unignored-later"}
 
 	sc.Put(service)
-	sc.Put(s2)
-	sc.Put(externalIPService)
 	sc.Put(ignoreService)
 	sc.Put(ignoreService2)
 	sc.Put(ignoreService3)
@@ -357,6 +359,9 @@ func TestServiceCache_GetLoadBalancer(t *testing.T) {
 	sc.Put(ignoreService2) //Ensuring that if the ignore label is added to a service, it's no longer found
 	sc.Put(ignoreService3) //And ensuring that if the ignore label is removed from a service, it becomes found
 
+	sc_fallbackToIp.Put(s2)
+	sc_fallbackToIp.Put(externalIPService)
+
 	testCases := []struct {
 		name           string
 		cache          *serviceCache
@@ -368,8 +373,8 @@ func TestServiceCache_GetLoadBalancer(t *testing.T) {
 		{
 			name:           "Find service load balancer when present",
 			cache:          &sc,
-			key:            "test-service",
-			ns:             "ns",
+			key:            common.IstioIngressGatewayLabelValue,
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "hostname.com",
 			expectedPort:   common.DefaultMtlsPort,
 		},
@@ -383,41 +388,41 @@ func TestServiceCache_GetLoadBalancer(t *testing.T) {
 		},
 		{
 			name:           "Falls back to IP",
-			cache:          &sc,
-			key:            "test-service-ip",
-			ns:             "ns",
+			cache:          &sc_fallbackToIp,
+			key:            common.IstioIngressGatewayLabelValue,
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "1.2.3.4",
 			expectedPort:   common.DefaultMtlsPort,
 		},
 		{
 			name:           "Falls back to externalIP",
-			cache:          &sc,
+			cache:          &sc_fallbackToIp,
 			key:            "test-service-externalip",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "1.2.3.4",
 			expectedPort:   30800,
 		},
 		{
 			name:           "Successfully ignores services with the ignore label",
 			cache:          &sc,
 			key:            "test-service-ignored",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "dummy.admiral.global",
 			expectedPort:   common.DefaultMtlsPort,
 		},
 		{
 			name:           "Successfully ignores services when the ignore label is added after the service had been added to the cache for the first time",
 			cache:          &sc,
 			key:            "test-service-ignored-later",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "dummy.admiral.global",
 			expectedPort:   common.DefaultMtlsPort,
 		},
 		{
 			name:           "Successfully finds services when the ignore label is added initially, then removed",
 			cache:          &sc,
 			key:            "test-service-unignored-later",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "hostname.com",
 			expectedPort:   common.DefaultMtlsPort,
 		},
@@ -443,15 +448,15 @@ func TestServiceCache_GetLoadBalancerWithAbsoluteFQDN(t *testing.T) {
 
 	service := &coreV1.Service{}
 	service.Name = "test-service"
-	service.Namespace = "ns"
+	service.Namespace = common.NamespaceIstioSystem
 	service.Status = coreV1.ServiceStatus{}
 	service.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	service.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
-	service.Labels = map[string]string{"app": "test-service"}
+	service.Labels = map[string]string{"app": common.IstioIngressGatewayLabelValue}
 
 	s2 := &coreV1.Service{}
 	s2.Name = "test-service-ip"
-	s2.Namespace = "ns"
+	s2.Namespace = common.NamespaceIstioSystem
 	s2.Status = coreV1.ServiceStatus{}
 	s2.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	s2.Status.LoadBalancer.Ingress = append(s2.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{IP: "1.2.3.4"})
@@ -476,7 +481,7 @@ func TestServiceCache_GetLoadBalancerWithAbsoluteFQDN(t *testing.T) {
 
 	ignoreService := &coreV1.Service{}
 	ignoreService.Name = "test-service-ignored"
-	ignoreService.Namespace = "ns"
+	ignoreService.Namespace = common.NamespaceIstioSystem
 	ignoreService.Status = coreV1.ServiceStatus{}
 	ignoreService.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	ignoreService.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
@@ -485,15 +490,15 @@ func TestServiceCache_GetLoadBalancerWithAbsoluteFQDN(t *testing.T) {
 
 	ignoreService2 := &coreV1.Service{}
 	ignoreService2.Name = "test-service-ignored-later"
-	ignoreService2.Namespace = "ns"
+	ignoreService2.Namespace = common.NamespaceIstioSystem
 	ignoreService2.Status = coreV1.ServiceStatus{}
 	ignoreService2.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	ignoreService2.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
 	ignoreService2.Labels = map[string]string{"app": "test-service-ignored-later"}
 
 	ignoreService3 := &coreV1.Service{}
 	ignoreService3.Name = "test-service-unignored-later"
-	ignoreService3.Namespace = "ns"
+	ignoreService3.Namespace = common.NamespaceIstioSystem
 	ignoreService3.Status = coreV1.ServiceStatus{}
 	ignoreService3.Status.LoadBalancer = coreV1.LoadBalancerStatus{}
 	ignoreService3.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, coreV1.LoadBalancerIngress{Hostname: "hostname.com"})
@@ -524,8 +529,8 @@ func TestServiceCache_GetLoadBalancerWithAbsoluteFQDN(t *testing.T) {
 		{
 			name:           "Given service and loadbalancer, should return endpoint with dot in the end",
 			cache:          &sc,
-			key:            "test-service",
-			ns:             "ns",
+			key:            common.IstioIngressGatewayLabelValue,
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "hostname.com.",
 			expectedPort:   common.DefaultMtlsPort,
 		},
@@ -541,31 +546,31 @@ func TestServiceCache_GetLoadBalancerWithAbsoluteFQDN(t *testing.T) {
 			name:           "Given host not present in load balancer, should fallback to IP without dot at the end",
 			cache:          &sc,
 			key:            "test-service-ip",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "1.2.3.4",
 			expectedPort:   common.DefaultMtlsPort,
 		},
 		{
 			name:           "Given ignore label, should return dummy",
 			cache:          &sc,
 			key:            "test-service-ignored",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "dummy.admiral.global",
 			expectedPort:   common.DefaultMtlsPort,
 		},
 		{
 			name:           "Successfully ignores services when the ignore label is added after the service had been added to the cache for the first time",
 			cache:          &sc,
 			key:            "test-service-ignored-later",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "dummy.admiral.global",
 			expectedPort:   common.DefaultMtlsPort,
 		},
 		{
 			name:           "Successfully finds services when the ignore label is added initially, then removed",
 			cache:          &sc,
 			key:            "test-service-unignored-later",
-			ns:             "ns",
+			ns:             common.NamespaceIstioSystem,
 			expectedReturn: "hostname.com.",
 			expectedPort:   common.DefaultMtlsPort,
 		},