fixed as per review comments

Signed-off-by: Shriram Sharma <[email protected]>
istio-ecosystem · Sep 18, 2024 · bf20985 · bf20985
1 parent 84e0c39
commit bf20985
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 8 deletions.
diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go
@@ -805,7 +805,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 = addUpdateVirtualServicesForSourceIngress(ctx, ctxLogger, remoteRegistry, sourceClusterToDestinations)
+	err = addUpdateVirtualServicesForIngress(ctx, ctxLogger, remoteRegistry, sourceClusterToDestinations)
 	if err != nil {
 		ctxLogger.Errorf(common.CtxLogFormat, "addUpdateVirtualServicesForSourceIngress",
 			deploymentOrRolloutName, deploymentOrRolloutNS, "", err)

diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go
@@ -448,22 +448,22 @@ func populateDestinationsForCanaryStrategy(
 // addUpdateVirtualServicesForSourceIngress adds or updates the cross-cluster routing VirtualServices
 // This is where the VirtualServices are created using the services that were discovered during the
 // discovery phase.
-func addUpdateVirtualServicesForSourceIngress(
+func addUpdateVirtualServicesForIngress(
 	ctx context.Context,
 	ctxLogger *log.Entry,
 	remoteRegistry *RemoteRegistry,
 	sourceClusterToDestinations map[string]map[string][]*networkingV1Alpha3.RouteDestination) error {
 
+	if remoteRegistry == nil {
+		return fmt.Errorf("remoteRegistry is nil")
+	}
+
 	for sourceCluster, destination := range sourceClusterToDestinations {
 
 		if !common.DoVSRoutingForCluster(sourceCluster) {
 			continue
 		}
 
-		if remoteRegistry == nil {
-			return fmt.Errorf("remoteRegistry is nil")
-		}
-
 		rc := remoteRegistry.GetRemoteController(sourceCluster)
 
 		if rc == nil {

diff --git a/admiral/pkg/clusters/virtualservice_routing_test.go b/admiral/pkg/clusters/virtualservice_routing_test.go
@@ -19,7 +19,7 @@ import (
 	metaV1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestAddUpdateVirtualServicesForSourceIngress(t *testing.T) {
+func TestAddUpdateVirtualServicesForIngress(t *testing.T) {
 
 	vsLabels := map[string]string{
 		vsRoutingLabel: "enabled",
@@ -449,7 +449,7 @@ func TestAddUpdateVirtualServicesForSourceIngress(t *testing.T) {
 			rc := rr.GetRemoteController("cluster-1")
 			rc.VirtualServiceController.IstioClient = tc.istioClient
 			rr.PutRemoteController("cluster-1", rc)
-			err := addUpdateVirtualServicesForSourceIngress(
+			err := addUpdateVirtualServicesForIngress(
 				context.Background(),
 				ctxLogger,
 				tc.remoteRegistry,

diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go
@@ -445,6 +445,9 @@ func DoVSRoutingForCluster(cluster string) bool {
 		return false
 	}
 	for _, c := range wrapper.params.VSRoutingEnabledClusters {
+		if c == "*" {
+			return true
+		}
 		if c == cluster {
 			return true
 		}

diff --git a/admiral/pkg/controller/common/config_test.go b/admiral/pkg/controller/common/config_test.go
@@ -56,6 +56,76 @@ func setupForConfigTests() {
 	}
 }
 
+func TestDoVSRoutingForCluster(t *testing.T) {
+	p := AdmiralParams{}
+
+	testCases := []struct {
+		name                      string
+		cluster                   string
+		enableVSRouting           bool
+		enableVSRoutingForCluster []string
+		expected                  bool
+	}{
+		{
+			name: "Given enableVSRouting is false, enableVSRoutingForCluster is empty" +
+				"When DoVSRoutingForCluster is called" +
+				"Then it should return false",
+			cluster:                   "cluster1",
+			enableVSRouting:           false,
+			enableVSRoutingForCluster: []string{},
+			expected:                  false,
+		},
+		{
+			name: "Given enableVSRouting is true, enableVSRoutingForCluster is empty" +
+				"When DoVSRoutingForCluster is called" +
+				"Then it should return false",
+			cluster:                   "cluster1",
+			enableVSRouting:           true,
+			enableVSRoutingForCluster: []string{},
+			expected:                  false,
+		},
+		{
+			name: "Given enableVSRouting is true, and given cluster doesn't exists in the list" +
+				"When DoVSRoutingForCluster is called" +
+				"Then it should return false",
+			cluster:                   "cluster2",
+			enableVSRouting:           true,
+			enableVSRoutingForCluster: []string{"cluster1"},
+			expected:                  false,
+		},
+		{
+			name: "Given enableVSRouting is true, and given cluster does exists in the list" +
+				"When DoVSRoutingForCluster is called" +
+				"Then it should return false",
+			cluster:                   "cluster1",
+			enableVSRouting:           true,
+			enableVSRoutingForCluster: []string{"cluster1"},
+			expected:                  true,
+		},
+		{
+			name: "Given enableVSRouting is true, and all VS routing is enabled in all clusters using '*'" +
+				"When DoVSRoutingForCluster is called" +
+				"Then it should return false",
+			cluster:                   "cluster1",
+			enableVSRouting:           true,
+			enableVSRoutingForCluster: []string{"*"},
+			expected:                  true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			p.EnableVSRouting = tc.enableVSRouting
+			p.VSRoutingEnabledClusters = tc.enableVSRoutingForCluster
+			ResetSync()
+			InitializeConfig(p)
+
+			assert.Equal(t, tc.expected, DoVSRoutingForCluster(tc.cluster))
+		})
+	}
+
+}
+
 func TestConfigManagement(t *testing.T) {
 	setupForConfigTests()