Azure: use the new Graph API to delete Service Principals (if possible) #5714
Conversation
|
/test e2e-azurestack |
|
/test e2e-azurestack |
|
/test e2e-azure |
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
Nice work. I left a few comments but have not had a chance to review everything.
There was a problem hiding this comment.
As this RequestAdapter is particular to the Graph client, I think it fits the pattern better to move this out of the Session and to where you are initializing the client: 26566c9#diff-a2fdf21d9b56bf657a0251ad81376851d27e5b801510337c8f5c9d2cd6495885R83-R87
There was a problem hiding this comment.
The problem is that to create a GraphRequestAdapter I need an AzureIdentityAuthenticationProvider which in turn needs an azidentity.ClientSecretCredential. So either I save in the session:
- the request adapter itself,
- the authentication provider,
- the client secret credential.
Which one would you rather pick, 2 or 3?
There was a problem hiding this comment.
3 because it is common across (the new) clients.
There was a problem hiding this comment.
I removed it from the session altogether since it's only used in one place for now. When more places start using it we can share it through the session.
|
/hold The deprecation of the graph API has been extended until at least openshift 4.12, which will give us more time to wait for the graph go client to stabilize. |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
the
needs-rebase
openshift-ci
bot
removed
the
needs-rebase
openshift-ci
bot
added
the
needs-rebase
openshift-ci
bot
removed
the
needs-rebase
|
Starting with pre-release v0.23.0 [1], [1] https://github.com/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazcore%2Fv0.23.0 |
openshift-ci
bot
added
the
needs-rebase
|
@r4f4: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-merge-robot
removed
the
needs-rebase
openshift-merge-robot
added
the
needs-rebase
|
/cc @patrickdillon |
openshift-ci
bot
requested review from
patrickdillon
and removed request for
jstuever
Microsoft is in the process of upgrading the Azure SDK for Go from V1 to V2. Our clients and authentication are on V1. The V1 authentication utilizes ADAL which will be deprecated June 30, 2022. All V2 clients, except the V2 auth client azidentity, are in beta (azidentity is scheduled to be stable in Q2 2022). [0] These changes remove the dependency on the ADAL API, replace the authentication with azidentity, and use an adapter so the auth will work with V1 clients. [0] https://azure.github.io/azure-sdk/releases/latest/index.html#go https://issues.redhat.com/browse/CORS-1910
Notice that azblob also had to be upgraded to 0.4.1, otherwise we hit the following build issue: vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_blob_lease_client.go:25:16: undefined: to.StringPtr vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_block_blob_client.go:145:20: undefined: to.StringPtr vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_container_lease_client.go:25:16: undefined: to.StringPtr vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_shared_policy_shared_key_credential.go:190:17: undefined: log.EventResponse vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_lease_request_options.go:63:16: undefined: to.StringPtr vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_lease_request_options.go:142:16: undefined: to.StringPtr and newer versions cause pkg/gather/azure/azure.go:224:29: undefined: azblob.NewBlobClientWithSharedKey pkg/gather/azure/azure.go:235:48: unknown field 'MaxRetryRequests' in struct literal of type blob.RetryReaderOptions
AD Graph API is being deprecated. This change tries to use the new MSGraph API to delete Applications and Service Principals. If there is an authentication error, we try the legacy AD Graph API, just in case (e.g, if clients haven't yet added the MSGraph permissions to their Azure subscription). https://issues.redhat.com/browse/CORS-1897
openshift-merge-robot
removed
the
needs-rebase
|
Depends on #6003 /hold |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@r4f4: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/close |
openshift-merge-robot
added
the
needs-rebase
|
@r4f4: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@patrickdillon: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
Try to use the new Graph API but fallback to the old one in case the Graph perm
Applications.ReadWrite.OwnedByis not yet granted.Tested with Azure Public Cloud and Openshift 4.9.23.