From 8e699a7543ada9b4685d19363ebd7b9d9b167f8c Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Sun, 24 Feb 2019 18:25:12 +0200 Subject: [PATCH 1/2] Detect changes in virtual service - ignore destination weight when comparing the two specs --- pkg/controller/router.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/controller/router.go b/pkg/controller/router.go index 9006065d..2e387402 100644 --- a/pkg/controller/router.go +++ b/pkg/controller/router.go @@ -2,8 +2,8 @@ package controller import ( "fmt" - "reflect" - + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" istiov1alpha3 "github.com/knative/pkg/apis/istio/v1alpha3" istioclientset "github.com/knative/pkg/client/clientset/versioned" flaggerv1 "github.com/stefanprodan/flagger/pkg/apis/flagger/v1alpha3" @@ -221,7 +221,8 @@ func (c *CanaryRouter) createVirtualService(cd *flaggerv1.Canary) error { return fmt.Errorf("VirtualService %s.%s create error %v", targetName, cd.Namespace, err) } c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("VirtualService %s.%s created", virtualService.GetName(), cd.Namespace) - } else if !reflect.DeepEqual(virtualService.Spec.Hosts, newSpec.Hosts) || !reflect.DeepEqual(virtualService.Spec.Gateways, newSpec.Gateways) { + } else if diff := cmp.Diff(newSpec, virtualService.Spec, cmpopts.IgnoreTypes(istiov1alpha3.DestinationWeight{})); diff != "" { + //fmt.Println(diff) virtualService.Spec = newSpec c.logger.Debugf("Updating VirtualService %s.%s", virtualService.GetName(), cd.Namespace) _, err = c.istioClient.NetworkingV1alpha3().VirtualServices(cd.Namespace).Update(virtualService) From 1dc7677dfb63f317c802c583c6412a0a99e9667c Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Sun, 24 Feb 2019 19:58:01 +0200 Subject: [PATCH 2/2] Add tests for virtual service sync --- pkg/controller/router.go | 43 ++++++++++------ pkg/controller/router_test.go | 96 ++++++++++++++++++++++++++++++----- 2 files changed, 109 insertions(+), 30 deletions(-) diff --git a/pkg/controller/router.go b/pkg/controller/router.go index 2e387402..4097ab42 100644 --- a/pkg/controller/router.go +++ b/pkg/controller/router.go @@ -27,15 +27,14 @@ type CanaryRouter struct { logger *zap.SugaredLogger } -// Sync creates the primary and canary ClusterIP services -// and sets up a virtual service with routes for the two services -// all traffic goes to primary +// Sync creates or updates the primary and canary ClusterIP services +// and the Istio virtual service. func (c *CanaryRouter) Sync(cd *flaggerv1.Canary) error { err := c.createServices(cd) if err != nil { return err } - err = c.createVirtualService(cd) + err = c.syncVirtualService(cd) if err != nil { return err } @@ -164,7 +163,7 @@ func (c *CanaryRouter) createServices(cd *flaggerv1.Canary) error { return nil } -func (c *CanaryRouter) createVirtualService(cd *flaggerv1.Canary) error { +func (c *CanaryRouter) syncVirtualService(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name primaryName := fmt.Sprintf("%s-primary", targetName) hosts := append(cd.Spec.Service.Hosts, targetName) @@ -199,8 +198,8 @@ func (c *CanaryRouter) createVirtualService(cd *flaggerv1.Canary) error { }, } + // insert if errors.IsNotFound(err) { - c.logger.Debugf("VirtualService %s.%s not found", targetName, cd.Namespace) virtualService = &istiov1alpha3.VirtualService{ ObjectMeta: metav1.ObjectMeta{ Name: targetName, @@ -215,21 +214,33 @@ func (c *CanaryRouter) createVirtualService(cd *flaggerv1.Canary) error { }, Spec: newSpec, } - c.logger.Debugf("Creating VirtualService %s.%s", virtualService.GetName(), cd.Namespace) _, err = c.istioClient.NetworkingV1alpha3().VirtualServices(cd.Namespace).Create(virtualService) if err != nil { return fmt.Errorf("VirtualService %s.%s create error %v", targetName, cd.Namespace, err) } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("VirtualService %s.%s created", virtualService.GetName(), cd.Namespace) - } else if diff := cmp.Diff(newSpec, virtualService.Spec, cmpopts.IgnoreTypes(istiov1alpha3.DestinationWeight{})); diff != "" { - //fmt.Println(diff) - virtualService.Spec = newSpec - c.logger.Debugf("Updating VirtualService %s.%s", virtualService.GetName(), cd.Namespace) - _, err = c.istioClient.NetworkingV1alpha3().VirtualServices(cd.Namespace).Update(virtualService) - if err != nil { - return fmt.Errorf("VirtualService %s.%s update error %v", targetName, cd.Namespace, err) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Infof("VirtualService %s.%s created", virtualService.GetName(), cd.Namespace) + return nil + } + + if err != nil { + return fmt.Errorf("VirtualService %s.%s query error %v", targetName, cd.Namespace, err) + } + + // update service + if virtualService != nil { + if diff := cmp.Diff(newSpec, virtualService.Spec, cmpopts.IgnoreTypes(istiov1alpha3.DestinationWeight{})); diff != "" { + vtClone := virtualService.DeepCopy() + vtClone.Spec = newSpec + //TODO: keep original destination weights + //vtClone.Spec.Http = virtualService.Spec.Http + _, err = c.istioClient.NetworkingV1alpha3().VirtualServices(cd.Namespace).Update(vtClone) + if err != nil { + return fmt.Errorf("VirtualService %s.%s update error %v", targetName, cd.Namespace, err) + } + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Infof("VirtualService %s.%s updated", virtualService.GetName(), cd.Namespace) } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("VirtualService %s.%s updated", virtualService.GetName(), cd.Namespace) } return nil diff --git a/pkg/controller/router_test.go b/pkg/controller/router_test.go index e9b7c9a9..41527924 100644 --- a/pkg/controller/router_test.go +++ b/pkg/controller/router_test.go @@ -8,7 +8,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestCanaryRouter_Sync(t *testing.T) { +func TestCanaryRouter_SyncClusterIPServices(t *testing.T) { mocks := SetupMocks() err := mocks.router.Sync(mocks.canary) if err != nil { @@ -40,19 +40,6 @@ func TestCanaryRouter_Sync(t *testing.T) { if primarySvc.Spec.Ports[0].Port != 9898 { t.Errorf("Got primary svc port %v wanted %v", primarySvc.Spec.Ports[0].Port, 9898) } - - vs, err := mocks.istioClient.NetworkingV1alpha3().VirtualServices("default").Get("podinfo", metav1.GetOptions{}) - if err != nil { - t.Fatal(err.Error()) - } - - if len(vs.Spec.Http) != 1 { - t.Errorf("Got Istio VS Http %v wanted %v", len(vs.Spec.Http), 1) - } - - if len(vs.Spec.Http[0].Route) != 2 { - t.Errorf("Got Istio VS routes %v wanted %v", len(vs.Spec.Http[0].Route), 2) - } } func TestCanaryRouter_GetRoutes(t *testing.T) { @@ -76,6 +63,87 @@ func TestCanaryRouter_GetRoutes(t *testing.T) { } } +func TestCanaryRouter_SyncVirtualService(t *testing.T) { + mocks := SetupMocks() + err := mocks.router.Sync(mocks.canary) + if err != nil { + t.Fatal(err.Error()) + } + + // test insert + vs, err := mocks.istioClient.NetworkingV1alpha3().VirtualServices("default").Get("podinfo", metav1.GetOptions{}) + if err != nil { + t.Fatal(err.Error()) + } + + if len(vs.Spec.Http) != 1 { + t.Errorf("Got Istio VS Http %v wanted %v", len(vs.Spec.Http), 1) + } + + if len(vs.Spec.Http[0].Route) != 2 { + t.Errorf("Got Istio VS routes %v wanted %v", len(vs.Spec.Http[0].Route), 2) + } + + // test update + cd, err := mocks.flaggerClient.FlaggerV1alpha3().Canaries("default").Get("podinfo", metav1.GetOptions{}) + if err != nil { + t.Fatal(err.Error()) + } + + cdClone := cd.DeepCopy() + hosts := cdClone.Spec.Service.Hosts + hosts = append(hosts, "test.example.com") + cdClone.Spec.Service.Hosts = hosts + canary, err := mocks.flaggerClient.FlaggerV1alpha3().Canaries("default").Update(cdClone) + if err != nil { + t.Fatal(err.Error()) + } + + // apply change + err = mocks.router.Sync(canary) + if err != nil { + t.Fatal(err.Error()) + } + + // verify + vs, err = mocks.istioClient.NetworkingV1alpha3().VirtualServices("default").Get("podinfo", metav1.GetOptions{}) + if err != nil { + t.Fatal(err.Error()) + } + if len(vs.Spec.Hosts) != 2 { + t.Errorf("Got Istio VS hosts %v wanted %v", vs.Spec.Hosts, 2) + } + + // test drift + vsClone := vs.DeepCopy() + gateways := vsClone.Spec.Gateways + gateways = append(gateways, "test-gateway.istio-system") + vsClone.Spec.Gateways = gateways + + vsGateways, err := mocks.istioClient.NetworkingV1alpha3().VirtualServices("default").Update(vsClone) + if err != nil { + t.Fatal(err.Error()) + } + if len(vsGateways.Spec.Gateways) != 2 { + t.Errorf("Got Istio VS gateway %v wanted %v", vsGateways.Spec.Gateways, 2) + } + + // undo change + err = mocks.router.Sync(mocks.canary) + if err != nil { + t.Fatal(err.Error()) + } + + // verify + vs, err = mocks.istioClient.NetworkingV1alpha3().VirtualServices("default").Get("podinfo", metav1.GetOptions{}) + if err != nil { + t.Fatal(err.Error()) + } + if len(vs.Spec.Gateways) != 1 { + t.Errorf("Got Istio VS gateways %v wanted %v", vs.Spec.Gateways, 1) + } +} + func TestCanaryRouter_SetRoutes(t *testing.T) { mocks := SetupMocks() err := mocks.router.Sync(mocks.canary)