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

Azure Rotating Secrets Changes #1143

Merged
merged 11 commits into from
Dec 5, 2024
Merged

Azure Rotating Secrets Changes #1143

merged 11 commits into from
Dec 5, 2024

Conversation

murali-partha
Copy link
Contributor

@murali-partha murali-partha commented Nov 22, 2024

🛠️ Description

Added hcp_vault_secrets_integration_azureresource and modified hcp_vault_secrets_rotating_secret to support Azure Application Password.

🏗️ Acceptance tests

  • Are there any feature flags that are required to use this functionality?
  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...
Screenshot 2024-11-25 at 4 57 41 PM

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@murali-partha murali-partha marked this pull request as ready for review November 25, 2024 16:40
@murali-partha murali-partha requested review from a team as code owners November 25, 2024 16:40
@@ -82,6 +94,7 @@ resource "hcp_vault_secrets_rotating_secret" "example_confluent" {
### Optional

- `aws_access_keys` (Attributes) AWS configuration to manage the access key rotation for the given IAM user. Required if `secret_provider` is `aws`. (see [below for nested schema](#nestedatt--aws_access_keys))
- `azure_application_password_params` (Attributes) Azure configuration to manage the application password rotation for the given application. Required if `secret_provider` is `confluent`. (see [below for nested schema](#nestedatt--azure_application_password_params))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `azure_application_password_params` (Attributes) Azure configuration to manage the application password rotation for the given application. Required if `secret_provider` is `confluent`. (see [below for nested schema](#nestedatt--azure_application_password_params))
- `azure_application_password` (Attributes) Azure configuration to manage the application password rotation for the given application. Required if `secret_provider` is `azure`. (see [below for nested schema](#nestedatt--azure_application_password_params))

The param suffix feels redundant and the other credential types did not add it. There is a Confluent copy-paste leftover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops I think this is the generated code. Nvm the suggestion but the comment are relevant to the resource schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing this. Updated.

@@ -206,6 +213,29 @@ func (r *resourceVaultSecretsRotatingSecret) Schema(_ context.Context, _ resourc
exactlyOneRotatingSecretTypeFieldsValidator,
},
},
"azure_application_password_params": schema.SingleNestedAttribute{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the params suffix to align with the other credential types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Changed it.

maxcoulombe
maxcoulombe previously approved these changes Dec 2, 2024
Copy link
Contributor

@maxcoulombe maxcoulombe left a comment

Choose a reason for hiding this comment

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

Looks good, couple of nits or copy-paste leftovers but the bulk of it looks solid.


### Required

- `capabilities` (Set of String) Capabilities enabled for the integration. See the Vault Secrets documentation for the list of supported capabilities per provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

as dicussed in a preivous PR, i didnt foudn this documentaiton

Copy link
Contributor Author

@murali-partha murali-partha Dec 4, 2024

Choose a reason for hiding this comment

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

This is a standard message across all integrations. AWS, GCP, Twilio

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think linking users to our docs' landing page would be helpful?

https://developer.hashicorp.com/hcp/docs/vault-secrets

Copy link
Contributor Author

@murali-partha murali-partha Dec 5, 2024

Choose a reason for hiding this comment

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

I could do that, the docs page doesn't directly talk about the capabilities option though. Another approach would be to list the options [ 'sync', 'rotation', 'dynamic'] in the same string. I am fine with adding either of these.

Note: This is a generated file, hence making this change will affect all the integration doc files where capabilities is listed.

page_title: "hcp_vault_secrets_integration_azure Resource - terraform-provider-hcp"
subcategory: ""
description: |-
The Vault Secrets Azure integration resource manages an Azure integration.
Copy link
Contributor

Choose a reason for hiding this comment

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

This feel incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the above comment, this is a standard message across integrations.


# hcp_vault_secrets_integration_azure (Resource)

The Vault Secrets Azure integration resource manages an Azure integration.
Copy link
Contributor

Choose a reason for hiding this comment

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

This feel incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@murali-partha murali-partha merged commit 00e266b into main Dec 5, 2024
5 of 6 checks passed
@murali-partha murali-partha deleted the azure-rotating-secrets branch December 5, 2024 17:28
@dhuckins dhuckins mentioned this pull request Dec 9, 2024
3 tasks
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.

3 participants