diff --git a/admiral/pkg/clusters/virtualservice_handler.go b/admiral/pkg/clusters/virtualservice_handler.go index e96c5ed3..2ba4b45d 100644 --- a/admiral/pkg/clusters/virtualservice_handler.go +++ b/admiral/pkg/clusters/virtualservice_handler.go @@ -59,6 +59,7 @@ type SyncVirtualServiceResource func( remoteRegistry *RemoteRegistry, sourceCluster string, syncNamespace string, + vsName string, ) error // VirtualServiceHandler responsible for handling Add/Update/Delete events for @@ -159,6 +160,8 @@ func (vh *VirtualServiceHandler) handleVirtualServiceEvent(ctx context.Context, return nil } + vSName := common.GenerateUniqueNameForVS(virtualService.Namespace, virtualService.Name) + dependentClusters := vh.remoteRegistry.AdmiralCache.CnameDependentClusterCache.Get(spec.Hosts[0]).CopyJustValues() if len(dependentClusters) > 0 { // Add source clusters to the list of clusters to copy the virtual service @@ -172,6 +175,7 @@ func (vh *VirtualServiceHandler) handleVirtualServiceEvent(ctx context.Context, vh.remoteRegistry, vh.clusterID, syncNamespace, + vSName, ) if err != nil { log.Warnf(LogErrFormat, "Sync", common.VirtualServiceResourceType, virtualService.Name, dependentClusters, err.Error()+": sync to dependent clusters will not be retried") @@ -192,6 +196,7 @@ func (vh *VirtualServiceHandler) handleVirtualServiceEvent(ctx context.Context, vh.remoteRegistry, vh.clusterID, syncNamespace, + vSName, ) if err != nil { log.Warnf(LogErrFormat, "Sync", common.VirtualServiceResourceType, virtualService.Name, "*", err.Error()+": sync to remote clusters will not be retried") @@ -252,8 +257,12 @@ func syncVirtualServicesToAllDependentClusters( remoteRegistry *RemoteRegistry, sourceCluster string, syncNamespace string, + vSName string, ) error { defer logElapsedTimeForVirtualService("syncVirtualServicesToAllDependentClusters="+string(event), "", virtualService)() + if vSName == "" { + return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, "VirtualService generated name is empty") + } if virtualService == nil { return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, "VirtualService is nil") } @@ -273,6 +282,7 @@ func syncVirtualServicesToAllDependentClusters( virtualServiceCopy, event, syncNamespace, + vSName, ) if err != nil { allClusterErrors = common.AppendError(allClusterErrors, err) @@ -289,47 +299,57 @@ func syncVirtualServiceToDependentCluster( remoteRegistry *RemoteRegistry, virtualService *v1alpha3.VirtualService, event common.Event, - syncNamespace string) error { + syncNamespace string, + vSName string) error { ctxLogger := log.WithFields(log.Fields{ "type": "syncVirtualServiceToDependentCluster", - "identity": virtualService.Name, + "identity": vSName, "txId": uuid.New().String(), }) defer logElapsedTimeForVirtualService("syncVirtualServiceToDependentCluster="+string(event), cluster, virtualService)() rc := remoteRegistry.GetRemoteController(cluster) if rc == nil { - return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, virtualService.Name, + return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, vSName, cluster, "dependent controller not initialized for cluster") } - ctxLogger.Infof(LogFormat, "Event", "VirtualService", virtualService.Name, cluster, "Processing") + ctxLogger.Infof(LogFormat, "Event", "VirtualService", vSName, cluster, "Processing") if rc.VirtualServiceController == nil { - return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, virtualService.Name, cluster, "VirtualService controller not initialized for cluster") + return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, vSName, cluster, "VirtualService controller not initialized for cluster") } + if event == common.Delete { - err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, virtualService.Name, metav1.DeleteOptions{}) + // Best effort delete for existing virtual service with old name + _ = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, virtualService.Name, metav1.DeleteOptions{}) + + err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, vSName, metav1.DeleteOptions{}) if err != nil { if k8sErrors.IsNotFound(err) { - ctxLogger.Infof(LogFormat, "Delete", "VirtualService", virtualService.Name, cluster, "Either VirtualService was already deleted, or it never existed") + ctxLogger.Infof(LogFormat, "Delete", "VirtualService", vSName, cluster, "Either VirtualService was already deleted, or it never existed") return nil } if isDeadCluster(err) { - ctxLogger.Warnf(LogErrFormat, "Create/Update", common.VirtualServiceResourceType, virtualService.Name, cluster, "dead cluster") + ctxLogger.Warnf(LogErrFormat, "Create/Update", common.VirtualServiceResourceType, vSName, cluster, "dead cluster") return nil } - return fmt.Errorf(LogErrFormat, "Delete", "VirtualService", virtualService.Name, cluster, err) + return fmt.Errorf(LogErrFormat, "Delete", "VirtualService", vSName, cluster, err) } - ctxLogger.Infof(LogFormat, "Delete", "VirtualService", virtualService.Name, cluster, "Success") + ctxLogger.Infof(LogFormat, "Delete", "VirtualService", vSName, cluster, "Success") return nil } - exist, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, virtualService.Name, metav1.GetOptions{}) + + oldVSname := virtualService.Name + //Update vs name to be unique per namespace + virtualService.Name = vSName + + exist, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { - ctxLogger.Infof(LogFormat, "Get", common.VirtualServiceResourceType, virtualService.Name, cluster, "VirtualService does not exist") + ctxLogger.Infof(LogFormat, "Get", common.VirtualServiceResourceType, vSName, cluster, "VirtualService does not exist") exist = nil } if isDeadCluster(err) { - ctxLogger.Warnf(LogErrFormat, "Create/Update", common.VirtualServiceResourceType, virtualService.Name, cluster, "dead cluster") + ctxLogger.Warnf(LogErrFormat, "Create/Update", common.VirtualServiceResourceType, vSName, cluster, "dead cluster") return nil } //change destination host for all http routes .. to same as host on the virtual service @@ -350,7 +370,12 @@ func syncVirtualServiceToDependentCluster( } } // nolint - return addUpdateVirtualService(ctxLogger, ctx, virtualService, exist, syncNamespace, rc, remoteRegistry) + err = addUpdateVirtualService(ctxLogger, ctx, virtualService, exist, syncNamespace, rc, remoteRegistry) + + // Best effort delete for existing virtual service with old name + _ = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, oldVSname, metav1.DeleteOptions{}) + + return err } func syncVirtualServicesToAllRemoteClusters( @@ -360,8 +385,12 @@ func syncVirtualServicesToAllRemoteClusters( event common.Event, remoteRegistry *RemoteRegistry, sourceCluster string, - syncNamespace string) error { + syncNamespace string, + vSName string) error { defer logElapsedTimeForVirtualService("syncVirtualServicesToAllRemoteClusters="+string(event), "*", virtualService)() + if vSName == "" { + return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, "VirtualService generated name is empty") + } if virtualService == nil { return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, "VirtualService is nil") } @@ -381,6 +410,7 @@ func syncVirtualServicesToAllRemoteClusters( virtualServiceCopy, event, syncNamespace, + vSName, ) if err != nil { allClusterErrors = common.AppendError(allClusterErrors, err) @@ -397,49 +427,62 @@ func syncVirtualServiceToRemoteCluster( remoteRegistry *RemoteRegistry, virtualService *v1alpha3.VirtualService, event common.Event, - syncNamespace string) error { + syncNamespace string, + vSName string) error { ctxLogger := log.WithFields(log.Fields{ "type": "syncVirtualServicesToAllRemoteClusters", - "identity": virtualService.Name, + "identity": vSName, "txId": uuid.New().String(), }) defer logElapsedTimeForVirtualService("syncVirtualServiceToRemoteCluster="+string(event), cluster, virtualService)() rc := remoteRegistry.GetRemoteController(cluster) if rc == nil { - return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, virtualService.Name, cluster, "remote controller not initialized for cluster") + return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, vSName, cluster, "remote controller not initialized for cluster") } if rc.VirtualServiceController == nil { - return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, virtualService.Name, cluster, "VirtualService controller not initialized for cluster") + return fmt.Errorf(LogFormat, "Event", common.VirtualServiceResourceType, vSName, cluster, "VirtualService controller not initialized for cluster") } + if event == common.Delete { - err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, virtualService.Name, metav1.DeleteOptions{}) + // Best effort delete for existing virtual service with old name + _ = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, virtualService.Name, metav1.DeleteOptions{}) + + err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, vSName, metav1.DeleteOptions{}) if err != nil { if k8sErrors.IsNotFound(err) { - ctxLogger.Infof(LogFormat, "Delete", common.VirtualServiceResourceType, virtualService.Name, cluster, "Either VirtualService was already deleted, or it never existed") + ctxLogger.Infof(LogFormat, "Delete", common.VirtualServiceResourceType, vSName, cluster, "Either VirtualService was already deleted, or it never existed") return nil } if isDeadCluster(err) { - ctxLogger.Warnf(LogErrFormat, "Delete", common.VirtualServiceResourceType, virtualService.Name, cluster, "dead cluster") + ctxLogger.Warnf(LogErrFormat, "Delete", common.VirtualServiceResourceType, vSName, cluster, "dead cluster") return nil } - return fmt.Errorf(LogErrFormat, "Delete", common.VirtualServiceResourceType, virtualService.Name, cluster, err) + + return fmt.Errorf(LogErrFormat, "Delete", common.VirtualServiceResourceType, vSName, cluster, err) } - ctxLogger.Infof(LogFormat, "Delete", common.VirtualServiceResourceType, virtualService.Name, cluster, "Success") + ctxLogger.Infof(LogFormat, "Delete", common.VirtualServiceResourceType, vSName, cluster, "Success") return nil } - exist, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, virtualService.Name, metav1.GetOptions{}) + oldVSname := virtualService.Name + //Update vs name to be unique per namespace + virtualService.Name = vSName + exist, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { - ctxLogger.Infof(LogFormat, "Get", common.VirtualServiceResourceType, virtualService.Name, cluster, "VirtualService does not exist") + ctxLogger.Infof(LogFormat, "Get", common.VirtualServiceResourceType, vSName, cluster, "VirtualService does not exist") exist = nil } if isDeadCluster(err) { - ctxLogger.Warnf(LogErrFormat, "Create/Update", common.VirtualServiceResourceType, virtualService.Name, cluster, "dead cluster") + ctxLogger.Warnf(LogErrFormat, "Create/Update", common.VirtualServiceResourceType, vSName, cluster, "dead cluster") return nil } + err = addUpdateVirtualService(ctxLogger, ctx, virtualService, exist, syncNamespace, rc, remoteRegistry) + + // Best effort delete of existing virtual service with old name + _ = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, oldVSname, metav1.DeleteOptions{}) // nolint - return addUpdateVirtualService(ctxLogger, ctx, virtualService, exist, syncNamespace, rc, remoteRegistry) + return err } func matchRolloutCanaryStrategy(rolloutStrategy argo.RolloutStrategy, virtualServiceName string) bool { diff --git a/admiral/pkg/clusters/virtualservice_handler_test.go b/admiral/pkg/clusters/virtualservice_handler_test.go index 298fde91..76b28c64 100644 --- a/admiral/pkg/clusters/virtualservice_handler_test.go +++ b/admiral/pkg/clusters/virtualservice_handler_test.go @@ -583,6 +583,8 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { }, } + vSName = common.GenerateUniqueNameForVS(workingVS.Namespace, workingVS.Name) + fakeIstioClientWithoutAnyVirtualServices = istioFake.NewSimpleClientset() fakeIstioClientWithoutKnownVirtualServices = newFakeIstioClient(ctx, namespace1, workingVS) nilVirtualServiceControllerForDependencyCluster1 = map[string]*RemoteController{ @@ -648,19 +650,19 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { "remoteRegistry is nil", ) nilRemoteControllerForDepCluster1Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster1, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster1, "dependent controller not initialized for cluster", ) nilRemoteControllerForDepCluster2Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster2, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster2, "dependent controller not initialized for cluster", ) virtualServiceControllerNotInitializedForCluster1Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster1, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster1, "VirtualService controller not initialized for cluster", ) virtualServiceControllerNotInitializedForCluster2Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster2, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster2, "VirtualService controller not initialized for cluster", ) ) @@ -673,6 +675,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry *RemoteRegistry sourceCluster string syncNamespace string + vSName string assertFunc func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) doSyncVSToSourceCluster bool expectedErr error @@ -682,6 +685,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { "When, syncVirtualServicesToAllDependentClusters is invoked, " + "Then, it should return '" + emptyVSErr.Error() + "' error", sourceCluster: sourceCluster, + vSName: vSName, expectedErr: emptyVSErr, }, { @@ -690,6 +694,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { "Then, it should return '" + emptyRemoteRegistryErr.Error() + "' error", sourceCluster: sourceCluster, virtualService: workingVS, + vSName: vSName, expectedErr: emptyRemoteRegistryErr, }, { @@ -700,6 +705,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, nil), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, expectedErr: nilRemoteControllerForDepCluster1Err, }, { @@ -712,17 +718,18 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithoutAnyVirtualServices), clusters: clusters1And2, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { // cluster with no nil pointer exception if cluster == dependentCluster1 { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -738,6 +745,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, nilVirtualServiceControllerForDependencyCluster1), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, expectedErr: virtualServiceControllerNotInitializedForCluster1Err, }, { @@ -750,17 +758,18 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForDepCluster1AndNilForCluster2), clusters: clusters1And2, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { // cluster with no nil pointer exception if cluster == dependentCluster1 { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -779,15 +788,16 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithoutAnyVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -805,15 +815,16 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithKnownVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -832,10 +843,11 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithKnownVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if !k8sErrors.IsNotFound(err) { t.Errorf("expected error to be Not Found, but got: %v", err) } @@ -853,10 +865,11 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithoutAnyVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if !k8sErrors.IsNotFound(err) { t.Errorf("expected error to be Not Found, but got: %v", err) } @@ -875,15 +888,16 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForSourceClustersWithoutAnyVirtualServices), clusters: clustersContainingSourceCluster, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -902,15 +916,16 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForSourceClustersWithKnownVirtualServices), clusters: clustersContainingSourceCluster, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -933,6 +948,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { c.remoteRegistry, c.sourceCluster, syncNamespace, + c.vSName, ) if err != nil && c.expectedErr == nil { t.Errorf("expected error to be nil but got %v", err) @@ -970,6 +986,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { Namespace: namespace1, }, } + vSName = common.GenerateUniqueNameForVS(workingVS.Namespace, workingVS.Name) fakeIstioClientWithoutAnyVirtualServices = istioFake.NewSimpleClientset() fakeIstioClientWithoutKnownVirtualServices = newFakeIstioClient(ctx, namespace1, workingVS) nilVirtualServiceControllerForKnownClusters = map[string]*RemoteController{ @@ -1028,24 +1045,28 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, "VirtualService is nil", ) + emptyVsNameErr = fmt.Errorf( + LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, + "VirtualService generated name is empty", + ) emptyRemoteRegistryErr = fmt.Errorf( LogFormat, "Event", common.VirtualServiceResourceType, "", sourceCluster, "remoteRegistry is nil", ) nilRemoteControllerForDepCluster1Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster1, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster1, "remote controller not initialized for cluster", ) nilRemoteControllerForDepCluster2Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster2, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster2, "remote controller not initialized for cluster", ) virtualServiceControllerNotInitializedForCluster1Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster1, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster1, "VirtualService controller not initialized for cluster", ) virtualServiceControllerNotInitializedForCluster2Err = fmt.Errorf( - LogFormat, "Event", common.VirtualServiceResourceType, workingVS.Name, dependentCluster2, + LogFormat, "Event", common.VirtualServiceResourceType, vSName, dependentCluster2, "VirtualService controller not initialized for cluster", ) ) @@ -1058,15 +1079,24 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry *RemoteRegistry sourceCluster string syncNamespace string + vSName string assertFunc func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) doSyncVSToSourceCluster bool expectedErr error }{ + { + name: "Given a nil vsName is passed , " + + "When, syncVirtualServicesToAllRemoteClusters is invoked, " + + "Then, it should return '" + emptyVsNameErr.Error() + "' error", + sourceCluster: sourceCluster, + expectedErr: emptyVsNameErr, + }, { name: "Given a nil VirtualService is passed , " + "When, syncVirtualServicesToAllRemoteClusters is invoked, " + "Then, it should return '" + emptyVSErr.Error() + "' error", sourceCluster: sourceCluster, + vSName: vSName, expectedErr: emptyVSErr, }, { @@ -1075,6 +1105,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { "Then, it should return '" + emptyRemoteRegistryErr.Error() + "' error", sourceCluster: sourceCluster, virtualService: workingVS, + vSName: vSName, expectedErr: emptyRemoteRegistryErr, }, { @@ -1085,6 +1116,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, nil), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, expectedErr: nilRemoteControllerForDepCluster1Err, }, { @@ -1097,17 +1129,18 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithoutAnyVirtualServices), clusters: cluster1And2, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { // cluster with no nil pointer exception if cluster == dependentCluster1 { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -1123,6 +1156,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, nilVirtualServiceControllerForKnownClusters), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, expectedErr: virtualServiceControllerNotInitializedForCluster1Err, }, { @@ -1135,6 +1169,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForDepCluster1AndNilForCluster2), clusters: cluster1And2, sourceCluster: sourceCluster, + vSName: vSName, expectedErr: virtualServiceControllerNotInitializedForCluster2Err, }, { @@ -1148,15 +1183,16 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithoutAnyVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -1174,15 +1210,16 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithKnownVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -1201,10 +1238,11 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithKnownVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if !k8sErrors.IsNotFound(err) { t.Errorf("expected error to be Not Found, but got: %v", err) } @@ -1222,10 +1260,11 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForKnownClustersWithoutAnyVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if !k8sErrors.IsNotFound(err) { t.Errorf("expected error to be Not Found, but got: %v", err) } @@ -1244,15 +1283,16 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForSourceClustersWithoutAnyVirtualServices), clusters: clustersContainingSourceCluster, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + vs, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if err != nil { t.Errorf("expected nil, but got error: %v", err) return } - if vs == nil || vs.Name != workingVS.Name { + if vs == nil || vs.Name != vSName { t.Errorf("expected VirtualService to be created, but it was not") } } @@ -1270,10 +1310,11 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { remoteRegistry: newRemoteRegistry(ctx, virtualServiceControllerForSourceClustersWithKnownVirtualServices), clusters: cluster1, sourceCluster: sourceCluster, + vSName: vSName, assertFunc: func(remoteRegistry *RemoteRegistry, clusters []string, t *testing.T) { for _, cluster := range clusters { rc := remoteRegistry.GetRemoteController(cluster) - _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, workingVS.Name, metaV1.GetOptions{}) + _, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Get(ctx, vSName, metaV1.GetOptions{}) if !k8sErrors.IsNotFound(err) { t.Errorf("expected error to be Not Found, but got: %v", err) } @@ -1297,6 +1338,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { c.remoteRegistry, c.sourceCluster, syncNamespace, + c.vSName, ) if err != nil && c.expectedErr == nil { t.Errorf("expected error to be nil but got %v", err) @@ -1518,7 +1560,8 @@ func newFakeSyncResource(err error) *fakeSyncResource { event common.Event, remoteRegistry *RemoteRegistry, clusterId string, - syncNamespace string) error { + syncNamespace string, + vSName string) error { f.called = true return err } @@ -1536,6 +1579,7 @@ func checkSourceClusterInDependentList(err error) *fakeSyncResource { _ common.Event, _ *RemoteRegistry, clusterId string, + _ string, _ string) error { for _, cluster := range dependentClusters { if cluster == clusterId { diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index 6127e7d5..af875d63 100644 --- a/admiral/pkg/controller/common/common.go +++ b/admiral/pkg/controller/common/common.go @@ -702,3 +702,24 @@ func GetMeshPortsHelper(meshPorts string, destService *k8sV1.Service, clusterNam } return ports } + +func GenerateUniqueNameForVS(syncNamespace string, vsName string) string { + + if syncNamespace == "" && vsName == "" { + return "" + } else if syncNamespace == "" { + return vsName + } else if vsName == "" { + return syncNamespace + } + + newVSName := syncNamespace + "-" + vsName + if len(newVSName) > 250 { + newVSName = newVSName[:250] + } + //"op=%v type=%v name=%v namespace=%s cluster=%s message=%v" + logrus.Debugf(LogFormatAdv, "VirtualService", newVSName, syncNamespace, "", "New VS name generated") + + return newVSName + +} diff --git a/admiral/pkg/controller/common/common_test.go b/admiral/pkg/controller/common/common_test.go index 2209b0be..bec57066 100644 --- a/admiral/pkg/controller/common/common_test.go +++ b/admiral/pkg/controller/common/common_test.go @@ -1181,3 +1181,59 @@ func TestGetGtpIdentityPartition(t *testing.T) { }) } } + +func TestGenerateUniqueNameForVS(t *testing.T) { + initConfig(true, true) + + testCases := []struct { + name string + vsName string + nsName string + expected string + }{ + { + name: "Given valid namespace name and virtual service name" + + "Should return valid vsname ", + vsName: "test-vs", + nsName: "test-ns", + expected: "test-ns-test-vs", + }, + { + name: "Given valid namespace name and nil virtual service name" + + "Should return valid vsname with only namespace name ", + vsName: "", + nsName: "test-ns", + expected: "test-ns", + }, + { + name: "Given nil namespace name and valid virtual service name" + + "Should return valid vsname with only vs name ", + vsName: "test-vs", + nsName: "", + expected: "test-vs", + }, + { + name: "Given valid namespace name and virtual service name over 250 chars long" + + "Should return valid truncated vsname", + vsName: "mbjwbsaabyxpitryeptqtgcwfkseodgqgvoccktivzfqlzdbxhctplqhqixuxpeqjcsrgzaxxbfphasmrkpkyeosxxljmsqalmfublwcecztwgdtkulcrfwiwqqza123", + nsName: "mbjwbsaabyxpitryeptqtgcwfkseodgqgvoccktivzfqlzdbxhctplqhqixuxpeqjcsrgzaxxbfphasmrkpkyeosxxljmsqalmfublwcecztwgdtkulcrfwiwqqza123", + expected: "mbjwbsaabyxpitryeptqtgcwfkseodgqgvoccktivzfqlzdbxhctplqhqixuxpeqjcsrgzaxxbfphasmrkpkyeosxxljmsqalmfublwcecztwgdtkulcrfwiwqqza123-mbjwbsaabyxpitryeptqtgcwfkseodgqgvoccktivzfqlzdbxhctplqhqixuxpeqjcsrgzaxxbfphasmrkpkyeosxxljmsqalmfublwcecztwgdtkulcrfwiw", + }, + { + name: "Given nil namespace name and nil virtual service name" + + "Should return nil vsname ", + vsName: "", + nsName: "", + expected: "", + }, + } + + for _, c := range testCases { + t.Run(c.name, func(t *testing.T) { + iVal := GenerateUniqueNameForVS(c.nsName, c.vsName) + if !(iVal == c.expected) { + t.Errorf("Wanted value: %s, got: %s", c.expected, iVal) + } + }) + } +}