feat: remove no-longer-desired edges on topology change (cleanup gating)

Signed-off-by: Shreesha001 <shettyshreesha552@gmail.com>

Author: Shreesha001

service/topology_resolver.go

@@ -67,3 +67,32 @@ func ResolveTopologyEdges(clusters []string, topology *controllerv1alpha1.Topolo
 	}
 	return edges
 }
+
+// TopologyEdgeSet answers direction-insensitive membership questions about a
+// set of desired edges: the two WorkerSliceGateway objects of a pair (the
+// server side and the client side) belong to the same logical edge.
+type TopologyEdgeSet struct {
+	members map[[2]string]bool
+}
+
+// NewTopologyEdgeSet builds a TopologyEdgeSet from resolved edges.
+func NewTopologyEdgeSet(edges []TopologyEdge) TopologyEdgeSet {
+	members := make(map[[2]string]bool, len(edges))
+	for _, edge := range edges {
+		members[edgeKey(edge.ServerCluster, edge.ClientCluster)] = true
+	}
+	return TopologyEdgeSet{members: members}
+}
+
+// Contains reports whether the given cluster pair, in either order, is a
+// desired edge.
+func (s TopologyEdgeSet) Contains(clusterA, clusterB string) bool {
+	return s.members[edgeKey(clusterA, clusterB)]
+}
+
+func edgeKey(clusterA, clusterB string) [2]string {
+	if clusterA > clusterB {
+		clusterA, clusterB = clusterB, clusterA
+	}
+	return [2]string{clusterA, clusterB}
+}

service/topology_resolver_test.go

@@ -78,3 +78,22 @@ func TestResolveTopologyEdges(t *testing.T) {
 		})
 	}
 }
+
+func TestTopologyEdgeSetContains(t *testing.T) {
+	// hub-and-spoke edges: hub=worker-1, spokes worker-2/worker-3
+	set := NewTopologyEdgeSet(ResolveTopologyEdges(
+		[]string{"worker-1", "worker-2", "worker-3"},
+		&controllerv1alpha1.TopologySpec{Mode: controllerv1alpha1.TopologyModeHubAndSpoke, Hubs: []string{"worker-1"}},
+	))
+	// desired hub<->spoke edges, both directions
+	if !set.Contains("worker-1", "worker-2") || !set.Contains("worker-2", "worker-1") {
+		t.Fatal("expected worker-1<->worker-2 to be a desired edge (either direction)")
+	}
+	if !set.Contains("worker-1", "worker-3") {
+		t.Fatal("expected worker-1<->worker-3 to be a desired edge")
+	}
+	// spoke<->spoke is NOT desired
+	if set.Contains("worker-2", "worker-3") || set.Contains("worker-3", "worker-2") {
+		t.Fatal("did not expect worker-2<->worker-3 (spoke-to-spoke) to be a desired edge")
+	}
+}

service/worker_slice_gateway_service.go

