fix: avoid status update conflicts during credential retries

internal/controller/acrpullbinding_controller.go

@@ -102,7 +102,7 @@ func NewV1beta1Reconciler(opts *V1beta1ReconcilerOpts) *AcrPullBindingReconciler
 				msiClientID, msiResourceID, acrServer := specOrDefault(opts, binding.Spec)
 				acrAccessToken, err := opts.Auth.AcquireACRAccessToken(ctx, msiResourceID, msiClientID, acrServer, binding.Spec.Scope)
 				if err != nil {
-					return "", time.Time{}, fmt.Errorf("failed to retrieve ACR access token: %w", err)
+					return "", time.Time{}, credentialGenerationError{operation: "failed to retrieve ACR access token", err: err}
 				}
 
 				dockerConfig, err := authorizer.CreateACRDockerCfg(acrServer, acrAccessToken)

internal/controller/acrpullbinding_v1beta2_controller.go

@@ -134,12 +134,12 @@ func NewV1beta2Reconciler(opts *V1beta2ReconcilerOpts) *PullBindingReconciler {
 
 				armToken, err := opts.fetchArmToken(ctx, binding.Spec, tenantId, clientId, token)
 				if err != nil {
-					return "", time.Time{}, fmt.Errorf("failed to retrieve ARM token: %v", err)
+					return "", time.Time{}, credentialGenerationError{operation: "failed to retrieve ARM token", err: err}
 				}
 
 				acrToken, err := opts.exchangeArmTokenForAcrToken(ctx, armToken, binding.Spec.ACR)
 				if err != nil {
-					return "", time.Time{}, fmt.Errorf("failed to retrieve ACR token: %v", err)
+					return "", time.Time{}, credentialGenerationError{operation: "failed to retrieve ACR token", err: err}
 				}
 
 				dockerConfig, err := authorizer.CreateACRDockerCfg(binding.Spec.ACR.Server, acrToken)

internal/controller/generic_controller.go

@@ -2,12 +2,17 @@ package controller
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"net/http"
 	"slices"
 	"strings"
 	"time"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	azruntime "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
+	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	msiacrpullv1beta1 "github.com/Azure/msi-acrpull/api/v1beta1"
 	msiacrpullv1beta2 "github.com/Azure/msi-acrpull/api/v1beta2"
 	"github.com/go-logr/logr"
@@ -191,7 +196,7 @@ func (r *genericReconciler[O]) reconcile(ctx context.Context, logger logr.Logger
 		dockerConfig, expiresOn, err := r.CreatePullCredential(ctx, acrBinding, serviceAccount)
 		if err != nil {
 			logger.Error(err, "failed to generate pull credential")
-			return r.statusErrorAction(acrBinding, err.Error(), !isPermanentCredentialError(err))
+			return r.statusErrorAction(acrBinding, credentialStatusMessage(err), !isPermanentCredentialError(err))
 		}
 
 		newSecret := newPullSecret(acrBinding, r.GetPullSecretName(acrBinding), dockerConfig, r.Scheme, expiresOn, r.now, inputHash)
@@ -263,6 +268,71 @@ func (r *genericReconciler[O]) statusErrorAction(acrBinding O, message string, r
 	return action
 }
 
+type credentialGenerationError struct {
+	operation string
+	err       error
+}
+
+func (e credentialGenerationError) Error() string {
+	return fmt.Sprintf("%s: %v", e.operation, e.err)
+}
+
+func (e credentialGenerationError) Unwrap() error {
+	return e.err
+}
+
+func credentialStatusMessage(err error) string {
+	response := credentialErrorResponse(err)
+	if response == nil {
+		return err.Error()
+	}
+
+	payload, readErr := azruntime.Payload(response)
+	if readErr != nil {
+		return err.Error()
+	}
+
+	var responseBody struct {
+		Errors []struct {
+			Code string `json:"code"`
+		} `json:"errors"`
+	}
+	if json.Unmarshal(payload, &responseBody) != nil {
+		return err.Error()
+	}
+
+	codes := make([]string, 0, len(responseBody.Errors))
+	for _, responseError := range responseBody.Errors {
+		if responseError.Code != "" && !slices.Contains(codes, responseError.Code) {
+			codes = append(codes, responseError.Code)
+		}
+	}
+	if len(codes) == 0 {
+		return err.Error()
+	}
+
+	operation := "failed to generate pull credential"
+	var generationError credentialGenerationError
+	if errors.As(err, &generationError) {
+		operation = generationError.operation
+	}
+	return fmt.Sprintf("%s: request failed with HTTP status %d: %s", operation, response.StatusCode, strings.Join(codes, ", "))
+}
+
+func credentialErrorResponse(err error) *http.Response {
+	var responseError *azcore.ResponseError
+	if errors.As(err, &responseError) {
+		return responseError.RawResponse
+	}
+
+	var authenticationError *azidentity.AuthenticationFailedError
+	if errors.As(err, &authenticationError) {
+		return authenticationError.RawResponse
+	}
+
+	return nil
+}
+
 // sortPullSecrets ensures the semantically-correct ordering of pull secrets for the service account. The order of pull
 // secrets determines the order in which the kubelet will use these credentials, so managing the order ensures we manage
 // the order of preference for credentials. We enforce the following order:

internal/controller/generic_controller_test.go

@@ -1,11 +1,18 @@
 package controller
 
 import (
+	"bytes"
 	"context"
+	"errors"
+	"fmt"
+	"io"
+	"net/http"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	msiacrpullv1beta1 "github.com/Azure/msi-acrpull/api/v1beta1"
 	"github.com/go-logr/logr"
 	"github.com/google/go-cmp/cmp"
@@ -172,7 +179,80 @@ func TestActionExecuteReturnsTransientErrorWithoutStatusUpdate(t *testing.T) {
 	}
 }
 
-func TestStatusErrorActionSkipsUnchangedStatus(t *testing.T) {
+func TestCredentialStatusMessageUsesStructuredResponseError(t *testing.T) {
+	statuses := make([]string, 0, 2)
+	for _, correlationID := range []string{
+		"92b4e2ff-be91-4ad1-bc95-ea0337098e30",
+		"336c85eb-f609-45c2-8a53-89396db5c5a3",
+	} {
+		body := fmt.Sprintf(`{"errors":[{"code":"REQUEST_BODY_INVALID","message":"Request body is invalid. CorrelationId: %s"}]}`, correlationID)
+		err := credentialGenerationError{
+			operation: "failed to retrieve ACR token",
+			err: &azcore.ResponseError{
+				StatusCode: http.StatusBadRequest,
+				RawResponse: &http.Response{
+					StatusCode: http.StatusBadRequest,
+					Body:       io.NopCloser(bytes.NewBufferString(body)),
+				},
+			},
+		}
+		statuses = append(statuses, credentialStatusMessage(err))
+	}
+
+	const expected = "failed to retrieve ACR token: request failed with HTTP status 400: REQUEST_BODY_INVALID"
+	for _, status := range statuses {
+		if status != expected {
+			t.Fatalf("expected stable structured status %q, got %q", expected, status)
+		}
+	}
+
+	binding := &msiacrpullv1beta1.AcrPullBinding{
+		Status: msiacrpullv1beta1.AcrPullBindingStatus{Error: statuses[0]},
+	}
+	reconciler := &genericReconciler[*msiacrpullv1beta1.AcrPullBinding]{
+		GetStatusError: func(binding *msiacrpullv1beta1.AcrPullBinding) string {
+			return binding.Status.Error
+		},
+		UpdateStatusError: func(*msiacrpullv1beta1.AcrPullBinding, string) *msiacrpullv1beta1.AcrPullBinding {
+			t.Fatal("stable authentication error should not update status")
+			return nil
+		},
+	}
+	action := reconciler.statusErrorAction(binding, statuses[1], true)
+	if action.updatePullBindingStatus != nil || action.retryError != statuses[1] {
+		t.Fatalf("expected retry without status update, got %#v", action)
+	}
+}
+
+func TestCredentialStatusMessageUsesStructuredAuthenticationError(t *testing.T) {
+	const body = `{"errors":[{"code":"IDENTITY_NOT_FOUND","message":"The requested identity wasn't found"}]}`
+	err := credentialGenerationError{
+		operation: "failed to retrieve ARM token",
+		err: &azidentity.AuthenticationFailedError{
+			RawResponse: &http.Response{
+				StatusCode: http.StatusBadRequest,
+				Body:       io.NopCloser(strings.NewReader(body)),
+			},
+		},
+	}
+
+	const expected = "failed to retrieve ARM token: request failed with HTTP status 400: IDENTITY_NOT_FOUND"
+	if status := credentialStatusMessage(err); status != expected {
+		t.Fatalf("expected structured status %q, got %q", expected, status)
+	}
+}
+
+func TestCredentialStatusMessagePreservesUnknownError(t *testing.T) {
+	err := credentialGenerationError{
+		operation: "failed to retrieve ARM token",
+		err:       errors.New("temporary Azure outage"),
+	}
+	if status := credentialStatusMessage(err); status != err.Error() {
+		t.Fatalf("expected original error %q, got %q", err.Error(), status)
+	}
+}
+
+func TestStatusErrorActionUpdatesDifferentStatus(t *testing.T) {
 	binding := &msiacrpullv1beta1.AcrPullBinding{
 		Status: msiacrpullv1beta1.AcrPullBindingStatus{Error: "temporary Azure outage"},
 	}
@@ -181,14 +261,16 @@ func TestStatusErrorActionSkipsUnchangedStatus(t *testing.T) {
 			return binding.Status.Error
 		},
 		UpdateStatusError: func(binding *msiacrpullv1beta1.AcrPullBinding, message string) *msiacrpullv1beta1.AcrPullBinding {
-			t.Fatal("unchanged status should not be updated")
-			return nil
+			updated := binding.DeepCopy()
+			updated.Status.Error = message
+			return updated
 		},
 	}
 
-	action := reconciler.statusErrorAction(binding, binding.Status.Error, true)
-	if action.updatePullBindingStatus != nil || action.retryError != binding.Status.Error {
-		t.Fatalf("expected retry without status update, got %#v", action)
+	const next = "authentication failed"
+	action := reconciler.statusErrorAction(binding, next, true)
+	if action.updatePullBindingStatus == nil || action.updatePullBindingStatus.Status.Error != next || action.retryError != next {
+		t.Fatalf("expected status update and retry, got %#v", action)
 	}
 }
 

test/e2e_test.go

@@ -140,6 +140,31 @@ type binding interface {
 // bindingMinter is a constructor for a non-nil pointer to a binding, since we can't create that with `B`
 type bindingMinter[B binding] func(namespace, name, scope, serviceAccount string, cfg *Config) B
 
+func updateBindingScope[B binding](
+	ctx context.Context,
+	client crclient.Client,
+	namespace, name, scope string,
+	newBinding func(namespace, name string) B,
+) error {
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		current := newBinding(namespace, name)
+		if err := client.Get(ctx, crclient.ObjectKeyFromObject(current), current); err != nil {
+			return err
+		}
+
+		switch binding := any(current).(type) {
+		case *msiacrpullv1beta1.AcrPullBinding:
+			binding.Spec.Scope = scope
+		case *msiacrpullv1beta2.AcrPullBinding:
+			binding.Spec.ACR.Scope = scope
+		default:
+			return fmt.Errorf("unsupported binding type %T", current)
+		}
+
+		return client.Update(ctx, current)
+	})
+}
+
 func testACRPullBinding[B binding](
 	t *testing.T, prefix string,
 	createBinding bindingMinter[B],
@@ -256,29 +281,13 @@ func testACRPullBinding[B binding](
 		eventuallyFulfillPullBinding[B](t, ctx, client, namespace, pullBinding, newBinding)
 
 		t.Logf("updating pull binding %s/%s to refer to invalid scope", namespace, pullBinding)
-		if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-			thisBinding := newBinding(namespace, pullBinding)
-			if err := client.Get(ctx, crclient.ObjectKeyFromObject(thisBinding), thisBinding); err != nil {
-				return err
-			}
-			updatedBinding := createBinding(namespace, pullBinding, "invalid!>?$q34m2,", serviceAccount, cfg)
-			updatedBinding.SetResourceVersion(thisBinding.GetResourceVersion())
-			return client.Update(ctx, updatedBinding)
-		}); err != nil {
+		if err := updateBindingScope(ctx, client, namespace, pullBinding, "invalid!>?$q34m2,", newBinding); err != nil {
 			t.Fatalf("failed to update pull binding %s/%s: %v", namespace, pullBinding, err)
 		}
 		eventuallyFailToFulfillPullBindingKeepingTimes[B](t, ctx, client, namespace, pullBinding, newBinding)
 
 		t.Logf("updating pull binding %s/%s to refer to valid scope", namespace, pullBinding)
-		if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-			thisBinding := newBinding(namespace, pullBinding)
-			if err := client.Get(ctx, crclient.ObjectKeyFromObject(thisBinding), thisBinding); err != nil {
-				return err
-			}
-			updatedBinding := createBinding(namespace, pullBinding, "repository:alice:pull", serviceAccount, cfg)
-			updatedBinding.SetResourceVersion(thisBinding.GetResourceVersion())
-			return client.Update(ctx, updatedBinding)
-		}); err != nil {
+		if err := updateBindingScope(ctx, client, namespace, pullBinding, "repository:alice:pull", newBinding); err != nil {
 			t.Fatalf("failed to update pull binding %s/%s: %v", namespace, pullBinding, err)
 		}
 		eventuallyFulfillPullBinding[B](t, ctx, client, namespace, pullBinding, newBinding)