fix: when Rollout has pingpong and stable/canary service defined, onl…

…y 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 <[email protected]> * fix lint error Signed-off-by: mayz985 <[email protected]> * added e2e Signed-off-by: mayz985 <[email protected]> --------- Signed-off-by: mayz985 <[email protected]>
argoproj · Jun 12, 2024 · 7df3d17 · 7df3d17
1 parent e20bcde
commit 7df3d17
Show file tree

Hide file tree

Showing 8 changed files with 310 additions and 15 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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").