Replace go-autorest MS Graph client with msgraph-sdk-go
#169
Conversation
| @@ -1,260 +0,0 @@ | |||
| // Code generated by MockGen. DO NOT EDIT. | |||
There was a problem hiding this comment.
Did not see this being used anywhere. Plus we have another provider_mock_test which seems to be achieving the same thing. Opted to delete this and update the latter with new MSGraph SDK mock tests
| } | ||
|
|
||
| // newAzureProvider creates an azureProvider, backed by Azure client objects for underlying services. | ||
| func newAzureProvider(settings *clientSettings, passwords api.Passwords) (api.AzureProvider, error) { | ||
| func newAzureProvider(settings *clientSettings, passwords api.Passwords) (AzureProvider, error) { |
There was a problem hiding this comment.
This is unrelated to your PR (sorry!) but probably a good time to figure out what's going on. It appears that passwords is unused here. Looking closer, it appears that our password Generate() func in api/passwords.go is also unused. It doesn't looks like Azure lets you provide a password but instead returns one to the client. This leads me to wonder if/how password_policy works? 🤔
There was a problem hiding this comment.
It doesn't appear as though it works. I wonder if we need to use SetSecretText in AddApplicationPassword()?
There was a problem hiding this comment.
Based on their API docs:
- https://learn.microsoft.com/en-us/graph/api/application-addpassword?view=graph-rest-1.0&tabs=http
- https://learn.microsoft.com/en-us/graph/api/serviceprincipal-addpassword?view=graph-rest-1.0&tabs=http
It doesn't seem like there's a way to set the password.
Maybe the fields were just added in case their API adds support for a user defined password in the future.
There was a problem hiding this comment.
Hmm, ya it doesn't look like you can set a password even with that SetSecretText() method. I think it's safe to say password policies don't work here. I'm thinking we'll want to deprecate the parameter, remove the code, and announce this in documentation / upgrade guides at minimum. Can be done in a separate task if that's preferred.
There was a problem hiding this comment.
Thanks for the context folks! Will add a ticket to track this in our backlog, and going to opt to tackle this in a follow-up PR 👍🏼
…p/vault-plugin-secrets-azure into VAULT-11806/msgraph-sdk-upgrade
| if ra == (armauthorization.RoleAssignmentsClientCreateResponse{}) { | ||
| return "", true, err | ||
| } | ||
| return *ra.ID, true, err |
There was a problem hiding this comment.
nit: I think it might be simpler to change Lines 187-196 to
if err != nil {
// Propagation delays within Azure can cause this error occasionally, so don't quit on it.
if strings.Contains(err.Error(), "PrincipalNotFound") {
return nil, false, nil
}
return "", true, err
}
return *ra.ID, true, nilIt looks like Create always returns an error when RoleAssignmentsClientCreateResponse is empty
There was a problem hiding this comment.
Seeing failures:
=== RUN TestRotateRootSuccess
path_rotate_root_test.go:39: failed to add new password: Resource 'd40cee02-4c15-4396-b857-3c52894762d3' does not exist or one of its queried reference-property objects are not present.
--- FAIL: TestRotateRootSuccess (0.65s)
=== RUN TestRotateRootPeriodicFunctionBeforeMinute
path_rotate_root_test.go:118: failed to add new password: Resource 'd40cee02-4c15-4396-b857-3c52894762d3' does not exist or one of its queried reference-property objects are not present.
--- FAIL: TestRotateRootPeriodicFunctionBeforeMinute (0.56s)
I accidentally switched to using the |
There was a problem hiding this comment.
A few small comments but otherwise LGTM! Nice job working through this one @vinay-gopalan!
Overview
This PR replaces the deprecated go-autorest module with msgraph-sdk-go.
Related Issues/Pull Requests
[ ] #166
[ ] PR #1234
Contributor Checklist
[x] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible