Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update authorization to 2018-01-01 preview #83

Merged
merged 2 commits into from Nov 24, 2021
Merged

Conversation

jasonodonnell
Copy link
Contributor

I'm updating the authorization packages to 2018-01-01 preview to support roles that use data actions in Azure. The existing authorization package points to 2015-07-01, which does not support them.

Fixes #82.

@jasonodonnell jasonodonnell requested a review from a team November 19, 2021 16:35
@@ -4,7 +4,7 @@ import (
"context"
"time"

"github.com/Azure/azure-sdk-for-go/profiles/latest/authorization/mgmt/authorization"
"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-01-01-preview/authorization"
Copy link
Member

@calvn calvn Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there are other versions under authorization/mgmt, is there a reason we're going with this one in particular?

There is also a non-preview version for 2018-03-01 and other newer ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the diff it appears we originally used 2018-01-01-preview prior to merging #67. That PR switched to using profiles/latest, which appears to point to 2015-07-01. I think that explains why this error is showing up now.

Using 2018-01-01-preview should be fine, but we might want to use something more recent as @calvn suggested. It looks like terraform-provider-azurerm is using 2020-04-01-preview.

Copy link
Member

@calvn calvn Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @austingebauer. Let's take a stepwise approach and upgrade back to what it was before, and we can look into bumping that up further afterwards (along with more testing). So upgrading to 2018-01-01-preview sounds good to me.

@kalafut kalafut added this to the 1.9.1 milestone Nov 23, 2021
@austingebauer
Copy link
Member

I was able to reproduce the error in #82 and verify that this PR fixes it for both AAD graph and MS graph configurations. Additionally, I fixed the acceptance tests and added coverage for using the Storage Blob Data Owner role which has DataActions/NotDataActions.

Acceptance test output:

➜  vault-plugin-secrets-azure git:(authorization) ✗ go test -v -run TestCredentialInteg_msgraph
=== RUN   TestCredentialInteg_msgraph
=== RUN   TestCredentialInteg_msgraph/service_principals
=== PAUSE TestCredentialInteg_msgraph/service_principals
=== CONT  TestCredentialInteg_msgraph/service_principals
--- PASS: TestCredentialInteg_msgraph (0.00s)
    --- PASS: TestCredentialInteg_msgraph/service_principals (30.37s)
PASS
ok      github.com/hashicorp/vault-plugin-secrets-azure 30.562s
➜  vault-plugin-secrets-azure git:(authorization) ✗ go test -v -run TestCredentialInteg_aad
=== RUN   TestCredentialInteg_aad
=== RUN   TestCredentialInteg_aad/service_principals
=== PAUSE TestCredentialInteg_aad/service_principals
=== RUN   TestCredentialInteg_aad/static_service_principals
=== PAUSE TestCredentialInteg_aad/static_service_principals
=== CONT  TestCredentialInteg_aad/service_principals
=== CONT  TestCredentialInteg_aad/static_service_principals
--- PASS: TestCredentialInteg_aad (0.00s)
    --- PASS: TestCredentialInteg_aad/service_principals (30.98s)
    --- PASS: TestCredentialInteg_aad/static_service_principals (50.91s)
PASS
ok      github.com/hashicorp/vault-plugin-secrets-azure 51.521s

@austingebauer austingebauer merged commit 241dd84 into master Nov 24, 2021
@austingebauer austingebauer deleted the authorization branch November 24, 2021 18:23
austingebauer added a commit that referenced this pull request Nov 24, 2021
* Update authorization to 2018-01-01 preview

* Fixes AAD tests; Adds coverage for Storage Blob Data Owner in MS graph tests

Co-authored-by: Austin Gebauer <agebauer@hashicorp.com>
austingebauer added a commit that referenced this pull request Nov 24, 2021
* Update authorization to 2018-01-01 preview

* Fixes AAD tests; Adds coverage for Storage Blob Data Owner in MS graph tests

Co-authored-by: Austin Gebauer <agebauer@hashicorp.com>

Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure secret backend seems to use an old API
4 participants