diff --git a/rollout/controller.go b/rollout/controller.go index 521d6a1528..8a39be3dd1 100644 --- a/rollout/controller.go +++ b/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{}) { diff --git a/service/service.go b/service/service.go index f890f637ef..4ab2aaa939 100644 --- a/service/service.go +++ b/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)) diff --git a/service/service_test.go b/service/service_test.go index c809855429..cb59f5452c 100644 --- a/service/service_test.go +++ b/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", diff --git a/utils/service/service.go b/utils/service/service.go index 49b507aa60..22bfc7cbaf 100644 --- a/utils/service/service.go +++ b/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 +} diff --git a/utils/service/service_test.go b/utils/service/service_test.go index 21ede0f65d..476d621ebc 100644 --- a/utils/service/service_test.go +++ b/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)) + }) +}