Generate Federated Identity Credentials for MIWI Cluster #3847

gouthamMN · 2024-09-17T20:42:53Z

Which issue this PR addresses:

Fixes: https://issues.redhat.com/browse/ARO-4375

What this PR does / why we need it:

This PR perform the followings:

Add/Create Role Assignments on each customer provided Platform Workload Identities over Managed Resource Group and Vnet Resource Group as mentioned in MIWI Test Matrix doc by reading the respective role definitions from PlatformWorkloadIdentityRoleSetProperties.PlatformWorkloadIdentityRole.RoleDefinitionID.
Add/Create Azure Red Hat OpenShift Federated Credential Role Role assignment to Cluster MSI over Managed Resource Group and Vnet Resource Group for Workload Identity clusters.
Create FederatedIdentityCredentials for each customer provided Platform Workload Identities during cluster install/create for each Service accounts in PlatformWorkloadIdentityRoleSetProperties.PlatformWorkloadIdentityRole.ServiceAccounts.
Delete/Cleanup FederatedIdentityCredentials for each customer provided Platform Workload Identities that were created during cluster delete.

Test plan for issue:

New Unit tests are added.
e2e

Is there any documentation that needs to be updated for this PR?

No

How do you know this will function as expected in production?

If the e2e pass without errors

…MSI RP

…dentials control plane and key vault data plane)

- Initialize the MSI dataplane client, using the mock MSI RP/stub if appropriate - Initialize key vault store client (for MSI certificates; functionality is implemented in MSI dataplane module) - Create a cluster MSI certificate and store it in the key vault during cluster bootstrap - Instantiate an Azure SDK FederatedIdentityCredential client using the cluster MSI certificate - Delete the cluster MSI certificate as needed during cluster deletion

already gone from the key vault (or was potentially never created) Realized this case was not being handled correctly while I was working on unit tests.

- Update doc comment for ensureClusterMsiCertificate - Simplify conditional logic in MSI cert deletion

- Move `clusterMsiResourceId` function to `OpenShiftCluster` type - When persisting the MSI cert to KV, use the `NotAfter` returned by the MSI RP (for the stub, just use an arbitrary value) - Move `getClientOptions` functionality to `AROEnvironment` type - Move logic for determining cluster MSI key vault name to `pkg/env` - Pull cloud name mapping stuff out to `AROEnvironment` type - Update msi-dataplane module to include new changes and use `UserAssignedIdentities` type to get Azure credential in `pkg/cluster/clustermsi.go` - Fix typo in https URL in comment in `pkg/cluster/delete.go` - Implement suggestion to use `errors.As` instead of a type assertion in `pkg/cluster/delete.go`

- Move new cluster MSI steps forward in bootstrap step order - Move MSI dataplane client options stuff to pkg/env - Explicitly check for a single cluster MSI in `ClusterMsiResourceId` - Other small tweaks

…dereference

…dation bootstrap step

…MSI RP

…dentials control plane and key vault data plane)

- Initialize the MSI dataplane client, using the mock MSI RP/stub if appropriate - Initialize key vault store client (for MSI certificates; functionality is implemented in MSI dataplane module) - Create a cluster MSI certificate and store it in the key vault during cluster bootstrap - Instantiate an Azure SDK FederatedIdentityCredential client using the cluster MSI certificate - Delete the cluster MSI certificate as needed during cluster deletion

github-actions · 2024-09-17T20:47:27Z

Please rebase pull request.

pkg/cluster/deploybaseresources.go

pkg/cluster/delete.go

pkg/cluster/deploybaseresources_additional.go

rajdeepc2792 · 2024-09-17T22:03:52Z

pkg/cluster/deploybaseresources_additional.go

