Fix: remove hash selector after switching from bg to canary

rollout/controller.go

@@ -169,6 +169,11 @@ func NewController(cfg ControllerConfig) *Controller {
 	cfg.RolloutsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: controller.enqueueRollout,
 		UpdateFunc: func(old, new interface{}) {
+			if r, ok := old.(*v1alpha1.Rollout); ok {
+				for _, s := range serviceutil.GetRolloutServiceKeys(r) {
+					controller.serviceWorkqueue.AddRateLimited(s)
+				}
+			}
 			controller.enqueueRollout(new)
 		},
 		DeleteFunc: func(obj interface{}) {

service/service.go

@@ -155,26 +155,37 @@ func (c *Controller) syncService(key string) error {
 	if err != nil {
 		return err
 	}
-	rollouts, err := c.getRolloutsByService(svc.Namespace, svc.Name)
-	if err != nil {
-		return nil
-	}
 
-	for i := range rollouts {
-		c.enqueueRollout(rollouts[i])
-	}
-	// Return early if the svc does not have a hash selector or there is a rollout with matching this service
-	if _, hasHashSelector := svc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]; !hasHashSelector || len(rollouts) > 0 {
+	// Return early if the svc does not have a hash selector
+	if _, hasHashSelector := svc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]; !hasHashSelector {
 		return nil
 	}
-	// Handles case where the controller is not watching all Rollouts in the cluster due to instance-ids.
+	// Handles case where the controller is not watching all Rollouts in the cluster due to instance-ids by making an
+	// API call to get Rollout and confirm it references the service.
 	rolloutName, hasManagedBy := serviceutil.HasManagedByAnnotation(svc)
 	if hasManagedBy {
-		_, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(svc.Namespace).Get(rolloutName, metav1.GetOptions{})
+		rollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(svc.Namespace).Get(rolloutName, metav1.GetOptions{})
 		if err == nil {
-			return nil
+			if serviceutil.CheckRolloutForService(rollout, svc) {
+				c.enqueueRollout(rollout)
+				return nil
+			}
+		}
+	} else {
+		// Checks if a service without a managed-by but has a hash selector doesn't have any rollouts reference it. If
+		// not, the controller removes the hash selector. This protects against case where users upgrade from a version
+		// of Argo Rollouts without managed-by. Otherwise, the has selector would just be removed.
+		rollouts, err := c.getRolloutsByService(svc.Namespace, svc.Name)
+		if err == nil {
+			for i := range rollouts {
+				if serviceutil.CheckRolloutForService(rollouts[i], svc) {
+					c.enqueueRollout(rollouts[i])
+					return nil
+				}
+			}
 		}
 	}
+
 	updatedSvc := svc.DeepCopy()
 	patch := generateRemovePatch(updatedSvc)
 	_, err = c.kubeclientset.CoreV1().Services(updatedSvc.Namespace).Patch(updatedSvc.Name, patchtypes.MergePatchType, []byte(patch))

service/service_test.go

@@ -131,7 +131,42 @@ func TestSyncServiceNotReferencedByRollout(t *testing.T) {
 	assert.True(t, ok)
 	assert.Equal(t, string(patch.GetPatch()), removeSelectorPatch)
 }
-func TestSyncServiceWithManagedBy(t *testing.T) {
+
+// TestSyncServiceWithNoManagedBy ensures a Rollout without a managed-by but has a Rollout referencing it
+// does not have the controller delete the hash selector
+func TestSyncServiceWithNoManagedBy(t *testing.T) {
+	svc := newService("test-service", 80, map[string]string{
+		v1alpha1.DefaultRolloutUniqueLabelKey: "abc",
+	})
+	ro := &v1alpha1.Rollout{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: metav1.NamespaceDefault,
+		},
+		Spec: v1alpha1.RolloutSpec{
+			Strategy: v1alpha1.RolloutStrategy{
+				BlueGreen: &v1alpha1.BlueGreenStrategy{
+					ActiveService: "test-service",
+				},
+			},
+		},
+	}
+
+	ctrl, kubeclient, client, _ := newFakeServiceController(svc, ro)
+
+	err := ctrl.syncService("default/test-service")
+	assert.NoError(t, err)
+	actions := kubeclient.Actions()
+	assert.Len(t, actions, 0)
+	// No argo api call since the controller reads from the indexer
+	argoActions := client.Actions()
+	assert.Len(t, argoActions, 0)
+}
+
+// TestSyncServiceWithManagedByWithNoRolloutReference ensures the service controller removes the
+// pod template service and managed-by annotation if the rollout listed doesn't have reference
+// the service.
+func TestSyncServiceWithManagedByWithNoRolloutReference(t *testing.T) {
 	svc := newService("test-service", 80, map[string]string{
 		v1alpha1.DefaultRolloutUniqueLabelKey: "abc",
 	})
@@ -150,7 +185,10 @@ func TestSyncServiceWithManagedBy(t *testing.T) {
 	err := ctrl.syncService("default/test-service")
 	assert.NoError(t, err)
 	actions := kubeclient.Actions()
-	assert.Len(t, actions, 0)
+	patch, ok := actions[0].(k8stesting.PatchAction)
+	assert.True(t, ok)
+	assert.Equal(t, string(patch.GetPatch()), removeSelectorAndManagedByPatch)
+	assert.Len(t, actions, 1)
 	argoActions := client.Actions()
 	assert.Len(t, argoActions, 1)
 }
@@ -159,7 +197,9 @@ func TestSyncServiceReferencedByRollout(t *testing.T) {
 	svc := newService("test-service", 80, map[string]string{
 		v1alpha1.DefaultRolloutUniqueLabelKey: "abc",
 	})
-
+	svc.Annotations = map[string]string{
+		v1alpha1.ManagedByRolloutsKey: "rollout",
+	}
 	rollout := &v1alpha1.Rollout{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "rollout",

utils/service/service.go

@@ -51,3 +51,13 @@ func HasManagedByAnnotation(service *corev1.Service) (string, bool) {
 	annotation, exists := service.Annotations[v1alpha1.ManagedByRolloutsKey]
 	return annotation, exists
 }
+
+// CheckRolloutForService Checks to if the Rollout references that service
+func CheckRolloutForService(rollout *v1alpha1.Rollout, svc *corev1.Service) bool {
+	for _, service := range GetRolloutServiceKeys(rollout) {
+		if service == fmt.Sprintf("%s/%s", svc.Namespace, svc.Name) {
+			return true
+		}
+	}
+	return false
+}

utils/service/service_test.go

@@ -93,3 +93,38 @@ func TestHasManagedByAnnotation(t *testing.T) {
 	assert.Equal(t, "test", managedBy)
 
 }
+
+func TestCheckRolloutForService(t *testing.T) {
+	ro := &v1alpha1.Rollout{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "default",
+		},
+		Spec: v1alpha1.RolloutSpec{
+			Strategy: v1alpha1.RolloutStrategy{
+				BlueGreen: &v1alpha1.BlueGreenStrategy{
+					PreviewService: "preview-service",
+					ActiveService:  "active-service",
+				},
+			},
+		},
+	}
+
+	t.Run("Rollout does not reference service", func(t *testing.T) {
+		service := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: metav1.NamespaceDefault,
+				Name:      "no-existing-service",
+			},
+		}
+		assert.False(t, CheckRolloutForService(ro, service))
+	})
+	t.Run("Rollout references Service", func(t *testing.T) {
+		service := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: metav1.NamespaceDefault,
+				Name:      "active-service",
+			},
+		}
+		assert.True(t, CheckRolloutForService(ro, service))
+	})
+}