fix: Update ro.Status.ALB when first creating rollout object

rollout/service_test.go

@@ -459,7 +459,10 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) {
 	}
 	fakeELB.On("DescribeTargetHealth", mock.Anything, mock.Anything).Return(&thOut, nil)
 
-	r1 := newCanaryRollout("foo", 3, nil, nil, nil, intstr.FromString("25%"), intstr.FromString("25%"))
+	r1 := newCanaryRollout("foo", 3, nil, []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(10),
+	}}, pointer.Int32Ptr(0), intstr.FromString("25%"), intstr.FromString("25%"))
+
 	r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
 		ALB: &v1alpha1.ALBTrafficRouting{
 			Ingress:     "ingress",
@@ -553,7 +556,9 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) {
 	}
 	fakeELB.On("DescribeTargetHealth", mock.Anything, mock.Anything).Return(&thOut, nil)
 
-	r1 := newCanaryRollout("foo", 3, nil, nil, nil, intstr.FromString("25%"), intstr.FromString("25%"))
+	r1 := newCanaryRollout("foo", 3, nil, []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(10),
+	}}, pointer.Int32Ptr(0), intstr.FromString("25%"), intstr.FromString("25%"))
 	r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
 		ALB: &v1alpha1.ALBTrafficRouting{
 			Ingress:     "ingress",
@@ -610,7 +615,9 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
 
-	r1 := newCanaryRollout("foo", 3, nil, nil, nil, intstr.FromString("25%"), intstr.FromString("25%"))
+	r1 := newCanaryRollout("foo", 3, nil, []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(10),
+	}}, pointer.Int32Ptr(0), intstr.FromString("25%"), intstr.FromString("25%"))
 	r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
 		ALB: &v1alpha1.ALBTrafficRouting{
 			Ingress:     "ingress",

rollout/trafficrouting.go

@@ -3,6 +3,7 @@ package rollout
 import (
 	"fmt"
 	"reflect"
+	"strconv"
 	"strings"
 
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
@@ -232,27 +233,26 @@ func (c *rolloutContext) reconcileTrafficRouting() error {
 			c.newStatus.Canary.Weights = newWeights
 		}
 
-		// If we are in the middle of an update at a setWeight step, also perform weight verification.
-		// Note that we don't do this every reconciliation because weight verification typically involves
-		// API calls to the cloud provider which could incur rate limiting
-		shouldVerifyWeight := c.rollout.Status.StableRS != "" &&
-			!rolloututil.IsFullyPromoted(c.rollout) &&
-			currentStep != nil && currentStep.SetWeight != nil
+		weightVerified, err := reconciler.VerifyWeight(desiredWeight, weightDestinations...)
+		c.newStatus.Canary.Weights.Verified = weightVerified
+		if err != nil {
+			c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.WeightVerifyErrorReason}, conditions.WeightVerifyErrorMessage, err)
+			return nil // return nil instead of error since we want to continue with normal reconciliation
+		}
 
-		if shouldVerifyWeight {
-			weightVerified, err := reconciler.VerifyWeight(desiredWeight, weightDestinations...)
-			c.newStatus.Canary.Weights.Verified = weightVerified
-			if err != nil {
-				c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.WeightVerifyErrorReason}, conditions.WeightVerifyErrorMessage, err)
-				return nil // return nil instead of error since we want to continue with normal reconciliation
-			}
-			if weightVerified != nil {
-				if *weightVerified {
-					c.log.Infof("Desired weight (stepIdx: %d) %d verified", *index, desiredWeight)
-				} else {
-					c.log.Infof("Desired weight (stepIdx: %d) %d not yet verified", *index, desiredWeight)
-					c.enqueueRolloutAfter(c.rollout, defaults.GetRolloutVerifyRetryInterval())
-				}
+		var indexString string
+		if index != nil {
+			indexString = strconv.FormatInt(int64(*index), 10)
+		} else {
+			indexString = "n/a"
+		}
+
+		if weightVerified != nil {
+			if *weightVerified {
+				c.log.Infof("Desired weight (stepIdx: %s) %d verified", indexString, desiredWeight)
+			} else {
+				c.log.Infof("Desired weight (stepIdx: %s) %d not yet verified", indexString, desiredWeight)
+				c.enqueueRolloutAfter(c.rollout, defaults.GetRolloutVerifyRetryInterval())
 			}
 		}
 	}

rollout/trafficrouting/alb/alb.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"strconv"
 
