diff --git a/pkg/cluster/adminupdate_test.go b/pkg/cluster/adminupdate_test.go index ec5fede66c4..e7122e3b93e 100644 --- a/pkg/cluster/adminupdate_test.go +++ b/pkg/cluster/adminupdate_test.go @@ -54,7 +54,7 @@ func TestAdminUpdateSteps(t *testing.T) { } generalFixesSteps := []string{ - "[Action ensureResourceGroup]", + "[AuthorizationRetryingAction ensureResourceGroup]", "[Action createOrUpdateDenyAssignment]", "[Action ensureServiceEndpoints]", "[Action populateRegistryStorageAccountName]", diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index e427d3e8f38..1b783308f0d 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -75,6 +75,7 @@ type manager struct { doc *api.OpenShiftClusterDocument subscriptionDoc *api.SubscriptionDocument fpAuthorizer refreshable.Authorizer + fpCredRefresher refreshable.Rebuilder localFpAuthorizer autorest.Authorizer metricsEmitter metrics.Emitter @@ -158,7 +159,7 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database return nil, err } - fpCredClusterTenant, err := _env.FPNewClientCertificateCredential(subscriptionDoc.Subscription.Properties.TenantID, nil) + fpCredClusterTenant, err := refreshable.NewFPTokenCredential(_env, subscriptionDoc.Subscription.Properties.TenantID, nil) if err != nil { return nil, err } @@ -173,11 +174,13 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database } fpspID := tokenClaims.ObjectId - fpCredRPTenant, err := _env.FPNewClientCertificateCredential(_env.TenantID(), nil) + fpCredRPTenant, err := refreshable.NewFPTokenCredential(_env, _env.TenantID(), nil) if err != nil { return nil, err } + fpCredRefresher := refreshable.NewMultiRebuilder(fpAuthorizer, fpCredClusterTenant, fpCredRPTenant) + msiCredential, err := _env.NewMSITokenCredential() if err != nil { return nil, err @@ -283,6 +286,7 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database doc: doc, subscriptionDoc: subscriptionDoc, fpAuthorizer: fpAuthorizer, + fpCredRefresher: fpCredRefresher, localFpAuthorizer: localFPAuthorizer, metricsEmitter: metricsEmitter, disks: compute.NewDisksClient(_env.Environment(), r.SubscriptionID, fpAuthorizer), diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index 221f8003a4f..01042f1a89f 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -95,9 +95,9 @@ func (m *manager) getZerothSteps() []steps.Step { steps.Action(m.fixupClusterMsiTenantID), steps.Action(m.ensureClusterMsiCertificate), steps.Action(m.initializeClusterMsiClients), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.persistPlatformWorkloadIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.clusterIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.platformWorkloadIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.persistPlatformWorkloadIdentityIDs, m.managedResourceGroupName()), } bootstrap = append(bootstrap, managedIdentitySteps...) @@ -122,7 +122,7 @@ func (m *manager) getEnsureAPIServerReadySteps() []steps.Step { func (m *manager) getGeneralFixesSteps() []steps.Step { stepsThatDontNeedAPIServer := []steps.Step{ - steps.Action(m.ensureResourceGroup), // re-create RP RBAC if needed after tenant migration + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.ensureResourceGroup, m.managedResourceGroupName()), // re-create RP RBAC if needed after tenant migration steps.Action(m.createOrUpdateDenyAssignment), steps.Action(m.ensureServiceEndpoints), steps.Action(m.populateRegistryStorageAccountName), // must go before migrateStorageAccounts @@ -130,8 +130,8 @@ func (m *manager) getGeneralFixesSteps() []steps.Step { steps.Action(m.fixSSH), } stepsThatNeedAPIServer := []steps.Step{ - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixSREKubeconfig, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixUserAdminKubeconfig, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.fixSREKubeconfig, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.fixUserAdminKubeconfig, m.managedResourceGroupName()), steps.Action(m.createOrUpdateRouterIPFromCluster), steps.Action(m.ensureGatewayUpgrade), @@ -152,7 +152,7 @@ func (m *manager) getCertificateRenewalSteps() []steps.Step { s := []steps.Step{ steps.Action(m.populateDatabaseIntIP), steps.Action(m.correctCertificateIssuer), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixMCSCert, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.fixMCSCert, m.managedResourceGroupName()), steps.Action(m.fixMCSUserData), steps.Action(m.configureAPIServerCertificate), steps.Action(m.configureIngressCertificate), @@ -248,12 +248,12 @@ func (m *manager) Update(ctx context.Context) error { ) } - s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources, m.managedResourceGroupName())) + s = append(s, steps.AuthorizationRetryingAction(m.fpCredRefresher, m.validateResources, m.managedResourceGroupName())) if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { s = append(s, - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.persistPlatformWorkloadIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.clusterIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.persistPlatformWorkloadIdentityIDs, m.managedResourceGroupName()), steps.Action(m.ensurePlatformWorkloadIdentityRBAC), steps.AuthorizationRetryingAction(nil, m.federateIdentityCredentials, m.managedResourceGroupName()), ) @@ -261,7 +261,7 @@ func (m *manager) Update(ctx context.Context) error { s = append(s, // Since ServicePrincipalProfile is now a pointer and our converters re-build the struct, // our update path needs to enrich the doc with SPObjectID since it was overwritten by our API on put/patch. - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixupClusterSPObjectID, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.fixupClusterSPObjectID, m.managedResourceGroupName()), // CSP credentials rotation flow steps steps.Action(m.createOrUpdateClusterServicePrincipalRBAC)) @@ -277,7 +277,7 @@ func (m *manager) Update(ctx context.Context) error { steps.Action(m.correctCertificateIssuer), steps.Action(m.configureAPIServerCertificate), steps.Action(m.configureIngressCertificate), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.fixUserAdminKubeconfig, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.fixUserAdminKubeconfig, m.managedResourceGroupName()), steps.Action(m.reconcileLoadBalancerProfile), steps.Action(m.reconcileSoftwareDefinedNetwork), steps.Action(m.ensureCredentialsRequest), @@ -398,19 +398,19 @@ func (m *manager) bootstrap() []steps.Step { } s = append(s, - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateResources, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.validateZones, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.validateResources, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.validateZones, m.managedResourceGroupName()), ) if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { s = append(s, - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.persistPlatformWorkloadIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.clusterIdentityIDs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.persistPlatformWorkloadIdentityIDs, m.managedResourceGroupName()), ) } else { s = append(s, steps.Action(m.initializeClusterSPClients), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.clusterSPObjectID, m.managedResourceGroupName()), ) } s = append(s, @@ -422,10 +422,10 @@ func (m *manager) bootstrap() []steps.Step { steps.Action(m.populateMTUSize), steps.Action(m.createDNS), steps.Action(m.createOIDC), - steps.Action(m.ensureResourceGroup), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.ensureResourceGroup, m.managedResourceGroupName()), steps.Action(m.ensureServiceEndpoints), steps.Action(m.setMasterSubnetPolicies), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.deployBaseResourceTemplate, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.deployBaseResourceTemplate, m.managedResourceGroupName()), ) if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { @@ -435,12 +435,12 @@ func (m *manager) bootstrap() []steps.Step { } s = append(s, - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.attachNSGs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.attachNSGs, m.managedResourceGroupName()), // Allow extra time for ARM authorization to propagate to the internal load balancer. - steps.AuthorizationRetryingActionWithTimeout(m.fpAuthorizer, m.updateAPIIPEarly, m.managedResourceGroupName(), 15*time.Minute), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.createOrUpdateRouterIPEarly, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.ensureGatewayCreate, m.managedResourceGroupName()), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.createAPIServerPrivateEndpoint, m.managedResourceGroupName()), + steps.AuthorizationRetryingActionWithTimeout(m.fpCredRefresher, m.updateAPIIPEarly, m.managedResourceGroupName(), 15*time.Minute), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.createOrUpdateRouterIPEarly, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.ensureGatewayCreate, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.createAPIServerPrivateEndpoint, m.managedResourceGroupName()), steps.Action(m.createCertificates), ) @@ -456,12 +456,12 @@ func (m *manager) bootstrap() []steps.Step { // Give Hive 60 minutes to install the cluster, since this includes // all of bootstrapping being complete steps.Condition(m.hiveClusterInstallationComplete, 60*time.Minute, true), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.generateKubeconfigs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.generateKubeconfigs, m.managedResourceGroupName()), ) } else { s = append(s, steps.Action(m.runPodmanInstaller), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.generateKubeconfigs, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.generateKubeconfigs, m.managedResourceGroupName()), ) if m.adoptViaHive { @@ -517,7 +517,7 @@ func (m *manager) Install(ctx context.Context) error { steps.Action(m.disableUpdates), steps.Condition(m.clusterVersionReady, 30*time.Minute, true), steps.Condition(m.aroDeploymentReady, 20*time.Minute, true), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.updateClusterData, m.managedResourceGroupName()), + steps.AuthorizationRetryingAction(m.fpCredRefresher, m.updateClusterData, m.managedResourceGroupName()), steps.Action(m.configureIngressCertificate), steps.Condition(m.ingressControllerReady, 30*time.Minute, true), steps.Action(m.configureDefaultStorageClass), diff --git a/pkg/util/refreshable/authorizer.go b/pkg/util/refreshable/authorizer.go new file mode 100644 index 00000000000..bf0e7b1ea7e --- /dev/null +++ b/pkg/util/refreshable/authorizer.go @@ -0,0 +1,45 @@ +package refreshable + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "github.com/Azure/go-autorest/autorest" + + "github.com/Azure/ARO-RP/pkg/env" +) + +type Authorizer interface { + autorest.Authorizer + Rebuilder +} + +type authorizer struct { + auth autorest.Authorizer + env env.Interface + tenantID string +} + +func (a *authorizer) Rebuild() error { + auth, err := a.env.FPAuthorizer(a.tenantID, nil, a.env.Environment().ResourceManagerScope) + if err != nil { + return err + } + a.auth = auth + return nil +} + +func (a *authorizer) WithAuthorization() autorest.PrepareDecorator { + return a.auth.WithAuthorization() +} + +// NewAuthorizer creates an Authorizer that can be rebuilt when needed to force +// token recreation. +func NewAuthorizer(_env env.Interface, tenantID string) (Authorizer, error) { + a := &authorizer{ + env: _env, + tenantID: tenantID, + } + err := a.Rebuild() + return a, err +} diff --git a/pkg/util/refreshable/refreshable.go b/pkg/util/refreshable/refreshable.go index 883773370d3..d1b7d3c6ce8 100644 --- a/pkg/util/refreshable/refreshable.go +++ b/pkg/util/refreshable/refreshable.go @@ -3,43 +3,27 @@ package refreshable // Copyright (c) Microsoft Corporation. // Licensed under the Apache License 2.0. -import ( - "github.com/Azure/go-autorest/autorest" - - "github.com/Azure/ARO-RP/pkg/env" -) - -type Authorizer interface { - autorest.Authorizer +type Rebuilder interface { Rebuild() error } -type authorizer struct { - auth autorest.Authorizer - env env.Interface - tenantID string -} +type multiRebuilder []Rebuilder + +func (m multiRebuilder) Rebuild() error { + for _, r := range m { + if r == nil { + continue + } -func (a *authorizer) Rebuild() error { - auth, err := a.env.FPAuthorizer(a.tenantID, nil, a.env.Environment().ResourceManagerScope) - if err != nil { - return err + err := r.Rebuild() + if err != nil { + return err + } } - a.auth = auth - return nil -} -func (a *authorizer) WithAuthorization() autorest.PrepareDecorator { - return a.auth.WithAuthorization() + return nil } -// NewAuthorizer creates an Authorizer that can be rebuilt when needed to force -// token recreation. -func NewAuthorizer(_env env.Interface, tenantID string) (Authorizer, error) { - a := &authorizer{ - env: _env, - tenantID: tenantID, - } - err := a.Rebuild() - return a, err +func NewMultiRebuilder(rebuilders ...Rebuilder) Rebuilder { + return multiRebuilder(rebuilders) } diff --git a/pkg/util/refreshable/refreshable_test.go b/pkg/util/refreshable/refreshable_test.go new file mode 100644 index 00000000000..37092f0c439 --- /dev/null +++ b/pkg/util/refreshable/refreshable_test.go @@ -0,0 +1,55 @@ +package refreshable + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "errors" + "testing" +) + +type fakeRebuilder struct { + calls int + err error +} + +func (r *fakeRebuilder) Rebuild() error { + r.calls++ + return r.err +} + +func TestMultiRebuilder(t *testing.T) { + wantErr := errors.New("boom") + + for _, tt := range []struct { + name string + first *fakeRebuilder + second *fakeRebuilder + wantErr error + wantCalls []int + }{ + { + name: "rebuilds all", + first: &fakeRebuilder{}, + second: &fakeRebuilder{}, + wantCalls: []int{1, 1}, + }, + { + name: "stops at first error", + first: &fakeRebuilder{err: wantErr}, + second: &fakeRebuilder{}, + wantErr: wantErr, + wantCalls: []int{1, 0}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + err := NewMultiRebuilder(tt.first, nil, tt.second).Rebuild() + if !errors.Is(err, tt.wantErr) { + t.Errorf("got %v, wanted %v", err, tt.wantErr) + } + if tt.first.calls != tt.wantCalls[0] || tt.second.calls != tt.wantCalls[1] { + t.Errorf("got calls %d,%d wanted %v", tt.first.calls, tt.second.calls, tt.wantCalls) + } + }) + } +} diff --git a/pkg/util/refreshable/tokencredential.go b/pkg/util/refreshable/tokencredential.go new file mode 100644 index 00000000000..bf048fa7205 --- /dev/null +++ b/pkg/util/refreshable/tokencredential.go @@ -0,0 +1,73 @@ +package refreshable + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "sync" + "time" + + sdkazcore "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + + "github.com/Azure/ARO-RP/pkg/env" + "github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/azcore" +) + +type TokenCredential interface { + azcore.TokenCredential + Rebuilder +} + +type tokenCredential struct { + env env.Interface + tenantID string + additionalTenants []string + + m sync.RWMutex + cred azcore.TokenCredential +} + +func (c *tokenCredential) GetToken(ctx context.Context, options policy.TokenRequestOptions) (sdkazcore.AccessToken, error) { + c.m.RLock() + cred := c.cred + c.m.RUnlock() + + tk, err := cred.GetToken(ctx, options) + if err != nil { + return tk, err + } + + tk.ExpiresOn = time.Time{} + + return tk, nil +} + +func (c *tokenCredential) Rebuild() error { + cred, err := c.env.FPNewClientCertificateCredential(c.tenantID, c.additionalTenants) + if err != nil { + return err + } + + c.m.Lock() + c.cred = cred + c.m.Unlock() + + return nil +} + +func NewFPTokenCredential(_env env.Interface, tenantID string, additionalTenants []string) (TokenCredential, error) { + c := &tokenCredential{ + env: _env, + tenantID: tenantID, + additionalTenants: additionalTenants, + } + + err := c.Rebuild() + if err != nil { + return nil, err + } + + return c, nil +} diff --git a/pkg/util/refreshable/tokencredential_test.go b/pkg/util/refreshable/tokencredential_test.go new file mode 100644 index 00000000000..88f6cac6668 --- /dev/null +++ b/pkg/util/refreshable/tokencredential_test.go @@ -0,0 +1,211 @@ +package refreshable + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "testing" + "time" + + "go.uber.org/mock/gomock" + + sdkazcore "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + + "github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/azcore" + mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env" +) + +type fakeCredential struct { + token string + calls int +} + +// GetToken returns a long-lived token, mirroring a real credential. The +// wrapper under test is responsible for stopping azcore's BearerTokenPolicy +// from caching it for that long. +func (c *fakeCredential) GetToken(ctx context.Context, options policy.TokenRequestOptions) (sdkazcore.AccessToken, error) { + c.calls++ + return sdkazcore.AccessToken{ + Token: c.token, + ExpiresOn: time.Now().Add(time.Hour), + RefreshOn: time.Now().Add(30 * time.Minute), + }, nil +} + +type transportFunc func(*http.Request) (*http.Response, error) + +func (f transportFunc) Do(req *http.Request) (*http.Response, error) { return f(req) } + +// TestTokenCredentialRebuildThroughBearerTokenPolicy drives a real azcore +// BearerTokenPolicy, which caches the AccessToken in its own pipeline, to prove +// Rebuild() actually changes the bearer token sent on the wire rather than only +// changing what a direct GetToken call returns. +func TestTokenCredentialRebuildThroughBearerTokenPolicy(t *testing.T) { + ctx := context.Background() + + controller := gomock.NewController(t) + _env := mock_env.NewMockInterface(controller) + + built := 0 + _env.EXPECT().FPNewClientCertificateCredential("tenant", nil).Times(2).DoAndReturn( + func(tenantID string, additionalTenants []string) (azcore.TokenCredential, error) { + built++ + return &fakeCredential{token: fmt.Sprintf("token-%d", built)}, nil + }) + + cred, err := NewFPTokenCredential(_env, "tenant", nil) + if err != nil { + t.Fatal(err) + } + + var sent []string + pipeline := runtime.NewPipeline("test", "v0.0.0", runtime.PipelineOptions{ + PerRetry: []policy.Policy{ + runtime.NewBearerTokenPolicy(cred, []string{"scope/.default"}, nil), + }, + }, &policy.ClientOptions{ + Transport: transportFunc(func(req *http.Request) (*http.Response, error) { + sent = append(sent, strings.TrimPrefix(req.Header.Get("Authorization"), "Bearer ")) + return &http.Response{ + StatusCode: http.StatusOK, + Body: http.NoBody, + Request: req, + }, nil + }), + }) + + do := func() { + t.Helper() + req, err := runtime.NewRequest(ctx, http.MethodGet, "https://management.azure.com/") + if err != nil { + t.Fatal(err) + } + _, err = pipeline.Do(req) + if err != nil { + t.Fatal(err) + } + } + + // two requests before any rebuild: the same token is expected + do() + do() + + err = cred.Rebuild() + if err != nil { + t.Fatal(err) + } + + // the pipeline must not keep serving the pre-rebuild token + do() + + want := []string{"token-1", "token-1", "token-2"} + if len(sent) != len(want) { + t.Fatalf("got %v, wanted %v", sent, want) + } + for i := range want { + if sent[i] != want[i] { + t.Errorf("request %d: got %q, wanted %q", i, sent[i], want[i]) + } + } +} + +func TestTokenCredentialRebuild(t *testing.T) { + ctx := context.Background() + + controller := gomock.NewController(t) + _env := mock_env.NewMockInterface(controller) + + built := 0 + _env.EXPECT().FPNewClientCertificateCredential("tenant", nil).Times(2).DoAndReturn( + func(tenantID string, additionalTenants []string) (azcore.TokenCredential, error) { + built++ + return &fakeCredential{token: fmt.Sprintf("token-%d", built)}, nil + }) + + cred, err := NewFPTokenCredential(_env, "tenant", nil) + if err != nil { + t.Fatal(err) + } + + token, err := cred.GetToken(ctx, policy.TokenRequestOptions{}) + if err != nil { + t.Fatal(err) + } + if token.Token != "token-1" { + t.Errorf("got %q, wanted token-1", token.Token) + } + + // without a rebuild, the same underlying credential is used + token, err = cred.GetToken(ctx, policy.TokenRequestOptions{}) + if err != nil { + t.Fatal(err) + } + if token.Token != "token-1" { + t.Errorf("got %q, wanted token-1", token.Token) + } + + err = cred.Rebuild() + if err != nil { + t.Fatal(err) + } + + token, err = cred.GetToken(ctx, policy.TokenRequestOptions{}) + if err != nil { + t.Fatal(err) + } + if token.Token != "token-2" { + t.Errorf("got %q, wanted token-2", token.Token) + } +} + +func TestTokenCredentialRebuildError(t *testing.T) { + ctx := context.Background() + wantErr := errors.New("cannot build credential") + + controller := gomock.NewController(t) + _env := mock_env.NewMockInterface(controller) + + gomock.InOrder( + _env.EXPECT().FPNewClientCertificateCredential("tenant", nil).Return(&fakeCredential{token: "token"}, nil), + _env.EXPECT().FPNewClientCertificateCredential("tenant", nil).Return(nil, wantErr), + ) + + cred, err := NewFPTokenCredential(_env, "tenant", nil) + if err != nil { + t.Fatal(err) + } + + err = cred.Rebuild() + if !errors.Is(err, wantErr) { + t.Errorf("got %v, wanted %v", err, wantErr) + } + + // a failed rebuild must leave the existing credential usable + token, err := cred.GetToken(ctx, policy.TokenRequestOptions{}) + if err != nil { + t.Fatal(err) + } + if token.Token != "token" { + t.Errorf("got %q, wanted token", token.Token) + } +} + +func TestNewFPTokenCredentialError(t *testing.T) { + wantErr := errors.New("cannot build credential") + + controller := gomock.NewController(t) + _env := mock_env.NewMockInterface(controller) + _env.EXPECT().FPNewClientCertificateCredential("tenant", nil).Return(nil, wantErr) + + _, err := NewFPTokenCredential(_env, "tenant", nil) + if !errors.Is(err, wantErr) { + t.Errorf("got %v, wanted %v", err, wantErr) + } +} diff --git a/pkg/util/steps/refreshing.go b/pkg/util/steps/refreshing.go index 163d22395f9..a13bd910897 100644 --- a/pkg/util/steps/refreshing.go +++ b/pkg/util/steps/refreshing.go @@ -24,16 +24,18 @@ import ( var ErrWantRefresh = errors.New("want refresh") // AuthorizationRetryingAction returns a wrapper Step which retries Azure -// authorization errors for up to 10 minutes. If authorizer is non-nil, it -// is rebuilt before each retry; a nil authorizer enables retry-only behavior. +// authorization errors for up to 10 minutes. If auth is non-nil, the +// credentials it holds are rebuilt before each retry (forcing a fresh access +// token, which in turn forces ARM to re-evaluate role assignments rather than +// serve its per-token cache); a nil auth enables retry-only behavior. // Any other error is returned directly. -func AuthorizationRetryingAction(r refreshable.Authorizer, action actionFunction, managedRGName string) Step { +func AuthorizationRetryingAction(r refreshable.Rebuilder, action actionFunction, managedRGName string) Step { return AuthorizationRetryingActionWithTimeout(r, action, managedRGName, 0) } // AuthorizationRetryingActionWithTimeout overrides the authorization retry window. // A zero timeout uses the default 10 minutes. Individual calls are not timed out. -func AuthorizationRetryingActionWithTimeout(r refreshable.Authorizer, action actionFunction, managedRGName string, timeout time.Duration) Step { +func AuthorizationRetryingActionWithTimeout(r refreshable.Rebuilder, action actionFunction, managedRGName string, timeout time.Duration) Step { return &authorizationRefreshingActionStep{ auth: r, f: action, @@ -44,7 +46,7 @@ func AuthorizationRetryingActionWithTimeout(r refreshable.Authorizer, action act type authorizationRefreshingActionStep struct { f actionFunction - auth refreshable.Authorizer + auth refreshable.Rebuilder retryTimeout time.Duration pollInterval time.Duration managedRGName string @@ -97,7 +99,7 @@ func (s *authorizationRefreshingActionStep) run(ctx context.Context, log *logrus if s.auth != nil { rebuildErr := s.auth.Rebuild() if rebuildErr != nil { - log.Printf("auth authorizer rebuild failed: %v", rebuildErr) + log.Printf("auth credential rebuild failed: %v", rebuildErr) err = rebuildErr } return false, rebuildErr