@@ -352,15 +352,15 @@ func (s *WorkerSliceGatewayService) CreateMinimumWorkerSliceGateways(ctx context
 	sliceSubnet string, clusterCidr string, sliceGwSvcTypeMap map[string]*controllerv1alpha1.SliceGatewayServiceType,
 	topology *controllerv1alpha1.TopologySpec) (ctrl.Result, error) {
 
-	err := s.cleanupObsoleteGateways(ctx, namespace, label, clusterNames, clusterMap)
+	desiredEdges := ResolveTopologyEdges(clusterNames, topology)
+	err := s.cleanupObsoleteGateways(ctx, namespace, label, clusterNames, clusterMap, NewTopologyEdgeSet(desiredEdges))
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 	if len(clusterNames) < 2 {
 		return ctrl.Result{}, nil
 	}
 
-	desiredEdges := ResolveTopologyEdges(clusterNames, topology)
 	_, err = s.createMinimumGatewaysIfNotExists(ctx, sliceName, desiredEdges, namespace, label, clusterMap, sliceSubnet, clusterCidr, sliceGwSvcTypeMap)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -381,7 +381,7 @@ func (s *WorkerSliceGatewayService) ListWorkerSliceGateways(ctx context.Context,
 
 // cleanupObsoleteGateways is a function delete outdated gateways
 func (s *WorkerSliceGatewayService) cleanupObsoleteGateways(ctx context.Context, namespace string, ownerLabel map[string]string,
-	clusters []string, clusterMap map[string]int) error {
+	clusters []string, clusterMap map[string]int, desiredEdges TopologyEdgeSet) error {
 
 	gateways, err := s.ListWorkerSliceGateways(ctx, ownerLabel, namespace)
 	if err != nil {
@@ -408,7 +408,10 @@ func (s *WorkerSliceGatewayService) cleanupObsoleteGateways(ctx context.Context,
 		clusterSource := gateway.Spec.LocalGatewayConfig.ClusterName
 		clusterDestination := gateway.Spec.RemoteGatewayConfig.ClusterName
 		gatewayExpectedNumber := s.calculateGatewayNumber(clusterMap[clusterSource], clusterMap[clusterDestination])
-		if !clusterExistMap[clusterSource] || !clusterExistMap[clusterDestination] || gatewayExpectedNumber != gateway.Spec.GatewayNumber {
+		// Delete a gateway when either cluster left the slice, its gateway number
+		// changed, or its edge is no longer in the desired topology (e.g. a
+		// spoke<->spoke link after a FullMesh->HubAndSpoke change).
+		if !clusterExistMap[clusterSource] || !clusterExistMap[clusterDestination] || gatewayExpectedNumber != gateway.Spec.GatewayNumber || !desiredEdges.Contains(clusterSource, clusterDestination) {
 			err = util.DeleteResource(ctx, &gateway)
 			if err != nil {
 				//Register an event for worker slice gateway deletion failure

service/worker_slice_gateway_service_test.go

@@ -57,16 +57,17 @@ func TestWorkerSliceGatewaySuite(t *testing.T) {
 }
 
 var WorkerSliceGatewayTestbed = map[string]func(*testing.T){
-	"TestWorkerSliceGatewayReconciliation_Success":                      testWorkerSliceGatewayReconciliationSuccess,
-	"TestWorkerSliceGatewayReconciliation_IfSliceConfigNotFound":        testWorkerSliceGatewayReconciliationIfSliceConfigNotFound,
-	"TestWorkerSliceGatewayReconciliation_IfGatewayNotFound":            testWorkerSliceGatewayReconciliationIfGatewayNotFound,
-	"TestWorkerSliceGatewayReconciliation_Delete":                       testWorkerSliceGatewayReconciliationDelete,
-	"TestWorkerSliceGatewayReconciliation_DeleteForcefully":             testWorkerSliceGatewayReconciliationDeleteForcefully,
-	"TestCreateMinimumWorkerSliceGateways_IfAlreadyExists":              testCreateMinimumWorkerSliceGatewaysAlreadyExists,
-	"TestCreateMinimumWorkerSliceGateways_HubAndSpokeSkipsSpokeToSpoke": testCreateMinimumWorkerSliceGatewaysHubAndSpokeSkipsSpokeToSpoke,
-	"TestCreateMinimumWorkerSliceGateways_IfNotExists":                  testCreateMinimumWorkerSliceGatewaysNotExists,
-	"TestDeleteWorkerSliceGatewaysByLabel_IfExists":                     testDeleteWorkerSliceGatewaysByLabelExists,
-	"TestNodeIpReconciliationOfWorkerSliceGateways_IfExists":            testNodeIpReconciliationOfWorkerSliceGatewaysExists,
+	"TestWorkerSliceGatewayReconciliation_Success":                         testWorkerSliceGatewayReconciliationSuccess,
+	"TestWorkerSliceGatewayReconciliation_IfSliceConfigNotFound":           testWorkerSliceGatewayReconciliationIfSliceConfigNotFound,
+	"TestWorkerSliceGatewayReconciliation_IfGatewayNotFound":               testWorkerSliceGatewayReconciliationIfGatewayNotFound,
+	"TestWorkerSliceGatewayReconciliation_Delete":                          testWorkerSliceGatewayReconciliationDelete,
+	"TestWorkerSliceGatewayReconciliation_DeleteForcefully":                testWorkerSliceGatewayReconciliationDeleteForcefully,
+	"TestCreateMinimumWorkerSliceGateways_IfAlreadyExists":                 testCreateMinimumWorkerSliceGatewaysAlreadyExists,
+	"TestCreateMinimumWorkerSliceGateways_HubAndSpokeSkipsSpokeToSpoke":    testCreateMinimumWorkerSliceGatewaysHubAndSpokeSkipsSpokeToSpoke,
+	"TestCreateMinimumWorkerSliceGateways_HubAndSpokeCleansUpSpokeToSpoke": testCreateMinimumWorkerSliceGatewaysHubAndSpokeCleansUpSpokeToSpoke,
+	"TestCreateMinimumWorkerSliceGateways_IfNotExists":                     testCreateMinimumWorkerSliceGatewaysNotExists,
+	"TestDeleteWorkerSliceGatewaysByLabel_IfExists":                        testDeleteWorkerSliceGatewaysByLabelExists,
+	"TestNodeIpReconciliationOfWorkerSliceGateways_IfExists":               testNodeIpReconciliationOfWorkerSliceGatewaysExists,
 }
 
 func testWorkerSliceGatewayReconciliationSuccess(t *testing.T) {
@@ -369,6 +370,69 @@ func testCreateMinimumWorkerSliceGatewaysHubAndSpokeSkipsSpokeToSpoke(t *testing
 	mMock.AssertExpectations(t)
 }
 
+// testCreateMinimumWorkerSliceGatewaysHubAndSpokeCleansUpSpokeToSpoke verifies
+// that when a slice already has a spoke<->spoke gateway pair (e.g. left over from
+// a FullMesh->HubAndSpoke change), cleanup deletes it purely because its edge is
+// no longer in the desired hub-and-spoke set. The stale pair is given the CORRECT
+// gateway number and both clusters are still slice members, so the ONLY reason it
+// gets removed is the topology edge check.
+func testCreateMinimumWorkerSliceGatewaysHubAndSpokeCleansUpSpokeToSpoke(t *testing.T) {
+	_, _, _, workerSliceGatewayService, requestObj, clientMock, _, ctx, mMock := setupWorkerSliceGatewayTest("slice_gateway", "namespace")
+	label := map[string]string{}
+	clusterNames := []string{"cluster-1", "cluster-2", "cluster-3"}
+	clusterMap := map[string]int{
+		"cluster-1": 1,
+		"cluster-2": 2,
+		"cluster-3": 3,
+	}
+	topology := &controllerv1alpha1.TopologySpec{
+		Mode: controllerv1alpha1.TopologyModeHubAndSpoke,
+		Hubs: []string{"cluster-1"},
+	}
+	mMock.On("WithProject", mock.AnythingOfType("string")).Return(&metrics.MetricRecorder{}).Once()
+	// existing spoke<->spoke pair (cluster-2 <-> cluster-3) with the CORRECT gateway
+	// number; both clusters are still members, so it survives the membership and
+	// number checks and is removed only because its edge is not desired.
+	spokeToSpokeNumber := ((3-1)*(3-2))/2 + 2 // calculateGatewayNumber(2, 3) = 3
+	pairWorkerSliceGateway := &workerv1alpha1.WorkerSliceGatewayList{}
+	clientMock.On("List", ctx, pairWorkerSliceGateway, mock.Anything, client.InNamespace(requestObj.Namespace)).Return(nil).Run(func(args mock.Arguments) {
+		arg := args.Get(1).(*workerv1alpha1.WorkerSliceGatewayList)
+		arg.Items = []workerv1alpha1.WorkerSliceGateway{
+			{
+				Spec: workerv1alpha1.WorkerSliceGatewaySpec{
+					LocalGatewayConfig:  workerv1alpha1.SliceGatewayConfig{ClusterName: "cluster-2"},
+					RemoteGatewayConfig: workerv1alpha1.SliceGatewayConfig{ClusterName: "cluster-3"},
+					GatewayNumber:       spokeToSpokeNumber,
+				},
+			},
+			{
+				Spec: workerv1alpha1.WorkerSliceGatewaySpec{
+					LocalGatewayConfig:  workerv1alpha1.SliceGatewayConfig{ClusterName: "cluster-3"},
+					RemoteGatewayConfig: workerv1alpha1.SliceGatewayConfig{ClusterName: "cluster-2"},
+					GatewayNumber:       spokeToSpokeNumber,
+				},
+			},
+		}
+	}).Once()
+	clientMock.On("Delete", ctx, mock.Anything).Return(nil).Twice()
+	clientMock.On("Create", ctx, mock.AnythingOfType("*v1.Event")).Return(nil).Once()
+	mMock.On("RecordCounterMetric", mock.Anything, mock.Anything).Return().Once()
+	clientMock.On("Update", ctx, mock.AnythingOfType("*v1.Event")).Return(nil).Once()
+	mMock.On("RecordCounterMetric", mock.Anything, mock.Anything).Return().Once()
+	// create pass for the two desired hub<->spoke edges: clusters fetched, gateways
+	// already exist -> nothing created.
+	cluster := &controllerv1alpha1.Cluster{}
+	clientMock.On("Get", ctx, mock.AnythingOfType("types.NamespacedName"), cluster).Return(nil).Times(3)
+	gateway := &workerv1alpha1.WorkerSliceGateway{}
+	clientMock.On("Get", ctx, mock.AnythingOfType("types.NamespacedName"), gateway).Return(nil).Times(4)
+
+	result, err := workerSliceGatewayService.CreateMinimumWorkerSliceGateways(ctx, "red", clusterNames, requestObj.Namespace, label, clusterMap, "10.10.10.10/16", "/16", nil, topology)
+	require.Equal(t, ctrl.Result{}, result)
+	require.Nil(t, err)
+	clientMock.AssertExpectations(t)
+	mMock.AssertExpectations(t)
+}
+
 func testCreateMinimumWorkerSliceGatewaysNotExists(t *testing.T) {
 	_, _, jobMock, workerSliceGatewayService, requestObj, clientMock, _, ctx, mMock := setupWorkerSliceGatewayTest("slice_gateway", "namespace")
 	label := map[string]string{