Merge pull request #4152 from nojnhuh/aso-no-wait-for-ready

[release-1.11] don't wait for ASO resources to be ready for updates

Author: k8s-ci-robot

azure/services/aso/aso.go

@@ -75,6 +75,7 @@ func (s *Service) CreateOrUpdateResource(ctx context.Context, spec azure.ASOReso
 
 	log = log.WithValues("service", serviceName, "resource", resourceName, "namespace", resourceNamespace)
 
+	var readyErr error
 	var adopt bool
 	var existing genruntime.MetaObject
 	if err := s.Client.Get(ctx, client.ObjectKeyFromObject(resource), resource); err != nil {
@@ -96,7 +97,6 @@ func (s *Service) CreateOrUpdateResource(ctx context.Context, spec azure.ASOReso
 		if !readyExists {
 			return nil, azure.WithTransientError(errors.New("ready status unknown"), requeueInterval)
 		}
-		var readyErr error
 		if cond := conds[i]; cond.Status != metav1.ConditionTrue {
 			switch {
 			case cond.Reason == conditions.ReasonAzureResourceNotFound.Name &&
@@ -124,9 +124,10 @@ func (s *Service) CreateOrUpdateResource(ctx context.Context, spec azure.ASOReso
 
 			if readyErr != nil {
 				if conds[i].Severity == conditions.ConditionSeverityError {
-					return nil, azure.WithTerminalError(readyErr)
+					readyErr = azure.WithTerminalError(readyErr)
+				} else {
+					readyErr = azure.WithTransientError(readyErr, requeueInterval)
 				}
-				return nil, azure.WithTransientError(readyErr, requeueInterval)
 			}
 		}
 	}
@@ -199,6 +200,11 @@ func (s *Service) CreateOrUpdateResource(ctx context.Context, spec azure.ASOReso
 
 	diff := cmp.Diff(existing, parameters)
 	if diff == "" {
+		if readyErr != nil {
+			// Only return this error when the resource is up to date in order to permit updates from
+			// Parameters which may fix the resource's current state.
+			return nil, readyErr
+		}
 		log.V(2).Info("resource up to date")
 		return existing, nil
 	}

azure/services/aso/aso_test.go

@@ -175,6 +175,10 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Namespace: "namespace",
 			},
 		})
+		specMock.EXPECT().Parameters(gomockinternal.AContext(), gomock.Not(gomock.Nil())).DoAndReturn(func(_ context.Context, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
+			return group, nil
+		})
+		specMock.EXPECT().WasManaged(gomock.Any()).Return(false)
 
 		ctx := context.Background()
 		g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{
@@ -184,6 +188,9 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Labels: map[string]string{
 					infrav1.OwnedByClusterLabelKey: clusterName,
 				},
+				Annotations: map[string]string{
+					asoannotations.PerResourceSecret: "cluster-aso-secret",
+				},
 			},
 			Status: asoresourcesv1.ResourceGroup_STATUS{
 				Conditions: []conditions.Condition{
@@ -203,6 +210,7 @@ func TestCreateOrUpdateResource(t *testing.T) {
 		var recerr azure.ReconcileError
 		g.Expect(errors.As(err, &recerr)).To(BeTrue())
 		g.Expect(recerr.IsTransient()).To(BeTrue())
+		g.Expect(recerr.IsTerminal()).To(BeFalse())
 	})
 
 	t.Run("resource is not ready in reconciling state", func(t *testing.T) {
@@ -223,6 +231,10 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Namespace: "namespace",
 			},
 		})
+		specMock.EXPECT().Parameters(gomockinternal.AContext(), gomock.Not(gomock.Nil())).DoAndReturn(func(_ context.Context, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
+			return group, nil
+		})
+		specMock.EXPECT().WasManaged(gomock.Any()).Return(false)
 
 		ctx := context.Background()
 		g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{
@@ -232,6 +244,9 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Labels: map[string]string{
 					infrav1.OwnedByClusterLabelKey: clusterName,
 				},
+				Annotations: map[string]string{
+					asoannotations.PerResourceSecret: "cluster-aso-secret",
+				},
 			},
 			Status: asoresourcesv1.ResourceGroup_STATUS{
 				Conditions: []conditions.Condition{
@@ -268,6 +283,10 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Namespace: "namespace",
 			},
 		})
+		specMock.EXPECT().Parameters(gomockinternal.AContext(), gomock.Not(gomock.Nil())).DoAndReturn(func(_ context.Context, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
+			return group, nil
+		})
+		specMock.EXPECT().WasManaged(gomock.Any()).Return(false)
 
 		ctx := context.Background()
 		g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{
@@ -277,6 +296,9 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Labels: map[string]string{
 					infrav1.OwnedByClusterLabelKey: clusterName,
 				},
+				Annotations: map[string]string{
+					asoannotations.PerResourceSecret: "cluster-aso-secret",
+				},
 			},
 			Status: asoresourcesv1.ResourceGroup_STATUS{
 				Conditions: []conditions.Condition{
@@ -296,6 +318,7 @@ func TestCreateOrUpdateResource(t *testing.T) {
 		var recerr azure.ReconcileError
 		g.Expect(errors.As(err, &recerr)).To(BeTrue())
 		g.Expect(recerr.IsTerminal()).To(BeTrue())
+		g.Expect(recerr.IsTransient()).To(BeFalse())
 	})
 
 	t.Run("error getting existing resource", func(t *testing.T) {