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.

Sign up for GitHub

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

Jump to bottom

Fix removed MSGraph param in Azure Secrets #1682

Merged
merged 18 commits into from
Dec 21, 2022
Merged

Fix removed MSGraph param in Azure Secrets #1682

merged 18 commits into from
Dec 21, 2022

Conversation

Zlaticanin
Copy link
Contributor

@Zlaticanin Zlaticanin commented Nov 28, 2022 •
edited
Loading

Fixing deprecated use_microsoft_graph_api param in Azure Secrets by choosing to write and read to/from Vault only if provider.IsAPISupported. If the Vault version is >=1.12, then we don't set it to TF state and write to Vault.

Output from reading the Azure config with Vault v1.13.0 (replacing ids with /):

Key                  Value
---                  -----
client_id            /
environment          AzurePublicCloud
root_password_ttl    4380h
subscription_id      /
tenant_id            /

Output from reading the Azure config with Vault v1.11.0+ent (replacing ids with /):

Key                        Value
---                        -----
client_id                  /
environment                AzurePublicCloud
root_password_ttl          4380h
subscription_id            /
tenant_id                  /
use_microsoft_graph_api    true

@Zlaticanin Zlaticanin marked this pull request as ready for review November 29, 2022 00:01
vinay-gopalan
vinay-gopalan reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

Looking good! I had a couple comments:

  • I think we should add a note in the documentation for this field stating that it has been deprecated in Vault 1.12+ and will be ignored
  • It would be nice to update this test so that we can run it for both before version 1.12 and after. In that case for versions below 1.12, we will test for all fields. For versions greater than 1.12, we will not test the deprecated field (you can use provider.IsAPISupported to conditionally achieve this behavior similar to what you have done already 😄 )

vinay-gopalan
vinay-gopalan reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

Looking great! Couple more comments and then we should be good

vault/resource_azure_secret_backend_test.go Outdated Show resolved Hide resolved
vault/resource_azure_secret_backend_test.go Show resolved Hide resolved
website/docs/r/azure_secret_backend.html.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/L and removed size/M labels Dec 16, 2022
@vinay-gopalan vinay-gopalan merged commit 41b3811 into main Dec 21, 2022
@vinay-gopalan vinay-gopalan added this to the 3.12.0 milestone Dec 21, 2022
@vinay-gopalan vinay-gopalan deleted the fix-msgraph-azure branch December 21, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinay-gopalan vinay-gopalan vinay-gopalan approved these changes

@benashz benashz Awaiting requested review from benashz

Assignees
No one assigned
Labels
documentation size/L
Projects
None yet
Milestone
3.12.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Zlaticanin @vinay-gopalan