From 1febcdd964e2a9fef376d9698a5d6c588f45e25d Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 9 Oct 2024 21:28:10 -0400 Subject: [PATCH] ARO-11049 resolve comments --- .../azuresdk/armauthorization/roledefinitions.go | 2 +- .../dynamic/platformworkloadidentityprofile.go | 6 ++++-- .../platformworkloadidentityprofile_test.go | 16 ++++++++-------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go index 531c1d14d20..727500cd376 100644 --- a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go +++ b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go @@ -32,6 +32,6 @@ func NewArmRoleDefinitionsClient(credential azcore.TokenCredential, subscription } func (client ArmRoleDefinitionsClient) GetByID(ctx context.Context, roleDefinitionID string, options *armauthorization.RoleDefinitionsClientGetByIDOptions) (armauthorization.RoleDefinitionsClientGetByIDResponse, error) { - roleID := fmt.Sprintf("/subscriptions/%s%s", client.subscriptionID, roleDefinitionID) + roleID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", client.subscriptionID, roleDefinitionID) return client.RoleDefinitionsClient.GetByID(ctx, roleID, options) } diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile.go b/pkg/validate/dynamic/platformworkloadidentityprofile.go index 8a2985739ca..f5b17adf737 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armauthorization" "github.com/Azure/ARO-RP/pkg/util/rbac" + "github.com/Azure/ARO-RP/pkg/util/stringutils" "github.com/Azure/ARO-RP/pkg/util/version" ) @@ -63,7 +64,8 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile(ctx context.Context, } for _, role := range platformWorkloadIdentityRolesByRoleName { - actions, err := getActionsForRoleDefinition(ctx, role.RoleDefinitionID, roleDefinitions) + roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/') + actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions) if err != nil { return err } @@ -89,7 +91,7 @@ func (dv *dynamic) validateClusterMSI(ctx context.Context, oc *api.OpenShiftClus // Validate that the cluster MSI has all permissions specified in AzureRedHatOpenShiftFederatedCredentialRole over each platform managed identity func (dv *dynamic) validateClusterMSIPermissions(ctx context.Context, oid string, platformIdentities []api.PlatformWorkloadIdentity, roleDefinitions armauthorization.RoleDefinitionsClient) error { - actions, err := getActionsForRoleDefinition(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), roleDefinitions) + actions, err := getActionsForRoleDefinition(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, roleDefinitions) if err != nil { return err } diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go index a33d43486db..6534221fd2a 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go @@ -201,7 +201,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -232,7 +232,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -267,7 +267,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -431,7 +431,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, errors.New("Generic Error")) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, errors.New("Generic Error")) }, wantErr: "Generic Error", }, @@ -458,7 +458,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) }, wantErr: "parsing failed for Invalid UUID. Invalid resource Id format", }, @@ -479,7 +479,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -508,7 +508,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -537,7 +537,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, errors.New("Generic Error")) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) {