Skip to content

Commit

Permalink
ARO-11049 resolve comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@rajdeepc2792
rajdeepc2792 committed Oct 10, 2024
1 parent b2aa108 commit 1febcdd
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 11 deletions.
2 changes: 1 addition & 1 deletion pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
6 changes: 4 additions & 2 deletions pkg/validate/dynamic/platformworkloadidentityprofile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
16 changes: 8 additions & 8 deletions pkg/validate/dynamic/platformworkloadidentityprofile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 1febcdd

Please sign in to comment.