+	rolloututil "github.com/argoproj/argo-rollouts/utils/rollout"
+
 	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -117,21 +119,34 @@ func (r *Reconciler) SetHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute) erro
 	return nil
 }
 
-func (r *Reconciler) shouldVerifyWeight() bool {
+// Gets the controller configuration flag for verifying alb weights
+func (r *Reconciler) getShouldVerifyWeightCfg() bool {
 	if r.cfg.VerifyWeight != nil {
 		return *r.cfg.VerifyWeight
 	}
 	return defaults.VerifyTargetGroup()
 }
 
 func (r *Reconciler) VerifyWeight(desiredWeight int32, additionalDestinations ...v1alpha1.WeightDestination) (*bool, error) {
-	if !r.shouldVerifyWeight() {
+	if !r.getShouldVerifyWeightCfg() {
 		r.cfg.Status.ALB = nil
 		return nil, nil
 	}
+
+	if !rolloututil.ShouldVerifyWeight(r.cfg.Rollout) {
+		// If we should not verify weight but the ALB status has not been set yet due to a Rollout resource just being
+		// installed in the cluster we want to actually run the rest of the function, so we do not return if
+		// r.cfg.Rollout.Status.ALB is nil. However, if we should not verify, and we have already updated the status once
+		// we return early to avoid calling AWS apis.
+		if r.cfg.Rollout.Status.ALB != nil {
+			return nil, nil
+		}
+	}
+
 	if r.cfg.Status.ALB == nil {
 		r.cfg.Status.ALB = &v1alpha1.ALBStatus{}
 	}
+
 	ctx := context.TODO()
 	rollout := r.cfg.Rollout
 	ingressName := rollout.Spec.Strategy.Canary.TrafficRouting.ALB.Ingress

rollout/trafficrouting/alb/alb_test.go

@@ -463,6 +463,10 @@ func (f *fakeAWSClient) getAlbStatus() *v1alpha1.ALBStatus {
 func TestVerifyWeight(t *testing.T) {
 	newFakeReconciler := func(status *v1alpha1.RolloutStatus) (*Reconciler, *fakeAWSClient) {
 		ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443)
+		ro.Status.StableRS = "a45fe23"
+		ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
+			SetWeight: pointer.Int32Ptr(10),
+		}}
 		i := ingress("ingress", STABLE_SVC, CANARY_SVC, STABLE_SVC, 443, 5, ro.Name, false)
 		i.Status.LoadBalancer = corev1.LoadBalancerStatus{
 			Ingress: []corev1.LoadBalancerIngress{
@@ -503,6 +507,29 @@ func TestVerifyWeight(t *testing.T) {
 		assert.False(t, *weightVerified)
 	}
 
+	// VeryifyWeight not needed
+	{
+		var status v1alpha1.RolloutStatus
+		r, _ := newFakeReconciler(&status)
+		status.StableRS = ""
+		r.cfg.Rollout.Status.StableRS = ""
+		weightVerified, err := r.VerifyWeight(10)
+		assert.NoError(t, err)
+		assert.False(t, *weightVerified)
+	}
+
+	// VeryifyWeight that we do not need to verify weight and status.ALB is already set
+	{
+		var status v1alpha1.RolloutStatus
+		r, _ := newFakeReconciler(&status)
+		r.cfg.Rollout.Status.ALB = &v1alpha1.ALBStatus{}
+		r.cfg.Rollout.Status.CurrentStepIndex = nil
+		r.cfg.Rollout.Spec.Strategy.Canary.Steps = nil
+		weightVerified, err := r.VerifyWeight(10)
+		assert.NoError(t, err)
+		assert.Nil(t, weightVerified)
+	}
+
 	// LoadBalancer found, not at weight
 	{
 		var status v1alpha1.RolloutStatus
@@ -642,6 +669,10 @@ func TestVerifyWeightWithAdditionalDestinations(t *testing.T) {
 	}
 	newFakeReconciler := func(status *v1alpha1.RolloutStatus) (*Reconciler, *fakeAWSClient) {
 		ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443)
+		ro.Status.StableRS = "a45fe23"
+		ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
+			SetWeight: pointer.Int32Ptr(10),
+		}}
 		i := ingress("ingress", STABLE_SVC, CANARY_SVC, STABLE_SVC, 443, 0, ro.Name, false)
 		i.Annotations["alb.ingress.kubernetes.io/actions.stable-svc"] = fmt.Sprintf(actionTemplateWithExperiments, CANARY_SVC, 443, 10, weightDestinations[0].ServiceName, 443, weightDestinations[0].Weight, weightDestinations[1].ServiceName, 443, weightDestinations[1].Weight, STABLE_SVC, 443, 85)
 

rollout/trafficrouting_test.go

@@ -177,7 +177,7 @@ func TestRolloutUseDesiredWeight(t *testing.T) {
 		return nil
 	})
 	f.fakeTrafficRouting.On("SetHeaderRoute", mock.Anything, mock.Anything).Return(nil)
-	f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(true, nil)
+	f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil)
 	f.run(getKey(r2, t))
 }
 
@@ -226,7 +226,7 @@ func TestRolloutUseDesiredWeight100(t *testing.T) {
 		return nil
 	})
 	f.fakeTrafficRouting.On("SetHeaderRoute", mock.Anything, mock.Anything).Return(nil)
