From 1a22777ca611ae2d8bc0dbd2fcda1daf17e87622 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 13 Feb 2024 20:41:33 -0600 Subject: [PATCH 01/10] feat: wip ping pong support for istio Signed-off-by: Zach Aller --- rollout/trafficrouting/istio/istio.go | 27 ++-- rollout/trafficrouting/istio/istio_test.go | 176 ++++++++++++++++++++- 2 files changed, 189 insertions(+), 14 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index dd44da7b25..f8c3f4a6c8 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/argoproj/argo-rollouts/rollout/trafficrouting" "strings" jsonpatch "github.com/evanphx/json-patch/v5" @@ -123,8 +124,9 @@ 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 + //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 +719,8 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1 return err } - canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService + //canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService + _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout) if destRuleHost != "" { canarySvc = destRuleHost } @@ -1022,8 +1025,9 @@ 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 := 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 +1064,9 @@ 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 := 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 +1087,9 @@ 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 := 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 +1197,8 @@ 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 := r.rollout.Spec.Strategy.Canary.CanaryService + _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout) if destRuleHost != "" { canarySvc = destRuleHost } diff --git a/rollout/trafficrouting/istio/istio_test.go b/rollout/trafficrouting/istio/istio_test.go index c18cdedae5..c6c0f8d9a1 100644 --- a/rollout/trafficrouting/istio/istio_test.go +++ b/rollout/trafficrouting/istio/istio_test.go @@ -64,6 +64,31 @@ func rollout(stableSvc, canarySvc string, istioVirtualService *v1alpha1.IstioVir } } +func rolloutPingPong(istioVirtualService *v1alpha1.IstioVirtualService) *v1alpha1.Rollout { + return &v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rollout", + Namespace: "default", + }, + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + PingPong: &v1alpha1.PingPongSpec{ + PingService: "ping", + PongService: "pong", + }, + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + Istio: &v1alpha1.IstioTrafficRouting{ + VirtualService: istioVirtualService, + }, + }, + }, + }, + }, + Status: v1alpha1.RolloutStatus{Canary: v1alpha1.CanaryStatus{StablePingPong: "ping"}}, + } +} + func rolloutWithHttpRoutes(stableSvc, canarySvc, vsvc string, httpRoutes []string) *v1alpha1.Rollout { istioVirtualService := &v1alpha1.IstioVirtualService{ Name: vsvc, @@ -98,6 +123,16 @@ func rolloutWithHttpAndTlsAndTcpRoutes(stableSvc, canarySvc, vsvc string, httpRo return rollout(stableSvc, canarySvc, istioVirtualService) } +func rolloutWithHttpAndTlsAndTcpRoutesPingPong(vsvc string, httpRoutes []string, tlsRoutes []v1alpha1.TLSRoute, tcpRoutes []v1alpha1.TCPRoute) *v1alpha1.Rollout { + istioVirtualService := &v1alpha1.IstioVirtualService{ + Name: vsvc, + Routes: httpRoutes, + TLSRoutes: tlsRoutes, + TCPRoutes: tcpRoutes, + } + return rolloutPingPong(istioVirtualService) +} + func checkDestination(t *testing.T, destinations []VirtualServiceRouteDestination, svc string, expectWeight int) { for _, destination := range destinations { if destination.Destination.Host == svc { @@ -263,6 +298,72 @@ spec: host: canary weight: 0` +const regularMixedVsvcPingPong = `apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: vsvc + namespace: default +spec: + gateways: + - istio-rollout-gateway + hosts: + - istio-rollout.dev.argoproj.io + http: + - name: primary + route: + - destination: + host: 'ping' + weight: 100 + - destination: + host: pong + weight: 0 + - name: secondary + route: + - destination: + host: 'ping' + weight: 100 + - destination: + host: pong + weight: 0 + tls: + - match: + - port: 3000 + route: + - destination: + host: 'ping' + weight: 100 + - destination: + host: pong + weight: 0 + - match: + - port: 3001 + route: + - destination: + host: 'ping' + weight: 100 + - destination: + host: pong + weight: 0 + tcp: + - match: + - port: 3000 + route: + - destination: + host: 'ping' + weight: 100 + - destination: + host: pong + weight: 0 + - match: + - port: 3001 + route: + - destination: + host: 'ping' + weight: 100 + - destination: + host: pong + weight: 0` + const regularMixedVsvcTwoHttpRoutes = `apiVersion: networking.istio.io/v1alpha3 kind: VirtualService metadata: @@ -597,6 +698,14 @@ func extractTcpRoutes(t *testing.T, modifiedObj *unstructured.Unstructured) []Vi } func assertTcpRouteWeightChanges(t *testing.T, tcpRoute VirtualServiceTCPRoute, portNum, canaryWeight, stableWeight int) { + assertTcpRouteWeightChangesBase(t, tcpRoute, portNum, canaryWeight, stableWeight, "stable", "canary") +} + +func assertTcpRouteWeightChangesPingPong(t *testing.T, tcpRoute VirtualServiceTCPRoute, portNum, canaryWeight, stableWeight int) { + assertTcpRouteWeightChangesBase(t, tcpRoute, portNum, canaryWeight, stableWeight, "ping", "pong") +} + +func assertTcpRouteWeightChangesBase(t *testing.T, tcpRoute VirtualServiceTCPRoute, portNum, canaryWeight, stableWeight int, stableSvc, canarySvc string) { portsMap := make(map[int64]bool) for _, routeMatch := range tcpRoute.Match { if routeMatch.Port != 0 { @@ -610,8 +719,8 @@ func assertTcpRouteWeightChanges(t *testing.T, tcpRoute VirtualServiceTCPRoute, if portNum != 0 { assert.Equal(t, portNum, port) } - checkDestination(t, tcpRoute.Route, "stable", stableWeight) - checkDestination(t, tcpRoute.Route, "canary", canaryWeight) + checkDestination(t, tcpRoute.Route, stableSvc, stableWeight) + checkDestination(t, tcpRoute.Route, canarySvc, canaryWeight) } func extractHttpRoutes(t *testing.T, modifiedObj *unstructured.Unstructured) []VirtualServiceHTTPRoute { @@ -643,6 +752,14 @@ func extractTlsRoutes(t *testing.T, modifiedObj *unstructured.Unstructured) []Vi } func assertTlsRouteWeightChanges(t *testing.T, tlsRoute VirtualServiceTLSRoute, snis []string, portNum, canaryWeight, stableWeight int) { + assertTlsRouteWeightChangesBase(t, tlsRoute, snis, portNum, canaryWeight, stableWeight, "stable", "canary") +} + +func assertTlsRouteWeightChangesPingPong(t *testing.T, tlsRoute VirtualServiceTLSRoute, snis []string, portNum, canaryWeight, stableWeight int) { + assertTlsRouteWeightChangesBase(t, tlsRoute, snis, portNum, canaryWeight, stableWeight, "ping", "pong") +} + +func assertTlsRouteWeightChangesBase(t *testing.T, tlsRoute VirtualServiceTLSRoute, snis []string, portNum, canaryWeight, stableWeight int, stableSvc, canarySvc string) { portsMap := make(map[int64]bool) sniHostsMap := make(map[string]bool) for _, routeMatch := range tlsRoute.Match { @@ -667,8 +784,8 @@ func assertTlsRouteWeightChanges(t *testing.T, tlsRoute VirtualServiceTLSRoute, if len(snis) != 0 { assert.Equal(t, evalUtils.Equal(snis, sniHosts), true) } - checkDestination(t, tlsRoute.Route, "stable", stableWeight) - checkDestination(t, tlsRoute.Route, "canary", canaryWeight) + checkDestination(t, tlsRoute.Route, stableSvc, stableWeight) + checkDestination(t, tlsRoute.Route, canarySvc, canaryWeight) } func TestHttpReconcileWeightsBaseCase(t *testing.T) { @@ -1104,6 +1221,57 @@ func TestReconcileWeightsBaseCase(t *testing.T) { assertTcpRouteWeightChanges(t, tcpRoutes[1], 3001, 0, 100) } +func TestReconcileWeightsPingPongBaseCase(t *testing.T) { + r := &Reconciler{ + rollout: rolloutWithHttpAndTlsAndTcpRoutesPingPong("vsvc", []string{"primary"}, + []v1alpha1.TLSRoute{ + { + Port: 3000, + }, + }, + []v1alpha1.TCPRoute{ + { + Port: 3000, + }, + }, + ), + } + obj := unstructuredutil.StrToUnstructuredUnsafe(regularMixedVsvcPingPong) + vsvcRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes + vsvcTLSRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.TLSRoutes + vsvcTCPRoutes := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.TCPRoutes + modifiedObj, _, err := r.reconcileVirtualService(obj, vsvcRoutes, vsvcTLSRoutes, vsvcTCPRoutes, 20) + assert.Nil(t, err) + assert.NotNil(t, modifiedObj) + + // HTTP Routes + httpRoutes := extractHttpRoutes(t, modifiedObj) + + // Assertions + assert.Equal(t, httpRoutes[0].Name, "primary") + checkDestination(t, httpRoutes[0].Route, "ping", 80) + checkDestination(t, httpRoutes[0].Route, "pong", 20) + + //assertHttpRouteWeightChanges(t, httpRoutes[1], "secondary", 0, 100) + assert.Equal(t, httpRoutes[1].Name, "secondary") + checkDestination(t, httpRoutes[1].Route, "ping", 100) + checkDestination(t, httpRoutes[1].Route, "pong", 0) + + // TLS Routes + tlsRoutes := extractTlsRoutes(t, modifiedObj) + // + // Assestions + assertTlsRouteWeightChangesPingPong(t, tlsRoutes[0], nil, 3000, 20, 80) + assertTlsRouteWeightChangesPingPong(t, tlsRoutes[1], nil, 3001, 0, 100) + // + // TCP Routes + tcpRoutes := extractTcpRoutes(t, modifiedObj) + + // Assestions + assertTcpRouteWeightChangesPingPong(t, tcpRoutes[0], 3000, 20, 80) + assertTcpRouteWeightChangesPingPong(t, tcpRoutes[1], 3001, 0, 100) +} + func TestReconcileUpdateVirtualService(t *testing.T) { ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"}) AssertReconcileUpdateVirtualService(t, regularVsvc, ro) From 2368234e4766a256756e0e56ee6abbed3c20d3f7 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 13 Feb 2024 21:02:26 -0600 Subject: [PATCH 02/10] feat: wip ping pong validation fixes Signed-off-by: Zach Aller --- pkg/apis/rollouts/validation/validation.go | 10 +++++----- pkg/apis/rollouts/validation/validation_test.go | 6 +++++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index 54a7336566..3884a2c8ec 100644 --- a/pkg/apis/rollouts/validation/validation.go +++ b/pkg/apis/rollouts/validation/validation.go @@ -78,8 +78,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" @@ -238,7 +238,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, @@ -259,8 +259,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)) diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go index 28f17c6858..470273dc90 100644 --- a/pkg/apis/rollouts/validation/validation_test.go +++ b/pkg/apis/rollouts/validation/validation_test.go @@ -284,6 +284,10 @@ func TestValidateRolloutStrategyCanary(t *testing.T) { 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("")) @@ -368,7 +372,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) { From 7b6714d4d382f62bd60e97392c3a6c92d16720c7 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 13 Feb 2024 22:17:44 -0600 Subject: [PATCH 03/10] add istio ping pong e2e Signed-off-by: Zach Aller --- .../e2e/istio/istio-host-split-ping-pong.yaml | 84 +++++++++++++++++++ test/e2e/istio_test.go | 51 +++++++++++ 2 files changed, 135 insertions(+) create mode 100644 test/e2e/istio/istio-host-split-ping-pong.yaml diff --git a/test/e2e/istio/istio-host-split-ping-pong.yaml b/test/e2e/istio/istio-host-split-ping-pong.yaml new file mode 100644 index 0000000000..b9eb38ad37 --- /dev/null +++ b/test/e2e/istio/istio-host-split-ping-pong.yaml @@ -0,0 +1,84 @@ +apiVersion: v1 +kind: Service +metadata: + name: pong +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: istio-host-split + +--- +apiVersion: v1 +kind: Service +metadata: + name: ping +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: istio-host-split + +--- +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: istio-host-split-vsvc +spec: + hosts: + - istio-host-split + http: + - name: primary + route: + - destination: + host: ping + weight: 100 + - destination: + host: pong + weight: 0 + +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: istio-host-split +spec: + strategy: + canary: + pingPong: + pingService: ping + pongService: pong + trafficRouting: + istio: + virtualService: + name: istio-host-split-vsvc + routes: + - primary + steps: + - setWeight: 25 + - pause: { duration: 5s } + selector: + matchLabels: + app: istio-host-split + template: + metadata: + labels: + app: istio-host-split + spec: + containers: + - name: istio-host-split + image: nginx:1.19-alpine + ports: + - name: http + containerPort: 80 + protocol: TCP + resources: + requests: + memory: 16Mi + cpu: 5m diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go index 7ecbd66fdf..e0fa454f22 100644 --- a/test/e2e/istio_test.go +++ b/test/e2e/istio_test.go @@ -492,3 +492,54 @@ func (s *IstioSuite) TestIstioSubsetSplitExperimentStep() { s.TearDownSuite() } + +func (s *IstioSuite) TestIstioPingPongUpdate() { + s.Given(). + RolloutObjects("@istio/istio-host-split-ping-pong.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) + }). + // 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) + }). + 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(0), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(100), vsvc.Spec.HTTP[0].Route[1].Weight) + }). + // 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(25), vsvc.Spec.HTTP[0].Route[0].Weight) + assert.Equal(s.T(), int64(75), vsvc.Spec.HTTP[0].Route[1].Weight) + }). + 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) + }) +} From 2a4e7a1747b09a858f903f6b2576958bc35ec9f0 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 13 Feb 2024 22:18:05 -0600 Subject: [PATCH 04/10] add istio ping pong e2e Signed-off-by: Zach Aller --- pkg/apis/rollouts/validation/validation.go | 2 +- rollout/trafficrouting/istio/istio.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index 3884a2c8ec..08f5e73594 100644 --- a/pkg/apis/rollouts/validation/validation.go +++ b/pkg/apis/rollouts/validation/validation.go @@ -238,7 +238,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.PingPong == 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, diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index f8c3f4a6c8..1b16ecdaff 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -4,9 +4,10 @@ import ( "context" "encoding/json" "fmt" - "github.com/argoproj/argo-rollouts/rollout/trafficrouting" "strings" + "github.com/argoproj/argo-rollouts/rollout/trafficrouting" + jsonpatch "github.com/evanphx/json-patch/v5" "github.com/mitchellh/mapstructure" log "github.com/sirupsen/logrus" From 81c27d5470042cd54b445bde0aecb3edc8dc32dc Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Wed, 14 Feb 2024 09:03:39 -0600 Subject: [PATCH 05/10] clean up comments Signed-off-by: Zach Aller --- rollout/trafficrouting/istio/istio.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go index 1b16ecdaff..5308d59a42 100644 --- a/rollout/trafficrouting/istio/istio.go +++ b/rollout/trafficrouting/istio/istio.go @@ -125,8 +125,6 @@ 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 := "" @@ -720,7 +718,6 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1 return err } - //canarySvc := r.rollout.Spec.Strategy.Canary.CanaryService _, canarySvc := trafficrouting.GetStableAndCanaryServices(r.rollout) if destRuleHost != "" { canarySvc = destRuleHost @@ -1026,8 +1023,6 @@ 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) @@ -1065,8 +1060,6 @@ 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) @@ -1088,8 +1081,6 @@ 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) @@ -1198,7 +1189,6 @@ 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 From 040462c8221ac614258e203ae1440fdd81109622 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Wed, 14 Feb 2024 10:45:12 -0600 Subject: [PATCH 06/10] add ping pong and destRule both configred e2e test Signed-off-by: Zach Aller --- .../istio/istio-subset-split-ping-pong.yaml | 119 ++++++++++++++++++ test/e2e/istio_test.go | 70 +++++++++++ 2 files changed, 189 insertions(+) create mode 100644 test/e2e/istio/istio-subset-split-ping-pong.yaml diff --git a/test/e2e/istio/istio-subset-split-ping-pong.yaml b/test/e2e/istio/istio-subset-split-ping-pong.yaml new file mode 100644 index 0000000000..e7ff7d26c6 --- /dev/null +++ b/test/e2e/istio/istio-subset-split-ping-pong.yaml @@ -0,0 +1,119 @@ +apiVersion: v1 +kind: Service +metadata: + name: istio-subset-split +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: istio-subset-split +--- + +apiVersion: v1 +kind: Service +metadata: + name: ping +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: istio-subset-split + +--- +apiVersion: v1 +kind: Service +metadata: + name: pong +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: istio-subset-split + +--- +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: istio-subset-split-vsvc +spec: + hosts: + - istio-subset-split + http: + - name: primary + route: + - destination: + host: istio-subset-split + subset: ping + weight: 100 + - destination: + host: istio-subset-split + subset: pong + weight: 0 + +--- +apiVersion: networking.istio.io/v1alpha3 +kind: DestinationRule +metadata: + name: istio-subset-split-destrule +spec: + host: istio-subset-split + subsets: + - name: ping + labels: + app: istio-subset-split + - name: pong + labels: + app: istio-subset-split + +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: istio-subset-split +spec: + strategy: + canary: + pingPong: + pingService: ping + pongService: pong + trafficRouting: + istio: + virtualService: + name: istio-subset-split-vsvc + routes: + - primary + destinationRule: + name: istio-subset-split-destrule + canarySubsetName: pong + stableSubsetName: ping + steps: + - setWeight: 25 + - pause: { duration: 5s } + selector: + matchLabels: + app: istio-subset-split + template: + metadata: + labels: + app: istio-subset-split + spec: + containers: + - name: istio-subset-split + image: nginx:1.19-alpine + ports: + - name: http + containerPort: 80 + protocol: TCP + resources: + requests: + memory: 16Mi + cpu: 5m diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go index e0fa454f22..3ceaf0f702 100644 --- a/test/e2e/istio_test.go +++ b/test/e2e/istio_test.go @@ -543,3 +543,73 @@ func (s *IstioSuite) TestIstioPingPongUpdate() { assert.Equal(s.T(), int64(0), vsvc.Spec.HTTP[0].Route[1].Weight) }) } + +func (s *IstioSuite) TestIstioSubsetSplitPingPong() { + s.Given(). + RolloutObjects("@istio/istio-subset-split-ping-pong.yaml"). + When(). + ApplyManifests(). + WaitForRolloutStatus("Healthy"). + Then(). + 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) + + rs1 := t.GetReplicaSetByRevision("1") + destrule := t.GetDestinationRule() + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // stable + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // canary + }). + When(). + UpdateSpec(). + WaitForRolloutStatus("Paused"). + Then(). + 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) + + rs1 := t.GetReplicaSetByRevision("1") + rs2 := t.GetReplicaSetByRevision("2") + destrule := t.GetDestinationRule() + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // stable + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // canary + }). + When(). + PromoteRollout(). + WaitForRolloutStatus("Healthy"). + Sleep(1*time.Second). // stable is currently set first, and then changes made to VirtualServices/DestinationRules + Then(). + 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) + + rs2 := t.GetReplicaSetByRevision("2") + destrule := t.GetDestinationRule() + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // stable + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // canary + }). + ExpectRevisionPodCount("1", 1). // don't scale down old replicaset since it will be within scaleDownDelay + When(). + // Verify we remove the injections on the DestinationRule when a rollout no longer references it + UpdateSpec(` +spec: + strategy: + canary: + trafficRouting: null +`). + Sleep(2*time.Second). + Then(). + Assert(func(t *fixtures.Then) { + destrule := t.GetDestinationRule() + _, ok := destrule.Annotations[v1alpha1.ManagedByRolloutsKey] + assert.False(s.T(), ok) + _, ok = destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + assert.False(s.T(), ok) + _, ok = destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + assert.False(s.T(), ok) + }). + ExpectRevisionPodCount("1", 0) // since we moved back to basic canary, we should scale down older RSs +} From 7b2f694478d6ffcf1d945aae0575241851ef8632 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 26 Mar 2024 11:55:58 -0500 Subject: [PATCH 07/10] cleanup not needed test Signed-off-by: Zach Aller --- .../istio/istio-subset-split-ping-pong.yaml | 119 ------------------ test/e2e/istio_test.go | 70 ----------- 2 files changed, 189 deletions(-) delete mode 100644 test/e2e/istio/istio-subset-split-ping-pong.yaml diff --git a/test/e2e/istio/istio-subset-split-ping-pong.yaml b/test/e2e/istio/istio-subset-split-ping-pong.yaml deleted file mode 100644 index e7ff7d26c6..0000000000 --- a/test/e2e/istio/istio-subset-split-ping-pong.yaml +++ /dev/null @@ -1,119 +0,0 @@ -apiVersion: v1 -kind: Service -metadata: - name: istio-subset-split -spec: - ports: - - port: 80 - targetPort: http - protocol: TCP - name: http - selector: - app: istio-subset-split ---- - -apiVersion: v1 -kind: Service -metadata: - name: ping -spec: - ports: - - port: 80 - targetPort: http - protocol: TCP - name: http - selector: - app: istio-subset-split - ---- -apiVersion: v1 -kind: Service -metadata: - name: pong -spec: - ports: - - port: 80 - targetPort: http - protocol: TCP - name: http - selector: - app: istio-subset-split - ---- -apiVersion: networking.istio.io/v1alpha3 -kind: VirtualService -metadata: - name: istio-subset-split-vsvc -spec: - hosts: - - istio-subset-split - http: - - name: primary - route: - - destination: - host: istio-subset-split - subset: ping - weight: 100 - - destination: - host: istio-subset-split - subset: pong - weight: 0 - ---- -apiVersion: networking.istio.io/v1alpha3 -kind: DestinationRule -metadata: - name: istio-subset-split-destrule -spec: - host: istio-subset-split - subsets: - - name: ping - labels: - app: istio-subset-split - - name: pong - labels: - app: istio-subset-split - ---- -apiVersion: argoproj.io/v1alpha1 -kind: Rollout -metadata: - name: istio-subset-split -spec: - strategy: - canary: - pingPong: - pingService: ping - pongService: pong - trafficRouting: - istio: - virtualService: - name: istio-subset-split-vsvc - routes: - - primary - destinationRule: - name: istio-subset-split-destrule - canarySubsetName: pong - stableSubsetName: ping - steps: - - setWeight: 25 - - pause: { duration: 5s } - selector: - matchLabels: - app: istio-subset-split - template: - metadata: - labels: - app: istio-subset-split - spec: - containers: - - name: istio-subset-split - image: nginx:1.19-alpine - ports: - - name: http - containerPort: 80 - protocol: TCP - resources: - requests: - memory: 16Mi - cpu: 5m diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go index 5c3eba5af5..9797b3e503 100644 --- a/test/e2e/istio_test.go +++ b/test/e2e/istio_test.go @@ -544,76 +544,6 @@ func (s *IstioSuite) TestIstioPingPongUpdate() { }) } -func (s *IstioSuite) TestIstioSubsetSplitPingPong() { - s.Given(). - RolloutObjects("@istio/istio-subset-split-ping-pong.yaml"). - When(). - ApplyManifests(). - WaitForRolloutStatus("Healthy"). - Then(). - 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) - - rs1 := t.GetReplicaSetByRevision("1") - destrule := t.GetDestinationRule() - assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // stable - assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // canary - }). - When(). - UpdateSpec(). - WaitForRolloutStatus("Paused"). - Then(). - 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) - - rs1 := t.GetReplicaSetByRevision("1") - rs2 := t.GetReplicaSetByRevision("2") - destrule := t.GetDestinationRule() - assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // stable - assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // canary - }). - When(). - PromoteRollout(). - WaitForRolloutStatus("Healthy"). - Sleep(1*time.Second). // stable is currently set first, and then changes made to VirtualServices/DestinationRules - Then(). - 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) - - rs2 := t.GetReplicaSetByRevision("2") - destrule := t.GetDestinationRule() - assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // stable - assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey]) // canary - }). - ExpectRevisionPodCount("1", 1). // don't scale down old replicaset since it will be within scaleDownDelay - When(). - // Verify we remove the injections on the DestinationRule when a rollout no longer references it - UpdateSpec(` -spec: - strategy: - canary: - trafficRouting: null -`). - Sleep(2*time.Second). - Then(). - Assert(func(t *fixtures.Then) { - destrule := t.GetDestinationRule() - _, ok := destrule.Annotations[v1alpha1.ManagedByRolloutsKey] - assert.False(s.T(), ok) - _, ok = destrule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - assert.False(s.T(), ok) - _, ok = destrule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - assert.False(s.T(), ok) - }). - ExpectRevisionPodCount("1", 0) // since we moved back to basic canary, we should scale down older RSs -} - func (s *IstioSuite) TestIstioSubsetSplitInStableDownscaleAfterCanaryAbort() { s.Given(). RolloutObjects("@istio/istio-subset-split-in-stable-downscale-after-canary-abort.yaml"). From 1546fc78b206b86facface24eb973adb51f738db Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 26 Mar 2024 12:27:22 -0500 Subject: [PATCH 08/10] add docs Signed-off-by: Zach Aller --- docs/features/traffic-management/istio.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/features/traffic-management/istio.md b/docs/features/traffic-management/istio.md index 415313841a..9d146d545c 100644 --- a/docs/features/traffic-management/istio.md +++ b/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 From 4abea331f86c8cce3d0db24d2585b8e37525268e Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 26 Mar 2024 13:11:09 -0500 Subject: [PATCH 09/10] add new test instead of piggy backing of an old one Signed-off-by: Zach Aller --- pkg/apis/rollouts/validation/validation_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go index 79aecf251d..33301d099a 100644 --- a/pkg/apis/rollouts/validation/validation_test.go +++ b/pkg/apis/rollouts/validation/validation_test.go @@ -280,6 +280,17 @@ func TestValidateRolloutStrategyCanary(t *testing.T) { }) t.Run("valid Istio missing canary and stable service", 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.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 Istio missing canary and stable service 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 = "" From f9bc1d9efcaa4348ee4256ba3e71f0ec11665e1a Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 26 Mar 2024 13:11:55 -0500 Subject: [PATCH 10/10] change test name Signed-off-by: Zach Aller --- pkg/apis/rollouts/validation/validation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go index 33301d099a..c9fc9ad923 100644 --- a/pkg/apis/rollouts/validation/validation_test.go +++ b/pkg/apis/rollouts/validation/validation_test.go @@ -290,7 +290,7 @@ func TestValidateRolloutStrategyCanary(t *testing.T) { assert.Empty(t, allErrs) }) - t.Run("valid Istio missing canary and stable service with ping pong", func(t *testing.T) { + 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 = ""