fix duplicate vs issue in vs based routing (#366)

istio-ecosystem · Dec 16, 2024 · 710f21a · 710f21a
1 parent 39eaa5c
commit 710f21a
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 21 deletions.
diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go
@@ -824,7 +824,7 @@ func modifyServiceEntryForNewServiceOrPod(
 	// VS Based Routing
 	// Writing phase: We update the base ingress virtualservices with the RouteDestinations
 	// gathered during the discovery phase and write them to the source cluster
-	err = addUpdateVirtualServicesForIngress(ctx, ctxLogger, remoteRegistry, sourceClusterToDestinations)
+	err = addUpdateVirtualServicesForIngress(ctx, ctxLogger, remoteRegistry, sourceClusterToDestinations, cname)
 	if err != nil {
 		ctxLogger.Errorf(common.CtxLogFormat, "addUpdateVirtualServicesForIngress",
 			deploymentOrRolloutName, namespace, "", err)

diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go
@@ -455,12 +455,17 @@ func addUpdateVirtualServicesForIngress(
 	ctx context.Context,
 	ctxLogger *log.Entry,
 	remoteRegistry *RemoteRegistry,
-	sourceClusterToDestinations map[string]map[string][]*networkingV1Alpha3.RouteDestination) error {
+	sourceClusterToDestinations map[string]map[string][]*networkingV1Alpha3.RouteDestination,
+	vsName string) error {
 
 	if remoteRegistry == nil {
 		return fmt.Errorf("remoteRegistry is nil")
 	}
 
+	if vsName == "" {
+		return fmt.Errorf("vsName is empty")
+	}
+
 	for sourceCluster, destination := range sourceClusterToDestinations {
 
 		if !common.DoVSRoutingForCluster(sourceCluster) {
@@ -486,25 +491,14 @@ func addUpdateVirtualServicesForIngress(
 			return err
 		}
 
-		var vsName string
 		vsHosts := make([]string, 0)
 		tlsRoutes := make([]*networkingV1Alpha3.TLSRoute, 0)
 
 		for globalFQDN, routeDestinations := range destination {
-			hostWithoutSNIPrefix, err := getFQDNFromSNIHost(globalFQDN)
-			if err != nil {
-				ctxLogger.Warnf(common.CtxLogFormat, "addUpdateVirtualServicesForIngress",
-					"", "", sourceCluster, err.Error())
-				continue
-			}
-			if !strings.HasPrefix(hostWithoutSNIPrefix, common.BlueGreenRolloutPreviewPrefix) &&
-				!strings.HasPrefix(hostWithoutSNIPrefix, common.CanaryRolloutCanaryPrefix) {
-				vsName = hostWithoutSNIPrefix + "-routing-vs"
-			}
 			if routeDestinations == nil || len(routeDestinations) == 0 {
 				ctxLogger.Warnf(common.CtxLogFormat, "addUpdateVirtualServicesForIngress",
 					"", "", sourceCluster,
-					fmt.Sprintf("skipped adding host %s, no valid route destinaton found", hostWithoutSNIPrefix))
+					fmt.Sprintf("skipped adding host %s, no valid route destinaton found", globalFQDN))
 				continue
 			}
 			tlsRoute := networkingV1Alpha3.TLSRoute{
@@ -541,12 +535,7 @@ func addUpdateVirtualServicesForIngress(
 		})
 		virtualService.Spec.Tls = tlsRoutes
 
-		// If we were unable to find the default host in the above loop,
-		// then we pick the first one
-		if vsName == "" {
-			vsName = vsHosts[0] + "-routing-vs"
-		}
-		virtualService.Name = vsName
+		virtualService.Name = vsName + "-routing-vs"
 
 		existingVS, err := getExistingVS(ctxLogger, ctx, rc, virtualService.Name, util.IstioSystemNamespace)
 		if err != nil {

diff --git a/admiral/pkg/clusters/virtualservice_routing_test.go b/admiral/pkg/clusters/virtualservice_routing_test.go
@@ -171,6 +171,7 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 	testCases := []struct {
 		name                        string
 		remoteRegistry              *RemoteRegistry
+		vsName                      string
 		sourceClusterToDestinations map[string]map[string][]*networkingV1Alpha3.RouteDestination
 		istioClient                 *istioFake.Clientset
 		expectedError               error
@@ -183,12 +184,21 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 			sourceClusterToDestinations: sourceDestinationsWithSingleDestinationSvc,
 			expectedError:               fmt.Errorf("remoteRegistry is nil"),
 		},
+		{
+			name: "Given a empty vsName, " +
+				"When addUpdateVirtualServicesForSourceIngress is invoked, " +
+				"Then it should return an error",
+			remoteRegistry:              rr,
+			sourceClusterToDestinations: sourceDestinationsWithSingleDestinationSvc,
+			expectedError:               fmt.Errorf("vsName is empty"),
+		},
 		{
 			name: "Given a valid sourceClusterToDestinations " +
 				"And the VS is a new VS" +
 				"When addUpdateVirtualServicesForSourceIngress is invoked, " +
 				"Then it should successfully create the VS",
 			remoteRegistry:              rr,
+			vsName:                      "test-env.test-identity.global",
 			istioClient:                 istioClientWithNoExistingVS,
 			sourceClusterToDestinations: sourceDestinationsWithSingleDestinationSvc,
 			expectedError:               nil,
@@ -231,6 +241,7 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 				"When addUpdateVirtualServicesForSourceIngress is invoked, " +
 				"Then it should successfully update the VS",
 			remoteRegistry:              rr,
+			vsName:                      "test-env.test-identity.global",
 			istioClient:                 istioClientWithExistingVS,
 			sourceClusterToDestinations: sourceDestinationsWithSingleDestinationSvc,
 			expectedError:               nil,
@@ -273,6 +284,7 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 				"When addUpdateVirtualServicesForSourceIngress is invoked, " +
 				"Then it should successfully create a VS including the preview endpoint route",
 			remoteRegistry:              rr,
+			vsName:                      "test-env.test-identity.global",
 			istioClient:                 istioClientWithNoExistingVS,
 			sourceClusterToDestinations: sourceDestinationsWithPreviewSvc,
 			expectedError:               nil,
@@ -336,6 +348,7 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 				"When addUpdateVirtualServicesForSourceIngress is invoked, " +
 				"Then it should successfully create a VS including the canary endpoint routes with weights",
 			remoteRegistry:              rr,
+			vsName:                      "test-env.test-identity.global",
 			istioClient:                 istioClientWithNoExistingVS,
 			sourceClusterToDestinations: sourceDestinationsWithCanarySvc,
 			expectedError:               nil,
@@ -410,6 +423,7 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 				"When addUpdateVirtualServicesForSourceIngress is invoked, " +
 				"Then the VS created should not have the preview sniHost match in the VS",
 			remoteRegistry:              rr,
+			vsName:                      "test-env.test-identity.global",
 			istioClient:                 istioClientWithNoExistingVS,
 			sourceClusterToDestinations: sourceDestinationsWithSingleDestinationSvc,
 			expectedError:               nil,
@@ -457,7 +471,8 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 				context.Background(),
 				ctxLogger,
 				tc.remoteRegistry,
-				tc.sourceClusterToDestinations)
+				tc.sourceClusterToDestinations,
+				tc.vsName)
 			if tc.expectedError != nil {
 				require.NotNil(t, err)
 				require.Equal(t, tc.expectedError.Error(), err.Error())
@@ -468,6 +483,7 @@ func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 					VirtualServices(util.IstioSystemNamespace).
 					Get(context.Background(), "test-env.test-identity.global-routing-vs", metaV1.GetOptions{})
 				require.Nil(t, err)
+				require.Equal(t, tc.expectedVS.ObjectMeta.Name, actualVS.ObjectMeta.Name)
 				require.Equal(t, tc.expectedVS.Spec.Tls, actualVS.Spec.Tls)
 				require.Equal(t, tc.expectedVS.Spec.ExportTo, actualVS.Spec.ExportTo)
 				require.Equal(t, tc.expectedVS.Spec.Gateways, actualVS.Spec.Gateways)