Skip to content

Commit

Permalink
prevent panic when cluster does not contain rollout OR deployment for…
Browse files Browse the repository at this point in the history 
… given asset

Signed-off-by: Anubhav Aeron <anubhav_aeron@intuit.com>
  • Loading branch information
Anubhav Aeron committed Mar 10, 2023
1 parent 1313ef0 commit 74bc178
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 22 deletions.
2 changes: 1 addition & 1 deletion admiral/pkg/clusters/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 19 additions & 19 deletions admiral/pkg/clusters/serviceentry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
18 changes: 17 additions & 1 deletion admiral/pkg/clusters/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
116 changes: 115 additions & 1 deletion admiral/pkg/clusters/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
}

0 comments on commit 74bc178

Please sign in to comment.