From 7df3d17ac4d65c56ef12f19581784bdbb5c24b8b Mon Sep 17 00:00:00 2001 From: May Zhang Date: Wed, 12 Jun 2024 13:18:02 -0700 Subject: [PATCH] fix: when Rollout has pingpong and stable/canary service defined, only alb traffic management uses pingpong. (#3628) * fix: when Rollout has pingpong and stable/canary service defined, GetStableAndCanaryServices returns based on isPingpongPreferred. Only when it is ALB controller, then isPringpongPreferred is true. Signed-off-by: mayz985 * fix lint error Signed-off-by: mayz985 * added e2e Signed-off-by: mayz985 --------- Signed-off-by: mayz985 --- rollout/canary.go | 2 +- rollout/service.go | 2 +- rollout/trafficrouting/alb/alb.go | 6 +- rollout/trafficrouting/istio/istio.go | 12 +- rollout/trafficrouting/service_helper.go | 17 ++- rollout/trafficrouting/service_helper_test.go | 82 +++++++++++ test/e2e/aws_test.go | 65 ++++++++ ...albmesh-pingpong-stablecanary-rollout.yaml | 139 ++++++++++++++++++ 8 files changed, 310 insertions(+), 15 deletions(-) create mode 100644 rollout/trafficrouting/service_helper_test.go create mode 100644 test/e2e/functional/albmesh-pingpong-stablecanary-rollout.yaml diff --git a/rollout/canary.go b/rollout/canary.go index fa33b43616..a3033fea02 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -296,7 +296,7 @@ func (c *rolloutContext) canProceedWithScaleDownAnnotation(oldRSs []*appsv1.Repl // AWS API calls. return true, nil } - stableSvcName, _ := trafficrouting.GetStableAndCanaryServices(c.rollout) + stableSvcName, _ := trafficrouting.GetStableAndCanaryServices(c.rollout, true) stableSvc, err := c.servicesLister.Services(c.rollout.Namespace).Get(stableSvcName) if err != nil { return false, err diff --git a/rollout/service.go b/rollout/service.go index 69739b9315..c0904ffd23 100644 --- a/rollout/service.go +++ b/rollout/service.go @@ -243,7 +243,7 @@ func (c *rolloutContext) getPreviewAndActiveServices() (*corev1.Service, *corev1 func (c *rolloutContext) reconcilePingAndPongService() error { if trafficrouting.IsPingPongEnabled(c.rollout) && !rolloututils.IsFullyPromoted(c.rollout) { - _, canaryService := trafficrouting.GetStableAndCanaryServices(c.rollout) + _, canaryService := trafficrouting.GetStableAndCanaryServices(c.rollout, true) return c.ensureSVCTargets(canaryService, c.newRS, false) } return nil diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index 2abeb052db..0a053857d6 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -242,7 +242,7 @@ func (r *Reconciler) VerifyWeightPerIngress(desiredWeight int32, ingresses []str } resourceIDToDest := map[string]v1alpha1.WeightDestination{} - stableService, canaryService := trafficrouting.GetStableAndCanaryServices(rollout) + stableService, canaryService := trafficrouting.GetStableAndCanaryServices(rollout, true) canaryResourceID := aws.BuildTargetGroupResourceID(rollout.Namespace, ingress.GetName(), canaryService, rollout.Spec.Strategy.Canary.TrafficRouting.ALB.ServicePort) stableResourceID := aws.BuildTargetGroupResourceID(rollout.Namespace, ingress.GetName(), stableService, rollout.Spec.Strategy.Canary.TrafficRouting.ALB.ServicePort) @@ -347,7 +347,7 @@ func updateTargetGroupStatus(status *v1alpha1.ALBStatus, tg *aws.TargetGroupMeta } func getForwardActionString(r *v1alpha1.Rollout, port int32, desiredWeight int32, additionalDestinations ...v1alpha1.WeightDestination) (string, error) { - stableService, canaryService := trafficrouting.GetStableAndCanaryServices(r) + stableService, canaryService := trafficrouting.GetStableAndCanaryServices(r, true) portStr := strconv.Itoa(int(port)) stableWeight := int32(100) targetGroups := make([]ingressutil.ALBTargetGroup, 0) @@ -479,7 +479,7 @@ func removeValue(array []string, key string) []string { } func getTrafficForwardActionString(r *v1alpha1.Rollout, port int32) (string, error) { - _, canaryService := trafficrouting.GetStableAndCanaryServices(r) + _, canaryService := trafficrouting.GetStableAndCanaryServices(r, true) portStr := strconv.Itoa(int(port)) weight := int64(100) targetGroups := make([]ingressutil.ALBTargetGroup, 0) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index 5308d59a42..bb9a15eb5e 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -125,7 +125,7 @@ func (patches virtualServicePatches) patchVirtualService(httpRoutes []any, tlsRo } func (r *Reconciler) generateVirtualServicePatches(rolloutVsvcRouteNames []string, httpRoutes []VirtualServiceHTTPRoute, rolloutVsvcTLSRoutes []v1alpha1.TLSRoute, tlsRoutes []VirtualServiceTLSRoute, rolloutVsvcTCPRoutes []v1alpha1.TCPRoute, tcpRoutes []VirtualServiceTCPRoute, desiredWeight int64, additionalDestinations ...v1alpha1.WeightDestination) virtualServicePatches { - stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout) + stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout, false) canarySubset := "" stableSubset := "" if r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule != nil { @@ -718,7 +718,7 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1 return err } - _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout) + _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout, false) if destRuleHost != "" { canarySvc = destRuleHost } @@ -1023,7 +1023,7 @@ func searchTcpRoute(tcpRoute v1alpha1.TCPRoute, istioTcpRoutes []VirtualServiceT // ValidateHTTPRoutes ensures that all the routes in the rollout exist func ValidateHTTPRoutes(r *v1alpha1.Rollout, routeNames []string, httpRoutes []VirtualServiceHTTPRoute) error { - stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r) + stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r, false) routeIndexesToPatch, err := getHttpRouteIndexesToPatch(routeNames, httpRoutes) if err != nil { @@ -1060,7 +1060,7 @@ func ValidateHTTPRoutes(r *v1alpha1.Rollout, routeNames []string, httpRoutes []V // ValidateTlsRoutes ensures that all the routes in the rollout exist and they only have two destinations func ValidateTlsRoutes(r *v1alpha1.Rollout, vsvcTLSRoutes []v1alpha1.TLSRoute, tlsRoutes []VirtualServiceTLSRoute) error { - stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r) + stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r, false) routeIndexesToPatch, err := getTlsRouteIndexesToPatch(vsvcTLSRoutes, tlsRoutes) if err != nil { @@ -1081,7 +1081,7 @@ func ValidateTlsRoutes(r *v1alpha1.Rollout, vsvcTLSRoutes []v1alpha1.TLSRoute, t // ValidateTcpRoutes ensures that all the routes in the rollout exist and they only have two destinations func ValidateTcpRoutes(r *v1alpha1.Rollout, vsvcTCPRoutes []v1alpha1.TCPRoute, tcpRoutes []VirtualServiceTCPRoute) error { - stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r) + stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r, false) routeIndexesToPatch, err := getTcpRouteIndexesToPatch(vsvcTCPRoutes, tcpRoutes) if err != nil { @@ -1189,7 +1189,7 @@ func (r *Reconciler) reconcileVirtualServiceMirrorRoutes(virtualService v1alpha1 if err != nil { return fmt.Errorf("[reconcileVirtualServiceMirrorRoutes] failed to get destination rule host: %w", err) } - _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout) + _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout, false) if destRuleHost != "" { canarySvc = destRuleHost } diff --git a/rollout/trafficrouting/service_helper.go b/rollout/trafficrouting/service_helper.go index 15d03cad53..8bae069fe7 100644 --- a/rollout/trafficrouting/service_helper.go +++ b/rollout/trafficrouting/service_helper.go @@ -7,17 +7,26 @@ import ( // GetStableAndCanaryServices return a service names for current stable and canary services. // If ping-pong feature enabled then the current ping or pong service will be returned. Which is a stable is defined // based on a rollout status field Status.Canary.StablePingPong -func GetStableAndCanaryServices(ro *v1alpha1.Rollout) (string, string) { - if IsPingPongEnabled(ro) { + +// isPingpongPreferred is needed when Rollout uses both pingpong service and stable/canary service +// for ALB trafficRouting, isPingpongPreferred is true. It uses pingpong service as priority +// for other trafficRouting, isPingpongPrefrered is false. It uses stable/canary service +// This is to ensure it is compatible with previous release. + +func GetStableAndCanaryServices(ro *v1alpha1.Rollout, isPingpongPreferred bool) (string, string) { + pingPongNotPreferredOtherServiceNotDefined := !isPingpongPreferred && ro.Spec.Strategy.Canary.StableService == "" && ro.Spec.Strategy.Canary.CanaryService == "" + if IsPingPongEnabled(ro) && + (isPingpongPreferred || pingPongNotPreferredOtherServiceNotDefined) { canary := ro.Spec.Strategy.Canary if IsStablePing(ro) { return canary.PingPong.PingService, canary.PingPong.PongService } else { return canary.PingPong.PongService, canary.PingPong.PingService } - } else { - return ro.Spec.Strategy.Canary.StableService, ro.Spec.Strategy.Canary.CanaryService } + + return ro.Spec.Strategy.Canary.StableService, ro.Spec.Strategy.Canary.CanaryService + } // IsStablePing return true if the 'ping' service is pointing to the stable replica set. diff --git a/rollout/trafficrouting/service_helper_test.go b/rollout/trafficrouting/service_helper_test.go new file mode 100644 index 0000000000..cc0634eb6f --- /dev/null +++ b/rollout/trafficrouting/service_helper_test.go @@ -0,0 +1,82 @@ +package trafficrouting + +import ( + "testing" + + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +const PING_SVC = "ping-service" +const PONG_SVC = "pong-service" + +func fakeRollout(stableSvc, canarySvc string, pingPong *v1alpha1.PingPongSpec, stableIng string, port int32) *v1alpha1.Rollout { + return &v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rollout", + Namespace: metav1.NamespaceDefault, + }, + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + StableService: stableSvc, + CanaryService: canarySvc, + PingPong: pingPong, + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + ALB: &v1alpha1.ALBTrafficRouting{ + Ingress: stableIng, + ServicePort: port, + }, + Istio: &v1alpha1.IstioTrafficRouting{ + VirtualService: &v1alpha1.IstioVirtualService{ + Name: "istio-vsvc", + }, + DestinationRule: &v1alpha1.IstioDestinationRule{ + Name: "istio-destrule", + CanarySubsetName: "canary", + StableSubsetName: "stable", + }, + }, + }, + }, + }, + }, + } +} + +func TestGetStableAndCanaryServices(t *testing.T) { + // Rollout has no pingPong + rollout := fakeRollout("stable-service", "canary-service", nil, "stable-ingress", 443) + + stableService, canaryService := GetStableAndCanaryServices(rollout, true) + assert.Equal(t, "stable-service", stableService) + assert.Equal(t, "canary-service", canaryService) + + stableService, canaryService = GetStableAndCanaryServices(rollout, false) + assert.Equal(t, "stable-service", stableService) + assert.Equal(t, "canary-service", canaryService) + + // Rollout has pingPong and stable/canary + pp := &v1alpha1.PingPongSpec{PingService: PING_SVC, PongService: PONG_SVC} + rollout = fakeRollout("stable-service", "canary-service", pp, "stable-ingress", 443) + + stableService, canaryService = GetStableAndCanaryServices(rollout, true) + assert.Equal(t, PONG_SVC, stableService) + assert.Equal(t, PING_SVC, canaryService) + + stableService, canaryService = GetStableAndCanaryServices(rollout, false) + assert.Equal(t, "stable-service", stableService) + assert.Equal(t, "canary-service", canaryService) + + // Rollout has pingPong, no stable/canary + rollout = fakeRollout("", "", pp, "stable-ingress", 443) + + stableService, canaryService = GetStableAndCanaryServices(rollout, true) + assert.Equal(t, PONG_SVC, stableService) + assert.Equal(t, PING_SVC, canaryService) + + stableService, canaryService = GetStableAndCanaryServices(rollout, false) + assert.Equal(t, PONG_SVC, stableService) + assert.Equal(t, PING_SVC, canaryService) +} diff --git a/test/e2e/aws_test.go b/test/e2e/aws_test.go index dab037e395..fcbb1a2ef5 100644 --- a/test/e2e/aws_test.go +++ b/test/e2e/aws_test.go @@ -94,6 +94,71 @@ func (s *AWSSuite) TestALBPingPongUpdate() { Assert(assertWeights(s, "ping-service", "pong-service", 100, 0)) } +// Rollout uses both alb and mesh for trafficRouting. +// also uses both pingpong service and stable/canary services +// Expecting: * alb is using pingpong +// - mesh is using stable/canary +func (s *AWSSuite) TestALBMesh_PingPong_StableCanary_Update() { + s.Given(). + RolloutObjects("@functional/albmesh-pingpong-stablecanary-rollout.yaml"). + When().ApplyManifests().WaitForRolloutStatus("Healthy"). + Then(). + Assert(assertWeights(s, "ping-service", "pong-service", 100, 0)). + Assert(func(t *fixtures.Then) { + vsvc := t.GetVirtualService() + assert.Equal(s.T(), int64(100), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(0), vsvc.Spec.HTTP[0].Route[1].Weight) + assert.Equal(s.T(), "stable-service", vsvc.Spec.HTTP[0].Route[0].Destination.Host) + assert.Equal(s.T(), "canary-service", vsvc.Spec.HTTP[0].Route[1].Destination.Host) + }). + // Update 1. Test the weight switch from ping => pong + When().UpdateSpec(). + WaitForRolloutCanaryStepIndex(1).Sleep(1 * time.Second).Then(). + Assert(assertWeights(s, "ping-service", "pong-service", 75, 25)). + Assert(func(t *fixtures.Then) { + vsvc := t.GetVirtualService() + assert.Equal(s.T(), int64(75), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(25), vsvc.Spec.HTTP[0].Route[1].Weight) + assert.Equal(s.T(), "stable-service", vsvc.Spec.HTTP[0].Route[0].Destination.Host) + assert.Equal(s.T(), "canary-service", vsvc.Spec.HTTP[0].Route[1].Destination.Host) + }). + When().PromoteRollout(). + WaitForRolloutStatus("Healthy"). + Sleep(1 * time.Second). + Then(). + Assert(assertWeights(s, "ping-service", "pong-service", 0, 100)). + Assert(func(t *fixtures.Then) { + vsvc := t.GetVirtualService() + assert.Equal(s.T(), int64(100), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(0), vsvc.Spec.HTTP[0].Route[1].Weight) + assert.Equal(s.T(), "stable-service", vsvc.Spec.HTTP[0].Route[0].Destination.Host) + assert.Equal(s.T(), "canary-service", vsvc.Spec.HTTP[0].Route[1].Destination.Host) + }). + // Update 2. Test the weight switch from pong => ping + When().UpdateSpec(). + WaitForRolloutCanaryStepIndex(1).Sleep(1 * time.Second).Then(). + Assert(assertWeights(s, "ping-service", "pong-service", 25, 75)). + Assert(func(t *fixtures.Then) { + vsvc := t.GetVirtualService() + assert.Equal(s.T(), int64(75), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(25), vsvc.Spec.HTTP[0].Route[1].Weight) + assert.Equal(s.T(), "stable-service", vsvc.Spec.HTTP[0].Route[0].Destination.Host) + assert.Equal(s.T(), "canary-service", vsvc.Spec.HTTP[0].Route[1].Destination.Host) + }). + When().PromoteRollout(). + WaitForRolloutStatus("Healthy"). + Sleep(1 * time.Second). + Then(). + Assert(assertWeights(s, "ping-service", "pong-service", 100, 0)). + Assert(func(t *fixtures.Then) { + vsvc := t.GetVirtualService() + assert.Equal(s.T(), int64(100), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(0), vsvc.Spec.HTTP[0].Route[1].Weight) + assert.Equal(s.T(), "stable-service", vsvc.Spec.HTTP[0].Route[0].Destination.Host) + assert.Equal(s.T(), "canary-service", vsvc.Spec.HTTP[0].Route[1].Destination.Host) + }) +} + func (s *AWSSuite) TestALBPingPongUpdateMultiIngress() { s.Given(). RolloutObjects("@functional/alb-pingpong-multi-ingress-rollout.yaml"). diff --git a/test/e2e/functional/albmesh-pingpong-stablecanary-rollout.yaml b/test/e2e/functional/albmesh-pingpong-stablecanary-rollout.yaml new file mode 100644 index 0000000000..0476b1076e --- /dev/null +++ b/test/e2e/functional/albmesh-pingpong-stablecanary-rollout.yaml @@ -0,0 +1,139 @@ +--- +apiVersion: v1 +kind: Service +metadata: + name: ping-service +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-canary +--- +apiVersion: v1 +kind: Service +metadata: + name: pong-service +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-canary +--- +--- +apiVersion: v1 +kind: Service +metadata: + name: stable-service +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-canary +--- +apiVersion: v1 +kind: Service +metadata: + name: canary-service +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-canary +--- +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: alb-canary-ingress + annotations: + kubernetes.io/ingress.class: alb +spec: + rules: + - http: + paths: + - path: /* + backend: + service: + name: alb-rollout-root + port: + name: use-annotation + pathType: ImplementationSpecific +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: alb-canary +spec: + replicas: 2 + selector: + matchLabels: + app: alb-canary + template: + metadata: + labels: + app: alb-canary + spec: + containers: + - name: alb-canary + image: "argoproj/rollouts-demo:red" + ports: + - name: http + containerPort: 80 + protocol: TCP + resources: + requests: + memory: 16Mi + cpu: 5m + strategy: + canary: + scaleDownDelaySeconds: 2 + canaryService: canary-service + stableService: stable-service + pingPong: + pingService: ping-service + pongService: pong-service + trafficRouting: + alb: + ingress: alb-canary-ingress + rootService: alb-rollout-root + servicePort: 80 + istio: + virtualService: + name: istio-host-split-vsvc + routes: + - primary + steps: + - setWeight: 25 + - pause: {} +--- +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: istio-host-split-vsvc +spec: + hosts: + - istio-host-split + http: + - name: primary + route: + - destination: + host: stable-service + weight: 100 + - destination: + host: canary-service + weight: 0