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

Add Terraform Cloud/Enterprise credential support #959

Merged
merged 14 commits into from
Mar 17, 2021
Merged

Conversation

Valarissa
Copy link
Contributor

This PR seeks to introduce support for the upcoming Terraform Cloud Secrets Engine. It will enable users to create and manage the Terraform Secrets engine config, any associated roles, and generate credentials from those roles.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Release note for CHANGELOG:

Adding support for Terraform Cloud/Enterprise token generation for users, teams, and orgs.

Output from acceptance testing:

/usr/local/Cellar/go/1.15.5/libexec/bin/go test -json ./... -run .*TerraformCloud.*
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/codegen	0.203s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/generated	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/util	0.488s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation	0.445s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role	0.691s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/decode	0.951s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet	1.198s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode	1.427s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template	1.628s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
=== RUN   TestAccDataSourceTerraformCloudAccessCredentialsOrganizationClientBasic
--- PASS: TestAccDataSourceTerraformCloudAccessCredentialsOrganizationClientBasic (1.48s)
=== RUN   TestAccDataSourceTerraformCloudAccessCredentialsTeamClientBasic
--- PASS: TestAccDataSourceTerraformCloudAccessCredentialsTeamClientBasic (1.42s)
=== RUN   TestAccDataSourceTerraformCloudAccessCredentialsUserBasic
--- PASS: TestAccDataSourceTerraformCloudAccessCredentialsUserBasic (2.79s)
=== RUN   TestTerraformCloudSecretBackend
--- PASS: TestTerraformCloudSecretBackend (0.46s)
=== RUN   TestTerraformCloudSecretRole
--- PASS: TestTerraformCloudSecretRole (2.02s)
=== RUN   TestTerraformCloudSecretBackendRoleNameFromPath
--- PASS: TestTerraformCloudSecretBackendRoleNameFromPath (0.00s)
=== RUN   TestTerraformCloudSecretBackendRoleBackendFromPath
--- PASS: TestTerraformCloudSecretBackendRoleBackendFromPath (0.00s)
PASS
ok  	github.com/hashicorp/terraform-provider-vault/vault	10.017s

Process finished with exit code 0

Lauren Voswinkel added 2 commits January 27, 2021 16:55
@Valarissa
Copy link
Contributor Author

Tests will continue to fail until tf secrets is bundled with vault, or I create a setup script for adding the plugin to vault.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Looks great so far! Some minor nits that may or may not be accurate, depending on how good my Terraform knowledge has kept up.

The comment on data source and resource will be had internally, I'll bring it up.

The other thing is documentation missing, will that be opened/add separately?

vault/data_source_terraform_cloud_credentials.go Outdated Show resolved Hide resolved
vault/data_source_terraform_cloud_credentials.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_role.go Outdated Show resolved Hide resolved
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

LGTM 👍 (data source -> resource discussion goes beyond this PR so we'll leave it as is)

vault/provider.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_role.go Show resolved Hide resolved
vault/resource_terraform_cloud_secret_role.go Show resolved Hide resolved
vault/resource_terraform_cloud_secret_role.go Show resolved Hide resolved
@husunal
Copy link
Contributor

husunal commented Feb 24, 2021

Great work thanks! We look forward to using this new TFE secret backend.

What happens to the Terraform resource when the generated token's TTL expires? Also, I didn't see any TTL argument in resource_terraform_cloud_secret_creds.go are you going to add this option as well so we can specify its TTL during secret creation.

@Valarissa
Copy link
Contributor Author

@husunal the creds' TTL should be based on the role used to create the creds.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

The secret resource cred needs to look up the current secret by lease ID to verify the token still exists, otherwise we generate a token with every read. I believe that for Team/Org tokens we wont have a lease ID so we can go ahead and do the full read again.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

We're getting close, but the handling of the lease look up across orgs/teams and users is not quite right. Let me know if you have any questions here or if I need to clarify anything

vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

A few more minor edits and I think we'll be good to go here. Please let me know when you have a chance to look!

vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
vault/resource_terraform_cloud_secret_creds.go Outdated Show resolved Hide resolved
@Valarissa Valarissa requested review from catsby and calvn March 16, 2021 20:32
organization := d.Get("organization")
teamId := d.Get("team_id")

if organization == "" && teamId == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also check for leaseId != "" now that it's an attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, fixed in latest commit.

organization := d.Get("organization")
teamId := d.Get("team_id")

if organization == "" && teamId == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, you can now lease perform the lookup if leaseId != "" (maybe even get rid of organization == "" && teamId == "" since it doesn't really depend on that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest commit.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

👍

return &schema.Resource{
Create: createTerraformCloudSecretCredsResource,
Read: readTerraformCloudSecretCredsResource,
Update: updateTerraformCloudSecretCredsResource,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in hindsight and looking at the updateTerraformCloudSecretCredsResource (which just destroys then recreates) I think we can just omit this and remove the updateTerraformCloudSecretCredsResource method completely. To my general knowledge and specifically here, I don't think we can update credentials, e.g. we can't do renewals through this resource (or in any other comparable credential resources, for that matter)

…FVP support. (#974)

* Add docs to accompany the release of the Terraform Cloud/Enterprise TFVP support.

* Add reference to lease_id for user tokens
@ghost ghost added the documentation label Mar 17, 2021
@Valarissa Valarissa merged commit 1f3a8d9 into master Mar 17, 2021
@fairclothjm fairclothjm deleted the add_tfc/e_support branch February 7, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants