diff --git a/internal/controller/acrpullbinding_controller.go b/internal/controller/acrpullbinding_controller.go index 268010e..f9eb0ac 100644 --- a/internal/controller/acrpullbinding_controller.go +++ b/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) diff --git a/internal/controller/acrpullbinding_v1beta2_controller.go b/internal/controller/acrpullbinding_v1beta2_controller.go index 19a22b9..d9498ea 100644 --- a/internal/controller/acrpullbinding_v1beta2_controller.go +++ b/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) diff --git a/internal/controller/generic_controller.go b/internal/controller/generic_controller.go index 46535b7..bf0ad48 100644 --- a/internal/controller/generic_controller.go +++ b/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: diff --git a/internal/controller/generic_controller_test.go b/internal/controller/generic_controller_test.go index 3adb009..3c4aba7 100644 --- a/internal/controller/generic_controller_test.go +++ b/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) } } diff --git a/test/e2e_test.go b/test/e2e_test.go index cfd2566..90a893f 100644 --- a/test/e2e_test.go +++ b/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) diff --git a/test/e2e_update_binding_scope_test.go b/test/e2e_update_binding_scope_test.go new file mode 100644 index 0000000..c9fbfdc --- /dev/null +++ b/test/e2e_update_binding_scope_test.go @@ -0,0 +1,202 @@ +//go:build e2e + +package test + +import ( + "context" + stderrors "errors" + "fmt" + "strconv" + "testing" + + msiacrpullv1beta1 "github.com/Azure/msi-acrpull/api/v1beta1" + msiacrpullv1beta2 "github.com/Azure/msi-acrpull/api/v1beta2" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + crclient "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestUpdateBindingScope(t *testing.T) { + t.Run("v1beta1", func(t *testing.T) { + current := &msiacrpullv1beta1.AcrPullBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "binding", + ResourceVersion: "1", + Labels: map[string]string{"preserved": "label"}, + Annotations: map[string]string{"preserved": "annotation"}, + }, + Spec: msiacrpullv1beta1.AcrPullBindingSpec{ + AcrServer: "example.azurecr.io", + Scope: "repository:old:pull", + ManagedIdentityResourceID: "identity", + ServiceAccountName: "service-account", + }, + Status: msiacrpullv1beta1.AcrPullBindingStatus{Error: "initial"}, + } + client := &scopeUpdateClient{current: current, conflicts: 2} + + err := updateBindingScope(context.Background(), client, "test", "binding", "repository:alice:pull", + func(namespace, name string) *msiacrpullv1beta1.AcrPullBinding { + return &msiacrpullv1beta1.AcrPullBinding{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} + }) + if err != nil { + t.Fatalf("update binding scope: %v", err) + } + + updated := client.updated.(*msiacrpullv1beta1.AcrPullBinding) + if updated.Spec.Scope != "repository:alice:pull" { + t.Fatalf("scope = %q, want %q", updated.Spec.Scope, "repository:alice:pull") + } + assertConcurrentChangesPreserved(t, client, updated) + }) + + t.Run("v1beta2", func(t *testing.T) { + current := &msiacrpullv1beta2.AcrPullBinding{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "binding", + ResourceVersion: "1", + Labels: map[string]string{"preserved": "label"}, + Annotations: map[string]string{"preserved": "annotation"}, + }, + Spec: msiacrpullv1beta2.AcrPullBindingSpec{ + ACR: msiacrpullv1beta2.AcrConfiguration{ + Server: "example.azurecr.io", + Scope: "repository:old:pull", + Environment: msiacrpullv1beta2.AzureEnvironmentPublicCloud, + }, + Auth: msiacrpullv1beta2.AuthenticationMethod{ + ManagedIdentity: &msiacrpullv1beta2.ManagedIdentityAuth{ResourceID: "identity"}, + }, + ServiceAccountName: "service-account", + }, + Status: msiacrpullv1beta2.AcrPullBindingStatus{Error: "initial"}, + } + client := &scopeUpdateClient{current: current, conflicts: 2} + + err := updateBindingScope(context.Background(), client, "test", "binding", "repository:alice:pull", + func(namespace, name string) *msiacrpullv1beta2.AcrPullBinding { + return &msiacrpullv1beta2.AcrPullBinding{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} + }) + if err != nil { + t.Fatalf("update binding scope: %v", err) + } + + updated := client.updated.(*msiacrpullv1beta2.AcrPullBinding) + if updated.Spec.ACR.Scope != "repository:alice:pull" { + t.Fatalf("scope = %q, want %q", updated.Spec.ACR.Scope, "repository:alice:pull") + } + assertConcurrentChangesPreserved(t, client, updated) + }) +} + +func TestUpdateBindingScopeReturnsGetErrors(t *testing.T) { + notFound := apierrors.NewNotFound( + schema.GroupResource{Group: msiacrpullv1beta1.GroupVersion.Group, Resource: "acrpullbindings"}, + "binding", + ) + client := &scopeUpdateClient{getErr: notFound} + + err := updateBindingScope(context.Background(), client, "test", "binding", "repository:alice:pull", + func(namespace, name string) *msiacrpullv1beta1.AcrPullBinding { + return &msiacrpullv1beta1.AcrPullBinding{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} + }) + + if !apierrors.IsNotFound(err) { + t.Fatalf("error = %v, want NotFound", err) + } + if client.gets != 1 { + t.Fatalf("Get calls = %d, want 1", client.gets) + } +} + +func assertConcurrentChangesPreserved(t *testing.T, client *scopeUpdateClient, updated crclient.Object) { + t.Helper() + + if client.gets != 3 { + t.Fatalf("Get calls = %d, want 3", client.gets) + } + if updated.GetResourceVersion() != "3" { + t.Fatalf("resourceVersion = %q, want %q", updated.GetResourceVersion(), "3") + } + if updated.GetLabels()["preserved"] != "label" { + t.Fatalf("labels were not preserved: %#v", updated.GetLabels()) + } + if updated.GetAnnotations()["preserved"] != "annotation" || updated.GetAnnotations()["concurrent"] != "3" { + t.Fatalf("annotations were not preserved: %#v", updated.GetAnnotations()) + } + + switch binding := updated.(type) { + case *msiacrpullv1beta1.AcrPullBinding: + if binding.Status.Error != "concurrent-3" { + t.Fatalf("status was not preserved: %#v", binding.Status) + } + case *msiacrpullv1beta2.AcrPullBinding: + if binding.Status.Error != "concurrent-3" { + t.Fatalf("status was not preserved: %#v", binding.Status) + } + default: + t.Fatalf("unexpected binding type %T", updated) + } +} + +type scopeUpdateClient struct { + crclient.Client + current crclient.Object + updated crclient.Object + conflicts int + gets int + getErr error +} + +func (c *scopeUpdateClient) Get(_ context.Context, _ crclient.ObjectKey, obj crclient.Object, _ ...crclient.GetOption) error { + c.gets++ + if c.getErr != nil { + return c.getErr + } + + switch target := obj.(type) { + case *msiacrpullv1beta1.AcrPullBinding: + *target = *c.current.(*msiacrpullv1beta1.AcrPullBinding).DeepCopy() + case *msiacrpullv1beta2.AcrPullBinding: + *target = *c.current.(*msiacrpullv1beta2.AcrPullBinding).DeepCopy() + default: + return fmt.Errorf("unexpected binding type %T", obj) + } + return nil +} + +func (c *scopeUpdateClient) Update(_ context.Context, obj crclient.Object, _ ...crclient.UpdateOption) error { + if c.conflicts > 0 { + c.conflicts-- + revision := 3 - c.conflicts + c.applyConcurrentUpdate(revision) + return apierrors.NewConflict( + schema.GroupResource{Group: "acrpull.microsoft.com", Resource: "acrpullbindings"}, + obj.GetName(), + stderrors.New("the object has been modified"), + ) + } + + c.updated = obj.DeepCopyObject().(crclient.Object) + return nil +} + +func (c *scopeUpdateClient) applyConcurrentUpdate(revision int) { + annotation := strconv.Itoa(revision) + resourceVersion := annotation + statusError := "concurrent-" + annotation + + switch binding := c.current.(type) { + case *msiacrpullv1beta1.AcrPullBinding: + binding.ResourceVersion = resourceVersion + binding.Annotations["concurrent"] = annotation + binding.Status.Error = statusError + case *msiacrpullv1beta2.AcrPullBinding: + binding.ResourceVersion = resourceVersion + binding.Annotations["concurrent"] = annotation + binding.Status.Error = statusError + } +}