Merge branch 'master' into issue-3455-notification-secrets

docs/features/traffic-management/istio.md

@@ -473,6 +473,18 @@ help address this problem. The proposed solution is to introduce an annotation t
 indicates to Argo CD to respect and preserve the differences at a specified path, in order to allow
 other controllers (e.g. Argo Rollouts) controller manage them instead.
 
+## Ping Pong
+
+!!! important
+
+    Available since v1.7
+
+Argo Rollouts also supports ping pong when using Istio this was added to support configuring both ALB and
+Istio traffic routers at the same time. When using an ALB, ping-pong is generally a best practice especially with ALB readiness 
+gates enabled. However, when we change the service selectors when a rollout is aborted back to stable pod hash it causes a blip 
+of traffic outage because the ALB controller will set the pod readiness gates to false for a short while due to the label changes.
+If we configure both ALB and Istio with ping-pong this selector change does not happen and hence we do not see any outages.
+
 ## Alternatives Considered
 
 ### Rollout ownership over the Virtual Service

pkg/apiclient/rollout/rollout.proto

@@ -96,6 +96,8 @@ message RolloutInfo {
   repeated ContainerInfo containers = 19;
 
   repeated github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.CanaryStep steps = 20;
+
+  repeated ContainerInfo initContainers = 21;
 }
 
 message ExperimentInfo {
@@ -125,6 +127,7 @@ message ReplicaSetInfo {
   repeated PodInfo pods = 14;
   bool ping = 15;
   bool pong = 16;
+  repeated string initContainerImages = 17;
 }
 
 message PodInfo {

pkg/apiclient/rollout/rollout.swagger.json

@@ -5990,6 +5990,12 @@
         },
         "pong": {
           "type": "boolean"
+        },
+        "initContainerImages": {
+          "type": "array",
+          "items": {
+            "type": "string"
+          }
         }
       }
     },
@@ -6097,6 +6103,12 @@
           "items": {
             "$ref": "#/definitions/github.com.argoproj.argo_rollouts.pkg.apis.rollouts.v1alpha1.CanaryStep"
           }
+        },
+        "initContainers": {
+          "type": "array",
+          "items": {
+            "$ref": "#/definitions/rollout.ContainerInfo"
+          }
         }
       }
     },

pkg/apis/rollouts/validation/validation.go

@@ -81,8 +81,8 @@ const (
 	DuplicatedPingPongServicesMessage = "This rollout uses the same service for the ping and pong services, but two different services are required."
 	// MissedAlbRootServiceMessage indicates that the rollout with ALB TrafficRouting and ping pong feature enabled must have root service provided
 	MissedAlbRootServiceMessage = "Root service field is required for the configuration with ALB and ping-pong feature enabled"
-	// PingPongWithAlbOnlyMessage At this moment ping-pong feature works with the ALB traffic routing only
-	PingPongWithAlbOnlyMessage = "Ping-pong feature works with the ALB traffic routing only"
+	// PingPongWithRouterOnlyMessage At this moment ping-pong feature works with the ALB traffic routing only
+	PingPongWithRouterOnlyMessage = "Ping-pong feature works with the ALB and Istio traffic routers only"
 	// InvalideStepRouteNameNotFoundInManagedRoutes A step has been configured that requires managedRoutes and the route name
 	// is missing from managedRoutes
 	InvalideStepRouteNameNotFoundInManagedRoutes = "Steps define a route that does not exist in spec.strategy.canary.trafficRouting.managedRoutes"
@@ -241,7 +241,7 @@ func requireCanaryStableServices(rollout *v1alpha1.Rollout) bool {
 
 	switch {
 	case canary.TrafficRouting.ALB != nil && canary.PingPong == nil,
-		canary.TrafficRouting.Istio != nil && canary.TrafficRouting.Istio.DestinationRule == nil,
+		canary.TrafficRouting.Istio != nil && canary.TrafficRouting.Istio.DestinationRule == nil && canary.PingPong == nil,
 		canary.TrafficRouting.SMI != nil,
 		canary.TrafficRouting.Apisix != nil,
 		canary.TrafficRouting.Ambassador != nil,
@@ -262,8 +262,8 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("stableService"), canary.StableService, DuplicatedServicesCanaryMessage))
 	}
 	if canary.PingPong != nil {
-		if canary.TrafficRouting != nil && canary.TrafficRouting.ALB == nil {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("trafficRouting").Child("alb"), canary.TrafficRouting.ALB, PingPongWithAlbOnlyMessage))
+		if canary.TrafficRouting != nil && canary.TrafficRouting.ALB == nil && canary.TrafficRouting.Istio == nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("trafficRouting").Child("alb"), canary.TrafficRouting.ALB, PingPongWithRouterOnlyMessage))
 		}
 		if canary.PingPong.PingService == "" {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("pingPong").Child("pingService"), canary.PingPong.PingService, InvalidPingPongProvidedMessage))

