diff --git a/go.mod b/go.mod index eeacba36..1649ca6b 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( github.com/aws/aws-sdk-go v1.29.29 github.com/davecgh/go-spew v1.1.1 github.com/google/go-cmp v0.4.0 - github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.5.1 github.com/stretchr/testify v1.5.1 go.uber.org/zap v1.14.1 diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index aea06f58..a7360e13 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -299,9 +299,8 @@ func (c *DaemonSetController) HaveDependenciesChanged(cd *flaggerv1.Canary) (boo //Finalize scale the reference instance from zero func (c *DaemonSetController) Finalize(cd *flaggerv1.Canary) error { - if err := c.ScaleFromZero(cd); err != nil { - return err + return fmt.Errorf("ScaleFromZero failed: %w", err) } return nil } diff --git a/pkg/canary/daemonset_controller_test.go b/pkg/canary/daemonset_controller_test.go index 5ced78c0..cddfcd31 100644 --- a/pkg/canary/daemonset_controller_test.go +++ b/pkg/canary/daemonset_controller_test.go @@ -207,6 +207,5 @@ func TestDaemonSetController_Finalize(t *testing.T) { require.NoError(t, err) _, ok := dep.Spec.Template.Spec.NodeSelector["flagger.app/scale-to-zero"] - assert.False(t, ok) } diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 82c85e0c..c8410e27 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -179,27 +179,6 @@ func (c *DeploymentController) ScaleFromZero(cd *flaggerv1.Canary) error { return nil } -// Scale sets the canary deployment replicas -func (c *DeploymentController) Scale(cd *flaggerv1.Canary, replicas int32) error { - targetName := cd.Spec.TargetRef.Name - dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) - if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) - } - - depCopy := dep.DeepCopy() - depCopy.Spec.Replicas = int32p(replicas) - - _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Update(depCopy) - if err != nil { - return fmt.Errorf("scaling %s.%s to %v failed: %v", depCopy.GetName(), depCopy.Namespace, replicas, err) - } - return nil -} - // GetMetadata returns the pod label selector and svc ports func (c *DeploymentController) GetMetadata(cd *flaggerv1.Canary) (string, map[string]int32, error) { targetName := cd.Spec.TargetRef.Name @@ -403,33 +382,48 @@ func (c *DeploymentController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bo // update the reference deployment replicas to the primary replicas func (c *DeploymentController) Finalize(cd *flaggerv1.Canary) error { - primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) - - //Get ref deployment + // get ref deployment refDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("deplyoment %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } - //2. Get primary if possible, if not scale from zero + // get primary if possible, if not scale from zero + primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) primaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { if errors.IsNotFound(err) { if err := c.ScaleFromZero(cd); err != nil { - return err + return fmt.Errorf("ScaleFromZero failed: %w", err) } return nil } - return err + return fmt.Errorf("deplyoment %s.%s get query error: %w", primaryName, cd.Namespace, err) } - //3. If both ref and primary present update the replicas of the ref to match the primary + // if both ref and primary present update the replicas of the ref to match the primary if refDep.Spec.Replicas != primaryDep.Spec.Replicas { - //3. Set the replicas value on the original reference deployment - if err := c.Scale(cd, int32Default(primaryDep.Spec.Replicas)); err != nil { - return err + // set the replicas value on the original reference deployment + if err := c.scale(cd, int32Default(primaryDep.Spec.Replicas)); err != nil { + return fmt.Errorf("scale failed: %w", err) } } + return nil +} +// Scale sets the canary deployment replicas +func (c *DeploymentController) scale(cd *flaggerv1.Canary, replicas int32) error { + targetName := cd.Spec.TargetRef.Name + dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("deployment %s.%s query error: %w", targetName, cd.Namespace, err) + } + + depCopy := dep.DeepCopy() + depCopy.Spec.Replicas = int32p(replicas) + _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Update(depCopy) + if err != nil { + return fmt.Errorf("scaling %s.%s to %v failed: %w", depCopy.GetName(), depCopy.Namespace, replicas, err) + } return nil } diff --git a/pkg/canary/deployment_controller_test.go b/pkg/canary/deployment_controller_test.go index d2c7631c..969df006 100644 --- a/pkg/canary/deployment_controller_test.go +++ b/pkg/canary/deployment_controller_test.go @@ -187,32 +187,24 @@ func TestDeploymentController_Finalize(t *testing.T) { mocks := newDeploymentFixture() for _, tc := range []struct { - mocks deploymentControllerFixture - callInitialize bool - shouldError bool - expectedReplicas int32 - canary *flaggerv1.Canary + mocks deploymentControllerFixture + callInitialize bool + canary *flaggerv1.Canary }{ // primary not found returns error - {mocks, false, false, 1, mocks.canary}, + {mocks, false, mocks.canary}, // happy path - {mocks, true, false, 1, mocks.canary}, + {mocks, true, mocks.canary}, } { if tc.callInitialize { mocks.initializeCanary(t) } err := mocks.controller.Finalize(tc.canary) - if tc.shouldError { - require.Error(t, err) - } else { - require.NoError(t, err) - } + require.NoError(t, err) - if tc.expectedReplicas > 0 { - c, err := mocks.kubeClient.AppsV1().Deployments(mocks.canary.Namespace).Get(mocks.canary.Name, metav1.GetOptions{}) - require.NoError(t, err) - require.Equal(t, tc.expectedReplicas, *c.Spec.Replicas) - } + c, err := mocks.kubeClient.AppsV1().Deployments(mocks.canary.Namespace).Get(mocks.canary.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, int32(1), *c.Spec.Replicas) } } diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index a971440f..75671308 100644 --- a/pkg/canary/service_controller.go +++ b/pkg/canary/service_controller.go @@ -222,6 +222,6 @@ func (c *ServiceController) IsCanaryReady(_ *flaggerv1.Canary) (bool, error) { return true, nil } -func (c *ServiceController) Finalize(cd *flaggerv1.Canary) error { +func (c *ServiceController) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/canary/status.go b/pkg/canary/status.go index cfe1d657..65b77902 100644 --- a/pkg/canary/status.go +++ b/pkg/canary/status.go @@ -56,7 +56,7 @@ func setStatusFailedChecks(flaggerClient clientset.Interface, cd *flaggerv1.Cana name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { if !firstTry { - cd, err = flaggerClient.FlaggerV1beta1().Canaries(name).Get(ns, metav1.GetOptions{}) + cd, err = flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } @@ -82,7 +82,7 @@ func setStatusWeight(flaggerClient clientset.Interface, cd *flaggerv1.Canary, va name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { if !firstTry { - cd, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) + cd, err = flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } @@ -108,7 +108,7 @@ func setStatusIterations(flaggerClient clientset.Interface, cd *flaggerv1.Canary name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { if !firstTry { - cd, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) + cd, err = flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } @@ -136,7 +136,7 @@ func setStatusPhase(flaggerClient clientset.Interface, cd *flaggerv1.Canary, pha name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { if !firstTry { - cd, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) + cd, err = flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 9708909b..13dda8fe 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -128,19 +128,19 @@ func NewController( } ctrl.enqueue(new) - } else if !newCanary.DeletionTimestamp.IsZero() && hasFinalizer(&newCanary, finalizer) || - !hasFinalizer(&newCanary, finalizer) && newCanary.Spec.RevertOnDeletion { - //If this was marked for deletion and has finalizers enqueue for finalizing or - //If this canary doesn't have finalizers and RevertOnDeletion is true updated speck enqueue + } else if !newCanary.DeletionTimestamp.IsZero() && hasFinalizer(&newCanary) || + !hasFinalizer(&newCanary) && newCanary.Spec.RevertOnDeletion { + // If this was marked for deletion and has finalizers enqueue for finalizing or + // if this canary doesn't have finalizers and RevertOnDeletion is true updated speck enqueue ctrl.enqueue(new) } - //If canary no longer desires reverting, finalizers should be removed + // If canary no longer desires reverting, finalizers should be removed if oldCanary.Spec.RevertOnDeletion && !newCanary.Spec.RevertOnDeletion { ctrl.logger.Infof("%s.%s opting out, deleting finalizers", newCanary.Name, newCanary.Namespace) - err := ctrl.removeFinalizer(&newCanary, finalizer) + err := ctrl.removeFinalizer(&newCanary) if err != nil { - ctrl.logger.Warnf("Failed to remove finalizers for %s.%s", oldCanary.Name, oldCanary.Namespace) + ctrl.logger.Warnf("Failed to remove finalizers for %s.%s: %v", oldCanary.Name, oldCanary.Namespace, err) return } } @@ -232,27 +232,27 @@ func (c *Controller) syncHandler(key string) error { return nil } - //Finalize if canary has been marked for deletion and revert is desired + // Finalize if canary has been marked for deletion and revert is desired if cd.Spec.RevertOnDeletion && cd.ObjectMeta.DeletionTimestamp != nil { - //If finalizers have been previously removed proceed - if !hasFinalizer(cd, finalizer) { + // If finalizers have been previously removed proceed + if !hasFinalizer(cd) { c.logger.Infof("Canary %s.%s has been finalized", cd.Name, cd.Namespace) return nil } if cd.Status.Phase != flaggerv1.CanaryPhaseTerminated { if err := c.finalize(cd); err != nil { - return fmt.Errorf("unable to finalize to canary %s.%s error %s", cd.Name, cd.Namespace, err) + return fmt.Errorf("unable to finalize to canary %s.%s error: %w", cd.Name, cd.Namespace, err) } } - //Remove finalizer from Canary - if err := c.removeFinalizer(cd, finalizer); err != nil { - return fmt.Errorf("unable to remove finalizer for canary %s.%s", cd.Name, cd.Namespace) + // Remove finalizer from Canary + if err := c.removeFinalizer(cd); err != nil { + return fmt.Errorf("unable to remove finalizer for canary %s.%s: %w", cd.Name, cd.Namespace, err) } - //record event + // record event c.recordEventInfof(cd, "Terminated canary %s.%s", cd.Name, cd.Namespace) c.logger.Infof("Canary %s.%s has been successfully processed and marked for deletion", cd.Name, cd.Namespace) @@ -276,10 +276,10 @@ func (c *Controller) syncHandler(key string) error { c.canaries.Store(fmt.Sprintf("%s.%s", cd.Name, cd.Namespace), cd) - //If opt in for revertOnDeletion add finaliers if not present - if cd.Spec.RevertOnDeletion && !hasFinalizer(cd, finalizer) { - if err := c.addFinalizer(cd, finalizer); err != nil { - return fmt.Errorf("unable to add finalizer to canary %s.%s", cd.Name, cd.Namespace) + // If opt in for revertOnDeletion add finalizer if not present + if cd.Spec.RevertOnDeletion && !hasFinalizer(cd) { + if err := c.addFinalizer(cd); err != nil { + return fmt.Errorf("unable to add finalizer to canary %s.%s: %w", cd.Name, cd.Namespace, err) } } diff --git a/pkg/controller/finalizer.go b/pkg/controller/finalizer.go index 3c631026..1a4ce055 100644 --- a/pkg/controller/finalizer.go +++ b/pkg/controller/finalizer.go @@ -3,215 +3,165 @@ package controller import ( "fmt" - ex "github.com/pkg/errors" - flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" - "github.com/weaveworks/flagger/pkg/canary" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/util/retry" + + flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" ) const finalizer = "finalizer.flagger.app" func (c *Controller) finalize(old interface{}) error { - var r *flaggerv1.Canary - var ok bool - //Ensure interface is a canary - if r, ok = old.(*flaggerv1.Canary); !ok { - c.logger.Warnf("Received unexpected object: %v", old) - return nil + r, ok := old.(*flaggerv1.Canary) + if !ok { + return fmt.Errorf("received unexpected object: %v", old) } _, err := c.flaggerClient.FlaggerV1beta1().Canaries(r.Namespace).Get(r.Name, metav1.GetOptions{}) if err != nil { - c.logger.With("canary", fmt.Sprintf("%s.%s", r.Name, r.Namespace)). - Errorf("Canary %s.%s not found nothing to finalize", r.Name, r.Namespace) - return nil + return fmt.Errorf("get query error: %w", err) } - //Retrieve a controller + // Retrieve a controller canaryController := c.canaryFactory.Controller(r.Spec.TargetRef.Kind) - //Set the status to terminating if not already in that state + // Set the status to terminating if not already in that state if r.Status.Phase != flaggerv1.CanaryPhaseTerminating { if err := canaryController.SetStatusPhase(r, flaggerv1.CanaryPhaseTerminating); err != nil { - c.logger.Infof("Failed to update status to finalizing %s", err) - return err + return fmt.Errorf("failed to update status: %w", err) } - //record event + + // record event c.recordEventInfof(r, "Terminating canary %s.%s", r.Name, r.Namespace) } - err = c.revertTargetRef(canaryController, r) + err = canaryController.Finalize(r) if err != nil { - if errors.IsNotFound(err) { - //No reason to wait not found - c.logger.Warnf("%s.%s failed due to %s not found", r.Name, r.Namespace, r.Spec.TargetRef.Kind) - return nil - } - c.logger.Debugf("%s.%s failed due to %s", r.Name, r.Namespace, err) - return err - } else { - //Ensure that targetRef has met a ready state - c.logger.Infof("Checking is canary is ready %s.%s", r.Name, r.Namespace) - ready, err := canaryController.IsCanaryReady(r) - if err != nil && ready { - return fmt.Errorf("%s.%s has not reached ready state during finalizing", r.Name, r.Namespace) - } + return fmt.Errorf("failed to revert target: %w", err) + } + c.logger.Infof("%s.%s kind %s reverted", r.Name, r.Namespace, r.Spec.TargetRef.Kind) + // Ensure that targetRef has met a ready state + c.logger.Infof("Checking is canary is ready %s.%s", r.Name, r.Namespace) + _, err = canaryController.IsCanaryReady(r) + if err != nil { + return fmt.Errorf("canary not ready during finalizing: %w", err) } c.logger.Infof("%s.%s moving forward with router finalizing", r.Name, r.Namespace) labelSelector, ports, err := canaryController.GetMetadata(r) if err != nil { - c.logger.Errorf("%s.%s failed to get metadata for router finalizing", r.Name, r.Namespace) - return err - } - //Revert the router - if err := c.revertRouter(r, labelSelector, ports); err != nil { - if errors.IsNotFound(err) { - return nil - } - return err + return fmt.Errorf("failed to get metadata for router finalizing: %w", err) } - c.logger.Infof("%s.%s moving forward with mesh finalizing", r.Name, r.Namespace) - //TODO if I can't revert the mesh continue on? - //Revert the Mesh + // Revert the router + router := c.routerFactory.KubernetesRouter(r.Spec.TargetRef.Kind, labelSelector, map[string]string{}, ports) + if err := router.Finalize(r); err != nil { + return fmt.Errorf("failed revert router: %w", err) + } + c.logger.Infof("%s.%s router reverted", r.Name, r.Namespace) + + // TODO if I can't revert the mesh continue on? + // Revert the Mesh if err := c.revertMesh(r); err != nil { - if errors.IsNotFound(err) { - return nil - } - return err + return fmt.Errorf("failed to revert mesh: %w", err) } c.logger.Infof("Finalization complete for %s.%s", r.Name, r.Namespace) - return nil } -func (c *Controller) revertTargetRef(ctrl canary.Controller, r *flaggerv1.Canary) error { - if err := ctrl.Finalize(r); err != nil { - return err - } - c.logger.Infof("%s.%s kind %s reverted", r.Name, r.Namespace, r.Spec.TargetRef.Kind) - return nil -} - -//revertRouter -func (c *Controller) revertRouter(r *flaggerv1.Canary, labelSelector string, ports map[string]int32) error { - router := c.routerFactory.KubernetesRouter(r.Spec.TargetRef.Kind, labelSelector, map[string]string{}, ports) - if err := router.Finalize(r); err != nil { - c.logger.Errorf("%s.%s router failed with error %s", r.Name, r.Namespace, err) - return err - } - c.logger.Infof("Service %s.%s reverted", r.Name, r.Namespace) - return nil -} - -//revertMesh reverts defined mesh provider based upon the implementation's respective Finalize method. -//If the Finalize method encounters and error that is returned, else revert is considered successful. +// revertMesh reverts defined mesh provider based upon the implementation's respective Finalize method. +// If the Finalize method encounters and error that is returned, else revert is considered successful. func (c *Controller) revertMesh(r *flaggerv1.Canary) error { provider := c.meshProvider if r.Spec.Provider != "" { provider = r.Spec.Provider } - //Establish provider meshRouter := c.routerFactory.MeshRouter(provider) - - //Finalize mesh - err := meshRouter.Finalize(r) - if err != nil { - c.logger.Errorf("%s.%s mesh failed with error %s", r.Name, r.Namespace, err) - return err + if err := meshRouter.Finalize(r); err != nil { + return fmt.Errorf("meshRouter.Finlize failed: %w", err) } c.logger.Infof("%s.%s mesh provider %s reverted", r.Name, r.Namespace, provider) return nil } -//hasFinalizer evaluates the finalizers of a given canary for for existence of a provide finalizer string. -//It returns a boolean, true if the finalizer is found false otherwise. -func hasFinalizer(canary *flaggerv1.Canary, finalizerString string) bool { - currentFinalizers := canary.ObjectMeta.Finalizers - - for _, f := range currentFinalizers { - if f == finalizerString { +// hasFinalizer evaluates the finalizers of a given canary for for existence of a provide finalizer string. +// It returns a boolean, true if the finalizer is found false otherwise. +func hasFinalizer(canary *flaggerv1.Canary) bool { + for _, f := range canary.ObjectMeta.Finalizers { + if f == finalizer { return true } } return false } -//addFinalizer adds a provided finalizer to the specified canary resource. -//If failures occur the error will be returned otherwise the action is deemed successful -//and error will be nil. -func (c *Controller) addFinalizer(canary *flaggerv1.Canary, finalizerString string) error { +// addFinalizer adds a provided finalizer to the specified canary resource. +// If failures occur the error will be returned otherwise the action is deemed successful +// and error will be nil. +func (c *Controller) addFinalizer(canary *flaggerv1.Canary) error { firstTry := true + name, ns := canary.GetName(), canary.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - - var selErr error if !firstTry { - canary, selErr = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Get(canary.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + canary, err = c.flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } - copy := canary.DeepCopy() - copy.ObjectMeta.Finalizers = append(copy.ObjectMeta.Finalizers, finalizerString) - - _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(copy) - + cCopy := canary.DeepCopy() + cCopy.ObjectMeta.Finalizers = append(cCopy.ObjectMeta.Finalizers, finalizer) + _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(cCopy) firstTry = false return }) if err != nil { - c.logger.Errorf("Failed to add finalizer %s", err) - return ex.Wrap(err, "Add finalizer failed") + return fmt.Errorf("failed after retries: %w", err) } return nil } -//removeFinalizer removes a provided finalizer to the specified canary resource. -//If failures occur the error will be returned otherwise the action is deemed successful -//and error will be nil. -func (c *Controller) removeFinalizer(canary *flaggerv1.Canary, finalizerString string) error { +// removeFinalizer removes a provided finalizer to the specified canary resource. +// If failures occur the error will be returned otherwise the action is deemed successful +// and error will be nil. +func (c *Controller) removeFinalizer(canary *flaggerv1.Canary) error { firstTry := true + name, ns := canary.GetName(), canary.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - - var selErr error if !firstTry { - canary, selErr = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Get(canary.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + canary, err = c.flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } - copy := canary.DeepCopy() - newSlice := make([]string, 0) - for _, item := range copy.ObjectMeta.Finalizers { - if item == finalizerString { - continue + cCopy := canary.DeepCopy() + + nfs := make([]string, 0, len(cCopy.ObjectMeta.Finalizers)) + for _, item := range cCopy.ObjectMeta.Finalizers { + if item != finalizer { + nfs = append(nfs, item) } - newSlice = append(newSlice, item) } - if len(newSlice) == 0 { - newSlice = nil + + if len(nfs) == 0 { + nfs = nil } - copy.ObjectMeta.Finalizers = newSlice - - _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(copy) + cCopy.ObjectMeta.Finalizers = nfs + _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(cCopy) firstTry = false return }) if err != nil { - return ex.Wrap(err, "Remove finalizer failed") + return fmt.Errorf("failed after retries: %w", err) } return nil } diff --git a/pkg/controller/finalizer_test.go b/pkg/controller/finalizer_test.go index 02d18907..696e8cd4 100644 --- a/pkg/controller/finalizer_test.go +++ b/pkg/controller/finalizer_test.go @@ -2,77 +2,56 @@ package controller import ( "fmt" + "testing" - flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" - fakeFlagger "github.com/weaveworks/flagger/pkg/client/clientset/versioned/fake" - "github.com/weaveworks/flagger/pkg/logger" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime" k8sTesting "k8s.io/client-go/testing" - "testing" + flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" + fakeFlagger "github.com/weaveworks/flagger/pkg/client/clientset/versioned/fake" ) -//Test has finalizers func TestFinalizer_hasFinalizer(t *testing.T) { + c := newDeploymentTestCanary() + require.False(t, hasFinalizer(c)) - withFinalizer := newDeploymentTestCanary() - withFinalizer.Finalizers = append(withFinalizer.Finalizers, finalizer) - - tables := []struct { - canary *flaggerv1.Canary - result bool - }{ - {newDeploymentTestCanary(), false}, - {withFinalizer, true}, - } - - for _, table := range tables { - isPresent := hasFinalizer(table.canary, finalizer) - if isPresent != table.result { - t.Errorf("Result of hasFinalizer returned [%t], but expected [%t]", isPresent, table.result) - } - } + c.Finalizers = append(c.Finalizers, finalizer) + require.True(t, hasFinalizer(c)) } func TestFinalizer_addFinalizer(t *testing.T) { - mockError := fmt.Errorf("failed to add finalizer to canary %s", "testCanary") cs := fakeFlagger.NewSimpleClientset(newDeploymentTestCanary()) - //prepend so it is evaluated over the catch all * + // prepend so it is evaluated over the catch all * cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, mockError + return true, nil, fmt.Errorf("failed to add finalizer to canary %s", "testCanary") }) - logger, _ := logger.NewLogger("debug") m := fixture{ canary: newDeploymentTestCanary(), flaggerClient: cs, - ctrl: &Controller{ - flaggerClient: cs, - logger: logger, - }, - logger: logger, + ctrl: &Controller{flaggerClient: cs}, } tables := []struct { mock fixture canary *flaggerv1.Canary - error error + expErr bool }{ - {newDeploymentFixture(nil), newDeploymentTestCanary(), nil}, - {m, m.canary, mockError}, + {newDeploymentFixture(nil), newDeploymentTestCanary(), false}, + {m, m.canary, true}, } for _, table := range tables { - response := table.mock.ctrl.addFinalizer(table.canary, finalizer) + err := table.mock.ctrl.addFinalizer(table.canary) - if table.error != nil && response == nil { - t.Errorf("Expected an error from addFinalizer, but wasn't present") - } else if table.error == nil && response != nil { - t.Errorf("Expected no error from addFinalizer, but returned error %s", response) + if table.expErr { + require.NotNil(t, err) + } else { + require.Nil(t, err) } } - } func TestFinalizer_removeFinalizer(t *testing.T) { @@ -80,11 +59,10 @@ func TestFinalizer_removeFinalizer(t *testing.T) { withFinalizer := newDeploymentTestCanary() withFinalizer.Finalizers = append(withFinalizer.Finalizers, finalizer) - mockError := fmt.Errorf("failed to add finalizer to canary %s", "testCanary") cs := fakeFlagger.NewSimpleClientset(newDeploymentTestCanary()) - //prepend so it is evaluated over the catch all * + // prepend so it is evaluated over the catch all * cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, mockError + return true, nil, fmt.Errorf("failed to add finalizer to canary %s", "testCanary") }) m := fixture{ canary: withFinalizer, @@ -95,20 +73,18 @@ func TestFinalizer_removeFinalizer(t *testing.T) { tables := []struct { mock fixture canary *flaggerv1.Canary - error error + expErr bool }{ - {newDeploymentFixture(nil), withFinalizer, nil}, - {m, m.canary, mockError}, + {newDeploymentFixture(nil), withFinalizer, false}, + {m, m.canary, true}, } for _, table := range tables { - response := table.mock.ctrl.removeFinalizer(table.canary, finalizer) - - if table.error != nil && response == nil { - t.Errorf("Expected an error from addFinalizer, but wasn't present") - } else if table.error == nil && response != nil { - t.Errorf("Expected no error from addFinalizer, but returned error %s", response) + err := table.mock.ctrl.removeFinalizer(table.canary) + if table.expErr { + require.NotNil(t, err) + } else { + require.Nil(t, err) } - } } diff --git a/pkg/router/appmesh.go b/pkg/router/appmesh.go index ac7335dc..9ec1e144 100644 --- a/pkg/router/appmesh.go +++ b/pkg/router/appmesh.go @@ -483,7 +483,7 @@ func (ar *AppMeshRouter) gatewayAnnotations(canary *flaggerv1.Canary) map[string return a } -func (ar *AppMeshRouter) Finalize(canary *flaggerv1.Canary) error { +func (ar *AppMeshRouter) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/contour.go b/pkg/router/contour.go index 0b362cb9..8540b1de 100644 --- a/pkg/router/contour.go +++ b/pkg/router/contour.go @@ -418,6 +418,6 @@ func (cr *ContourRouter) makeLinkerdHeaderValue(canary *flaggerv1.Canary, servic } -func (cr *ContourRouter) Finalize(canary *flaggerv1.Canary) error { +func (cr *ContourRouter) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/gloo.go b/pkg/router/gloo.go index 3eab5f0a..2f224a5d 100644 --- a/pkg/router/gloo.go +++ b/pkg/router/gloo.go @@ -186,6 +186,6 @@ func (gr *GlooRouter) SetRoutes( return nil } -func (gr *GlooRouter) Finalize(canary *flaggerv1.Canary) error { +func (gr *GlooRouter) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/ingress.go b/pkg/router/ingress.go index 14d2b03f..75f9deab 100644 --- a/pkg/router/ingress.go +++ b/pkg/router/ingress.go @@ -238,6 +238,6 @@ func (i *IngressRouter) GetAnnotationWithPrefix(suffix string) string { return fmt.Sprintf("%v/%v", i.annotationsPrefix, suffix) } -func (i *IngressRouter) Finalize(canary *flaggerv1.Canary) error { +func (i *IngressRouter) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/istio.go b/pkg/router/istio.go index 54718319..8ac3cf66 100644 --- a/pkg/router/istio.go +++ b/pkg/router/istio.go @@ -366,33 +366,27 @@ func (ir *IstioRouter) SetRoutes( } func (ir *IstioRouter) Finalize(canary *flaggerv1.Canary) error { - - //Need to see if I can get the annotation orig-configuration + // Need to see if I can get the annotation orig-configuration apexName, _, _ := canary.GetServiceNames() vs, err := ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("VirtualService %s.%s get query error: %w", apexName, canary.Namespace, err) } var storedSpec istiov1alpha3.VirtualServiceSpec - //If able to get and unMarshal update the spec if a, ok := vs.ObjectMeta.Annotations[kubectlAnnotation]; ok { var storedVS istiov1alpha3.VirtualService - err := json.Unmarshal([]byte(a), &storedVS) - if err != nil { - return fmt.Errorf("VirtualService %s.%s failed to unMarshal annotation %s, unable to revert", + if err := json.Unmarshal([]byte(a), &storedVS); err != nil { + return fmt.Errorf("VirtualService %s.%s failed to unMarshal annotation %s", apexName, canary.Namespace, kubectlAnnotation) } storedSpec = storedVS.Spec } else if a, ok := vs.ObjectMeta.Annotations[configAnnotation]; ok { - var spec istiov1alpha3.VirtualServiceSpec - err := json.Unmarshal([]byte(a), &spec) - if err != nil { - return fmt.Errorf("VirtualService %s.%s failed to unMarshal annotation %s, unable to revert", + if err := json.Unmarshal([]byte(a), &storedSpec); err != nil { + return fmt.Errorf("VirtualService %s.%s failed to unMarshal annotation %s", apexName, canary.Namespace, configAnnotation) } - storedSpec = spec } else { ir.logger.Warnf("VirtualService %s.%s original configuration not found, unable to revert", apexName, canary.Namespace) return nil @@ -403,9 +397,8 @@ func (ir *IstioRouter) Finalize(canary *flaggerv1.Canary) error { _, err = ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Update(clone) if err != nil { - return fmt.Errorf("VirtualService %s.%s update error %v, unable to revert", apexName, canary.Namespace, err) + return fmt.Errorf("VirtualService %s.%s update error: %w", apexName, canary.Namespace, err) } - return nil } diff --git a/pkg/router/istio_test.go b/pkg/router/istio_test.go index 3e592630..cfe55785 100644 --- a/pkg/router/istio_test.go +++ b/pkg/router/istio_test.go @@ -5,8 +5,6 @@ import ( "fmt" "testing" - "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -379,72 +377,54 @@ func TestIstioRouter_Finalize(t *testing.T) { callReconcile bool annotation string }{ - //VS not found + // VS not found {router: router, spec: nil, shouldError: true, createVS: false, canary: mocks.canary, callReconcile: false, annotation: ""}, - //No annotation found but still finalizes + // No annotation found but still finalizes {router: router, spec: nil, shouldError: false, createVS: false, canary: mocks.canary, callReconcile: true, annotation: ""}, - //Spec should match annotation after finalize + // Spec should match annotation after finalize {router: router, spec: flaggerSpec, shouldError: false, createVS: true, canary: mocks.canary, callReconcile: true, annotation: "flagger"}, - //Need to test kubectl annotation + // Need to test kubectl annotation {router: router, spec: kubectlSpec, shouldError: false, createVS: true, canary: mocks.canary, callReconcile: true, annotation: "kubectl"}, } for _, table := range tables { - var err error - if table.createVS { vs, err := router.istioClient.NetworkingV1alpha3().VirtualServices(table.canary.Namespace).Get(table.canary.Name, metav1.GetOptions{}) - if err != nil { - t.Fatal(err.Error()) - } + require.NoError(t, err) if vs.Annotations == nil { vs.Annotations = make(map[string]string) } - if table.annotation == "flagger" { + switch table.annotation { + case "flagger": b, err := json.Marshal(table.spec) - if err != nil { - t.Fatal(err.Error()) - } - + require.NoError(t, err) vs.Annotations[configAnnotation] = string(b) - } else if table.annotation == "kubectl" { + case "kubectl": vs.Annotations[kubectlAnnotation] = `{"apiVersion": "networking.istio.io/v1alpha3","kind": "VirtualService","metadata": {"annotations": {},"name": "podinfo","namespace": "test"}, "spec": {"gateways": ["ingressgateway.istio-system.svc.cluster.local"],"hosts": ["podinfo"],"http": [{"route": [{"destination": {"host": "podinfo"}}]}]}}` - } _, err = router.istioClient.NetworkingV1alpha3().VirtualServices(table.canary.Namespace).Update(vs) - if err != nil { - t.Fatal(err.Error()) - } + require.NoError(t, err) } if table.callReconcile { err = router.Reconcile(table.canary) - if err != nil { - t.Fatal(err.Error()) - - } + require.NoError(t, err) } err = router.Finalize(table.canary) - - if table.shouldError && err == nil { - t.Errorf("Expected error from Finalize but error was not returned") - } else if !table.shouldError && err != nil { - t.Errorf("Expected no error from Finalize but error was returned %s", err) - } else if table.spec != nil { - vs, err := router.istioClient.NetworkingV1alpha3().VirtualServices(table.canary.Namespace).Get(table.canary.Name, metav1.GetOptions{}) - if err != nil { - t.Fatal(err.Error()) - - } - if cmp.Diff(vs.Spec, *table.spec) != "" { - - t.Errorf("Expected spec %+v but recieved %+v", table.spec, vs.Spec) - } + if table.shouldError { + require.Error(t, err) + } else { + require.NoError(t, err) } + if table.spec != nil { + vs, err := router.istioClient.NetworkingV1alpha3().VirtualServices(table.canary.Namespace).Get(table.canary.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, *table.spec, vs.Spec) + } } } diff --git a/pkg/router/kubernetes_default.go b/pkg/router/kubernetes_default.go index 831d2c47..67992d08 100644 --- a/pkg/router/kubernetes_default.go +++ b/pkg/router/kubernetes_default.go @@ -178,60 +178,57 @@ func (c *KubernetesDefaultRouter) reconcileService(canary *flaggerv1.Canary, nam return nil } -//Finalize reverts the apex router if not owned by the Flagger controller. +// Finalize reverts the apex router if not owned by the Flagger controller. func (c *KubernetesDefaultRouter) Finalize(canary *flaggerv1.Canary) error { apexName, _, _ := canary.GetServiceNames() svc, err := c.kubeClient.CoreV1().Services(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("service %s.%s get query error: %w", apexName, canary.Namespace, err) } - //No need to do any reconciliation if the router is owned by the controller + // No need to do any reconciliation if the router is owned by the controller if hasCanaryOwnerRef, isOwned := c.isOwnedByCanary(svc, canary.Name); !hasCanaryOwnerRef && !isOwned { - //If kubectl annotation is present that will be utilized, else reconcile + // If kubectl annotation is present that will be utilized, else reconcile if a, ok := svc.Annotations[kubectlAnnotation]; ok { var storedSvc corev1.Service - err := json.Unmarshal([]byte(a), &storedSvc) - if err != nil { - return fmt.Errorf("router %s.%s failed to unMarshal annotation %s, unable to revert", + if err := json.Unmarshal([]byte(a), &storedSvc); err != nil { + return fmt.Errorf("router %s.%s failed to unMarshal annotation %s", svc.Name, svc.Namespace, kubectlAnnotation) } clone := svc.DeepCopy() clone.Spec.Selector = storedSvc.Spec.Selector - _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(clone) - if err != nil { + if _, err := c.kubeClient.CoreV1().Services(canary.Namespace).Update(clone); err != nil { return fmt.Errorf("service %s update error: %w", clone.Name, err) } - } else { err = c.reconcileService(canary, apexName, canary.Spec.TargetRef.Name) if err != nil { - return err + return fmt.Errorf("reconcileService failed: %w", err) } } } - return nil } -//isOwnedByCanary evaluates if an object contains an OwnerReference declaration, that is of kind Canary and -//has the same ref name as the Canary under evaluation. It returns two bool the first returns true if -//an OwnerReference is present and the second, returns if it is owned by the supplied name. +// isOwnedByCanary evaluates if an object contains an OwnerReference declaration, that is of kind Canary and +// has the same ref name as the Canary under evaluation. It returns two bool the first returns true if +// an OwnerReference is present and the second returns true if it is owned by the supplied name. func (c KubernetesDefaultRouter) isOwnedByCanary(obj interface{}, name string) (bool, bool) { - var object metav1.Object - var ok bool - if object, ok = obj.(metav1.Object); ok { - if ownerRef := metav1.GetControllerOf(object); ownerRef != nil { - if ownerRef.Kind == flaggerv1.CanaryKind { - //And the name exists return true - if name == ownerRef.Name { - return true, true - } - return true, false - } - } + object, ok := obj.(metav1.Object) + if !ok { + return false, false } - return false, false + + ownerRef := metav1.GetControllerOf(object) + if ownerRef == nil { + return false, false + } + + if ownerRef.Kind != flaggerv1.CanaryKind { + return false, false + } + + return true, ownerRef.Name == name } diff --git a/pkg/router/kubernetes_default_test.go b/pkg/router/kubernetes_default_test.go index a5efd24a..87293eed 100644 --- a/pkg/router/kubernetes_default_test.go +++ b/pkg/router/kubernetes_default_test.go @@ -127,7 +127,7 @@ func TestServiceRouter_isOwnedByCanary(t *testing.T) { isOwned bool hasOwnerRef bool }{ - //owned + // owned { svc: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -152,7 +152,7 @@ func TestServiceRouter_isOwnedByCanary(t *testing.T) { }, }, isOwned: true, hasOwnerRef: true, }, - //Owner ref but kind not Canary + // Owner ref but kind not Canary { svc: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -177,7 +177,7 @@ func TestServiceRouter_isOwnedByCanary(t *testing.T) { }, }, isOwned: false, hasOwnerRef: false, }, - //Owner ref but name doesn't match + // Owner ref but name doesn't match { svc: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -202,7 +202,7 @@ func TestServiceRouter_isOwnedByCanary(t *testing.T) { }, }, isOwned: false, hasOwnerRef: true, }, - //No ownerRef + // No ownerRef { svc: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -305,13 +305,13 @@ func TestServiceRouter_Finalize(t *testing.T) { canary *v1beta1.Canary shouldMutate bool }{ - //Won't reconcile since it is owned and would be garbage collected + // Won't reconcile since it is owned and would be garbage collected {router: router, callSetupMethods: true, shouldError: false, canary: mocks.canary, shouldMutate: false}, - //Service not found + // Service not found {router: &KubernetesDefaultRouter{kubeClient: fake.NewSimpleClientset(), logger: mocks.logger}, callSetupMethods: false, shouldError: true, canary: mocks.canary, shouldMutate: false}, - //Not owned + // Not owned {router: &KubernetesDefaultRouter{kubeClient: fake.NewSimpleClientset(svc), logger: mocks.logger}, callSetupMethods: false, shouldError: false, canary: mocks.canary, shouldMutate: true}, - //Kubectl annotation + // Kubectl annotation {router: &KubernetesDefaultRouter{kubeClient: fake.NewSimpleClientset(kubectlSvc), logger: mocks.logger}, callSetupMethods: false, shouldError: false, canary: mocks.canary, shouldMutate: true}, } @@ -319,46 +319,30 @@ func TestServiceRouter_Finalize(t *testing.T) { if table.callSetupMethods { err := table.router.Initialize(table.canary) - if err != nil { - t.Fatal(err.Error()) - } - + require.NoError(t, err) err = table.router.Reconcile(table.canary) - if err != nil { - t.Fatal(err.Error()) - } + require.NoError(t, err) } err := table.router.Finalize(table.canary) - - if table.shouldError && err == nil { - t.Error("Should have errored") - } else if !table.shouldError && err != nil { - t.Errorf("Shouldn't error but did %s", err) + if table.shouldError { + require.Error(t, err) + } else { + require.NoError(t, err) } svc, err := table.router.kubeClient.CoreV1().Services(table.canary.Namespace).Get(table.canary.Name, metav1.GetOptions{}) if err != nil { if !errors.IsNotFound(err) { - if svc.Spec.Ports[0].Name != "http" { - t.Errorf("Got svc port name %s wanted %s", svc.Spec.Ports[0].Name, "http") - } - - if svc.Spec.Ports[0].Port != 9898 { - t.Errorf("Got svc port %v wanted %v", svc.Spec.Ports[0].Port, 9898) - } + require.Equal(t, "http", svc.Spec.Ports[0].Name) + require.Equal(t, 9898, svc.Spec.Ports[0].Port) if table.shouldMutate { - if svc.Spec.Selector["app"] != table.canary.Name { - t.Errorf("Got svc selector %v wanted %v", svc.Spec.Selector["app"], table.canary.Name) - } + require.Equal(t, table.canary.Name, svc.Spec.Selector["app"]) } else { - if svc.Spec.Selector["app"] != fmt.Sprintf("%s-primary", table.canary.Name) { - t.Errorf("Got svc selector %v wanted %v", svc.Spec.Selector["app"], fmt.Sprintf("%s-primary", table.canary.Name)) - } + require.Equal(t, fmt.Sprintf("%s-primary", table.canary.Name), svc.Spec.Selector["app"]) } } } } - } diff --git a/pkg/router/kubernetes_noop.go b/pkg/router/kubernetes_noop.go index b561c686..f8f0a64a 100644 --- a/pkg/router/kubernetes_noop.go +++ b/pkg/router/kubernetes_noop.go @@ -17,6 +17,6 @@ func (c *KubernetesNoopRouter) Reconcile(_ *flaggerv1.Canary) error { return nil } -func (c *KubernetesNoopRouter) Finalize(canary *flaggerv1.Canary) error { +func (c *KubernetesNoopRouter) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/nop.go b/pkg/router/nop.go index 91d468b3..13c675a2 100644 --- a/pkg/router/nop.go +++ b/pkg/router/nop.go @@ -23,6 +23,6 @@ func (*NopRouter) GetRoutes(canary *flaggerv1.Canary) (primaryWeight int, canary return 100, 0, false, nil } -func (c *NopRouter) Finalize(canary *flaggerv1.Canary) error { +func (c *NopRouter) Finalize(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/smi.go b/pkg/router/smi.go index a20e4528..a9a4071e 100644 --- a/pkg/router/smi.go +++ b/pkg/router/smi.go @@ -225,6 +225,6 @@ func (sr *SmiRouter) getWithConvert(canary *flaggerv1.Canary, host string) (*smi return ts, nil } -func (sr *SmiRouter) Finalize(canary *flaggerv1.Canary) error { +func (sr *SmiRouter) Finalize(_ *flaggerv1.Canary) error { return nil }