-	f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(true, nil)
+	f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil)
 	f.run(getKey(r2, t))
 }
 
@@ -723,7 +723,9 @@ func TestCanaryWithTrafficRoutingAddScaleDownDelay(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
 
-	r1 := newCanaryRollout("foo", 1, nil, nil, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(10),
+	}}, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1))
 	r1.Spec.Strategy.Canary.CanaryService = "canary"
 	r1.Spec.Strategy.Canary.StableService = "stable"
 	r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{
@@ -735,7 +737,6 @@ func TestCanaryWithTrafficRoutingAddScaleDownDelay(t *testing.T) {
 	rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 	r2 = updateCanaryRolloutStatus(r2, rs2PodHash, 2, 1, 2, false)
 	r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation))
-	r2.Status.CurrentStepIndex = nil
 	availableCondition, _ := newAvailableCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, availableCondition)
 	_, r2.Status.Canary.Weights = calculateWeightStatus(r2, rs2PodHash, rs2PodHash, 0)
@@ -763,7 +764,9 @@ func TestCanaryWithTrafficRoutingScaleDownLimit(t *testing.T) {
 
 	inTheFuture := timeutil.MetaNow().Add(10 * time.Second).UTC().Format(time.RFC3339)
 
-	r1 := newCanaryRollout("foo", 1, nil, nil, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1))
+	r1 := newCanaryRollout("foo", 1, nil, []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(10),
+	}}, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1))
 	rs1 := newReplicaSetWithStatus(r1, 1, 1)
 	rs1.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = inTheFuture
 	r1.Spec.Strategy.Canary.ScaleDownDelayRevisionLimit = pointer.Int32Ptr(1)

utils/rollout/rolloututil.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"strconv"
 
+	replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset"
+
 	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	"github.com/argoproj/argo-rollouts/utils/annotations"
 	"github.com/argoproj/argo-rollouts/utils/conditions"
@@ -179,3 +181,16 @@ func CanaryStepString(c v1alpha1.CanaryStep) string {
 	}
 	return "invalid"
 }
+
+// ShouldVerifyWeight We use this to test if we should verify weights because weight verification could involve
+// API calls to the cloud provider which could incur rate limiting
+func ShouldVerifyWeight(ro *v1alpha1.Rollout) bool {
+	currentStep, _ := replicasetutil.GetCurrentCanaryStep(ro)
+	// If we are in the middle of an update at a setWeight step, also perform weight verification.
+	// Note that we don't do this every reconciliation because weight verification typically involves
+	// API calls to the cloud provider which could incur rate limitingq
+	shouldVerifyWeight := ro.Status.StableRS != "" &&
+		!IsFullyPromoted(ro) &&
+		currentStep != nil && currentStep.SetWeight != nil
+	return shouldVerifyWeight
+}

utils/rollout/rolloututil_test.go

@@ -414,3 +414,21 @@ func TestIsUnpausing(t *testing.T) {
 	assert.Equal(t, v1alpha1.RolloutPhaseProgressing, status)
 	assert.Equal(t, "waiting for rollout to unpause", message)
 }
+
+func TestShouldVerifyWeight(t *testing.T) {
+	ro := newCanaryRollout()
+	ro.Status.StableRS = "34feab23f"
+	ro.Status.CurrentStepIndex = pointer.Int32Ptr(0)
+	ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
+		SetWeight: pointer.Int32Ptr(20),
+	}}
+	assert.Equal(t, true, ShouldVerifyWeight(ro))
+
+	ro.Status.StableRS = ""
+	assert.Equal(t, false, ShouldVerifyWeight(ro))
+
+	ro.Status.StableRS = "34feab23f"
+	ro.Status.CurrentStepIndex = nil
+	ro.Spec.Strategy.Canary.Steps = nil
+	assert.Equal(t, false, ShouldVerifyWeight(ro))
+}