pkg/apis/rollouts/validation/validation_test.go

@@ -290,6 +290,21 @@ func TestValidateRolloutStrategyCanary(t *testing.T) {
 		assert.Empty(t, allErrs)
 	})
 
+	t.Run("valid Istio with ping pong", func(t *testing.T) {
+		validRo := ro.DeepCopy()
+		validRo.Spec.Strategy.Canary.Steps[0].SetWeight = pointer.Int32(10)
+		validRo.Spec.Strategy.Canary.CanaryService = ""
+		validRo.Spec.Strategy.Canary.StableService = ""
+		validRo.Spec.Strategy.Canary.PingPong = &v1alpha1.PingPongSpec{
+			PingService: "ping",
+			PongService: "pong",
+		}
+		validRo.Spec.Strategy.Canary.TrafficRouting.Istio = &v1alpha1.IstioTrafficRouting{DestinationRule: &v1alpha1.IstioDestinationRule{Name: "destination-rule"}}
+		validRo.Spec.Strategy.Canary.TrafficRouting.ALB = nil
+		allErrs := ValidateRolloutStrategyCanary(validRo, field.NewPath(""))
+		assert.Empty(t, allErrs)
+	})
+
 	t.Run("valid PingPong missing canary and stable service", func(t *testing.T) {
 		validRo := ro.DeepCopy()
 		validRo.Spec.Strategy.Canary.Steps[0].SetWeight = pointer.Int32(10)
@@ -368,7 +383,7 @@ func TestValidateRolloutStrategyCanary(t *testing.T) {
 			Nginx: &v1alpha1.NginxTrafficRouting{StableIngress: "stable-ingress"},
 		}
 		allErrs := ValidateRolloutStrategyCanary(invalidRo, field.NewPath(""))
-		assert.Equal(t, PingPongWithAlbOnlyMessage, allErrs[0].Detail)
+		assert.Equal(t, PingPongWithRouterOnlyMessage, allErrs[0].Detail)
 	})
 
 	t.Run("invalid traffic routing", func(t *testing.T) {

pkg/kubectl-argo-rollouts/info/replicaset_info.go

@@ -73,6 +73,11 @@ func GetReplicaSetInfo(ownerUID types.UID, ro *v1alpha1.Rollout, allReplicaSets
 		for _, ctr := range rs.Spec.Template.Spec.Containers {
 			rsInfo.Images = append(rsInfo.Images, ctr.Image)
 		}
+
+		for _, ctr := range rs.Spec.Template.Spec.InitContainers {
+			rsInfo.InitContainerImages = append(rsInfo.InitContainerImages, ctr.Image)
+		}
+
 		rsInfos = append(rsInfos, rsInfo)
 	}
 	sort.Slice(rsInfos[:], func(i, j int) bool {

pkg/kubectl-argo-rollouts/info/rollout_info.go

@@ -85,16 +85,23 @@ func NewRolloutInfo(
 	roInfo.Containers = []*rollout.ContainerInfo{}
 
 	var containerList []corev1.Container
+	var initContainerList []corev1.Container
 	if workloadRef != nil {
 		containerList = workloadRef.Spec.Template.Spec.Containers
+		initContainerList = workloadRef.Spec.Template.Spec.InitContainers
 	} else {
 		containerList = ro.Spec.Template.Spec.Containers
+		initContainerList = ro.Spec.Template.Spec.InitContainers
 	}
 
 	for _, c := range containerList {
 		roInfo.Containers = append(roInfo.Containers, &rollout.ContainerInfo{Name: c.Name, Image: c.Image})
 	}
 
+	for _, c := range initContainerList {
+		roInfo.InitContainers = append(roInfo.InitContainers, &rollout.ContainerInfo{Name: c.Name, Image: c.Image})
+	}
+
 	if ro.Status.RestartedAt != nil {
 		roInfo.RestartedAt = ro.Status.RestartedAt.String()
 	} else {

rollout/trafficrouting.go

@@ -305,7 +305,7 @@ func (c *rolloutContext) calculateDesiredWeightOnAbortOrStableRollback() int32 {
 	}
 	// When using dynamic stable scaling, we must dynamically decreasing the weight to the canary
 	// according to the availability of the stable (whatever it can support).
-	desiredWeight := weightutil.MaxTrafficWeight(c.rollout) - ((weightutil.MaxTrafficWeight(c.rollout) * c.stableRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas)
+	desiredWeight := maxInt(0, weightutil.MaxTrafficWeight(c.rollout)-((weightutil.MaxTrafficWeight(c.rollout)*c.stableRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas))
 	if c.rollout.Status.Canary.Weights != nil {
 		// This ensures that if we are already at a lower weight, then we will not
 		// increase the weight because stable availability is flapping (e.g. pod restarts)

rollout/trafficrouting/istio/istio.go

@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/argoproj/argo-rollouts/rollout/trafficrouting"
+
 	jsonpatch "github.com/evanphx/json-patch/v5"
 	"github.com/mitchellh/mapstructure"
 	log "github.com/sirupsen/logrus"
@@ -123,8 +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 {
-	canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService
-	stableSvc := r.rollout.Spec.Strategy.Canary.StableService
+	stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout)
 	canarySubset := ""
 	stableSubset := ""
 	if r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule != nil {
@@ -717,7 +718,7 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1
 		return err
 	}
 
-	canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService
+	_, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout)
 	if destRuleHost != "" {
 		canarySvc = destRuleHost
 	}
@@ -1022,8 +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 := r.Spec.Strategy.Canary.StableService
-	canarySvc := r.Spec.Strategy.Canary.CanaryService
+	stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r)
 
 	routeIndexesToPatch, err := getHttpRouteIndexesToPatch(routeNames, httpRoutes)
 	if err != nil {
@@ -1060,8 +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 := r.Spec.Strategy.Canary.StableService
-	canarySvc := r.Spec.Strategy.Canary.CanaryService
+	stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r)
 
 	routeIndexesToPatch, err := getTlsRouteIndexesToPatch(vsvcTLSRoutes, tlsRoutes)
 	if err != nil {
@@ -1082,8 +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 := r.Spec.Strategy.Canary.StableService
-	canarySvc := r.Spec.Strategy.Canary.CanaryService
+	stableSvc, canarySvc := trafficrouting.GetStableAndCanaryServices(r)
 
 	routeIndexesToPatch, err := getTcpRouteIndexesToPatch(vsvcTCPRoutes, tcpRoutes)
 	if err != nil {
@@ -1191,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 := r.rollout.Spec.Strategy.Canary.CanaryService
+	_, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout)
 	if destRuleHost != "" {
 		canarySvc = destRuleHost
 	}