From 7fe81861d06cd7dcecd67257f06ada55e9ba9841 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 23 Sep 2024 16:06:35 -0400 Subject: [PATCH 01/14] Add new clusterIdentityIDs manager function --- pkg/cluster/clustermsi.go | 46 +++++++++ pkg/cluster/clustermsi_test.go | 165 +++++++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+) diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 3e2e3bf1bea..2d09b392001 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -14,6 +14,7 @@ import ( "github.com/Azure/msi-dataplane/pkg/dataplane" "github.com/Azure/msi-dataplane/pkg/store" + "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/env" "github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armmsi" ) @@ -22,6 +23,10 @@ const ( mockMsiCertValidityDays = 90 ) +var ( + errClusterMsiNotPresentInResponse = errors.New("cluster msi not present in msi credentials response") +) + // ensureClusterMsiCertificate leverages the MSI dataplane module to fetch the MSI's // backing certificate (if needed) and store the certificate in the cluster MSI key // vault. It does not concern itself with whether an existing certificate is valid @@ -137,3 +142,44 @@ func (m *manager) clusterMsiSecretName() (string, error) { return fmt.Sprintf("%s-%s", m.doc.ID, clusterMsi.Name), nil } + +func (m *manager) clusterIdentityIDs(ctx context.Context) error { + clusterMsiResourceId, err := m.doc.OpenShiftCluster.ClusterMsiResourceId() + if err != nil { + return err + } + + uaMsiRequest := dataplane.UserAssignedMSIRequest{ + IdentityURL: m.doc.OpenShiftCluster.Identity.IdentityURL, + ResourceIDs: []string{clusterMsiResourceId.String()}, + TenantID: m.doc.OpenShiftCluster.Identity.TenantID, + } + + msiCredObj, err := m.msiDataplane.GetUserAssignedIdentities(ctx, uaMsiRequest) + if err != nil { + return err + } + + if msiCredObj.CredentialsObject.ExplicitIdentities == nil || + len(msiCredObj.CredentialsObject.ExplicitIdentities) == 0 || + msiCredObj.CredentialsObject.ExplicitIdentities[0] == nil || + msiCredObj.CredentialsObject.ExplicitIdentities[0].ClientID == nil || + msiCredObj.CredentialsObject.ExplicitIdentities[0].ObjectID == nil { + return errClusterMsiNotPresentInResponse + } + + clientId := *msiCredObj.CredentialsObject.ExplicitIdentities[0].ClientID + principalId := *msiCredObj.CredentialsObject.ExplicitIdentities[0].ObjectID + + m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error { + identity := doc.OpenShiftCluster.Identity.UserAssignedIdentities[clusterMsiResourceId.String()] + identity.ClientID = clientId + identity.PrincipalID = principalId + + doc.OpenShiftCluster.Identity.UserAssignedIdentities[clusterMsiResourceId.String()] = identity + + return nil + }) + + return err +} diff --git a/pkg/cluster/clustermsi_test.go b/pkg/cluster/clustermsi_test.go index ed1956ffcd1..a1879e2035d 100644 --- a/pkg/cluster/clustermsi_test.go +++ b/pkg/cluster/clustermsi_test.go @@ -6,6 +6,7 @@ package cluster import ( "context" "fmt" + "strings" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -16,12 +17,14 @@ import ( "github.com/Azure/msi-dataplane/pkg/store" mockkvclient "github.com/Azure/msi-dataplane/pkg/store/mock_kvclient" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" "go.uber.org/mock/gomock" "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/env" "github.com/Azure/ARO-RP/pkg/frontend/middleware" mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env" + testdatabase "github.com/Azure/ARO-RP/test/database" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -301,3 +304,165 @@ func TestClusterMsiSecretName(t *testing.T) { }) } } + +func TestClusterIdentityIDs(t *testing.T) { + ctx := context.Background() + + mockGuid := "00000000-0000-0000-0000-000000000000" + clusterRGName := "aro-cluster" + clusterName := "aro-cluster" + + clusterResourceId := strings.ToLower(fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenShift/openShiftClusters/%s", mockGuid, clusterRGName, clusterName)) + + miName := "aro-cluster-msi" + miResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", mockGuid, clusterRGName, miName) + + msiDataPlaneNotFoundError := `failed to get credentials: Request information not available +-------------------------------------------------------------------------------- +RESPONSE 404: +ERROR CODE UNAVAILABLE +-------------------------------------------------------------------------------- +Response contained no body +-------------------------------------------------------------------------------- +` + miClientId := "ffffffff-ffff-ffff-ffff-ffffffffffff" + miObjectId := "99999999-9999-9999-9999-999999999999" + placeholderString := "placeholder" + + for _, tt := range []struct { + name string + doc *api.OpenShiftClusterDocument + msiDataplaneStub policy.ClientOptions + wantDoc *api.OpenShiftClusterDocument + wantErr string + }{ + { + name: "error - invalid resource ID (theoretically not possible, but still)", + doc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Identity: &api.Identity{ + UserAssignedIdentities: api.UserAssignedIdentities{ + "Hi hello I'm not a valid resource ID": api.ClusterUserAssignedIdentity{}, + }, + }, + }, + }, + wantErr: "invalid resource ID: resource id 'Hi hello I'm not a valid resource ID' must start with '/'", + }, + { + name: "error - encounter error in MSI dataplane", + doc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Identity: &api.Identity{ + IdentityURL: middleware.MockIdentityURL, + TenantID: mockGuid, + UserAssignedIdentities: api.UserAssignedIdentities{ + miResourceId: api.ClusterUserAssignedIdentity{}, + }, + }, + }, + }, + msiDataplaneStub: policy.ClientOptions{ + Transport: dataplane.NewStub([]*dataplane.CredentialsObject{ + { + CredentialsObject: swagger.CredentialsObject{}, + }, + }), + }, + wantErr: msiDataPlaneNotFoundError, + }, + { + name: "success - ClientID and PrincipalID are updated", + doc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Identity: &api.Identity{ + IdentityURL: middleware.MockIdentityURL, + TenantID: mockGuid, + UserAssignedIdentities: api.UserAssignedIdentities{ + miResourceId: api.ClusterUserAssignedIdentity{}, + }, + }, + }, + }, + msiDataplaneStub: policy.ClientOptions{ + Transport: dataplane.NewStub([]*dataplane.CredentialsObject{ + { + CredentialsObject: swagger.CredentialsObject{ + ExplicitIdentities: []*swagger.NestedCredentialsObject{ + { + ClientID: &miClientId, + ObjectID: &miObjectId, + ResourceID: &miResourceId, + + ClientSecret: &placeholderString, + TenantID: &placeholderString, + AuthenticationEndpoint: &placeholderString, + CannotRenewAfter: &placeholderString, + ClientSecretURL: &placeholderString, + MtlsAuthenticationEndpoint: &placeholderString, + NotAfter: &placeholderString, + NotBefore: &placeholderString, + RenewAfter: &placeholderString, + CustomClaims: &swagger.CustomClaims{ + XMSAzNwperimid: []*string{&placeholderString}, + XMSAzTm: &placeholderString, + }, + }, + }, + }, + }, + }), + }, + wantDoc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Identity: &api.Identity{ + IdentityURL: middleware.MockIdentityURL, + TenantID: mockGuid, + UserAssignedIdentities: api.UserAssignedIdentities{ + miResourceId: api.ClusterUserAssignedIdentity{ + ClientID: miClientId, + PrincipalID: miObjectId, + }, + }, + }, + }, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + msiDataplane, err := dataplane.NewClient(dataplane.AzurePublicCloud, nil, &tt.msiDataplaneStub) + if err != nil { + t.Fatal(err) + } + + openShiftClustersDatabase, _ := testdatabase.NewFakeOpenShiftClusters() + fixture := testdatabase.NewFixture().WithOpenShiftClusters(openShiftClustersDatabase) + fixture.AddOpenShiftClusterDocuments(tt.doc) + if err = fixture.Create(); err != nil { + t.Fatal(err) + } + + m := manager{ + log: logrus.NewEntry(logrus.StandardLogger()), + doc: tt.doc, + db: openShiftClustersDatabase, + msiDataplane: msiDataplane, + } + + err = m.clusterIdentityIDs(ctx) + utilerror.AssertErrorMessage(t, err, tt.wantErr) + + if tt.wantDoc != nil { + assert.Equal(t, tt.wantDoc.OpenShiftCluster, m.doc.OpenShiftCluster) + } + }) + } +} From dd566b5a31f881bdb9ea41b0f35695c77056b922 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 23 Sep 2024 16:16:49 -0400 Subject: [PATCH 02/14] Add clusterIdentityIDs step to install for WI clusters --- pkg/cluster/install.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index bb8ef88e894..e83908889e7 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -300,11 +300,14 @@ func setFieldCreatedByHive(createdByHive bool) database.OpenShiftClusterDocument func (m *manager) bootstrap() []steps.Step { s := []steps.Step{} + // initialize required client to manage cluster credentials, for both CSP or WI clusters if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { s = append(s, steps.Action(m.ensureClusterMsiCertificate), steps.Action(m.initializeClusterMsiClients), ) + } else { + s = append(s, steps.Action(m.initializeClusterSPClients)) } s = append(s, @@ -319,14 +322,13 @@ func (m *manager) bootstrap() []steps.Step { steps.Action(m.createOIDC), ) - if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { - s = append(s, - steps.Action(m.initializeClusterSPClients), // must run before clusterSPObjectID - // TODO: this relies on an authorizer that isn't exposed in the manager - // struct, so we'll rebuild the fpAuthorizer and use the error catching - // to advance - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID), - ) + // TODO: these rely on authorizers that aren't exposed in the manager + // struct, so we'll rebuild the fpAuthorizer and use the error catching + // to advance + if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs)) + } else { + s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID)) } s = append(s, From ebd2d65717d5daf4c6789dffbf52109e76d7af98 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 23 Sep 2024 16:43:38 -0400 Subject: [PATCH 03/14] Add new client wrapper for armmsi UserAssignedIdentitiesClient --- .../azureclient/azuresdk/armmsi/generate.go | 1 + .../armmsi/user_assigned_identities.go | 34 ++++++++ .../azureclient/azuresdk/armmsi/armmsi.go | 81 +++++-------------- 3 files changed, 53 insertions(+), 63 deletions(-) create mode 100644 pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go diff --git a/pkg/util/azureclient/azuresdk/armmsi/generate.go b/pkg/util/azureclient/azuresdk/armmsi/generate.go index 4ba6f260aa6..7b8db7bb812 100644 --- a/pkg/util/azureclient/azuresdk/armmsi/generate.go +++ b/pkg/util/azureclient/azuresdk/armmsi/generate.go @@ -6,4 +6,5 @@ package armmsi // Use source mode to prevent some issues related to generics being present in the interface. //go:generate rm -rf ../../../../../pkg/util/mocks/azureclient/azuresdk/$GOPACKAGE //go:generate mockgen -source ./federated_identity_credentials.go -destination=../../../mocks/azureclient/azuresdk/$GOPACKAGE/$GOPACKAGE.go github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/$GOPACKAGE FederatedIdentityCredentialsClient +//go:generate mockgen -source ./user_assigned_identities.go -destination=../../../mocks/azureclient/azuresdk/$GOPACKAGE/$GOPACKAGE.go github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/$GOPACKAGE UserAssignedIdentitiesClient //go:generate goimports -local=github.com/Azure/ARO-RP -e -w ../../../mocks/azureclient/azuresdk/$GOPACKAGE/$GOPACKAGE.go diff --git a/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go b/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go new file mode 100644 index 00000000000..ce7507c1eee --- /dev/null +++ b/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go @@ -0,0 +1,34 @@ +package armmsi + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi" + + "github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/azcore" +) + +type UserAssignedIdentitiesClient interface { + Get(ctx context.Context, resourceGroupName string, resourceName string, options *armmsi.UserAssignedIdentitiesClientGetOptions) (armmsi.UserAssignedIdentitiesClientGetResponse, error) +} + +type ArmUserAssignedIdentitiesClient struct { + *armmsi.UserAssignedIdentitiesClient +} + +var _ UserAssignedIdentitiesClient = &ArmUserAssignedIdentitiesClient{} + +// NewUserAssignedIdentitiesClient creates a new UserAssignedIdentitiesClient +func NewUserAssignedIdentitiesClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (UserAssignedIdentitiesClient, error) { + clientFactory, err := armmsi.NewClientFactory(subscriptionID, credential, options) + if err != nil { + return nil, err + } + return &ArmUserAssignedIdentitiesClient{ + UserAssignedIdentitiesClient: clientFactory.NewUserAssignedIdentitiesClient(), + }, nil +} diff --git a/pkg/util/mocks/azureclient/azuresdk/armmsi/armmsi.go b/pkg/util/mocks/azureclient/azuresdk/armmsi/armmsi.go index d4f88240b0d..99410df780d 100644 --- a/pkg/util/mocks/azureclient/azuresdk/armmsi/armmsi.go +++ b/pkg/util/mocks/azureclient/azuresdk/armmsi/armmsi.go @@ -1,9 +1,9 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: ./federated_identity_credentials.go +// Source: ./user_assigned_identities.go // // Generated by this command: // -// mockgen -source ./federated_identity_credentials.go -destination=../../../mocks/azureclient/azuresdk/armmsi/armmsi.go github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armmsi FederatedIdentityCredentialsClient +// mockgen -source ./user_assigned_identities.go -destination=../../../mocks/azureclient/azuresdk/armmsi/armmsi.go github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armmsi UserAssignedIdentitiesClient // // Package mock_armmsi is a generated GoMock package. @@ -13,89 +13,44 @@ import ( context "context" reflect "reflect" - runtime "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" armmsi "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi" gomock "go.uber.org/mock/gomock" ) -// MockFederatedIdentityCredentialsClient is a mock of FederatedIdentityCredentialsClient interface. -type MockFederatedIdentityCredentialsClient struct { +// MockUserAssignedIdentitiesClient is a mock of UserAssignedIdentitiesClient interface. +type MockUserAssignedIdentitiesClient struct { ctrl *gomock.Controller - recorder *MockFederatedIdentityCredentialsClientMockRecorder + recorder *MockUserAssignedIdentitiesClientMockRecorder } -// MockFederatedIdentityCredentialsClientMockRecorder is the mock recorder for MockFederatedIdentityCredentialsClient. -type MockFederatedIdentityCredentialsClientMockRecorder struct { - mock *MockFederatedIdentityCredentialsClient +// MockUserAssignedIdentitiesClientMockRecorder is the mock recorder for MockUserAssignedIdentitiesClient. +type MockUserAssignedIdentitiesClientMockRecorder struct { + mock *MockUserAssignedIdentitiesClient } -// NewMockFederatedIdentityCredentialsClient creates a new mock instance. -func NewMockFederatedIdentityCredentialsClient(ctrl *gomock.Controller) *MockFederatedIdentityCredentialsClient { - mock := &MockFederatedIdentityCredentialsClient{ctrl: ctrl} - mock.recorder = &MockFederatedIdentityCredentialsClientMockRecorder{mock} +// NewMockUserAssignedIdentitiesClient creates a new mock instance. +func NewMockUserAssignedIdentitiesClient(ctrl *gomock.Controller) *MockUserAssignedIdentitiesClient { + mock := &MockUserAssignedIdentitiesClient{ctrl: ctrl} + mock.recorder = &MockUserAssignedIdentitiesClientMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockFederatedIdentityCredentialsClient) EXPECT() *MockFederatedIdentityCredentialsClientMockRecorder { +func (m *MockUserAssignedIdentitiesClient) EXPECT() *MockUserAssignedIdentitiesClientMockRecorder { return m.recorder } -// CreateOrUpdate mocks base method. -func (m *MockFederatedIdentityCredentialsClient) CreateOrUpdate(ctx context.Context, resourceGroupName, resourceName, federatedIdentityCredentialResourceName string, parameters armmsi.FederatedIdentityCredential, options *armmsi.FederatedIdentityCredentialsClientCreateOrUpdateOptions) (armmsi.FederatedIdentityCredentialsClientCreateOrUpdateResponse, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdate", ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, parameters, options) - ret0, _ := ret[0].(armmsi.FederatedIdentityCredentialsClientCreateOrUpdateResponse) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// CreateOrUpdate indicates an expected call of CreateOrUpdate. -func (mr *MockFederatedIdentityCredentialsClientMockRecorder) CreateOrUpdate(ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, parameters, options any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdate", reflect.TypeOf((*MockFederatedIdentityCredentialsClient)(nil).CreateOrUpdate), ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, parameters, options) -} - -// Delete mocks base method. -func (m *MockFederatedIdentityCredentialsClient) Delete(ctx context.Context, resourceGroupName, resourceName, federatedIdentityCredentialResourceName string, options *armmsi.FederatedIdentityCredentialsClientDeleteOptions) (armmsi.FederatedIdentityCredentialsClientDeleteResponse, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, options) - ret0, _ := ret[0].(armmsi.FederatedIdentityCredentialsClientDeleteResponse) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Delete indicates an expected call of Delete. -func (mr *MockFederatedIdentityCredentialsClientMockRecorder) Delete(ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, options any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockFederatedIdentityCredentialsClient)(nil).Delete), ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, options) -} - // Get mocks base method. -func (m *MockFederatedIdentityCredentialsClient) Get(ctx context.Context, resourceGroupName, resourceName, federatedIdentityCredentialResourceName string, options *armmsi.FederatedIdentityCredentialsClientGetOptions) (armmsi.FederatedIdentityCredentialsClientGetResponse, error) { +func (m *MockUserAssignedIdentitiesClient) Get(ctx context.Context, resourceGroupName, resourceName string, options *armmsi.UserAssignedIdentitiesClientGetOptions) (armmsi.UserAssignedIdentitiesClientGetResponse, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, options) - ret0, _ := ret[0].(armmsi.FederatedIdentityCredentialsClientGetResponse) + ret := m.ctrl.Call(m, "Get", ctx, resourceGroupName, resourceName, options) + ret0, _ := ret[0].(armmsi.UserAssignedIdentitiesClientGetResponse) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockFederatedIdentityCredentialsClientMockRecorder) Get(ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, options any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockFederatedIdentityCredentialsClient)(nil).Get), ctx, resourceGroupName, resourceName, federatedIdentityCredentialResourceName, options) -} - -// NewListPager mocks base method. -func (m *MockFederatedIdentityCredentialsClient) NewListPager(resourceGroupName, resourceName string, options *armmsi.FederatedIdentityCredentialsClientListOptions) *runtime.Pager[armmsi.FederatedIdentityCredentialsClientListResponse] { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewListPager", resourceGroupName, resourceName, options) - ret0, _ := ret[0].(*runtime.Pager[armmsi.FederatedIdentityCredentialsClientListResponse]) - return ret0 -} - -// NewListPager indicates an expected call of NewListPager. -func (mr *MockFederatedIdentityCredentialsClientMockRecorder) NewListPager(resourceGroupName, resourceName, options any) *gomock.Call { +func (mr *MockUserAssignedIdentitiesClientMockRecorder) Get(ctx, resourceGroupName, resourceName, options any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewListPager", reflect.TypeOf((*MockFederatedIdentityCredentialsClient)(nil).NewListPager), resourceGroupName, resourceName, options) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockUserAssignedIdentitiesClient)(nil).Get), ctx, resourceGroupName, resourceName, options) } From 9dff1bfb56573a5dfa334a4d4e27947a254e0b9c Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 23 Sep 2024 16:48:04 -0400 Subject: [PATCH 04/14] Add userAssignedIdentities client to cluster manager --- pkg/cluster/cluster.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 17a0740b6d6..6777d52d74d 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -98,6 +98,7 @@ type manager struct { fpPrivateEndpoints network.PrivateEndpointsClient // TODO: use armFPPrivateEndpoints instead. armFPPrivateEndpoints armnetwork.PrivateEndpointsClient armRPPrivateLinkServices armnetwork.PrivateLinkServicesClient + userAssignedIdentities armmsi.UserAssignedIdentitiesClient dns dns.Manager storage storage.Manager @@ -315,6 +316,12 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database m.msiDataplane = msiDataplane m.clusterMsiKeyVaultStore = store.NewMsiKeyVaultStore(clusterMsiSecretsClient) + + userAssignedIdentitiesClient, err := armmsi.NewUserAssignedIdentitiesClient(r.SubscriptionID, fpCredClusterTenant, clientOptions) + if err != nil { + return nil, err + } + m.userAssignedIdentities = userAssignedIdentitiesClient } return m, nil From 8096fdbb0a1b040314407705c106d34b8f69544e Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 23 Sep 2024 18:02:32 -0400 Subject: [PATCH 05/14] Add new platformWorkloadIdentityIDs manager function --- pkg/cluster/platformworkloadidentities.go | 50 +++++ .../platformworkloadidentities_test.go | 201 ++++++++++++++++++ 2 files changed, 251 insertions(+) create mode 100644 pkg/cluster/platformworkloadidentities.go create mode 100644 pkg/cluster/platformworkloadidentities_test.go diff --git a/pkg/cluster/platformworkloadidentities.go b/pkg/cluster/platformworkloadidentities.go new file mode 100644 index 00000000000..13797dc1dde --- /dev/null +++ b/pkg/cluster/platformworkloadidentities.go @@ -0,0 +1,50 @@ +package cluster + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "fmt" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi" + + "github.com/Azure/ARO-RP/pkg/api" +) + +func (m *manager) platformWorkloadIdentityIDs(ctx context.Context) error { + var err error + if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + return fmt.Errorf("platformWorkloadIdentityIDs called for CSP cluster") + } + + identities := m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities + updatedIdentities := make([]api.PlatformWorkloadIdentity, len(identities)) + + for i, identity := range identities { + resourceId, err := arm.ParseResourceID(identity.ResourceID) + if err != nil { + return fmt.Errorf("platform workload identity '%s' invalid: %w", identity.OperatorName, err) + } + + identityDetails, err := m.userAssignedIdentities.Get(ctx, resourceId.ResourceGroupName, resourceId.Name, &armmsi.UserAssignedIdentitiesClientGetOptions{}) + if err != nil { + return fmt.Errorf("error occured when retrieving platform workload identity '%s' details: %w", identity.OperatorName, err) + } + + updatedIdentities[i] = api.PlatformWorkloadIdentity{ + OperatorName: identity.OperatorName, + ResourceID: identity.ResourceID, + ClientID: *identityDetails.Properties.ClientID, + ObjectID: *identityDetails.Properties.PrincipalID, + } + } + + m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error { + doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = updatedIdentities + return nil + }) + + return err +} diff --git a/pkg/cluster/platformworkloadidentities_test.go b/pkg/cluster/platformworkloadidentities_test.go new file mode 100644 index 00000000000..a0210f74d96 --- /dev/null +++ b/pkg/cluster/platformworkloadidentities_test.go @@ -0,0 +1,201 @@ +package cluster + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + + "github.com/Azure/ARO-RP/pkg/api" + mock_armmsi "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/azuresdk/armmsi" + testdatabase "github.com/Azure/ARO-RP/test/database" + utilerror "github.com/Azure/ARO-RP/test/util/error" +) + +func TestPlatformWorkloadIdentityIDs(t *testing.T) { + subscriptionId := "00000000-0000-0000-0000-000000000000" + clusterRG := "aro-cluster" + + clusterName := "aro-cluster" + clusterId := strings.ToLower(fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenShift/openShiftClusters/%s", subscriptionId, clusterRG, clusterName)) + + identityFooName := "foo" + identityFooResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", subscriptionId, clusterRG, identityFooName) + identityFooClientId := "0f000f00-0f00-0f00-0f00-0f000f000f00" + identityFooObjectId := "1f001f00-1f00-1f00-1f00-1f001f001f00" + + identityBarName := "bar" + identityBarResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", subscriptionId, clusterRG, identityBarName) + identityBarClientId := "0ba40ba4-0ba4-0ba4-0ba4-0ba40ba40ba4" + identityBarObjectId := "1ba41ba4-1ba4-1ba4-1ba4-1ba41ba41ba4" + + validWIClusterDoc := &api.OpenShiftClusterDocument{ + ID: clusterId, + Key: clusterId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + PlatformWorkloadIdentities: []api.PlatformWorkloadIdentity{ + { + OperatorName: identityFooName, + ResourceID: identityFooResourceId, + }, + { + OperatorName: identityBarName, + ResourceID: identityBarResourceId, + }, + }, + }, + }, + }, + } + + ctx := context.Background() + for _, tt := range []struct { + name string + doc *api.OpenShiftClusterDocument + userAssignedIdentitiesClientMocks func(*mock_armmsi.MockUserAssignedIdentitiesClient) + wantErr string + wantIdentities *[]api.PlatformWorkloadIdentity + }{ + { + name: "error - CSP cluster", + doc: &api.OpenShiftClusterDocument{ + ID: clusterId, + Key: clusterId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: &api.ServicePrincipalProfile{ + ClientID: "asdf", + ClientSecret: "asdf", + }, + }, + }, + }, + wantErr: "platformWorkloadIdentityIDs called for CSP cluster", + }, + { + name: "error - platform workload identity has invalid resource id", + doc: &api.OpenShiftClusterDocument{ + ID: clusterId, + Key: clusterId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + PlatformWorkloadIdentities: []api.PlatformWorkloadIdentity{ + { + + OperatorName: "invalid", + ResourceID: "I am not a resource ID.", + }, + }, + }, + }, + }, + }, + wantErr: "platform workload identity 'invalid' invalid: invalid resource ID: resource id 'I am not a resource ID.' must start with '/'", + }, + { + name: "error - unknown error when fetching details from ARM", + doc: &api.OpenShiftClusterDocument{ + ID: clusterId, + Key: clusterId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ + PlatformWorkloadIdentities: []api.PlatformWorkloadIdentity{ + { + OperatorName: identityFooName, + ResourceID: identityFooResourceId, + }, + }, + }, + }, + }, + }, + userAssignedIdentitiesClientMocks: func(mock *mock_armmsi.MockUserAssignedIdentitiesClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + Return(armmsi.UserAssignedIdentitiesClientGetResponse{}, fmt.Errorf("some error occurred")) + }, + wantErr: "error occured when retrieving platform workload identity 'foo' details: some error occurred", + }, + { + name: "success - all clientIDs and objectIDs updated in clusterdoc", + doc: validWIClusterDoc, + userAssignedIdentitiesClientMocks: func(mock *mock_armmsi.MockUserAssignedIdentitiesClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Eq(clusterRG), gomock.Eq(identityFooName), gomock.Any()).Times(1). + Return(armmsi.UserAssignedIdentitiesClientGetResponse{ + Identity: armmsi.Identity{ + Properties: &armmsi.UserAssignedIdentityProperties{ + ClientID: &identityFooClientId, + PrincipalID: &identityFooObjectId, + }, + }, + }, nil) + + mock.EXPECT().Get(gomock.Any(), gomock.Eq(clusterRG), gomock.Eq(identityBarName), gomock.Any()).Times(1). + Return(armmsi.UserAssignedIdentitiesClientGetResponse{ + Identity: armmsi.Identity{ + Properties: &armmsi.UserAssignedIdentityProperties{ + ClientID: &identityBarClientId, + PrincipalID: &identityBarObjectId, + }, + }, + }, nil) + }, + wantIdentities: &[]api.PlatformWorkloadIdentity{ + { + OperatorName: identityFooName, + ResourceID: identityFooResourceId, + ClientID: identityFooClientId, + ObjectID: identityFooObjectId, + }, + { + OperatorName: identityBarName, + ResourceID: identityBarResourceId, + ClientID: identityBarClientId, + ObjectID: identityBarObjectId, + }, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + + mockUserAssignedIdentities := mock_armmsi.NewMockUserAssignedIdentitiesClient(controller) + if tt.userAssignedIdentitiesClientMocks != nil { + tt.userAssignedIdentitiesClientMocks(mockUserAssignedIdentities) + } + + openShiftClustersDatabase, _ := testdatabase.NewFakeOpenShiftClusters() + fixture := testdatabase.NewFixture().WithOpenShiftClusters(openShiftClustersDatabase) + fixture.AddOpenShiftClusterDocuments(tt.doc) + if err := fixture.Create(); err != nil { + t.Fatal(err) + } + + m := manager{ + log: logrus.NewEntry(logrus.StandardLogger()), + doc: tt.doc, + db: openShiftClustersDatabase, + userAssignedIdentities: mockUserAssignedIdentities, + } + + err := m.platformWorkloadIdentityIDs(ctx) + utilerror.AssertErrorMessage(t, err, tt.wantErr) + + if tt.wantIdentities != nil { + assert.ElementsMatch(t, *tt.wantIdentities, m.doc.OpenShiftCluster.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities) + } + }) + } +} From e73fd7d7e3281a39d7ced038389ac80bf67ed242 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 23 Sep 2024 18:11:07 -0400 Subject: [PATCH 06/14] Add platformWorkloadIdentityIDs step to install for WI clusters --- pkg/cluster/install.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index e83908889e7..71870e087b1 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -326,7 +326,10 @@ func (m *manager) bootstrap() []steps.Step { // struct, so we'll rebuild the fpAuthorizer and use the error catching // to advance if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { - s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs)) + s = append(s, + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs), + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs), + ) } else { s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID)) } From 2c1847f529ec35e4998af43bf7aa468977123015 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Tue, 24 Sep 2024 12:02:36 -0400 Subject: [PATCH 07/14] Do not allow clusterIdentityIDs to be called for a CSP cluster --- pkg/cluster/clustermsi.go | 4 ++++ pkg/cluster/clustermsi_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 2d09b392001..075ba9178c3 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -144,6 +144,10 @@ func (m *manager) clusterMsiSecretName() (string, error) { } func (m *manager) clusterIdentityIDs(ctx context.Context) error { + if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + return fmt.Errorf("platformWorkloadIdentityIDs called for CSP cluster") + } + clusterMsiResourceId, err := m.doc.OpenShiftCluster.ClusterMsiResourceId() if err != nil { return err diff --git a/pkg/cluster/clustermsi_test.go b/pkg/cluster/clustermsi_test.go index a1879e2035d..397bc40dc05 100644 --- a/pkg/cluster/clustermsi_test.go +++ b/pkg/cluster/clustermsi_test.go @@ -336,12 +336,31 @@ Response contained no body wantDoc *api.OpenShiftClusterDocument wantErr string }{ + { + name: "error - CSP cluster", + doc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ServicePrincipalProfile: &api.ServicePrincipalProfile{ + ClientID: "asdf", + ClientSecret: "asdf", + }, + }, + }, + }, + wantErr: "platformWorkloadIdentityIDs called for CSP cluster", + }, { name: "error - invalid resource ID (theoretically not possible, but still)", doc: &api.OpenShiftClusterDocument{ ID: clusterResourceId, Key: clusterResourceId, OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, Identity: &api.Identity{ UserAssignedIdentities: api.UserAssignedIdentities{ "Hi hello I'm not a valid resource ID": api.ClusterUserAssignedIdentity{}, @@ -357,6 +376,9 @@ Response contained no body ID: clusterResourceId, Key: clusterResourceId, OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, Identity: &api.Identity{ IdentityURL: middleware.MockIdentityURL, TenantID: mockGuid, @@ -381,6 +403,9 @@ Response contained no body ID: clusterResourceId, Key: clusterResourceId, OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, Identity: &api.Identity{ IdentityURL: middleware.MockIdentityURL, TenantID: mockGuid, @@ -423,6 +448,9 @@ Response contained no body ID: clusterResourceId, Key: clusterResourceId, OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, Identity: &api.Identity{ IdentityURL: middleware.MockIdentityURL, TenantID: mockGuid, From 8303a758ab98e3f0c78cd4790ed4aeaf1fbfd204 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Tue, 24 Sep 2024 13:16:16 -0400 Subject: [PATCH 08/14] Perform all clientID/objectID enrichment before dynamic validation --- pkg/cluster/install.go | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index 71870e087b1..8eac43d914d 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -300,14 +300,23 @@ func setFieldCreatedByHive(createdByHive bool) database.OpenShiftClusterDocument func (m *manager) bootstrap() []steps.Step { s := []steps.Step{} - // initialize required client to manage cluster credentials, for both CSP or WI clusters + // initialize required clients to manage cluster credentials and populate + // clientIDs/objectIDs for them (both CSP and WI) + // TODO: ensuring credential IDs relies on authorizers that aren't exposed in the + // manager struct, so we'll rebuild the fpAuthorizer and use the error catching + // to advance if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { s = append(s, steps.Action(m.ensureClusterMsiCertificate), steps.Action(m.initializeClusterMsiClients), + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs), + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs), ) } else { - s = append(s, steps.Action(m.initializeClusterSPClients)) + s = append(s, + steps.Action(m.initializeClusterSPClients), + steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID), + ) } s = append(s, @@ -320,21 +329,6 @@ func (m *manager) bootstrap() []steps.Step { steps.Action(m.populateMTUSize), steps.Action(m.createDNS), steps.Action(m.createOIDC), - ) - - // TODO: these rely on authorizers that aren't exposed in the manager - // struct, so we'll rebuild the fpAuthorizer and use the error catching - // to advance - if m.doc.OpenShiftCluster.UsesWorkloadIdentity() { - s = append(s, - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterIdentityIDs), - steps.AuthorizationRetryingAction(m.fpAuthorizer, m.platformWorkloadIdentityIDs), - ) - } else { - s = append(s, steps.AuthorizationRetryingAction(m.fpAuthorizer, m.clusterSPObjectID)) - } - - s = append(s, steps.Action(m.ensureResourceGroup), steps.Action(m.ensureServiceEndpoints), steps.Action(m.setMasterSubnetPolicies), From c44c191b8a870a5fc9aab48e1ccab6c3e9af43a0 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Wed, 25 Sep 2024 16:03:00 -0400 Subject: [PATCH 09/14] Return UserAssignedIdentitiesClient implementation instead of interface in constructor --- .../azureclient/azuresdk/armmsi/user_assigned_identities.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go b/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go index ce7507c1eee..828e84a131c 100644 --- a/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go +++ b/pkg/util/azureclient/azuresdk/armmsi/user_assigned_identities.go @@ -23,7 +23,7 @@ type ArmUserAssignedIdentitiesClient struct { var _ UserAssignedIdentitiesClient = &ArmUserAssignedIdentitiesClient{} // NewUserAssignedIdentitiesClient creates a new UserAssignedIdentitiesClient -func NewUserAssignedIdentitiesClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (UserAssignedIdentitiesClient, error) { +func NewUserAssignedIdentitiesClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*ArmUserAssignedIdentitiesClient, error) { clientFactory, err := armmsi.NewClientFactory(subscriptionID, credential, options) if err != nil { return nil, err From 767d7345637824a6a98f03b61254b6eaa815bf1d Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Wed, 25 Sep 2024 16:05:44 -0400 Subject: [PATCH 10/14] Use cluster MSI credentials for userAssignedIdentities client This requires moving client instantiation from the cluster manager constructor to the initializeClusterMsiClients install step. --- pkg/cluster/cluster.go | 6 ------ pkg/cluster/clustermsi.go | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 6777d52d74d..eb0d166c482 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -316,12 +316,6 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database m.msiDataplane = msiDataplane m.clusterMsiKeyVaultStore = store.NewMsiKeyVaultStore(clusterMsiSecretsClient) - - userAssignedIdentitiesClient, err := armmsi.NewUserAssignedIdentitiesClient(r.SubscriptionID, fpCredClusterTenant, clientOptions) - if err != nil { - return nil, err - } - m.userAssignedIdentities = userAssignedIdentitiesClient } return m, nil diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 075ba9178c3..099fdeca0da 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -128,7 +128,13 @@ func (m *manager) initializeClusterMsiClients(ctx context.Context) error { return err } + userAssignedIdentities, err := armmsi.NewUserAssignedIdentitiesClient(subId, azureCred, clientOptions) + if err != nil { + return err + } + m.clusterMsiFederatedIdentityCredentials = clusterMsiFederatedIdentityCredentials + m.userAssignedIdentities = userAssignedIdentities return nil } From 2c87af89d1466238bcd27eaa3494981615672faf Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Wed, 25 Sep 2024 16:54:18 -0400 Subject: [PATCH 11/14] Extract ExplicitIdentity access/handling in clustermsi to common function --- pkg/cluster/clustermsi.go | 33 ++++++++++++++++++++++++--------- pkg/cluster/clustermsi_test.go | 2 +- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 099fdeca0da..882e10f0dd8 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -12,6 +12,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/msi-dataplane/pkg/dataplane" + "github.com/Azure/msi-dataplane/pkg/dataplane/swagger" "github.com/Azure/msi-dataplane/pkg/store" "github.com/Azure/ARO-RP/pkg/api" @@ -66,12 +67,16 @@ func (m *manager) ensureClusterMsiCertificate(ctx context.Context) error { if m.env.FeatureIsSet(env.FeatureUseMockMsiRp) { expirationDate = now.AddDate(0, 0, mockMsiCertValidityDays) } else { - if msiCredObj.CredentialsObject.ExplicitIdentities == nil || len(msiCredObj.CredentialsObject.ExplicitIdentities) == 0 || msiCredObj.CredentialsObject.ExplicitIdentities[0] == nil || msiCredObj.CredentialsObject.ExplicitIdentities[0].NotAfter == nil { + identity, err := getSingleExplicitIdentity(msiCredObj) + if err != nil { + return err + } + if identity.NotAfter == nil { return errors.New("unable to pull NotAfter from the MSI CredentialsObject") } // The swagger API spec for the MI RP specifies that NotAfter will be "in the format 2017-03-01T14:11:00Z". - expirationDate, err = time.Parse(time.RFC3339, *msiCredObj.CredentialsObject.ExplicitIdentities[0].NotAfter) + expirationDate, err = time.Parse(time.RFC3339, *identity.NotAfter) if err != nil { return err } @@ -151,7 +156,7 @@ func (m *manager) clusterMsiSecretName() (string, error) { func (m *manager) clusterIdentityIDs(ctx context.Context) error { if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { - return fmt.Errorf("platformWorkloadIdentityIDs called for CSP cluster") + return fmt.Errorf("clusterIdentityIDs called for CSP cluster") } clusterMsiResourceId, err := m.doc.OpenShiftCluster.ClusterMsiResourceId() @@ -170,12 +175,12 @@ func (m *manager) clusterIdentityIDs(ctx context.Context) error { return err } - if msiCredObj.CredentialsObject.ExplicitIdentities == nil || - len(msiCredObj.CredentialsObject.ExplicitIdentities) == 0 || - msiCredObj.CredentialsObject.ExplicitIdentities[0] == nil || - msiCredObj.CredentialsObject.ExplicitIdentities[0].ClientID == nil || - msiCredObj.CredentialsObject.ExplicitIdentities[0].ObjectID == nil { - return errClusterMsiNotPresentInResponse + identity, err := getSingleExplicitIdentity(msiCredObj) + if err != nil { + return err + } + if identity.ClientID == nil || identity.ObjectID == nil { + return fmt.Errorf("unable to pull clientID and objectID from the MSI CredentialsObject") } clientId := *msiCredObj.CredentialsObject.ExplicitIdentities[0].ClientID @@ -193,3 +198,13 @@ func (m *manager) clusterIdentityIDs(ctx context.Context) error { return err } + +func getSingleExplicitIdentity(msiCredObj *dataplane.UserAssignedIdentities) (*swagger.NestedCredentialsObject, error) { + if msiCredObj.CredentialsObject.ExplicitIdentities == nil || + len(msiCredObj.CredentialsObject.ExplicitIdentities) == 0 || + msiCredObj.CredentialsObject.ExplicitIdentities[0] == nil { + return nil, errClusterMsiNotPresentInResponse + } + + return msiCredObj.CredentialsObject.ExplicitIdentities[0], nil +} diff --git a/pkg/cluster/clustermsi_test.go b/pkg/cluster/clustermsi_test.go index 397bc40dc05..0e99d2b5f81 100644 --- a/pkg/cluster/clustermsi_test.go +++ b/pkg/cluster/clustermsi_test.go @@ -350,7 +350,7 @@ Response contained no body }, }, }, - wantErr: "platformWorkloadIdentityIDs called for CSP cluster", + wantErr: "clusterIdentityIDs called for CSP cluster", }, { name: "error - invalid resource ID (theoretically not possible, but still)", From 7744d316816594b4349d4dd520d4801bd4a2fde8 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Mon, 30 Sep 2024 16:13:34 -0400 Subject: [PATCH 12/14] Preserve passed-in casing on cluster identity resource IDs --- pkg/cluster/clustermsi.go | 20 +++++--- pkg/cluster/clustermsi_test.go | 94 ++++++++++++++++++++++++---------- 2 files changed, 82 insertions(+), 32 deletions(-) diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 882e10f0dd8..6adb01305d9 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "strings" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -187,13 +188,20 @@ func (m *manager) clusterIdentityIDs(ctx context.Context) error { principalId := *msiCredObj.CredentialsObject.ExplicitIdentities[0].ObjectID m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error { - identity := doc.OpenShiftCluster.Identity.UserAssignedIdentities[clusterMsiResourceId.String()] - identity.ClientID = clientId - identity.PrincipalID = principalId - - doc.OpenShiftCluster.Identity.UserAssignedIdentities[clusterMsiResourceId.String()] = identity + // we iterate through the existing identities to find the identity matching + // the expected resourceID with casefolding, to ensure we preserve the + // passed-in casing on IDs even if it may be incorrect + for k, v := range doc.OpenShiftCluster.Identity.UserAssignedIdentities { + if strings.EqualFold(k, clusterMsiResourceId.String()) { + v.ClientID = clientId + v.PrincipalID = principalId + + doc.OpenShiftCluster.Identity.UserAssignedIdentities[k] = v + return nil + } + } - return nil + return fmt.Errorf("no entries found matching clusterMsiResourceId") }) return err diff --git a/pkg/cluster/clustermsi_test.go b/pkg/cluster/clustermsi_test.go index 0e99d2b5f81..ce4d3bcd0e8 100644 --- a/pkg/cluster/clustermsi_test.go +++ b/pkg/cluster/clustermsi_test.go @@ -316,6 +316,7 @@ func TestClusterIdentityIDs(t *testing.T) { miName := "aro-cluster-msi" miResourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", mockGuid, clusterRGName, miName) + miResourceIdIncorrectCasing := fmt.Sprintf("/subscriptions/%s/resourcegroups/%s/providers/Microsoft.ManagedIdentity/userAssignedIdentities/%s", mockGuid, clusterRGName, miName) msiDataPlaneNotFoundError := `failed to get credentials: Request information not available -------------------------------------------------------------------------------- @@ -329,6 +330,36 @@ Response contained no body miObjectId := "99999999-9999-9999-9999-999999999999" placeholderString := "placeholder" + msiDataPlaneValidStub := policy.ClientOptions{ + Transport: dataplane.NewStub([]*dataplane.CredentialsObject{ + { + CredentialsObject: swagger.CredentialsObject{ + ExplicitIdentities: []*swagger.NestedCredentialsObject{ + { + ClientID: &miClientId, + ObjectID: &miObjectId, + ResourceID: &miResourceId, + + ClientSecret: &placeholderString, + TenantID: &placeholderString, + AuthenticationEndpoint: &placeholderString, + CannotRenewAfter: &placeholderString, + ClientSecretURL: &placeholderString, + MtlsAuthenticationEndpoint: &placeholderString, + NotAfter: &placeholderString, + NotBefore: &placeholderString, + RenewAfter: &placeholderString, + CustomClaims: &swagger.CustomClaims{ + XMSAzNwperimid: []*string{&placeholderString}, + XMSAzTm: &placeholderString, + }, + }, + }, + }, + }, + }), + } + for _, tt := range []struct { name string doc *api.OpenShiftClusterDocument @@ -415,35 +446,46 @@ Response contained no body }, }, }, - msiDataplaneStub: policy.ClientOptions{ - Transport: dataplane.NewStub([]*dataplane.CredentialsObject{ - { - CredentialsObject: swagger.CredentialsObject{ - ExplicitIdentities: []*swagger.NestedCredentialsObject{ - { - ClientID: &miClientId, - ObjectID: &miObjectId, - ResourceID: &miResourceId, - - ClientSecret: &placeholderString, - TenantID: &placeholderString, - AuthenticationEndpoint: &placeholderString, - CannotRenewAfter: &placeholderString, - ClientSecretURL: &placeholderString, - MtlsAuthenticationEndpoint: &placeholderString, - NotAfter: &placeholderString, - NotBefore: &placeholderString, - RenewAfter: &placeholderString, - CustomClaims: &swagger.CustomClaims{ - XMSAzNwperimid: []*string{&placeholderString}, - XMSAzTm: &placeholderString, - }, - }, + msiDataplaneStub: msiDataPlaneValidStub, + wantDoc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, + Identity: &api.Identity{ + IdentityURL: middleware.MockIdentityURL, + TenantID: mockGuid, + UserAssignedIdentities: api.UserAssignedIdentities{ + miResourceId: api.ClusterUserAssignedIdentity{ + ClientID: miClientId, + PrincipalID: miObjectId, }, }, }, - }), + }, + }, + }, + { + name: "success - existing identity resourceID casing is preserved even if it differs from ARM resourceID parsing", + doc: &api.OpenShiftClusterDocument{ + ID: clusterResourceId, + Key: clusterResourceId, + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{}, + }, + Identity: &api.Identity{ + IdentityURL: middleware.MockIdentityURL, + TenantID: mockGuid, + UserAssignedIdentities: api.UserAssignedIdentities{ + miResourceIdIncorrectCasing: api.ClusterUserAssignedIdentity{}, + }, + }, + }, }, + msiDataplaneStub: msiDataPlaneValidStub, wantDoc: &api.OpenShiftClusterDocument{ ID: clusterResourceId, Key: clusterResourceId, @@ -455,7 +497,7 @@ Response contained no body IdentityURL: middleware.MockIdentityURL, TenantID: mockGuid, UserAssignedIdentities: api.UserAssignedIdentities{ - miResourceId: api.ClusterUserAssignedIdentity{ + miResourceIdIncorrectCasing: api.ClusterUserAssignedIdentity{ ClientID: miClientId, PrincipalID: miObjectId, }, From 1205d4b0aa40faa8b61a14585dca2af3f19b6981 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Wed, 2 Oct 2024 15:55:57 -0400 Subject: [PATCH 13/14] Actually use extracted identity from getSingleExpectedIdentity --- pkg/cluster/clustermsi.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 6adb01305d9..082d21f1663 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -184,17 +184,14 @@ func (m *manager) clusterIdentityIDs(ctx context.Context) error { return fmt.Errorf("unable to pull clientID and objectID from the MSI CredentialsObject") } - clientId := *msiCredObj.CredentialsObject.ExplicitIdentities[0].ClientID - principalId := *msiCredObj.CredentialsObject.ExplicitIdentities[0].ObjectID - m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error { // we iterate through the existing identities to find the identity matching // the expected resourceID with casefolding, to ensure we preserve the // passed-in casing on IDs even if it may be incorrect for k, v := range doc.OpenShiftCluster.Identity.UserAssignedIdentities { if strings.EqualFold(k, clusterMsiResourceId.String()) { - v.ClientID = clientId - v.PrincipalID = principalId + v.ClientID = *identity.ClientID + v.PrincipalID = *identity.ObjectID doc.OpenShiftCluster.Identity.UserAssignedIdentities[k] = v return nil From 1ece0d9922c5a1c4539ba1c02b8abab8d3572c5d Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Wed, 2 Oct 2024 16:08:15 -0400 Subject: [PATCH 14/14] Clarify purpose of getSingleExplicitIdentity function Adds a comment and unit tests indicating its usage --- pkg/cluster/clustermsi.go | 11 ++-- pkg/cluster/clustermsi_test.go | 92 ++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/pkg/cluster/clustermsi.go b/pkg/cluster/clustermsi.go index 082d21f1663..df6e82635d2 100644 --- a/pkg/cluster/clustermsi.go +++ b/pkg/cluster/clustermsi.go @@ -204,12 +204,15 @@ func (m *manager) clusterIdentityIDs(ctx context.Context) error { return err } +// We expect the GetUserAssignedIdentities request to only ever be made for one identity +// at a time (the cluster MSI) and thus we expect the response to only contain a single +// identity's details. func getSingleExplicitIdentity(msiCredObj *dataplane.UserAssignedIdentities) (*swagger.NestedCredentialsObject, error) { - if msiCredObj.CredentialsObject.ExplicitIdentities == nil || - len(msiCredObj.CredentialsObject.ExplicitIdentities) == 0 || - msiCredObj.CredentialsObject.ExplicitIdentities[0] == nil { + if msiCredObj.ExplicitIdentities == nil || + len(msiCredObj.ExplicitIdentities) == 0 || + msiCredObj.ExplicitIdentities[0] == nil { return nil, errClusterMsiNotPresentInResponse } - return msiCredObj.CredentialsObject.ExplicitIdentities[0], nil + return msiCredObj.ExplicitIdentities[0], nil } diff --git a/pkg/cluster/clustermsi_test.go b/pkg/cluster/clustermsi_test.go index ce4d3bcd0e8..94a2fd52146 100644 --- a/pkg/cluster/clustermsi_test.go +++ b/pkg/cluster/clustermsi_test.go @@ -536,3 +536,95 @@ Response contained no body }) } } + +func TestGetSingleExplicitIdentity(t *testing.T) { + placeholderString := "placeholder" + validIdentity := &swagger.NestedCredentialsObject{ + ClientID: &placeholderString, + ClientSecret: &placeholderString, + TenantID: &placeholderString, + ResourceID: &placeholderString, + AuthenticationEndpoint: &placeholderString, + CannotRenewAfter: &placeholderString, + ClientSecretURL: &placeholderString, + MtlsAuthenticationEndpoint: &placeholderString, + NotAfter: &placeholderString, + NotBefore: &placeholderString, + RenewAfter: &placeholderString, + CustomClaims: &swagger.CustomClaims{ + XMSAzNwperimid: []*string{&placeholderString}, + XMSAzTm: &placeholderString, + }, + ObjectID: &placeholderString, + } + + for _, tt := range []struct { + name string + msiCredObj *dataplane.UserAssignedIdentities + want *swagger.NestedCredentialsObject + wantErr string + }{ + { + name: "ExplicitIdentities nil, returns error", + msiCredObj: &dataplane.UserAssignedIdentities{}, + wantErr: errClusterMsiNotPresentInResponse.Error(), + }, + { + name: "ExplicitIdentities empty, returns error", + msiCredObj: &dataplane.UserAssignedIdentities{ + CredentialsObject: dataplane.CredentialsObject{ + CredentialsObject: swagger.CredentialsObject{ + ExplicitIdentities: []*swagger.NestedCredentialsObject{}, + }, + }, + }, + wantErr: errClusterMsiNotPresentInResponse.Error(), + }, + { + name: "ExplicitIdentities first element is nil, returns error", + msiCredObj: &dataplane.UserAssignedIdentities{ + CredentialsObject: dataplane.CredentialsObject{ + CredentialsObject: swagger.CredentialsObject{ + ExplicitIdentities: []*swagger.NestedCredentialsObject{ + nil, + }, + }, + }, + }, + wantErr: errClusterMsiNotPresentInResponse.Error(), + }, + { + name: "ExplicitIdentities first element is nil, returns error", + msiCredObj: &dataplane.UserAssignedIdentities{ + CredentialsObject: dataplane.CredentialsObject{ + CredentialsObject: swagger.CredentialsObject{ + ExplicitIdentities: []*swagger.NestedCredentialsObject{ + nil, + }, + }, + }, + }, + wantErr: errClusterMsiNotPresentInResponse.Error(), + }, + { + name: "ExplicitIdentities first element is valid, returns it", + msiCredObj: &dataplane.UserAssignedIdentities{ + CredentialsObject: dataplane.CredentialsObject{ + CredentialsObject: swagger.CredentialsObject{ + ExplicitIdentities: []*swagger.NestedCredentialsObject{ + validIdentity, + }, + }, + }, + }, + want: validIdentity, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got, err := getSingleExplicitIdentity(tt.msiCredObj) + + assert.Equal(t, tt.want, got) + utilerror.AssertErrorMessage(t, err, tt.wantErr) + }) + } +}