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

vault secrets | rotated secrets data source #854

Merged
merged 24 commits into from
Jun 5, 2024

Conversation

dhuckins
Copy link
Contributor

@dhuckins dhuckins commented Jun 3, 2024

🛠️ Description

🏗️ 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'

...

@dhuckins dhuckins changed the base branch from main to dh/vault-secrets/rotated-secrets2 June 3, 2024 15:22
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

resp.Diagnostics.Append(diag...)

// TODO: what is ID supposed to be?
// data.ID = ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ID actually used for unique identification of data sources or by anything TF-provider-specific? If not, maybe one option is to just remove it altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im honestly not sure, would like some feedback from @hashicorp/cloud-foundations
otherwise maybe something like
project/<project id>/app/<app name>/secret/<secret name>
wdyt?

Copy link
Contributor

@averche averche Jun 3, 2024

Choose a reason for hiding this comment

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

If our goal here is a unique resource identifier, this will almost work. The only scenario where I see it failing is if you delete the secret and re-create it with the same name. However, this is much better than the AppName that we are using elsewhere.

My preferred solution would still be to remove it if possible.

@dhuckins dhuckins marked this pull request as ready for review June 3, 2024 20:43
@dhuckins dhuckins requested review from a team as code owners June 3, 2024 20:43
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

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

I think we'll want to change these once the schema version is bumped to latest

WithProjectID(loc.ProjectID).
WithBody(body)

resp, err := client.VaultSecretsPreview.CreateMongoDBAtlasRotationIntegration(params, nil)
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
resp, err := client.VaultSecretsPreview.CreateMongoDBAtlasRotationIntegration(params, nil)
resp, err := client.VaultSecretsPreview.CreateMongoDBAtlasIntegration(params, nil)

WithProjectID(loc.ProjectID).
WithIntegrationName(integrationName)

_, err := client.VaultSecretsPreview.DeleteMongoDBAtlasRotationIntegration(params, nil)
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
_, err := client.VaultSecretsPreview.DeleteMongoDBAtlasRotationIntegration(params, nil)
_, err := client.VaultSecretsPreview.DeleteMongoDBAtlasIntegration(params, nil)

MongodbAPIPublicKey: mongodbAtlasPublicKey,
MongodbAPIPrivateKey: mongodbAtlasPrivateKey,
}
params := secret_service.NewCreateMongoDBAtlasRotationIntegrationParamsWithContext(ctx).
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
params := secret_service.NewCreateMongoDBAtlasRotationIntegrationParamsWithContext(ctx).
params := secret_service.NewCreateMongoDBAtlasIntegrationParamsWithContext(ctx).


// DeleteMongoDBAtlasRotationIntegration NOTE: currently just needed for tests
func DeleteMongoDBAtlasRotationIntegration(ctx context.Context, client *Client, loc *sharedmodels.HashicorpCloudLocationLocation, integrationName string) error {
params := secret_service.NewDeleteMongoDBAtlasRotationIntegrationParamsWithContext(ctx).
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
params := secret_service.NewDeleteMongoDBAtlasRotationIntegrationParamsWithContext(ctx).
params := secret_service.NewDeleteMongoDBAtlasIntegrationParamsWithContext(ctx).

Base automatically changed from dh/vault-secrets/rotated-secrets2 to main June 4, 2024 18:33
@dhuckins dhuckins requested a review from a team as a code owner June 4, 2024 18:33
@dhuckins dhuckins removed the request for review from a team June 4, 2024 18:33
…to dh/vault-secrets/rotated-secrets-data-source

// block until the secret is done
// TODO: is the time amount excessive?
timer := time.AfterFunc(20*time.Minute, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: timeout might be a better name

t.Log("secret successfully rotated")
return
default:
time.Sleep(time.Minute)
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
time.Sleep(time.Minute)
time.Sleep(10*time.Second)

@dhuckins dhuckins merged commit e0b3611 into main Jun 5, 2024
6 checks passed
@dhuckins dhuckins deleted the dh/vault-secrets/rotated-secrets-data-source branch June 5, 2024 15:23
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