Migrate RP from Azure AD Graph to Microsoft Graph #1970

mbarnes · 2022-02-16T15:32:15Z

Which issue this PR addresses:

[ARO-1919] AAD: Migrate ARO-RP Azure AD Graph -> Microsoft Graph

What this PR does / why we need it:

Microsoft has announced the impending end-of-support for Azure Active Directory Graph API:

We will retire the Azure AD Graph API any time after June 30th, 2023.

We need to migrate the RP code to the Microsoft Graph SDK for Go.

Test plan for issue:

Pass all* GitHub checks, especially E2E tests.

* Except perhaps CodeQL, see additional notes below.

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

None that I'm aware of. This is an internal RP change and should not alter its behavior.

Additional notes:

CodeQL currently fails due to the vendoring of Microsoft Graph SDK for Go, which, due to its enormous API surface, causes memory consumption during CodeQL analysis to double, putting it well beyond the 8 GiB memory limit of standard GitHub Action runners. I'm working on moving CodeQL analysis to an Azure Pipeline, where we can use a virtual machine with adequate RAM (see the "pipelines" commit).

ross-bryan · 2022-02-28T19:20:02Z

@mbarnes FYI changed this PR to "Draft" to help make navigating open PRs a little easier for me.

github-actions · 2022-03-03T20:58:01Z

Please rebase pull request.

github-actions · 2022-03-17T00:56:18Z

Please rebase pull request.

github-actions · 2022-05-04T20:32:06Z

Please rebase pull request.

github-actions · 2022-05-26T04:39:36Z

Please rebase pull request.

SudoBrendan

While there's still open discussion on *string vs string and returning a 404 error, I'm ultimately going to call this a stylistic choice - we can disagree and commit. I added one final nit that also wouldn't stop me from merging this. LGTM, great work Matt, thank you!

pkg/util/cluster/cluster.go

s-amann · 2023-06-09T17:35:15Z

pkg/util/graph/graph.go

+	matches := result.GetValue()
+	switch len(matches) {
+	case 0:
+		return nil, nil


this still feels like it should be an error, but could be addressed later on post deadline.

To aid debugging failed MS Graph requests. MS Graph's top-level APIError message is hard-coded and only says "error status code received from the API". Further details have to be extracted from the "ODataErrorable" interface type.

No longer used.

Vendoring the Microsoft Graph SDK for Go causes memory consumption during CodeQL analysis to double due to its enormous API surface, putting it well beyond the memory limit of standard GitHub Action runners. I inquired with the Azure organization admins about provisioning larger GitHub runners, but was directed instead to use the 1ES Hosted Pool which runs our other CI checks. Since ARO controls the VM type for Hosted Pool agents, we can use a VM type with adequate memory for CodeQL analysis with the Graph SDK. Note: Implemented CodeQL commands in a template in case we ever decide to move Javascript or Python analysis to 1ES Hosted Pool as well.

bennerv

Mostly nits and some questions.

pkg/env/armhelper.go

pkg/util/cluster/aad.go

pkg/util/cluster/cluster.go

bennerv · 2023-06-09T18:07:23Z

pkg/util/cluster/cluster.go

+	if spObjID == nil {
 		return nil
 	}

-	roleAssignments, err := c.roleassignments.ListForResourceGroup(ctx, vnetResourceGroup, fmt.Sprintf("principalId eq '%s'", spObjID))
+	roleAssignments, err := c.roleassignments.ListForResourceGroup(ctx, vnetResourceGroup, fmt.Sprintf("principalId eq '%s'", *spObjID))


but not expected

I wouldn't return an error, but log a message stating no role assignments found.

If the cluster installation fails or is requeued for any reason, this role assignment may not exist. And that's totally fine, we're just trying to clean up after ourselves on cluster deletion.

pkg/cluster/serviceprincipal.go

pkg/env/armhelper.go

pkg/util/cluster/aad.go

bennerv · 2023-06-09T21:01:01Z

pkg/cluster/serviceprincipal.go


-	m.spApplications = graphrbac.NewApplicationsClient(m.env.Environment(), m.subscriptionDoc.Subscription.Properties.TenantID, spGraphAuthorizer)
-	return nil
+	return err
 }

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


Anywhere this is called, we can remove the refreshing action from now and test the previous theory about Authorization refreshing in azcore.

Also on the fixClusterSPObjectID call too.

What azcore refreshing theory? If I got it right, we reduce the amount of code here, which is always good direction.

@petrkotas Deep inside azcore there's a bit of logic to automatically force a token refresh upon an authorization error, which in theory obviates the need for our AuthorizationRefreshingAction. But the catch is it requires Azure clients explicitly define an authorization callback to handle this: specifically the "OnChallenge" callback shown in the azcore link.

We discovered the hard way (cluster install failures) that this logic isn't currently utilized for ARM calls since we're not fully migrated to the newer azure-sdk-for-go SDK. I'm not yet certain if the MS Graph client handles this situation automatically either.

bennerv

