diff --git a/admiral/pkg/clusters/handler.go b/admiral/pkg/clusters/handler.go index c218296d..1cf4bc21 100644 --- a/admiral/pkg/clusters/handler.go +++ b/admiral/pkg/clusters/handler.go @@ -758,7 +758,7 @@ func getServiceForDeployment(rc *RemoteController, deployment *k8sAppsV1.Deploym var match = common.IsServiceMatch(service.Spec.Selector, deployment.Spec.Selector) //make sure the service matches the deployment Selector and also has a mesh port in the port spec if match { - ports := GetMeshPorts(rc.ClusterID, service, deployment) + ports := GetMeshPortsForDeployment(rc.ClusterID, service, deployment) if len(ports) > 0 { matchedService = service break diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index a5f315e6..2d95df83 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -125,7 +125,7 @@ func modifyServiceEntryForNewServiceOrPod( continue } namespace = deployment.Namespace - localMeshPorts := GetMeshPorts(rc.ClusterID, serviceInstance, deployment) + localMeshPorts := GetMeshPortsForDeployment(rc.ClusterID, serviceInstance, deployment) cname = common.GetCname(deployment, common.GetWorkloadIdentifier(), common.GetHostnameSuffix()) sourceDeployments[rc.ClusterID] = deployment @@ -189,24 +189,24 @@ func modifyServiceEntryForNewServiceOrPod( start = time.Now() for sourceCluster, serviceInstance := range sourceServices { - localFqdn := serviceInstance.Name + common.Sep + serviceInstance.Namespace + common.DotLocalDomainSuffix - rc := remoteRegistry.GetRemoteController(sourceCluster) - var meshPorts map[string]uint32 - blueGreenStrategy := isBlueGreenStrategy(sourceRollouts[sourceCluster]) - - var deploymentRolloutLabels map[string]string - if len(sourceDeployments) > 0 { - meshPorts = GetMeshPorts(sourceCluster, serviceInstance, sourceDeployments[sourceCluster]) - deployment = sourceDeployments[sourceCluster] - deploymentRolloutLabels = deployment.Labels - } else { - meshPorts = GetMeshPortsForRollout(sourceCluster, serviceInstance, sourceRollouts[sourceCluster]) - rollout := sourceRollouts[sourceCluster] - deploymentRolloutLabels = rollout.Labels + var ( + meshPorts map[string]uint32 + localFqdn = serviceInstance.Name + common.Sep + serviceInstance.Namespace + common.DotLocalDomainSuffix + rc = remoteRegistry.GetRemoteController(sourceCluster) + blueGreenStrategy = isBlueGreenStrategy(sourceRollouts[sourceCluster]) + ) + + meshPorts, labels := GetMeshPortAndLabelsFromDeploymentOrRollout( + sourceCluster, serviceInstance, sourceDeployments, sourceRollouts, + ) + if meshPorts == nil { + log.Infof("Unable to determine mesh ports for service=%s in cluster=%s", serviceInstance.Name, sourceCluster) + continue + } + if labels != nil { + // check if additional endpoint generation is required + isAdditionalEndpointGenerationEnabled = doGenerateAdditionalEndpoints(labels) } - - // check if additional endpoint generation is required - isAdditionalEndpointGenerationEnabled = doGenerateAdditionalEndpoints(deploymentRolloutLabels) for key, serviceEntry := range serviceEntries { if len(serviceEntry.Endpoints) == 0 { @@ -216,7 +216,7 @@ func modifyServiceEntryForNewServiceOrPod( } clusterIngress, _ := rc.ServiceController.Cache.GetLoadBalancer(common.GetAdmiralParams().LabelSet.GatewayApp, common.NamespaceIstioSystem) for _, ep := range serviceEntry.Endpoints { - //replace istio ingress-gateway address with local fqdn, note that ingress-gateway can be empty (not provisoned, or is not up) + //replace istio ingress-gateway address with local fqdn, note that ingress-gateway can be empty (not provisioned, or is not up) if ep.Address == clusterIngress || ep.Address == "" { // Update endpoints with locafqdn for active and preview se of bluegreen rollout if blueGreenStrategy { diff --git a/admiral/pkg/clusters/util.go b/admiral/pkg/clusters/util.go index 5c509ac4..9371957d 100644 --- a/admiral/pkg/clusters/util.go +++ b/admiral/pkg/clusters/util.go @@ -14,7 +14,23 @@ import ( k8sV1 "k8s.io/api/core/v1" ) -func GetMeshPorts(clusterName string, destService *k8sV1.Service, +func GetMeshPortAndLabelsFromDeploymentOrRollout( + cluster string, serviceInstance *k8sV1.Service, + deploymentsByCluster map[string]*k8sAppsV1.Deployment, + rolloutsByCluster map[string]*argo.Rollout, +) (portsByProtocol map[string]uint32, labels map[string]string) { + if len(deploymentsByCluster) > 0 && deploymentsByCluster[cluster] != nil { + deployment := deploymentsByCluster[cluster] + return GetMeshPortsForDeployment(cluster, serviceInstance, deployment), deployment.Labels + } + if len(rolloutsByCluster) > 0 && rolloutsByCluster[cluster] != nil { + rollout := rolloutsByCluster[cluster] + return GetMeshPortsForRollout(cluster, serviceInstance, rollout), rollout.Labels + } + return nil, nil +} + +func GetMeshPortsForDeployment(clusterName string, destService *k8sV1.Service, destDeployment *k8sAppsV1.Deployment) map[string]uint32 { var meshPorts = destDeployment.Spec.Template.Annotations[common.SidecarEnabledPorts] ports := getMeshPortsHelper(meshPorts, destService, clusterName) diff --git a/admiral/pkg/clusters/util_test.go b/admiral/pkg/clusters/util_test.go index e5d8dd01..0a5268f6 100644 --- a/admiral/pkg/clusters/util_test.go +++ b/admiral/pkg/clusters/util_test.go @@ -142,7 +142,7 @@ func TestGetMeshPorts(t *testing.T) { for _, c := range testCases { t.Run(c.name, func(t *testing.T) { - meshPorts := GetMeshPorts(c.clusterName, &c.service, &c.deployment) + meshPorts := GetMeshPortsForDeployment(c.clusterName, &c.service, &c.deployment) if !reflect.DeepEqual(meshPorts, c.expected) { t.Errorf("Wanted meshPorts: %v, got: %v", c.expected, meshPorts) } @@ -336,3 +336,117 @@ func TestGetMeshPortsForRollout(t *testing.T) { }) } } + +func TestGetMeshPortAndLabelsFromDeploymentOrRollout(t *testing.T) { + var ( + service = &k8sV1.Service{ + Spec: k8sV1.ServiceSpec{ + Ports: []k8sV1.ServicePort{ + { + Name: common.Http, + Port: 8090, + }, + }, + }, + } + clusterNameWithExistingDeployment = "cluster_with_deployment-ppd-k8s" + clusterNameWithExistingRollout = "cluster_with_rollout-ppd-k8s" + clusterNameWithoutExistingRolloutOrDeployment = "cluster_without_deployment_rollout-ppd-k8s" + deploymentByClusterNameForExistingClusterWithDeployment = map[string]*k8sAppsV1.Deployment{ + clusterNameWithExistingDeployment: { + ObjectMeta: v1.ObjectMeta{ + Labels: map[string]string{ + "key": "value", + }, + }, + }, + } + rolloutByClusterNameForExistingClusterWithRollout = map[string]*argo.Rollout{ + clusterNameWithExistingRollout: { + ObjectMeta: v1.ObjectMeta{ + Labels: map[string]string{ + "key": "value", + }, + }, + }, + } + ) + cases := []struct { + name string + cluster string + serviceInstance *k8sV1.Service + deploymentByCluster map[string]*k8sAppsV1.Deployment + rolloutsByCluster map[string]*argo.Rollout + expectedMeshPort map[string]uint32 + expectedLabels map[string]string + }{ + { + name: "Given a deployment with labels exists in a cluster, " + + "When GetMeshPortAndLabelsFromDeploymentOrRollout is called with," + + "this cluster, with a valid service, " + + "Then, it should return mesh ports and labels", + cluster: clusterNameWithExistingDeployment, + serviceInstance: service, + deploymentByCluster: deploymentByClusterNameForExistingClusterWithDeployment, + rolloutsByCluster: rolloutByClusterNameForExistingClusterWithRollout, + expectedMeshPort: map[string]uint32{ + common.Http: 8090, + }, + expectedLabels: map[string]string{ + "key": "value", + }, + }, + { + name: "Given a rollout with labels exists in a cluster, " + + "When GetMeshPortAndLabelsFromDeploymentOrRollout is called with," + + "this cluster, with a valid service, " + + "Then, it should return mesh ports and labels", + cluster: clusterNameWithExistingRollout, + serviceInstance: service, + deploymentByCluster: deploymentByClusterNameForExistingClusterWithDeployment, + rolloutsByCluster: rolloutByClusterNameForExistingClusterWithRollout, + expectedMeshPort: map[string]uint32{ + common.Http: 8090, + }, + expectedLabels: map[string]string{ + "key": "value", + }, + }, + { + name: "Given neither a deployment nor a rollout with labels exists in a cluster, " + + "When GetMeshPortAndLabelsFromDeploymentOrRollout is called with," + + "this cluster, with a valid service, " + + "Then, it should return nil for mesh ports, and nil for labels", + cluster: clusterNameWithoutExistingRolloutOrDeployment, + serviceInstance: service, + deploymentByCluster: deploymentByClusterNameForExistingClusterWithDeployment, + rolloutsByCluster: rolloutByClusterNameForExistingClusterWithRollout, + expectedMeshPort: nil, + expectedLabels: nil, + }, + { + name: "Given neither a deployment nor a rollout with labels exists in a cluster, " + + "When GetMeshPortAndLabelsFromDeploymentOrRollout is called with," + + "this cluster, with a valid service, but empty deployment by cluster and rollout by cluster maps " + + "Then, it should return nil for mesh ports, and nil for labels", + cluster: clusterNameWithoutExistingRolloutOrDeployment, + serviceInstance: service, + deploymentByCluster: nil, + rolloutsByCluster: nil, + expectedMeshPort: nil, + expectedLabels: nil, + }, + } + + for _, c := range cases { + meshPort, labels := GetMeshPortAndLabelsFromDeploymentOrRollout( + c.cluster, c.serviceInstance, c.deploymentByCluster, c.rolloutsByCluster, + ) + if !reflect.DeepEqual(meshPort, c.expectedMeshPort) { + t.Errorf("expected: %v, got: %v", c.expectedMeshPort, meshPort) + } + if !reflect.DeepEqual(labels, c.expectedLabels) { + t.Errorf("expected: %v, got: %v", c.expectedLabels, labels) + } + } +}