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

New Data Source: azurerm_key_vault_secret_versions #27393

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

New Data Source: azurerm_key_vault_secret_versions #27393

wants to merge 5 commits into from
+366 −1

Conversation

ned1313
Copy link
Contributor

@ned1313 ned1313 commented Sep 13, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This is a new data source that will retrieve the versions of an existing Key Vault Secret. The returned value is called versions and is a list of each version and its attributes. It does not include the secret value for each version. The number of returned results can be set with the max_results argument that defaults to 25.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)
$ make acctests SERVICE='keyvault' TESTARGS='-run=TestAccDataSourceKeyVaultSecretVersions_complete' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/keyvault -run=TestAccDataSourceKeyVaultSecretVersions_complete -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceKeyVaultSecretVersions_complete
=== PAUSE TestAccDataSourceKeyVaultSecretVersions_complete
=== CONT  TestAccDataSourceKeyVaultSecretVersions_complete
--- PASS: TestAccDataSourceKeyVaultSecretVersions_complete (824.35s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault      824.387s

$ make acctests SERVICE='keyvault' TESTARGS='-run=TestAccDataSourceKeyVaultSecretVersions_basic' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/keyvault -run=TestAccDataSourceKeyVaultSecretVersions_basic -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceKeyVaultSecretVersions_basic
=== PAUSE TestAccDataSourceKeyVaultSecretVersions_basic
=== CONT  TestAccDataSourceKeyVaultSecretVersions_basic
--- PASS: TestAccDataSourceKeyVaultSecretVersions_basic (826.40s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault      826.415s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • azurerm_key_vault_secret_versions - new data source to retrieve versions of a secret [GH-00000]

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #27347

@katbyte katbyte requested review from katbyte and a team as code owners November 14, 2024 00:08
@stephybun stephybun self-assigned this Jan 9, 2025
@rcskosir
Copy link
Contributor

Thank you so much for taking the time to put this together! We really appreciate your contribution.
Apologies for the delay in reviewing it. The topic you've raised led to a discussion around the max_results argument defaulting to 25. Rest assured, we'll review this as soon as we can.
Thanks again for your patience, and we'll keep you updated on the progress!

stephybun
stephybun requested changes Jan 14, 2025
View reviewed changes
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @ned1313! I left a few suggestions in-line, mind taking a look?

internal/services/keyvault/key_vault_secret_versions_data_source.go
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-provider-azurerm/internal/sdk"
keyVaultValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault/validate"
"github.com/tombuildsstuff/kermit/sdk/keyvault/7.4/keyvault"
Copy link
Member

Choose a reason for hiding this comment

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

We should reference kermit from jackofallops

Suggested change
"github.com/tombuildsstuff/kermit/sdk/keyvault/7.4/keyvault"
"github.com/jackofallops/kermit/sdk/keyvault/7.4/keyvault"

internal/services/keyvault/key_vault_secret_versions_data_source.go
func (r KeyVaultSecretVersionsDataSource) Read() sdk.ResourceFunc {
return sdk.ResourceFunc{
Timeout: 5 * time.Minute,

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

internal/services/keyvault/key_vault_secret_versions_data_source.go
Comment on lines +140 to +142
maxResults32 := int32(model.MaxResults)

resp, err := client.GetSecretVersions(ctx, *keyVaultUri, model.Name, &maxResults32)
Copy link
Member

Choose a reason for hiding this comment

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

Since this variable is only referenced once we would prefer to use the pointer.To function to pass the casted results directly into the function call

Suggested change
maxResults32 := int32(model.MaxResults)
resp, err := client.GetSecretVersions(ctx, *keyVaultUri, model.Name, &maxResults32)
resp, err := client.GetSecretVersions(ctx, *keyVaultUri, model.Name, pointer.To(int32(model.MaxResults)))

internal/services/keyvault/key_vault_secret_versions_data_source.go
}
}

metadata.ResourceData.SetId(fmt.Sprintf("%s/%s", model.KeyVaultId, model.Name))
Copy link
Member

Choose a reason for hiding this comment

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

We can generate the ID using existing functions in the keyvault package, we can then use the SetID method on metadata

Suggested change
metadata.ResourceData.SetId(fmt.Sprintf("%s/%s", model.KeyVaultId, model.Name))
id := keyVaultParse.NewSecretVersionlessID(keyVaultId.SubscriptionId, keyVaultId.ResourceGroupName, keyVaultId.VaultName, model.Name)
metadata.SetID(id)

internal/services/keyvault/key_vault_secret_versions_data_source.go
var item secretVersionModel
item.Uri = *v.ID
item.ID = (*v.ID)[strings.LastIndex(*v.ID, "/")+1:]
item.CreatedDate = time.Time(*v.Attributes.Created).Format(time.RFC3339)
Copy link
Member

Choose a reason for hiding this comment

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

We're nil checking the other dates/times below before casting, I think we should do that here as well

Suggested change
item.CreatedDate = time.Time(*v.Attributes.Created).Format(time.RFC3339)
createdDate := v.Attributes.Created; createdDate != nil {
item.CreatedDate = time.Time(*createdDate).Format(time.RFC3339)
}

internal/services/keyvault/key_vault_secret_versions_data_source.go

func expandSecretVersion(v *keyvault.SecretItem) secretVersionModel {
var item secretVersionModel
item.Uri = *v.ID
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand it's highly unlikely for keyvault that these fields are not returned or empty, but we can still use pointer.From to do a nil check on this for us to prevent any potential crashes

Suggested change
item.Uri = *v.ID
item.Uri = pointer.From(v.ID)

internal/services/keyvault/key_vault_secret_versions_data_source.go
item.Uri = *v.ID
item.ID = (*v.ID)[strings.LastIndex(*v.ID, "/")+1:]
item.CreatedDate = time.Time(*v.Attributes.Created).Format(time.RFC3339)
item.Enabled = *v.Attributes.Enabled
Copy link
Member

Choose a reason for hiding this comment

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

Same for this

Suggested change
item.Enabled = *v.Attributes.Enabled
item.Enabled = pointer.From(v.Attributes.Enabled)

internal/services/keyvault/key_vault_secret_versions_data_source.go
func expandSecretVersion(v *keyvault.SecretItem) secretVersionModel {
var item secretVersionModel
item.Uri = *v.ID
item.ID = (*v.ID)[strings.LastIndex(*v.ID, "/")+1:]
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be good to only do this transformation if v.ID is not nil or an empty string to prevent a potential panic

website/docs/d/key_vault_secret_versions.html.markdown

# Data Source: azurerm_key_vault_secret_versions

Use this data source to access information about an existing Key Vault Secret's versions. The secret version values is not included. The `key_vault_secret` data source can be used to retrieve the value of a given secret version using it's `id`.
Copy link
Member

Choose a reason for hiding this comment

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

With the introduction of ephemeral resources we might want to point users towards the ephemeral resource instead of the data source

Suggested change
Use this data source to access information about an existing Key Vault Secret's versions. The secret version values is not included. The `key_vault_secret` data source can be used to retrieve the value of a given secret version using it's `id`.
Use this data source to access information about an existing Key Vault Secret's versions. The secret version values are not included. The `azurerm_key_vault_secret` ephemeral resource can be used to retrieve the value of a given secret version using it's `id` without storing the information in state. Alternatively the `azurerm_key_vault_secret` data source can be used to retrieve this information, but will store this information in state.

website/docs/d/key_vault_secret_versions.html.markdown
Comment on lines +37 to +41
* `name` - (Required) The name of the Key Vault Secret to retrieve versions from.

---

* `max_results` - (Optional) Maximum number of versions to retrieve. Defaults to `25`.
Copy link
Member

Choose a reason for hiding this comment

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

A space here is sufficient

Suggested change
* `name` - (Required) The name of the Key Vault Secret to retrieve versions from.
---
* `max_results` - (Optional) Maximum number of versions to retrieve. Defaults to `25`.
* `name` - (Required) The name of the Key Vault Secret to retrieve versions from.
* `max_results` - (Optional) Maximum number of versions to retrieve. Defaults to `25`.

@markti
Copy link
Contributor

markti commented Jan 21, 2025

Do we also have corresponding / related PRs / Issues open for keyvault secret version resource?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephybun stephybun stephybun requested changes

@katbyte katbyte Awaiting requested review from katbyte

Requested changes must be addressed to merge this pull request.

Assignees

@stephybun stephybun

Labels
documentation new-data-source service/key-vault Key Vault size/XL waiting-response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for enumerating all versions of a keyvault secret
4 participants
@ned1313 @rcskosir @markti @stephybun