lgtm - few nits, but looks great otherwise.

petrkotas

@mbarnes thank you for outstanding work, especially for documenting graph api quirks.

petrkotas · 2023-06-12T10:19:44Z

pkg/util/cluster/cluster.go

+	if spObjID == nil {
 		return nil
 	}

-	roleAssignments, err := c.roleassignments.ListForResourceGroup(ctx, vnetResourceGroup, fmt.Sprintf("principalId eq '%s'", spObjID))
+	roleAssignments, err := c.roleassignments.ListForResourceGroup(ctx, vnetResourceGroup, fmt.Sprintf("principalId eq '%s'", *spObjID))


I do not consider this as an error either. There simply are not roleAssignments to delete. +1 on logging

petrkotas · 2023-06-12T10:25:17Z

pkg/cluster/serviceprincipal.go


-	m.spApplications = graphrbac.NewApplicationsClient(m.env.Environment(), m.subscriptionDoc.Subscription.Properties.TenantID, spGraphAuthorizer)
-	return nil
+	return err
 }

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


What azcore refreshing theory? If I got it right, we reduce the amount of code here, which is always good direction.

mbarnes · 2023-06-12T12:17:59Z

@mbarnes thank you for outstanding work, especially for documenting graph api quirks.

Thanks! If the quirk you're referring to is the ByApplicationId comments, I did file a complaint upstream about it. Previous graph SDK versions didn't have this confusing naming.

mbarnes · 2023-06-12T14:27:41Z

Noting for myself:

The MS Graph SDK uses different logic for handling 401 Unauthorized errors in the package kiota-http-go:
https://github.com/microsoft/kiota-http-go/blob/main/nethttp_request_adapter.go#L173-L198

From what I can make of the code, it is indeed requesting a new authorization token. We should still test it to verify.

* go.mod: Add github.com/microsoftgraph/msgraph-sdk-go * azureclient: Add NewGraphServiceClient Creates a GraphServiceClient with scope and graph endpoint set appropriately for the cloud environment (public or US government). * pkg/util/graph: Add GetServicePrincipalIDByAppID * armhelper: Use MS Graph to obtain service principal ID * armhelper: Remove unused authorizer parameter * Use MS Graph endpoint to validate service principal I don't think it matters for the purpose of validation, but the AD Graph endpoint is nearing its end-of-life. * pkg/cluster: Use MS Graph to obtain service principal ID * pkg/util/cluster: Use MS Graph to create and delete clusters * Pretty-print OData errors from MS Graph To aid debugging failed MS Graph requests. MS Graph's top-level APIError message is hard-coded and only says "error status code received from the API". Further details have to be extracted from the "ODataErrorable" interface type. * azureclient: Remove ActiveDirectoryGraphScope No longer used. * Remove pkg/util/azureclient/graphrbac No longer used. * pipelines: Run CodeQL analysis for Go on 1ES Hosted Pool Vendoring the Microsoft Graph SDK for Go causes memory consumption during CodeQL analysis to double due to its enormous API surface, putting it well beyond the memory limit of standard GitHub Action runners. I inquired with the Azure organization admins about provisioning larger GitHub runners, but was directed instead to use the 1ES Hosted Pool which runs our other CI checks. Since ARO controls the VM type for Hosted Pool agents, we can use a VM type with adequate memory for CodeQL analysis with the Graph SDK. Note: Implemented CodeQL commands in a template in case we ever decide to move Javascript or Python analysis to 1ES Hosted Pool as well.

mbarnes requested review from jewzaam, m1kola, bennerv, hawkowl, mwoodson, rogbas, petrkotas and ross-bryan as code owners February 16, 2022 15:32

mbarnes added the work-in-progress label Feb 16, 2022

bennerv added the hold Hold label Feb 16, 2022

ross-bryan marked this pull request as draft February 28, 2022 19:19

mbarnes force-pushed the aadgraph-to-msgraph branch from edcfb49 to 374f79c Compare March 3, 2022 20:57

github-actions bot added the needs-rebase branch needs a rebase label Mar 3, 2022

mbarnes force-pushed the aadgraph-to-msgraph branch from 374f79c to c451a2b Compare March 3, 2022 21:34

github-actions bot removed the needs-rebase branch needs a rebase label Mar 3, 2022

github-actions bot added the needs-rebase branch needs a rebase label Mar 17, 2022

mbarnes force-pushed the aadgraph-to-msgraph branch from baabb54 to 33f545a Compare April 11, 2022 16:59

github-actions bot removed the needs-rebase branch needs a rebase label Apr 11, 2022

mbarnes force-pushed the aadgraph-to-msgraph branch 2 times, most recently from bcb23d3 to f8f344f Compare April 15, 2022 02:33

mbarnes force-pushed the aadgraph-to-msgraph branch from f8f344f to f5fdc3a Compare April 26, 2022 15:06

github-actions bot added the needs-rebase branch needs a rebase label May 4, 2022

