From b39add9ee67cb399eafaa2a0969333cf335fa43b Mon Sep 17 00:00:00 2001 From: Tanner Altares Date: Fri, 6 Mar 2020 20:57:13 -0600 Subject: [PATCH 1/5] introduction of finalizer --- Dockerfile | 13 +- go.mod | 2 +- go.sum | 12 ++ pkg/apis/flagger/v1beta1/status.go | 6 + pkg/canary/controller.go | 1 + pkg/canary/daemonset_controller.go | 4 + pkg/canary/deployment_controller.go | 37 +++++ pkg/canary/service_controller.go | 4 + pkg/controller/finalizer.go | 221 ++++++++++++++++++++++++++++ pkg/controller/finalizer_test.go | 107 ++++++++++++++ pkg/router/kubernetes.go | 2 + pkg/router/kubernetes_default.go | 38 +++++ pkg/router/router.go | 1 + 13 files changed, 445 insertions(+), 3 deletions(-) create mode 100644 pkg/controller/finalizer.go create mode 100644 pkg/controller/finalizer_test.go diff --git a/Dockerfile b/Dockerfile index 8f693b771..557341528 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,8 +1,17 @@ FROM alpine:3.11 + +RUN echo 'https://repository.walmart.com/content/repositories/alpine-v38/community' > /etc/apk/repositories \ + && echo 'https://repository.walmart.com/content/repositories/alpine-v38/main' >> /etc/apk/repositories \ + && apk update && apk upgrade && apk --no-cache add \ + ca-certificates + RUN addgroup -S flagger \ - && adduser -S -g flagger flagger \ - && apk --no-cache add ca-certificates + && adduser -S -g flagger flagger + +#RUN addgroup -S flagger \ +# && adduser -S -g flagger flagger \ +# && apk --no-cache add ca-certificates WORKDIR /home/flagger diff --git a/go.mod b/go.mod index 75f58ddb2..ed7aaf92c 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( k8s.io/api v0.17.2 k8s.io/apimachinery v0.17.2 k8s.io/client-go v0.17.2 - k8s.io/code-generator v0.17.2 + k8s.io/code-generator v0.17.3 k8s.io/utils v0.0.0-20191114184206-e782cd3c129f ) diff --git a/go.sum b/go.sum index 923489986..b4359eb65 100644 --- a/go.sum +++ b/go.sum @@ -15,8 +15,10 @@ github.com/Masterminds/semver/v3 v3.0.3 h1:znjIyLfpXEDQjOIEWh+ehwpTU14UzUPub3c3s github.com/Masterminds/semver/v3 v3.0.3/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs= github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= github.com/PuerkitoBio/purell v1.0.0/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= +github.com/PuerkitoBio/purell v1.1.1 h1:WEQqlqaGbrPkxLJWfBwQmfEAE1Z7ONdDLqrN38tNFfI= github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= github.com/PuerkitoBio/urlesc v0.0.0-20160726150825-5bd2802263f2/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE= +github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 h1:d+Bc7a5rLufV/sSk/8dngufqelfh6jnri85riMAaF/M= github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= @@ -34,6 +36,7 @@ github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZm github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM= github.com/elazarl/goproxy v0.0.0-20170405201442-c4fc26588b6e/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= github.com/emicklei/go-restful v0.0.0-20170410110728-ff4f55a20633/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= +github.com/emicklei/go-restful v2.9.5+incompatible h1:spTtZBk5DYEvbxMVutUuTyh1Ao2r4iyvLdACqsl/Ljk= github.com/emicklei/go-restful v2.9.5+incompatible/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= github.com/evanphx/json-patch v4.2.0+incompatible h1:fUDGZCv/7iAN7u0puUVhvKCcsR6vRfwrJatElLBEf0I= github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= @@ -46,14 +49,18 @@ github.com/go-logfmt/logfmt v0.3.0 h1:8HUsc87TaSWLKwrnumgC8/YconD2fJQsRJAsWaPg2i github.com/go-logfmt/logfmt v0.3.0/go.mod h1:Qt1PoO58o5twSAckw1HlFXLmHsOX5/0LbT9GBnD5lWE= github.com/go-openapi/jsonpointer v0.0.0-20160704185906-46af16f9f7b1/go.mod h1:+35s3my2LFTysnkMfxsJBAMHj/DoqoB9knIWoYG/Vk0= github.com/go-openapi/jsonpointer v0.19.2/go.mod h1:3akKfEdA7DF1sugOqz1dVQHBcuDBPKZGEoHC/NkiQRg= +github.com/go-openapi/jsonpointer v0.19.3 h1:gihV7YNZK1iK6Tgwwsxo2rJbD1GTbdm72325Bq8FI3w= github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= github.com/go-openapi/jsonreference v0.0.0-20160704190145-13c6e3589ad9/go.mod h1:W3Z9FmVs9qj+KR4zFKmDPGiLdk1D9Rlm7cyMvf57TTg= github.com/go-openapi/jsonreference v0.19.2/go.mod h1:jMjeRr2HHw6nAVajTXJ4eiUwohSTlpa0o73RUL1owJc= +github.com/go-openapi/jsonreference v0.19.3 h1:5cxNfTy0UVC3X8JL5ymxzyoUZmo8iZb+jeTWn7tUa8o= github.com/go-openapi/jsonreference v0.19.3/go.mod h1:rjx6GuL8TTa9VaixXglHmQmIL98+wF9xc8zWvFonSJ8= github.com/go-openapi/spec v0.0.0-20160808142527-6aced65f8501/go.mod h1:J8+jY1nAiCcj+friV/PDoE1/3eeccG9LYBs0tYvLOWc= +github.com/go-openapi/spec v0.19.3 h1:0XRyw8kguri6Yw4SxhsQA/atC88yqrk0+G4YhI2wabc= github.com/go-openapi/spec v0.19.3/go.mod h1:FpwSN1ksY1eteniUU7X0N/BgJ7a4WvBFVA8Lj9mJglo= github.com/go-openapi/swag v0.0.0-20160704191624-1d0bd113de87/go.mod h1:DXUve3Dpr1UfpPtxFw+EFuQ41HhCWZfha5jSVRG7C7I= github.com/go-openapi/swag v0.19.2/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= +github.com/go-openapi/swag v0.19.5 h1:lTz6Ys4CmqqCQmZPBlbQENR1/GucA2bzYTE12Pw4tFY= github.com/go-openapi/swag v0.19.5/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= github.com/go-sql-driver/mysql v1.5.0/go.mod h1:DCzpHaOWr8IXmIStZouvnhqoel9Qv2LBy8hT2VhHyBg= github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= @@ -123,6 +130,7 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/mailru/easyjson v0.0.0-20160728113105-d5b7844b561a/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= github.com/mailru/easyjson v0.0.0-20190614124828-94de47d64c63/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= github.com/mailru/easyjson v0.0.0-20190626092158-b2ccc519800e/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= +github.com/mailru/easyjson v0.7.0 h1:aizVhC/NAAcKWb+5QsU1iNOZb4Yws5UO2I+aIprQITM= github.com/mailru/easyjson v0.7.0/go.mod h1:KAzv3t3aY1NaHWoQz1+4F1ccyAH66Jk7yos7ldAVICs= github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= @@ -297,6 +305,8 @@ gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10= +gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= k8s.io/api v0.17.2 h1:NF1UFXcKN7/OOv1uxdRz3qfra8AHsPav5M93hlV9+Dc= @@ -307,6 +317,8 @@ k8s.io/client-go v0.17.2 h1:ndIfkfXEGrNhLIgkr0+qhRguSD3u6DCmonepn1O6NYc= k8s.io/client-go v0.17.2/go.mod h1:QAzRgsa0C2xl4/eVpeVAZMvikCn8Nm81yqVx3Kk9XYI= k8s.io/code-generator v0.17.2 h1:pTwl3rLB1fUyxmvEzmVPMM0tBSdUehd7z+bDzpj4lPE= k8s.io/code-generator v0.17.2/go.mod h1:DVmfPQgxQENqDIzVR2ddLXMH34qeszkKSdH/N+s+38s= +k8s.io/code-generator v0.17.3 h1:q/hDMk2cvFzSxol7k/VA1qCssR7VSMXHQHhzuX29VJ8= +k8s.io/code-generator v0.17.3/go.mod h1:l8BLVwASXQZTo2xamW5mQNFCe1XPiAesVq7Y1t7PiQQ= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6 h1:4s3/R4+OYYYUKptXPhZKjQ04WJ6EhQQVFdjOFvCazDk= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= k8s.io/gengo v0.0.0-20190822140433-26a664648505 h1:ZY6yclUKVbZ+SdWnkfY+Je5vrMpKOxmGeKRbsXVmqYM= diff --git a/pkg/apis/flagger/v1beta1/status.go b/pkg/apis/flagger/v1beta1/status.go index b44341e4c..0c83f5a59 100644 --- a/pkg/apis/flagger/v1beta1/status.go +++ b/pkg/apis/flagger/v1beta1/status.go @@ -57,6 +57,12 @@ const ( // CanaryPhaseFailed means the canary analysis failed // and the canary deployment has been scaled to zero CanaryPhaseFailed CanaryPhase = "Failed" + // CanaryPhaseTerminating means the canary has been marked + // for deletion and in the finalizing state + CanaryPhaseTerminating CanaryPhase = "Terminating" + // CanaryPhaseTerminated means the canary has been finalized + // and successfully deleted + CanaryPhaseTerminated CanaryPhase = "Terminated" ) // CanaryStatus is used for state persistence (read-only) diff --git a/pkg/canary/controller.go b/pkg/canary/controller.go index d54c1a202..635f1af10 100644 --- a/pkg/canary/controller.go +++ b/pkg/canary/controller.go @@ -19,4 +19,5 @@ type Controller interface { HaveDependenciesChanged(canary *flaggerv1.Canary) (bool, error) ScaleToZero(canary *flaggerv1.Canary) error ScaleFromZero(canary *flaggerv1.Canary) error + Finalize(canary *flaggerv1.Canary) error } diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index 0642f6b83..e0e80d7b2 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -296,3 +296,7 @@ func (c *DaemonSetController) getSelectorLabel(daemonSet *appsv1.DaemonSet) (str func (c *DaemonSetController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) { return c.configTracker.HasConfigChanged(cd) } + +func (c *DaemonSetController) Finalize(cd *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 3a89526c9..19bc874e1 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -376,3 +376,40 @@ func (c *DeploymentController) getSelectorLabel(deployment *appsv1.Deployment) ( func (c *DeploymentController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) { return c.configTracker.HasConfigChanged(cd) } + + +// revertDeployment will set the replica count from the primary to the reference instance. This method is used +// during a delete to attempt to revert the deployment back to the original state. Error is returned if unable +// update the reference deployment replicas to the primary replicas +func (c *DeploymentController) Finalize(cd *flaggerv1.Canary) error { + + //1. Get the Primary deployment if possible + primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) + //2. Get the replicas value + primaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) + if err != nil { + /*if errors.IsNotFound(err) { + c.logger.Warnf("deployment %s.%s not found while finalizing", primaryName, cd.Namespace) + return nil + }*/ + return err + } + refDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) + if err != nil { + /*if errors.IsNotFound(err) { + c.logger.Warnf("deployment %s.%s not found while finalizing", cd.Spec.TargetRef.Name, cd.Namespace) + return nil + }*/ + return err + } + + if refDep.Spec.Replicas != primaryDep.Spec.Replicas { + //3. Set the replicas value on the original reference deployment + desiredReplicas := primaryDep.Spec.Replicas + if err := c.Scale(cd, int32Default(desiredReplicas)); err != nil { + return err + } + } + + return nil +} diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index c62949a25..fa7ff8ef1 100644 --- a/pkg/canary/service_controller.go +++ b/pkg/canary/service_controller.go @@ -221,3 +221,7 @@ func (c *ServiceController) IsPrimaryReady(_ *flaggerv1.Canary) error { func (c *ServiceController) IsCanaryReady(_ *flaggerv1.Canary) (bool, error) { return true, nil } + +func (c *ServiceController) Finalize(cd *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/controller/finalizer.go b/pkg/controller/finalizer.go new file mode 100644 index 000000000..3f7f136b3 --- /dev/null +++ b/pkg/controller/finalizer.go @@ -0,0 +1,221 @@ +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" +) + +const finalizer = "finalizer.flagger.com" + +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 + } + + //Retrieve a controller + canaryController := c.canaryFactory.Controller(r.Spec.TargetRef.Kind) + + //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 + } + //record event + c.recordEventInfof(r, "Terminating canary %s.%s", r.Name, r.Namespace) + } + + err := c.revertTargetRef(canaryController, 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.Kind) + return nil + } + c.logger.Errorf("%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) + } + + } + + c.logger.Infof("%s.%s moving forward with router finalizing", r.Name, r.Namespace) + //TODO if I can't revert continue on? + 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 + } + + 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 + if err := c.revertMesh(r); err != nil { + if errors.IsNotFound(err) { + return nil + } + return 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 + } + /*if err != nil { + if errors.IsNotFound(err) { + return false, err + } + return true, fmt.Errorf("%s.%s failed to revert deployment to original replicas", r.Name, r.Namespace) + }*/ + 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 + } + /*if err != nil { + return fmt.Errorf("%s.%s failed to revert service to original state", r.Name, r.Namespace) + }*/ + 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. +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 + } + + 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 { + 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 { + firstTry := true + 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 + } + } + + copy := canary.DeepCopy() + copy.ObjectMeta.Finalizers = append(copy.ObjectMeta.Finalizers, finalizerString) + + _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(copy) + + firstTry = false + return + }) + + if err != nil { + return ex.Wrap(err, "Remove finalizer failed") + } + 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 { + firstTry := true + 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 + } + } + copy := canary.DeepCopy() + + newSlice := make([]string, 0) + for _, item := range copy.ObjectMeta.Finalizers { + if item == finalizerString { + continue + } + newSlice = append(newSlice, item) + } + if len(newSlice) == 0 { + newSlice = nil + } + copy.ObjectMeta.Finalizers = newSlice + + _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(copy) + + firstTry = false + return + }) + + if err != nil { + return ex.Wrap(err, "Remove finalizer failed") + } + return nil + +} \ No newline at end of file diff --git a/pkg/controller/finalizer_test.go b/pkg/controller/finalizer_test.go new file mode 100644 index 000000000..b5ed4d3c9 --- /dev/null +++ b/pkg/controller/finalizer_test.go @@ -0,0 +1,107 @@ +package controller + +import ( + "fmt" + flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" + fakeFlagger "github.com/weaveworks/flagger/pkg/client/clientset/versioned/fake" + "k8s.io/apimachinery/pkg/runtime" + k8sTesting "k8s.io/client-go/testing" + + "testing" +) + +//Test has finalizers +func TestFinalizer_hasFinalizer(t *testing.T) { + + withFinalizer := newTestCanary() + withFinalizer.Finalizers = append(withFinalizer.Finalizers, finalizer) + + tables := []struct{ + canary *flaggerv1.Canary + result bool + }{ + {newTestCanary(), 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) + } + } +} + +func TestFinalizer_addFinalizer(t *testing.T) { + + mockError := fmt.Errorf("failed to add finalizer to canary %s", "testCanary") + cs := fakeFlagger.NewSimpleClientset(newTestCanary()) + //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 + }) + m := Mocks{ + canary:newTestCanary(), + flaggerClient: cs, + ctrl:&Controller{flaggerClient:cs}, + } + + + tables := []struct{ + mock Mocks + canary *flaggerv1.Canary + error error + }{ + {SetupMocks(nil), newTestCanary(), nil}, + {m, m.canary, mockError}, + } + + for _, table := range tables { + response := table.mock.ctrl.addFinalizer(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) + } + } + +} + +func TestFinalizer_removeFinalizer(t *testing.T) { + + withFinalizer := newTestCanary() + withFinalizer.Finalizers = append(withFinalizer.Finalizers, finalizer) + + mockError := fmt.Errorf("failed to add finalizer to canary %s", "testCanary") + cs := fakeFlagger.NewSimpleClientset(newTestCanary()) + //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 + }) + m := Mocks{ + canary:withFinalizer, + flaggerClient: cs, + ctrl:&Controller{flaggerClient:cs}, + } + + tables := []struct{ + mock Mocks + canary *flaggerv1.Canary + error error + }{ + {SetupMocks(nil), withFinalizer, nil}, + {m, m.canary, mockError}, + } + + 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) + } + + } +} \ No newline at end of file diff --git a/pkg/router/kubernetes.go b/pkg/router/kubernetes.go index a81a3f68a..663d4302c 100644 --- a/pkg/router/kubernetes.go +++ b/pkg/router/kubernetes.go @@ -10,4 +10,6 @@ type KubernetesRouter interface { Initialize(canary *flaggerv1.Canary) error // Reconcile creates or updates the main service Reconcile(canary *flaggerv1.Canary) error + // Revert router + Finalize(canary *flaggerv1.Canary) error } diff --git a/pkg/router/kubernetes_default.go b/pkg/router/kubernetes_default.go index 4511ecb97..2b5f6bdff 100644 --- a/pkg/router/kubernetes_default.go +++ b/pkg/router/kubernetes_default.go @@ -162,3 +162,41 @@ func (c *KubernetesDefaultRouter) reconcileService(canary *flaggerv1.Canary, nam return nil } + +func (c *KubernetesDeploymentRouter) 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 + } + + if hasCanaryOwnerRef, isOwned := c.isOwnedByCanary(svc, canary.Name); !hasCanaryOwnerRef && !isOwned { + err = c.reconcileService(canary, apexName, canary.Spec.TargetRef.Name) + if err != nil { + return 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. +func (c KubernetesDeploymentRouter) 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 + } + } + } + return false, false +} \ No newline at end of file diff --git a/pkg/router/router.go b/pkg/router/router.go index bd72e583a..5a3a414a0 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -6,4 +6,5 @@ type Interface interface { Reconcile(canary *flaggerv1.Canary) error SetRoutes(canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, mirrored bool) error GetRoutes(canary *flaggerv1.Canary) (primaryWeight int, canaryWeight int, mirrored bool, err error) + Finalize(canary *flaggerv1.Canary) error } From 92937a8f48e6f284fe1d5ec7aad362edcb39e260 Mon Sep 17 00:00:00 2001 From: Tanner Altares Date: Tue, 10 Mar 2020 10:11:47 -0500 Subject: [PATCH 2/5] kubectl annotation support rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller add unit tests for finalizing introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller run fmt to clean up formatting review changes add kubectl annotation add kubectl annotation support introduction of finalizer introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller add unit tests for finalizing introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller run fmt to clean up formatting review changes introduction of finalizer introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller add unit tests for finalizing introduction of finalizer rebase and squash fix fmt issues revert Dockerfile revert go.mod and go.sum introduction of finalizer introduction of finalizer remove test for finalizer add istio tests fix fmt issues revert go.mod and go.sum revert Dockerfile and main.go fmt deployment controller run fmt to clean up formatting review changes --- Dockerfile | 13 +- artifacts/flagger/crd.yaml | 5 + charts/flagger/crds/crd.yaml | 5 + go.mod | 2 +- go.sum | 14 +- kustomize/base/flagger/crd.yaml | 3 + pkg/apis/flagger/v1beta1/canary.go | 4 + pkg/canary/daemonset_controller.go | 2 +- pkg/canary/deployment_controller.go | 50 +++-- pkg/canary/deployment_controller_test.go | 45 ++++ pkg/canary/service_controller.go | 2 +- pkg/controller/controller.go | 50 +++++ pkg/controller/finalizer.go | 28 +-- pkg/controller/finalizer_test.go | 50 ++--- pkg/router/appmesh.go | 4 + pkg/router/contour.go | 4 + pkg/router/gloo.go | 4 + pkg/router/ingress.go | 4 + pkg/router/istio.go | 61 ++++++ pkg/router/istio_test.go | 119 +++++++++++ pkg/router/kubernetes_default.go | 33 ++- pkg/router/kubernetes_default_test.go | 258 +++++++++++++++++++++++ pkg/router/kubernetes_noop.go | 4 + pkg/router/nop.go | 4 + pkg/router/router.go | 3 + pkg/router/smi.go | 4 + 26 files changed, 682 insertions(+), 93 deletions(-) diff --git a/Dockerfile b/Dockerfile index 557341528..8f693b771 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,17 +1,8 @@ FROM alpine:3.11 - -RUN echo 'https://repository.walmart.com/content/repositories/alpine-v38/community' > /etc/apk/repositories \ - && echo 'https://repository.walmart.com/content/repositories/alpine-v38/main' >> /etc/apk/repositories \ - && apk update && apk upgrade && apk --no-cache add \ - ca-certificates - RUN addgroup -S flagger \ - && adduser -S -g flagger flagger - -#RUN addgroup -S flagger \ -# && adduser -S -g flagger flagger \ -# && apk --no-cache add ca-certificates + && adduser -S -g flagger flagger \ + && apk --no-cache add ca-certificates WORKDIR /home/flagger diff --git a/artifacts/flagger/crd.yaml b/artifacts/flagger/crd.yaml index 3ae25fa8b..a39bb578c 100644 --- a/artifacts/flagger/crd.yaml +++ b/artifacts/flagger/crd.yaml @@ -502,6 +502,9 @@ spec: skipAnalysis: description: Skip analysis and promote canary type: boolean + revertOnDeletion: + description: Revert mutated resources to original spec on deletion + type: boolean analysis: description: Canary analysis for this canary type: object @@ -650,6 +653,8 @@ spec: - Finalising - Succeeded - Failed + - Terminating + - Terminated canaryWeight: description: Traffic weight percentage routed to canary type: number diff --git a/charts/flagger/crds/crd.yaml b/charts/flagger/crds/crd.yaml index 3ae25fa8b..a39bb578c 100644 --- a/charts/flagger/crds/crd.yaml +++ b/charts/flagger/crds/crd.yaml @@ -502,6 +502,9 @@ spec: skipAnalysis: description: Skip analysis and promote canary type: boolean + revertOnDeletion: + description: Revert mutated resources to original spec on deletion + type: boolean analysis: description: Canary analysis for this canary type: object @@ -650,6 +653,8 @@ spec: - Finalising - Succeeded - Failed + - Terminating + - Terminated canaryWeight: description: Traffic weight percentage routed to canary type: number diff --git a/go.mod b/go.mod index ed7aaf92c..75f58ddb2 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( k8s.io/api v0.17.2 k8s.io/apimachinery v0.17.2 k8s.io/client-go v0.17.2 - k8s.io/code-generator v0.17.3 + k8s.io/code-generator v0.17.2 k8s.io/utils v0.0.0-20191114184206-e782cd3c129f ) diff --git a/go.sum b/go.sum index b4359eb65..dc82318c5 100644 --- a/go.sum +++ b/go.sum @@ -15,10 +15,8 @@ github.com/Masterminds/semver/v3 v3.0.3 h1:znjIyLfpXEDQjOIEWh+ehwpTU14UzUPub3c3s github.com/Masterminds/semver/v3 v3.0.3/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs= github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= github.com/PuerkitoBio/purell v1.0.0/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= -github.com/PuerkitoBio/purell v1.1.1 h1:WEQqlqaGbrPkxLJWfBwQmfEAE1Z7ONdDLqrN38tNFfI= github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbtSwDGJws/X0= github.com/PuerkitoBio/urlesc v0.0.0-20160726150825-5bd2802263f2/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE= -github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 h1:d+Bc7a5rLufV/sSk/8dngufqelfh6jnri85riMAaF/M= github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= @@ -36,7 +34,6 @@ github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZm github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM= github.com/elazarl/goproxy v0.0.0-20170405201442-c4fc26588b6e/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= github.com/emicklei/go-restful v0.0.0-20170410110728-ff4f55a20633/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= -github.com/emicklei/go-restful v2.9.5+incompatible h1:spTtZBk5DYEvbxMVutUuTyh1Ao2r4iyvLdACqsl/Ljk= github.com/emicklei/go-restful v2.9.5+incompatible/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= github.com/evanphx/json-patch v4.2.0+incompatible h1:fUDGZCv/7iAN7u0puUVhvKCcsR6vRfwrJatElLBEf0I= github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= @@ -49,18 +46,14 @@ github.com/go-logfmt/logfmt v0.3.0 h1:8HUsc87TaSWLKwrnumgC8/YconD2fJQsRJAsWaPg2i github.com/go-logfmt/logfmt v0.3.0/go.mod h1:Qt1PoO58o5twSAckw1HlFXLmHsOX5/0LbT9GBnD5lWE= github.com/go-openapi/jsonpointer v0.0.0-20160704185906-46af16f9f7b1/go.mod h1:+35s3my2LFTysnkMfxsJBAMHj/DoqoB9knIWoYG/Vk0= github.com/go-openapi/jsonpointer v0.19.2/go.mod h1:3akKfEdA7DF1sugOqz1dVQHBcuDBPKZGEoHC/NkiQRg= -github.com/go-openapi/jsonpointer v0.19.3 h1:gihV7YNZK1iK6Tgwwsxo2rJbD1GTbdm72325Bq8FI3w= github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= github.com/go-openapi/jsonreference v0.0.0-20160704190145-13c6e3589ad9/go.mod h1:W3Z9FmVs9qj+KR4zFKmDPGiLdk1D9Rlm7cyMvf57TTg= github.com/go-openapi/jsonreference v0.19.2/go.mod h1:jMjeRr2HHw6nAVajTXJ4eiUwohSTlpa0o73RUL1owJc= -github.com/go-openapi/jsonreference v0.19.3 h1:5cxNfTy0UVC3X8JL5ymxzyoUZmo8iZb+jeTWn7tUa8o= github.com/go-openapi/jsonreference v0.19.3/go.mod h1:rjx6GuL8TTa9VaixXglHmQmIL98+wF9xc8zWvFonSJ8= github.com/go-openapi/spec v0.0.0-20160808142527-6aced65f8501/go.mod h1:J8+jY1nAiCcj+friV/PDoE1/3eeccG9LYBs0tYvLOWc= -github.com/go-openapi/spec v0.19.3 h1:0XRyw8kguri6Yw4SxhsQA/atC88yqrk0+G4YhI2wabc= github.com/go-openapi/spec v0.19.3/go.mod h1:FpwSN1ksY1eteniUU7X0N/BgJ7a4WvBFVA8Lj9mJglo= github.com/go-openapi/swag v0.0.0-20160704191624-1d0bd113de87/go.mod h1:DXUve3Dpr1UfpPtxFw+EFuQ41HhCWZfha5jSVRG7C7I= github.com/go-openapi/swag v0.19.2/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= -github.com/go-openapi/swag v0.19.5 h1:lTz6Ys4CmqqCQmZPBlbQENR1/GucA2bzYTE12Pw4tFY= github.com/go-openapi/swag v0.19.5/go.mod h1:POnQmlKehdgb5mhVOsnJFsivZCEZ/vjK9gh66Z9tfKk= github.com/go-sql-driver/mysql v1.5.0/go.mod h1:DCzpHaOWr8IXmIStZouvnhqoel9Qv2LBy8hT2VhHyBg= github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= @@ -130,7 +123,6 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/mailru/easyjson v0.0.0-20160728113105-d5b7844b561a/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= github.com/mailru/easyjson v0.0.0-20190614124828-94de47d64c63/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= github.com/mailru/easyjson v0.0.0-20190626092158-b2ccc519800e/go.mod h1:C1wdFJiN94OJF2b5HbByQZoLdCWB1Yqtg26g4irojpc= -github.com/mailru/easyjson v0.7.0 h1:aizVhC/NAAcKWb+5QsU1iNOZb4Yws5UO2I+aIprQITM= github.com/mailru/easyjson v0.7.0/go.mod h1:KAzv3t3aY1NaHWoQz1+4F1ccyAH66Jk7yos7ldAVICs= github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= @@ -305,8 +297,6 @@ gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10= -gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= k8s.io/api v0.17.2 h1:NF1UFXcKN7/OOv1uxdRz3qfra8AHsPav5M93hlV9+Dc= @@ -317,8 +307,6 @@ k8s.io/client-go v0.17.2 h1:ndIfkfXEGrNhLIgkr0+qhRguSD3u6DCmonepn1O6NYc= k8s.io/client-go v0.17.2/go.mod h1:QAzRgsa0C2xl4/eVpeVAZMvikCn8Nm81yqVx3Kk9XYI= k8s.io/code-generator v0.17.2 h1:pTwl3rLB1fUyxmvEzmVPMM0tBSdUehd7z+bDzpj4lPE= k8s.io/code-generator v0.17.2/go.mod h1:DVmfPQgxQENqDIzVR2ddLXMH34qeszkKSdH/N+s+38s= -k8s.io/code-generator v0.17.3 h1:q/hDMk2cvFzSxol7k/VA1qCssR7VSMXHQHhzuX29VJ8= -k8s.io/code-generator v0.17.3/go.mod h1:l8BLVwASXQZTo2xamW5mQNFCe1XPiAesVq7Y1t7PiQQ= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6 h1:4s3/R4+OYYYUKptXPhZKjQ04WJ6EhQQVFdjOFvCazDk= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= k8s.io/gengo v0.0.0-20190822140433-26a664648505 h1:ZY6yclUKVbZ+SdWnkfY+Je5vrMpKOxmGeKRbsXVmqYM= @@ -334,4 +322,4 @@ modernc.org/strutil v1.0.0/go.mod h1:lstksw84oURvj9y3tn8lGvRxyRC1S2+g5uuIzNfIOBs modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I= sigs.k8s.io/structured-merge-diff v0.0.0-20190525122527-15d366b2352e/go.mod h1:wWxsB5ozmmv/SG7nM11ayaAW51xMvak/t1r0CSlcokI= sigs.k8s.io/yaml v1.1.0 h1:4A07+ZFc2wgJwo8YNlQpr1rVlgUDlxXHhPJciaPY5gs= -sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= +sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= \ No newline at end of file diff --git a/kustomize/base/flagger/crd.yaml b/kustomize/base/flagger/crd.yaml index 3ae25fa8b..5d6ef4dbb 100644 --- a/kustomize/base/flagger/crd.yaml +++ b/kustomize/base/flagger/crd.yaml @@ -502,6 +502,9 @@ spec: skipAnalysis: description: Skip analysis and promote canary type: boolean + revertOnDeletion: + description: Revert mutated resources to original spec on deletion + type: boolean analysis: description: Canary analysis for this canary type: object diff --git a/pkg/apis/flagger/v1beta1/canary.go b/pkg/apis/flagger/v1beta1/canary.go index a68a0a43f..24a643df9 100644 --- a/pkg/apis/flagger/v1beta1/canary.go +++ b/pkg/apis/flagger/v1beta1/canary.go @@ -94,6 +94,10 @@ type CanarySpec struct { // SkipAnalysis promotes the canary without analysing it // +optional SkipAnalysis bool `json:"skipAnalysis,omitempty"` + + // revert canary mutation on deletion of canary resource + // +optional + RevertOnDeletion bool `json:"revertOnDeletion,omitempty"` } // CanaryService defines how ClusterIP services, service mesh or ingress routing objects are generated diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index e0e80d7b2..d52f5d34d 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -297,6 +297,6 @@ func (c *DaemonSetController) HaveDependenciesChanged(cd *flaggerv1.Canary) (boo return c.configTracker.HasConfigChanged(cd) } -func (c *DaemonSetController) Finalize(cd *flaggerv1.Canary) error { +func (c *DaemonSetController) Finalize(cd *flaggerv1.Canary) error { return nil } diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 19bc874e1..9eae0cc7a 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -179,6 +179,27 @@ 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 @@ -377,36 +398,35 @@ func (c *DeploymentController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bo return c.configTracker.HasConfigChanged(cd) } - // revertDeployment will set the replica count from the primary to the reference instance. This method is used // during a delete to attempt to revert the deployment back to the original state. Error is returned if unable // update the reference deployment replicas to the primary replicas -func (c *DeploymentController) Finalize(cd *flaggerv1.Canary) error { +func (c *DeploymentController) Finalize(cd *flaggerv1.Canary) error { - //1. Get the Primary deployment if possible primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) - //2. Get the replicas value - primaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) + + //Get ref deployment + refDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { - /*if errors.IsNotFound(err) { - c.logger.Warnf("deployment %s.%s not found while finalizing", primaryName, cd.Namespace) - return nil - }*/ return err } - refDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) + + //2. Get primary if possible, if not scale from zero + primaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - /*if errors.IsNotFound(err) { - c.logger.Warnf("deployment %s.%s not found while finalizing", cd.Spec.TargetRef.Name, cd.Namespace) + if errors.IsNotFound(err) { + if err := c.ScaleFromZero(cd); err != nil { + return err + } return nil - }*/ + } return err } + //3. 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 - desiredReplicas := primaryDep.Spec.Replicas - if err := c.Scale(cd, int32Default(desiredReplicas)); err != nil { + if err := c.Scale(cd, int32Default(primaryDep.Spec.Replicas)); err != nil { return err } } diff --git a/pkg/canary/deployment_controller_test.go b/pkg/canary/deployment_controller_test.go index 0c24d8e79..9ef5e312d 100644 --- a/pkg/canary/deployment_controller_test.go +++ b/pkg/canary/deployment_controller_test.go @@ -188,3 +188,48 @@ func TestDeploymentController_HasTargetChanged(t *testing.T) { require.NoError(t, err) assert.True(t, isNew) } + +func TestCanaryDeployer_Finalize(t *testing.T) { + + mocks := newDeploymentFixture() + + tables := []struct { + mocks deploymentControllerFixture + callInitialize bool + shouldError bool + expectedReplicas int32 + canary *flaggerv1.Canary + }{ + //Primary not found returns error + {mocks, false, false, 1, mocks.canary}, + //Happy path + {mocks, true, false, 1, mocks.canary}, + } + + for _, table := range tables { + if table.callInitialize { + err := mocks.controller.Initialize(table.canary, true) + if err != nil { + t.Fatal(err.Error()) + } + } + + err := mocks.controller.Finalize(table.canary) + + if table.shouldError && err == nil { + t.Error("Expected error while calling Finalize, but none was returned") + } else if !table.shouldError && err != nil { + t.Errorf("Expected no error would be returned while calling Finalize, but returned %s", err) + } + + if table.expectedReplicas > 0 { + c, err := mocks.kubeClient.AppsV1().Deployments(mocks.canary.Namespace).Get(mocks.canary.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err.Error()) + } + if int32Default(c.Spec.Replicas) != table.expectedReplicas { + t.Errorf("Expected replicas %d recieved replicas %d", table.expectedReplicas, c.Spec.Replicas) + } + } + } +} diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index fa7ff8ef1..94e14c736 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(cd *flaggerv1.Canary) error { return nil } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 0b92c458a..37a2779a7 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -128,6 +128,21 @@ 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 + ctrl.enqueue(new) + } + + //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) + if err != nil { + ctrl.logger.Warnf("Failed to finalizers for %s.%s", oldCanary.Name, oldCanary.Namespace) + return + } } }, DeleteFunc: func(old interface{}) { @@ -217,6 +232,33 @@ func (c *Controller) syncHandler(key string) error { return nil } + //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) { + 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) + } + } + + //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) + } + + //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) + return nil + } + // set status condition for new canaries if cd.Status.Conditions == nil { if ok, conditions := canary.MakeStatusConditions(cd, flaggerv1.CanaryPhaseInitializing); ok { @@ -233,6 +275,14 @@ 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) + } + + } c.logger.Infof("Synced %s", key) return nil diff --git a/pkg/controller/finalizer.go b/pkg/controller/finalizer.go index 3f7f136b3..49301ba88 100644 --- a/pkg/controller/finalizer.go +++ b/pkg/controller/finalizer.go @@ -2,6 +2,7 @@ package controller import ( "fmt" + ex "github.com/pkg/errors" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" "github.com/weaveworks/flagger/pkg/canary" @@ -10,7 +11,7 @@ import ( "k8s.io/client-go/util/retry" ) -const finalizer = "finalizer.flagger.com" +const finalizer = "finalizer.flagger.app" func (c *Controller) finalize(old interface{}) error { var r *flaggerv1.Canary @@ -39,7 +40,7 @@ func (c *Controller) finalize(old interface{}) error { 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.Kind) + c.logger.Warnf("%s.%s failed due to %s not found", r.Name, r.Namespace, r.Spec.TargetRef.Kind) return nil } c.logger.Errorf("%s.%s failed due to %s", r.Name, r.Namespace, err) @@ -55,7 +56,6 @@ func (c *Controller) finalize(old interface{}) error { } c.logger.Infof("%s.%s moving forward with router finalizing", r.Name, r.Namespace) - //TODO if I can't revert continue on? 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) @@ -82,20 +82,12 @@ func (c *Controller) finalize(old interface{}) error { 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 } - /*if err != nil { - if errors.IsNotFound(err) { - return false, err - } - return true, fmt.Errorf("%s.%s failed to revert deployment to original replicas", r.Name, r.Namespace) - }*/ c.logger.Infof("%s.%s kind %s reverted", r.Name, r.Namespace, r.Spec.TargetRef.Kind) return nil } @@ -107,9 +99,6 @@ func (c *Controller) revertRouter(r *flaggerv1.Canary, labelSelector string, por c.logger.Errorf("%s.%s router failed with error %s", r.Name, r.Namespace, err) return err } - /*if err != nil { - return fmt.Errorf("%s.%s failed to revert service to original state", r.Name, r.Namespace) - }*/ c.logger.Infof("Service %s.%s reverted", r.Name, r.Namespace) return nil } @@ -158,7 +147,7 @@ func (c *Controller) addFinalizer(canary *flaggerv1.Canary, finalizerString stri var selErr error if !firstTry { - canary, selErr = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Get(canary.GetName(),metav1.GetOptions{}) + canary, selErr = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Get(canary.GetName(), metav1.GetOptions{}) if selErr != nil { return selErr } @@ -167,7 +156,7 @@ func (c *Controller) addFinalizer(canary *flaggerv1.Canary, finalizerString stri copy := canary.DeepCopy() copy.ObjectMeta.Finalizers = append(copy.ObjectMeta.Finalizers, finalizerString) - _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(copy) + _, err = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Update(copy) firstTry = false return @@ -182,13 +171,13 @@ func (c *Controller) addFinalizer(canary *flaggerv1.Canary, finalizerString stri //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 { +func (c *Controller) removeFinalizer(canary *flaggerv1.Canary, finalizerString string) error { firstTry := true 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{}) + canary, selErr = c.flaggerClient.FlaggerV1beta1().Canaries(canary.Namespace).Get(canary.GetName(), metav1.GetOptions{}) if selErr != nil { return selErr } @@ -217,5 +206,4 @@ func (c *Controller) removeFinalizer(canary *flaggerv1.Canary, finalizerString s return ex.Wrap(err, "Remove finalizer failed") } return nil - -} \ No newline at end of file +} diff --git a/pkg/controller/finalizer_test.go b/pkg/controller/finalizer_test.go index b5ed4d3c9..f5861934a 100644 --- a/pkg/controller/finalizer_test.go +++ b/pkg/controller/finalizer_test.go @@ -2,6 +2,7 @@ package controller import ( "fmt" + flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" fakeFlagger "github.com/weaveworks/flagger/pkg/client/clientset/versioned/fake" "k8s.io/apimachinery/pkg/runtime" @@ -13,15 +14,15 @@ import ( //Test has finalizers func TestFinalizer_hasFinalizer(t *testing.T) { - withFinalizer := newTestCanary() + withFinalizer := newDeploymentTestCanary() withFinalizer.Finalizers = append(withFinalizer.Finalizers, finalizer) - tables := []struct{ + tables := []struct { canary *flaggerv1.Canary result bool }{ - {newTestCanary(), false}, - { withFinalizer, true}, + {newDeploymentTestCanary(), false}, + {withFinalizer, true}, } for _, table := range tables { @@ -35,24 +36,23 @@ func TestFinalizer_hasFinalizer(t *testing.T) { func TestFinalizer_addFinalizer(t *testing.T) { mockError := fmt.Errorf("failed to add finalizer to canary %s", "testCanary") - cs := fakeFlagger.NewSimpleClientset(newTestCanary()) + cs := fakeFlagger.NewSimpleClientset(newDeploymentTestCanary()) //prepend so it is evaluated over the catch all * - cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error){ + cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, mockError }) - m := Mocks{ - canary:newTestCanary(), + m := fixture{ + canary: newDeploymentTestCanary(), flaggerClient: cs, - ctrl:&Controller{flaggerClient:cs}, + ctrl: &Controller{flaggerClient: cs}, } - - tables := []struct{ - mock Mocks + tables := []struct { + mock fixture canary *flaggerv1.Canary - error error + error error }{ - {SetupMocks(nil), newTestCanary(), nil}, + {newDeploymentFixture(nil), newDeploymentTestCanary(), nil}, {m, m.canary, mockError}, } @@ -70,27 +70,27 @@ func TestFinalizer_addFinalizer(t *testing.T) { func TestFinalizer_removeFinalizer(t *testing.T) { - withFinalizer := newTestCanary() + withFinalizer := newDeploymentTestCanary() withFinalizer.Finalizers = append(withFinalizer.Finalizers, finalizer) mockError := fmt.Errorf("failed to add finalizer to canary %s", "testCanary") - cs := fakeFlagger.NewSimpleClientset(newTestCanary()) + cs := fakeFlagger.NewSimpleClientset(newDeploymentTestCanary()) //prepend so it is evaluated over the catch all * - cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error){ + cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, mockError }) - m := Mocks{ - canary:withFinalizer, + m := fixture{ + canary: withFinalizer, flaggerClient: cs, - ctrl:&Controller{flaggerClient:cs}, + ctrl: &Controller{flaggerClient: cs}, } - tables := []struct{ - mock Mocks + tables := []struct { + mock fixture canary *flaggerv1.Canary - error error + error error }{ - {SetupMocks(nil), withFinalizer, nil}, + {newDeploymentFixture(nil), withFinalizer, nil}, {m, m.canary, mockError}, } @@ -104,4 +104,4 @@ func TestFinalizer_removeFinalizer(t *testing.T) { } } -} \ No newline at end of file +} diff --git a/pkg/router/appmesh.go b/pkg/router/appmesh.go index 83a32cf34..ac7335dca 100644 --- a/pkg/router/appmesh.go +++ b/pkg/router/appmesh.go @@ -483,6 +483,10 @@ func (ar *AppMeshRouter) gatewayAnnotations(canary *flaggerv1.Canary) map[string return a } +func (ar *AppMeshRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} + func int64p(i int64) *int64 { return &i } diff --git a/pkg/router/contour.go b/pkg/router/contour.go index dfd7dd113..0b362cb90 100644 --- a/pkg/router/contour.go +++ b/pkg/router/contour.go @@ -417,3 +417,7 @@ func (cr *ContourRouter) makeLinkerdHeaderValue(canary *flaggerv1.Canary, servic } } + +func (cr *ContourRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/router/gloo.go b/pkg/router/gloo.go index c2401e04e..3eab5f0a4 100644 --- a/pkg/router/gloo.go +++ b/pkg/router/gloo.go @@ -185,3 +185,7 @@ func (gr *GlooRouter) SetRoutes( } return nil } + +func (gr *GlooRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/router/ingress.go b/pkg/router/ingress.go index 70682aac0..14d2b03f1 100644 --- a/pkg/router/ingress.go +++ b/pkg/router/ingress.go @@ -237,3 +237,7 @@ func (i *IngressRouter) makeHeaderAnnotations(annotations map[string]string, func (i *IngressRouter) GetAnnotationWithPrefix(suffix string) string { return fmt.Sprintf("%v/%v", i.annotationsPrefix, suffix) } + +func (i *IngressRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/router/istio.go b/pkg/router/istio.go index 3cdfd220a..613d4513b 100644 --- a/pkg/router/istio.go +++ b/pkg/router/istio.go @@ -1,6 +1,7 @@ package router import ( + "encoding/json" "fmt" "github.com/google/go-cmp/cmp" @@ -211,6 +212,22 @@ func (ir *IstioRouter) reconcileVirtualService(canary *flaggerv1.Canary) error { vtClone := virtualService.DeepCopy() vtClone.Spec = newSpec + //If annotation kubectl.kubernetes.io/last-applied-configuration is present no need to duplicate + //serialization. If not present store the serialized object in annotation + //flagger.kubernetes.io/original-configuration + if _, ok := vtClone.Annotations[kubectlAnnotation]; !ok { + b, err := json.Marshal(virtualService.Spec) + if err != nil { + ir.logger.Warnf("Unable to marshal VS %s for orig-configuration annotation", virtualService.Name) + } + + if vtClone.ObjectMeta.Annotations == nil { + vtClone.ObjectMeta.Annotations = make(map[string]string) + } + + vtClone.ObjectMeta.Annotations[configAnnotation] = string(b) + } + _, err = ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Update(vtClone) if err != nil { return fmt.Errorf("VirtualService %s.%s update error: %w", apexName, canary.Namespace, err) @@ -348,6 +365,50 @@ func (ir *IstioRouter) SetRoutes( return nil } +func (ir *IstioRouter) Finalize(canary *flaggerv1.Canary) error { + + //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 + } + + 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", + 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", + 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 + } + + clone := vs.DeepCopy() + clone.Spec = storedSpec + + _, 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 nil +} + // mergeMatchConditions appends the URI match rules to canary conditions func mergeMatchConditions(canary, defaults []istiov1alpha3.HTTPMatchRequest) []istiov1alpha3.HTTPMatchRequest { for i := range canary { diff --git a/pkg/router/istio_test.go b/pkg/router/istio_test.go index 0b9c3591d..3e5926303 100644 --- a/pkg/router/istio_test.go +++ b/pkg/router/istio_test.go @@ -1,13 +1,17 @@ package router import ( + "encoding/json" "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" + "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" istiov1alpha3 "github.com/weaveworks/flagger/pkg/apis/istio/v1alpha3" ) @@ -329,3 +333,118 @@ func TestIstioRouter_GatewayPort(t *testing.T) { port := vs.Spec.Http[0].Route[0].Destination.Port.Number assert.Equal(t, uint32(mocks.canary.Spec.Service.Port), port) } + +func TestIstioRouter_Finalize(t *testing.T) { + mocks := newFixture(nil) + router := &IstioRouter{ + logger: mocks.logger, + flaggerClient: mocks.flaggerClient, + istioClient: mocks.meshClient, + kubeClient: mocks.kubeClient, + } + + flaggerSpec := &istiov1alpha3.VirtualServiceSpec{ + Http: []istiov1alpha3.HTTPRoute{ + { + Match: mocks.canary.Spec.Service.Match, + Rewrite: mocks.canary.Spec.Service.Rewrite, + Timeout: mocks.canary.Spec.Service.Timeout, + Retries: mocks.canary.Spec.Service.Retries, + CorsPolicy: mocks.canary.Spec.Service.CorsPolicy, + }, + }, + } + + kubectlSpec := &istiov1alpha3.VirtualServiceSpec{ + Hosts: []string{"podinfo"}, + Gateways: []string{"ingressgateway.istio-system.svc.cluster.local"}, + Http: []istiov1alpha3.HTTPRoute{ + { + Match: nil, + Route: []istiov1alpha3.DestinationWeight{ + { + Destination: istiov1alpha3.Destination{Host: "podinfo"}, + }, + }, + }, + }, + } + + tables := []struct { + router *IstioRouter + spec *istiov1alpha3.VirtualServiceSpec + shouldError bool + createVS bool + canary *v1beta1.Canary + callReconcile bool + annotation string + }{ + //VS not found + {router: router, spec: nil, shouldError: true, createVS: false, canary: mocks.canary, callReconcile: false, annotation: ""}, + //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 + {router: router, spec: flaggerSpec, shouldError: false, createVS: true, canary: mocks.canary, callReconcile: true, annotation: "flagger"}, + //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()) + } + + if vs.Annotations == nil { + vs.Annotations = make(map[string]string) + } + + if table.annotation == "flagger" { + b, err := json.Marshal(table.spec) + if err != nil { + t.Fatal(err.Error()) + } + + vs.Annotations[configAnnotation] = string(b) + } else if table.annotation == "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()) + } + } + + if table.callReconcile { + err = router.Reconcile(table.canary) + if err != nil { + t.Fatal(err.Error()) + + } + } + + 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) + } + } + + } +} diff --git a/pkg/router/kubernetes_default.go b/pkg/router/kubernetes_default.go index 2b5f6bdff..4c23f0fb5 100644 --- a/pkg/router/kubernetes_default.go +++ b/pkg/router/kubernetes_default.go @@ -1,6 +1,7 @@ package router import ( + "encoding/json" "fmt" "github.com/google/go-cmp/cmp" @@ -163,7 +164,8 @@ func (c *KubernetesDefaultRouter) reconcileService(canary *flaggerv1.Canary, nam return nil } -func (c *KubernetesDeploymentRouter) Finalize(canary *flaggerv1.Canary) error { +//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{}) @@ -171,10 +173,29 @@ func (c *KubernetesDeploymentRouter) Finalize(canary *flaggerv1.Canary) error { return err } + //No need to do any reconciliation if the router is owned by the controller if hasCanaryOwnerRef, isOwned := c.isOwnedByCanary(svc, canary.Name); !hasCanaryOwnerRef && !isOwned { - err = c.reconcileService(canary, apexName, canary.Spec.TargetRef.Name) - if err != nil { - return err + //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", + svc.Name, svc.Namespace, kubectlAnnotation) + } + clone := svc.DeepCopy() + clone.Spec = storedSvc.Spec + + _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(clone) + if 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 + } } } @@ -184,7 +205,7 @@ func (c *KubernetesDeploymentRouter) Finalize(canary *flaggerv1.Canary) error { //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. -func (c KubernetesDeploymentRouter) isOwnedByCanary(obj interface{}, name string) (bool, bool) { +func (c KubernetesDefaultRouter) isOwnedByCanary(obj interface{}, name string) (bool, bool) { var object metav1.Object var ok bool if object, ok = obj.(metav1.Object); ok { @@ -199,4 +220,4 @@ func (c KubernetesDeploymentRouter) isOwnedByCanary(obj interface{}, name string } } return false, false -} \ No newline at end of file +} diff --git a/pkg/router/kubernetes_default_test.go b/pkg/router/kubernetes_default_test.go index 89002885a..a5efd24a1 100644 --- a/pkg/router/kubernetes_default_test.go +++ b/pkg/router/kubernetes_default_test.go @@ -1,11 +1,17 @@ package router import ( + "fmt" "testing" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/kubernetes/fake" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -104,3 +110,255 @@ func TestServiceRouter_Undo(t *testing.T) { assert.Equal(t, "http", canarySvc.Spec.Ports[0].Name) assert.Equal(t, int32(9898), canarySvc.Spec.Ports[0].Port) } + +func TestServiceRouter_isOwnedByCanary(t *testing.T) { + mocks := newFixture(nil) + router := &KubernetesDefaultRouter{ + kubeClient: mocks.kubeClient, + flaggerClient: mocks.flaggerClient, + logger: mocks.logger, + } + + isController := new(bool) + *isController = true + + tables := []struct { + svc *corev1.Service + isOwned bool + hasOwnerRef bool + }{ + //owned + { + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podinfo", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "flagger.app/v1alpha3", + Kind: "Canary", + Name: "podinfo", + Controller: isController, + }, + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: 8080, + }}, + Selector: map[string]string{"app": "podinfo"}, + }, + }, isOwned: true, hasOwnerRef: true, + }, + //Owner ref but kind not Canary + { + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podinfo", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "flagger.app/v1alpha3", + Kind: "Deployment", + Name: "podinfo", + Controller: isController, + }, + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: 8080, + }}, + Selector: map[string]string{"app": "podinfo"}, + }, + }, isOwned: false, hasOwnerRef: false, + }, + //Owner ref but name doesn't match + { + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podinfo", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "flagger.app/v1alpha3", + Kind: "Canary", + Name: "notpodinfo", + Controller: isController, + }, + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: 8080, + }}, + Selector: map[string]string{"app": "podinfo"}, + }, + }, isOwned: false, hasOwnerRef: true, + }, + //No ownerRef + { + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podinfo", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: 8080, + }}, + Selector: map[string]string{"app": "podinfo"}, + }, + }, isOwned: false, hasOwnerRef: false, + }, + } + + for _, table := range tables { + hasOwnerRef, wasOwned := router.isOwnedByCanary(table.svc, mocks.canary.Name) + if table.isOwned && !wasOwned { + t.Error("Expected to be owned, but was not") + } else if !table.isOwned && wasOwned { + t.Error("Expected not to be owned but was") + } else if table.hasOwnerRef && !hasOwnerRef { + t.Error("Expected to contain OwnerReference but not present") + } else if !table.hasOwnerRef && hasOwnerRef { + t.Error("Expected not to have an OwnerReference but present") + } + } + +} + +func TestServiceRouter_Finalize(t *testing.T) { + + mocks := newFixture(nil) + router := &KubernetesDefaultRouter{ + kubeClient: mocks.kubeClient, + flaggerClient: mocks.flaggerClient, + logger: mocks.logger, + labelSelector: "app", + } + + isController := new(bool) + *isController = true + + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podinfo", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "flagger.app/v1alpha3", + Kind: "Canary", + Name: "NotOwned", + Controller: isController, + }, + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: 9898, + }}, + Selector: map[string]string{"app": "podinfo"}, + }, + } + + kubectlSvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podinfo", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "flagger.app/v1alpha3", + Kind: "Canary", + Name: "NotOwned", + Controller: isController, + }, + }, + Annotations: map[string]string{ + kubectlAnnotation: `{"apiVersion":"v1","kind":"Service","metadata":{"annotations":{},"labels":{"app":"podinfo"},"name":"podinfo","namespace":"test"},"spec":{"ports":[{"name":"http","port":9898,"protocol":"TCP","targetPort":9898}],"selector":{"app":"podinfo"},"type":"ClusterIP"}}`, + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: 9898, + }}, + Selector: map[string]string{"app": "podinfo"}, + }, + } + + tables := []struct { + router *KubernetesDefaultRouter + callSetupMethods bool + shouldError bool + canary *v1beta1.Canary + shouldMutate bool + }{ + //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 + {router: &KubernetesDefaultRouter{kubeClient: fake.NewSimpleClientset(), logger: mocks.logger}, callSetupMethods: false, shouldError: true, canary: mocks.canary, shouldMutate: false}, + //Not owned + {router: &KubernetesDefaultRouter{kubeClient: fake.NewSimpleClientset(svc), logger: mocks.logger}, callSetupMethods: false, shouldError: false, canary: mocks.canary, shouldMutate: true}, + //Kubectl annotation + {router: &KubernetesDefaultRouter{kubeClient: fake.NewSimpleClientset(kubectlSvc), logger: mocks.logger}, callSetupMethods: false, shouldError: false, canary: mocks.canary, shouldMutate: true}, + } + + for _, table := range tables { + + if table.callSetupMethods { + err := table.router.Initialize(table.canary) + if err != nil { + t.Fatal(err.Error()) + } + + err = table.router.Reconcile(table.canary) + if err != nil { + t.Fatal(err.Error()) + } + } + + 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) + } + + 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) + } + + 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) + } + } 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)) + } + } + } + } + } + +} diff --git a/pkg/router/kubernetes_noop.go b/pkg/router/kubernetes_noop.go index b516cee8c..b561c6861 100644 --- a/pkg/router/kubernetes_noop.go +++ b/pkg/router/kubernetes_noop.go @@ -16,3 +16,7 @@ func (c *KubernetesNoopRouter) Initialize(_ *flaggerv1.Canary) error { func (c *KubernetesNoopRouter) Reconcile(_ *flaggerv1.Canary) error { return nil } + +func (c *KubernetesNoopRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/router/nop.go b/pkg/router/nop.go index 4775cfda8..91d468b3d 100644 --- a/pkg/router/nop.go +++ b/pkg/router/nop.go @@ -22,3 +22,7 @@ func (*NopRouter) GetRoutes(canary *flaggerv1.Canary) (primaryWeight int, canary } return 100, 0, false, nil } + +func (c *NopRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} diff --git a/pkg/router/router.go b/pkg/router/router.go index 5a3a414a0..91bd63efe 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -2,6 +2,9 @@ package router import flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" +const configAnnotation = "flagger.kubernetes.io/original-configuration" +const kubectlAnnotation = "kubectl.kubernetes.io/last-applied-configuration" + type Interface interface { Reconcile(canary *flaggerv1.Canary) error SetRoutes(canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, mirrored bool) error diff --git a/pkg/router/smi.go b/pkg/router/smi.go index 332822073..a20e4528a 100644 --- a/pkg/router/smi.go +++ b/pkg/router/smi.go @@ -224,3 +224,7 @@ func (sr *SmiRouter) getWithConvert(canary *flaggerv1.Canary, host string) (*smi } return ts, nil } + +func (sr *SmiRouter) Finalize(canary *flaggerv1.Canary) error { + return nil +} From c9a07cec876ea75e2a2d4b0b5ab5dc4cca21d2a0 Mon Sep 17 00:00:00 2001 From: Tanner Altares Date: Mon, 16 Mar 2020 17:02:55 -0500 Subject: [PATCH 3/5] add e2e tests istio add e2e tests istio clean up comment from review add e2e tests istio clean up comment from review clean up logging statement add e2e tests istio clean up comment from review clean up logging statement add log statement on e2e iteration add e2e tests istio clean up comment from review clean up logging statement add log statement on e2e iteration extend timeout for finalizing add e2e tests istio clean up comment from review clean up logging statement add log statement on e2e iteration extend timeout for finalizing add phase to kustomize crd add e2e tests istio clean up comment from review clean up logging statement add log statement on e2e iteration extend timeout for finalizing add phase to kustomize crd revert timeout on circleci vs and svc checks for istio e2e tests fix fmt errors and tests add get statement in e2e test add get statement in e2e test add namespace to e2e use only selector for service revert --- kustomize/base/flagger/crd.yaml | 2 + pkg/apis/flagger/v1beta1/canary.go | 2 +- pkg/canary/deployment_controller.go | 2 +- pkg/controller/controller.go | 2 +- pkg/controller/finalizer.go | 5 +- pkg/controller/finalizer_test.go | 9 +- pkg/router/istio.go | 2 +- pkg/router/kubernetes_default.go | 2 +- test/e2e-istio-tests.sh | 231 ++++++++++++++++++++++++++++ 9 files changed, 249 insertions(+), 8 deletions(-) diff --git a/kustomize/base/flagger/crd.yaml b/kustomize/base/flagger/crd.yaml index 5d6ef4dbb..a39bb578c 100644 --- a/kustomize/base/flagger/crd.yaml +++ b/kustomize/base/flagger/crd.yaml @@ -653,6 +653,8 @@ spec: - Finalising - Succeeded - Failed + - Terminating + - Terminated canaryWeight: description: Traffic weight percentage routed to canary type: number diff --git a/pkg/apis/flagger/v1beta1/canary.go b/pkg/apis/flagger/v1beta1/canary.go index 24a643df9..25bafe0da 100644 --- a/pkg/apis/flagger/v1beta1/canary.go +++ b/pkg/apis/flagger/v1beta1/canary.go @@ -302,7 +302,7 @@ type CanaryWebhook struct { URL string `json:"url"` // Request timeout for this webhook - Timeout string `json:"timeout"` + Timeout string `json:"timeout,omitempty"` // Metadata (key-value pairs) for this webhook // +optional diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 9eae0cc7a..746c951c8 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -398,7 +398,7 @@ func (c *DeploymentController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bo return c.configTracker.HasConfigChanged(cd) } -// revertDeployment will set the replica count from the primary to the reference instance. This method is used +// Finalize will set the replica count from the primary to the reference instance. This method is used // during a delete to attempt to revert the deployment back to the original state. Error is returned if unable // update the reference deployment replicas to the primary replicas func (c *DeploymentController) Finalize(cd *flaggerv1.Canary) error { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 37a2779a7..9708909ba 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -140,7 +140,7 @@ func NewController( ctrl.logger.Infof("%s.%s opting out, deleting finalizers", newCanary.Name, newCanary.Namespace) err := ctrl.removeFinalizer(&newCanary, finalizer) if err != nil { - ctrl.logger.Warnf("Failed to finalizers for %s.%s", oldCanary.Name, oldCanary.Namespace) + ctrl.logger.Warnf("Failed to remove finalizers for %s.%s", oldCanary.Name, oldCanary.Namespace) return } } diff --git a/pkg/controller/finalizer.go b/pkg/controller/finalizer.go index 49301ba88..d3f76bf4e 100644 --- a/pkg/controller/finalizer.go +++ b/pkg/controller/finalizer.go @@ -43,7 +43,7 @@ func (c *Controller) finalize(old interface{}) error { c.logger.Warnf("%s.%s failed due to %s not found", r.Name, r.Namespace, r.Spec.TargetRef.Kind) return nil } - c.logger.Errorf("%s.%s failed due to %s", r.Name, r.Namespace, err) + 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 @@ -163,7 +163,8 @@ func (c *Controller) addFinalizer(canary *flaggerv1.Canary, finalizerString stri }) if err != nil { - return ex.Wrap(err, "Remove finalizer failed") + c.logger.Errorf("Failed to add finalizer %s", err) + return ex.Wrap(err, "Add finalizer failed") } return nil } diff --git a/pkg/controller/finalizer_test.go b/pkg/controller/finalizer_test.go index f5861934a..02d189071 100644 --- a/pkg/controller/finalizer_test.go +++ b/pkg/controller/finalizer_test.go @@ -5,6 +5,7 @@ import ( 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" "k8s.io/apimachinery/pkg/runtime" k8sTesting "k8s.io/client-go/testing" @@ -41,10 +42,16 @@ func TestFinalizer_addFinalizer(t *testing.T) { cs.PrependReactor("update", "canaries", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, mockError }) + + logger, _ := logger.NewLogger("debug") m := fixture{ canary: newDeploymentTestCanary(), flaggerClient: cs, - ctrl: &Controller{flaggerClient: cs}, + ctrl: &Controller{ + flaggerClient: cs, + logger: logger, + }, + logger: logger, } tables := []struct { diff --git a/pkg/router/istio.go b/pkg/router/istio.go index 613d4513b..547183191 100644 --- a/pkg/router/istio.go +++ b/pkg/router/istio.go @@ -214,7 +214,7 @@ func (ir *IstioRouter) reconcileVirtualService(canary *flaggerv1.Canary) error { //If annotation kubectl.kubernetes.io/last-applied-configuration is present no need to duplicate //serialization. If not present store the serialized object in annotation - //flagger.kubernetes.io/original-configuration + //flagger.kubernetes.app/original-configuration if _, ok := vtClone.Annotations[kubectlAnnotation]; !ok { b, err := json.Marshal(virtualService.Spec) if err != nil { diff --git a/pkg/router/kubernetes_default.go b/pkg/router/kubernetes_default.go index 4c23f0fb5..80ecb58a2 100644 --- a/pkg/router/kubernetes_default.go +++ b/pkg/router/kubernetes_default.go @@ -184,7 +184,7 @@ func (c *KubernetesDefaultRouter) Finalize(canary *flaggerv1.Canary) error { svc.Name, svc.Namespace, kubectlAnnotation) } clone := svc.DeepCopy() - clone.Spec = storedSvc.Spec + clone.Spec.Selector = storedSvc.Spec.Selector _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(clone) if err != nil { diff --git a/test/e2e-istio-tests.sh b/test/e2e-istio-tests.sh index 316006018..53231439d 100755 --- a/test/e2e-istio-tests.sh +++ b/test/e2e-istio-tests.sh @@ -328,6 +328,237 @@ done echo '✔ A/B testing promotion test passed' +cat <>> Waiting for finalizers to be present' +retries=50 +count=0 +ok=false +until ${ok}; do + kubectl get canary podinfo -n test -o jsonpath='{.metadata.finalizers}' | grep "finalizer.flagger.app" && ok=true || ok=false + sleep 10 + count=$(($count + 1)) + if [[ ${count} -eq ${retries} ]]; then + kubectl -n test describe canary/podinfo + echo "No more retries left" + exit 1 + fi +done + +kubectl delete canary podinfo -n test + +echo '>>> Waiting for primary to revert' +retries=50 +count=0 +ok=false +until ${ok}; do + kubectl get deployment podinfo -n test -o jsonpath='{.spec.replicas}' | grep 1 && ok=true || ok=false + sleep 10 + kubectl -n istio-system logs deployment/flagger --tail 10 + count=$(($count + 1)) + if [[ ${count} -eq ${retries} ]]; then + kubectl -n test describe canary/podinfo + echo "No more retries left" + exit 1 + fi +done +echo '✔ Delete testing passed' + + + +cat <>> Waiting for canary to initialize' +retries=50 +count=0 +ok=false +until ${ok}; do + kubectl -n test get canary/podinfo | grep 'Initialized' && ok=true || ok=false + sleep 5 + count=$(($count + 1)) + if [[ ${count} -eq ${retries} ]]; then + kubectl -n istio-system logs deployment/flagger + echo "No more retries left" + exit 1 + fi +done + +kubectl delete canary podinfo -n test + +echo '>>> Waiting for revert' +retries=50 +count=0 +ok=false +until ${ok}; do + kubectl get svc/podinfo vs/podinfo -n test -o jsonpath="{range .items[*]}{.metadata.name}{'\n'}{end}" | wc -l | grep 2 && ok=true || ok=false + sleep 10 + kubectl -n istio-system logs deployment/flagger --tail 10 + count=$(($count + 1)) + if [[ ${count} -eq ${retries} ]]; then + kubectl -n test describe canary/podinfo + kubectl -n test describe svc/podinfo + kubectl -n test describe vs/podinfo + echo "No more retries left" + exit 1 + fi +done +echo '✔ Revert testing passed' + + kubectl -n istio-system logs deployment/flagger echo '✔ All tests passed' From a3e3567f1ead458d47d5fe9fde7f936e9e7cc11f Mon Sep 17 00:00:00 2001 From: Tanner Altares Date: Wed, 18 Mar 2020 15:01:19 -0500 Subject: [PATCH 4/5] finalize docs --- docs/gitbook/usage/how-it-works.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/gitbook/usage/how-it-works.md b/docs/gitbook/usage/how-it-works.md index 1b247074a..e534f24a0 100644 --- a/docs/gitbook/usage/how-it-works.md +++ b/docs/gitbook/usage/how-it-works.md @@ -240,6 +240,31 @@ kubectl wait canary/podinfo --for=condition=promoted --timeout=5m kubectl get canary/podinfo | grep Succeeded ``` +### Canary finalizers + +The default behavior of Flagger on canary deletion is to leave resources that aren't owned by the controller +in their current state. This simplifies the deletion action and avoids possible deadlocks during resource +finalization. In the event the canary was introduced with existing resource(s) (i.e. service, virtual service, etc.), +they would be mutated during the initialization phase and no longer reflect their initial state. If the desired +functionality upon deletion is to revert the resources to their initial state, the `revertOnDeletion` attribute +can be enabled. + +```yaml +spec: + revertOnDeletion: true +``` + +When a deletion action is submitted to the cluster, Flagger will attempt to revert the following resources: + +* [Canary target](#canary-target) replicas will be updated to the primary replica count +* [Canary service](#canary-service) selector will be reverted +* Mesh/Ingress traffic routed to the target + +The recommended approach to disable canary analysis would be utilization of the `skipAnalysis` +attribute, which limits the need for resource reconciliation. Utilizing the `revertOnDeletion` attribute should be +enabled when you no longer plan to rely on Flagger for deployment management. + + ### Canary analysis The canary analysis defines: From 7cf836e98203a64433a8e8d96e8b01396c861116 Mon Sep 17 00:00:00 2001 From: Tanner Altares Date: Wed, 18 Mar 2020 16:22:48 -0500 Subject: [PATCH 5/5] support for daemonset finalize fmt update e2e test typo finalizer return if not found fix typo --- docs/gitbook/usage/how-it-works.md | 1 + pkg/canary/daemonset_controller.go | 5 ++ pkg/canary/daemonset_controller_test.go | 16 +++++ pkg/canary/deployment_controller_test.go | 2 +- pkg/controller/finalizer.go | 9 ++- test/e2e-kubernetes-tests-daemonset.sh | 88 ++++++++++++++++++++++++ 6 files changed, 119 insertions(+), 2 deletions(-) diff --git a/docs/gitbook/usage/how-it-works.md b/docs/gitbook/usage/how-it-works.md index e534f24a0..4fa930861 100644 --- a/docs/gitbook/usage/how-it-works.md +++ b/docs/gitbook/usage/how-it-works.md @@ -264,6 +264,7 @@ The recommended approach to disable canary analysis would be utilization of the attribute, which limits the need for resource reconciliation. Utilizing the `revertOnDeletion` attribute should be enabled when you no longer plan to rely on Flagger for deployment management. +**Note** When this feature is enabled expect a delay in the delete action due to the reconciliation. ### Canary analysis diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index d52f5d34d..2bbcf57a1 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -297,6 +297,11 @@ func (c *DaemonSetController) HaveDependenciesChanged(cd *flaggerv1.Canary) (boo return c.configTracker.HasConfigChanged(cd) } +//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 nil } diff --git a/pkg/canary/daemonset_controller_test.go b/pkg/canary/daemonset_controller_test.go index 0222a7103..3899b1638 100644 --- a/pkg/canary/daemonset_controller_test.go +++ b/pkg/canary/daemonset_controller_test.go @@ -194,3 +194,19 @@ func TestDaemonSetController_Scale(t *testing.T) { } }) } + +func TestDaemonSetController_Finalize(t *testing.T) { + mocks := newDaemonSetFixture() + err := mocks.controller.Initialize(mocks.canary, true) + require.NoError(t, err) + + err = mocks.controller.Finalize(mocks.canary) + require.NoError(t, err) + + dep, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo", metav1.GetOptions{}) + 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_test.go b/pkg/canary/deployment_controller_test.go index 9ef5e312d..ed0aee50f 100644 --- a/pkg/canary/deployment_controller_test.go +++ b/pkg/canary/deployment_controller_test.go @@ -189,7 +189,7 @@ func TestDeploymentController_HasTargetChanged(t *testing.T) { assert.True(t, isNew) } -func TestCanaryDeployer_Finalize(t *testing.T) { +func TestDeploymentController_Finalize(t *testing.T) { mocks := newDeploymentFixture() diff --git a/pkg/controller/finalizer.go b/pkg/controller/finalizer.go index d3f76bf4e..3c6310263 100644 --- a/pkg/controller/finalizer.go +++ b/pkg/controller/finalizer.go @@ -23,6 +23,13 @@ func (c *Controller) finalize(old interface{}) error { return nil } + _, 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 + } + //Retrieve a controller canaryController := c.canaryFactory.Controller(r.Spec.TargetRef.Kind) @@ -36,7 +43,7 @@ func (c *Controller) finalize(old interface{}) error { c.recordEventInfof(r, "Terminating canary %s.%s", r.Name, r.Namespace) } - err := c.revertTargetRef(canaryController, r) + err = c.revertTargetRef(canaryController, r) if err != nil { if errors.IsNotFound(err) { //No reason to wait not found diff --git a/test/e2e-kubernetes-tests-daemonset.sh b/test/e2e-kubernetes-tests-daemonset.sh index 443f4dfbb..e5ab93c39 100755 --- a/test/e2e-kubernetes-tests-daemonset.sh +++ b/test/e2e-kubernetes-tests-daemonset.sh @@ -108,4 +108,92 @@ done echo '✔ Canary promotion test passed' + +cat <>> Waiting for finalizers to be present' +retries=50 +count=0 +ok=false +until ${ok}; do + kubectl get canary podinfo -n test -o jsonpath='{.metadata.finalizers}' | grep "finalizer.flagger.app" && ok=true || ok=false + sleep 10 + count=$(($count + 1)) + if [[ ${count} -eq ${retries} ]]; then + kubectl -n test describe canary/podinfo + echo "No more retries left" + exit 1 + fi +done + +kubectl delete canary podinfo -n test + +echo '>>> Waiting for primary to revert' +retries=50 +count=0 +ok=false +until ${ok}; do + kubectl get daemonset podinfo -n test -o jsonpath='{.status.numberReady}' | grep 1 && ok=true || ok=false + sleep 10 + kubectl -n flagger-system logs deployment/flagger --tail 1 + count=$(($count + 1)) + if [[ ${count} -eq ${retries} ]]; then + kubectl -n test describe canary/podinfo + echo "No more retries left" + exit 1 + fi +done + +echo '✔ Canary finalize passed' + kubectl -n flagger-system logs deployment/flagger