+ resources = append(resources, m.workloadIdentityResourceGroupRBAC(roleID, identity.ObjectID, managedRG))
+
+ if role.OperatorName != "StorageOperator" {
+ resources = append(resources, m.workloadIdentityResourceGroupRBAC(roleID, identity.ObjectID, vnetRG))


Why are we handling the RBAC for vnetRG? The RBAC is only needed for the managedRG, FPSP won't have permissions for role assignment on vnetRG. And for managedRG, all the identities existing in platformWIRolesByRoleName will have a role assignment performed for their corresponding roles.

I followed this MI/WI Test Matrix doc, which mention the scopes for each operator. So, should I assume, the customer will handle this for Vnet in production?

Yes, the VNET is to be handled by customer, and that check will already done till this point as part of dynamic validation.

rajdeepc2792 · 2024-09-17T22:07:04Z

pkg/cluster/install.go

+
+ if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
+ s = append(s,
+ steps.Action(m.generateFederatedIdentityCredentials),


Not sure if the FederatedIdentityCredentials ensure need to be part of this PR or not. But for update flow, the credentials will need to be ensure such that they exists and with correct values of issuer, audience etc.

rajdeepc2792

Thanks for the PR!!
Performed a quick review of the PR, added some comments for the federated credentials creation/deletion and RBAC part.
The review would be easier to do after the MSI PR is merged.

niontive · 2024-09-19T20:47:04Z

hack/devtools/local_dev_env.sh

- if [[ "${operatorName}" == "MachineApiOperator" || "${operatorName}" == "NetworkOperator" \
- || "${operatorName}" == "AzureFilesStorageOperator" || "${operatorName}" == "ServiceOperator" ]]; then
+ # Storage Operator don't require access to customer BYO virtual network
+ if [[ "${operatorName}" != "StorageOperator" ]]; then


What about image registry operator? or cloud controller manager?

niontive · 2024-09-19T21:34:48Z

pkg/cluster/delete.go

 if m.doc.OpenShiftCluster.UsesWorkloadIdentity() {
 m.log.Printf("deleting cluster MSI certificate")
 err = m.deleteClusterMsiCertificate(ctx)
 if err != nil {
 return err
 }
+
+ err = m.deleteFederatedCredentials(ctx)


Nit: we can call this "deleteIdentityFederations"

niontive · 2024-09-19T21:43:29Z

pkg/cluster/workloadidentityresources.go

+ if err != nil {
+ return "", err
+ }
+ return fmt.Sprintf("%s-%s", m.doc.ID, identityResourceId.ResourceName), nil


Any justification for this naming convention?

Want to make sure it fits within the requirements for Azure resource naming. For example, how long is doc.ID?

niontive · 2024-09-19T21:45:21Z

pkg/cluster/deploybaseresources.go

@@ -444,6 +454,60 @@ func (m *manager) setMasterSubnetPolicies(ctx context.Context) error {
 return err
 }

+func (m *manager) generateFederatedIdentityCredentials(ctx context.Context) error {


Nit: I'd call this federateIdentityCredentials - we aren't "generating" anything per say; we are "federating" the identities.

niontive · 2024-09-19T21:45:46Z

pkg/cluster/deploybaseresources.go

+ return fmt.Errorf("OIDCIssuer is nil for the cluster with ID: %s", m.doc.OpenShiftCluster.ID)
+ }
+
+ Issuer := to.StringPtr((string)(*m.doc.OpenShiftCluster.Properties.ClusterProfile.OIDCIssuer))


nit: issuer := ...

niontive · 2024-09-19T21:49:57Z

pkg/cluster/deploybaseresources.go

+ identityResourceId.ResourceName,
+ federatedIdentityCredentialResourceName,
+ armmsi.FederatedIdentityCredential{
+ Properties: &armmsi.FederatedIdentityCredentialProperties{


See this as an example for setting properties: https://azure.github.io/azure-workload-identity/docs/quick-start.html#4-create-an-aad-application-or-user-assigned-managed-identity-and-grant-permissions-to-access-the-secret

kimorris27 added 30 commits September 11, 2024 14:55

Add a parameter for enabling Entra ID RBAC on key vaults

9c8eee2

Add an RP-level feature flag for determining whether to use the mock …

f4fbce8

…MSI RP

Tweak the mock identity URL to play nicely with the mock MSI RP

fd37e7c

Add Azure SDK client wrappers for new clients (federated identity cre…

8e317e3

…dentials control plane and key vault data plane)

Vendor in new Azure SDK clients and update msi-dataplane

898b50d

Appease the linter

2f906c7

Don't fail during cluster deletion if the cluster MSI certificate is

4cbceb2

already gone from the key vault (or was potentially never created) Realized this case was not being handled correctly while I was working on unit tests.

Add unit tests

556054a

Vendor in stuff added while writing unit tests

cdcba57

Appease the linter

49a7779

Establish an RP-Config variable for the MSI RP endpoint

52762d7

Respond to PR comments

b1e0992

- Update doc comment for ensureClusterMsiCertificate - Simplify conditional logic in MSI cert deletion

Use pointer conversion functions that aren't deprecated

29ee691

Update documentation with info about new feature flag

87f8c95

Fix things in response to PR comments

e3ac345

- Move new cluster MSI steps forward in bootstrap step order - Move MSI dataplane client options stuff to pkg/env - Explicitly check for a single cluster MSI in `ClusterMsiResourceId` - Other small tweaks

Vendor in msi-dataplane update that prevents a potential nil pointer …

b97f54b

…dereference

Add missing method to internal key vault client

4dd0fa4

Make error messages more specific in ClusterMsiResourceId

6de513b

Fix mistakes I made during a large rebase - sorry for the mess

4a7a1da

Add missing env vars to run-rp make target and uncomment dynamic vali…

7310706

…dation bootstrap step

Revendor again to fix issues I probably introduced during a rebase

a5f9e43

Add a parameter for enabling Entra ID RBAC on key vaults

a88d1f3

Add an RP-level feature flag for determining whether to use the mock …

172d1ec

…MSI RP

Tweak the mock identity URL to play nicely with the mock MSI RP

4d6dba1

Add Azure SDK client wrappers for new clients (federated identity cre…

2a520dd

…dentials control plane and key vault data plane)

Vendor in new Azure SDK clients and update msi-dataplane

9a19638

Appease the linter

7ae8878

gouthamMN requested review from yjst2012, jaitaiwan, anshulvermapatel, hlipsig, tiguelu, SrinivasAtmakuri, mociarain, kimorris27, tsatam and bitoku as code owners September 17, 2024 20:42

gouthamMN added go Pull requests that update Go code chainsaw Pull requests or issues owned by Team Chainsaw labels Sep 17, 2024

github-actions bot added the needs-rebase branch needs a rebase label Sep 17, 2024

fix comment

99758b7

rajdeepc2792 reviewed Sep 17, 2024

View reviewed changes

pkg/cluster/deploybaseresources.go Show resolved Hide resolved

rajdeepc2792 reviewed Sep 17, 2024

View reviewed changes

pkg/cluster/delete.go Show resolved Hide resolved

rajdeepc2792 reviewed Sep 17, 2024

View reviewed changes

pkg/cluster/deploybaseresources_additional.go Show resolved Hide resolved

rajdeepc2792 reviewed Sep 17, 2024

View reviewed changes

rajdeepc2792 requested changes Sep 17, 2024

View reviewed changes

niontive reviewed Sep 19, 2024

View reviewed changes

merge from kips changes

d18cee4

github-actions bot removed the needs-rebase branch needs a rebase label Sep 19, 2024

niontive reviewed Sep 19, 2024

View reviewed changes

continue when no operator found

4382afe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate Federated Identity Credentials for MIWI Cluster #3847

Generate Federated Identity Credentials for MIWI Cluster #3847

gouthamMN commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

rajdeepc2792 Sep 17, 2024

gouthamMN Sep 18, 2024 •

edited

Loading

rajdeepc2792 Sep 18, 2024

rajdeepc2792 Sep 17, 2024

rajdeepc2792 left a comment

niontive Sep 19, 2024

niontive Sep 19, 2024

niontive Sep 19, 2024

niontive Sep 19, 2024

niontive Sep 19, 2024

niontive Sep 19, 2024

Generate Federated Identity Credentials for MIWI Cluster #3847

Are you sure you want to change the base?

Generate Federated Identity Credentials for MIWI Cluster #3847

Conversation

gouthamMN commented Sep 17, 2024

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

How do you know this will function as expected in production?

github-actions bot commented Sep 17, 2024

Choose a reason for hiding this comment

gouthamMN Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajdeepc2792 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamMN Sep 18, 2024 •

edited

Loading