mbarnes force-pushed the aadgraph-to-msgraph branch from f5fdc3a to a0ed155 Compare May 6, 2022 00:46

github-actions bot removed the needs-rebase branch needs a rebase label May 6, 2022

github-actions bot added the needs-rebase branch needs a rebase label May 26, 2022

pkg/cluster: Use MS Graph to obtain service principal ID

6de830a

mbarnes force-pushed the aadgraph-to-msgraph branch from 4a2f6e9 to 6a7b18d Compare June 9, 2023 15:08

SudoBrendan self-requested a review June 9, 2023 16:36

SudoBrendan previously approved these changes Jun 9, 2023

View reviewed changes

pkg/util/cluster/cluster.go Outdated Show resolved Hide resolved

s-amann reviewed Jun 9, 2023

View reviewed changes

s-amann previously approved these changes Jun 9, 2023

View reviewed changes

mbarnes added 5 commits June 9, 2023 13:38

pkg/util/cluster: Use MS Graph to create and delete clusters

bcb2cb9

Pretty-print OData errors from MS Graph

7257d6c

To aid debugging failed MS Graph requests. MS Graph's top-level APIError message is hard-coded and only says "error status code received from the API". Further details have to be extracted from the "ODataErrorable" interface type.

azureclient: Remove ActiveDirectoryGraphScope

5af68ff

No longer used.

Remove pkg/util/azureclient/graphrbac

dd4f4a6

No longer used.

mbarnes dismissed stale reviews from s-amann and SudoBrendan via 643ff9e June 9, 2023 17:43

mbarnes force-pushed the aadgraph-to-msgraph branch from 6a7b18d to 643ff9e Compare June 9, 2023 17:43

SudoBrendan approved these changes Jun 9, 2023

View reviewed changes

s-amann approved these changes Jun 9, 2023

View reviewed changes

bennerv reviewed Jun 9, 2023

View reviewed changes

bennerv approved these changes Jun 9, 2023

View reviewed changes

SudoBrendan added next-up next-release To be included in the next RP release rollout labels Jun 9, 2023

petrkotas approved these changes Jun 12, 2023

View reviewed changes

This was referenced Jun 12, 2023

Rename TokenCredential variables to indicate credential used #2950

Merged

pkg/cluster: Use Action for fixupClusterSPObjectID #2961

Closed

SudoBrendan merged commit 156383c into Azure:master Jun 14, 2023

mbarnes deleted the aadgraph-to-msgraph branch June 14, 2023 17:11

ehvs mentioned this pull request Jul 15, 2024

pkg/cluster: Use Action for fixupClusterSPObjectID #3694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate RP from Azure AD Graph to Microsoft Graph #1970

Migrate RP from Azure AD Graph to Microsoft Graph #1970

mbarnes commented Feb 16, 2022 •

edited

Loading

ross-bryan commented Feb 28, 2022

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 17, 2022

github-actions bot commented May 4, 2022

github-actions bot commented May 26, 2022

SudoBrendan left a comment •

edited

Loading

s-amann Jun 9, 2023

bennerv left a comment

bennerv Jun 9, 2023

bennerv Jun 9, 2023

petrkotas Jun 12, 2023

mbarnes Jun 12, 2023 •

edited

Loading

bennerv left a comment

petrkotas left a comment

petrkotas Jun 12, 2023

petrkotas Jun 12, 2023

mbarnes commented Jun 12, 2023 •

edited

Loading

mbarnes commented Jun 12, 2023 •

edited

Loading

Migrate RP from Azure AD Graph to Microsoft Graph #1970

Migrate RP from Azure AD Graph to Microsoft Graph #1970

Conversation

mbarnes commented Feb 16, 2022 • edited Loading

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?

Additional notes:

ross-bryan commented Feb 28, 2022

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 17, 2022

github-actions bot commented May 4, 2022

github-actions bot commented May 26, 2022

SudoBrendan left a comment • edited Loading

Choose a reason for hiding this comment

s-amann Jun 9, 2023

Choose a reason for hiding this comment

bennerv left a comment

Choose a reason for hiding this comment

bennerv Jun 9, 2023

Choose a reason for hiding this comment

bennerv Jun 9, 2023

Choose a reason for hiding this comment

petrkotas Jun 12, 2023

Choose a reason for hiding this comment

mbarnes Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

bennerv left a comment

Choose a reason for hiding this comment

petrkotas left a comment

Choose a reason for hiding this comment

petrkotas Jun 12, 2023

Choose a reason for hiding this comment

petrkotas Jun 12, 2023

Choose a reason for hiding this comment

mbarnes commented Jun 12, 2023 • edited Loading

mbarnes commented Jun 12, 2023 • edited Loading

mbarnes commented Feb 16, 2022 •

edited

Loading

SudoBrendan left a comment •

edited

Loading

mbarnes Jun 12, 2023 •

edited

Loading

mbarnes commented Jun 12, 2023 •

edited

Loading

mbarnes commented Jun 12, 2023 •

edited

Loading