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

Update entity alias creation to use entity lookup api #1517

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Update entity alias creation to use entity lookup api #1517

merged 1 commit into from
Jul 25, 2022

Conversation

mbillow
Copy link
Contributor

@mbillow mbillow commented Jun 28, 2022

When creating an entity alias, the Vault Terraform Provider is currently listing all existing entities, making individual GET requests to those entities, and then inspecting the responses to make sure you aren’t making a duplicate alias.

Vault exposes a entity lookup API that we can leverage to accomplish this same duplication check in a single API call.

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

Release note for CHANGELOG:

- `resource/vault_identity_entity_alias`: Leverage entity lookup API to handle duplication pre-flight check instead of listing and retrieving all entities.

Output from acceptance testing:

terraform-provider-vault on  entity-alias-lookup [!] via 🐹 v1.17.6 
➜ TESTARGS="--run TestAccIdentityEntityAlias" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -v --run TestAccIdentityEntityAlias -timeout 30m ./...
?       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.845s [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/generated/datasources/transform/decode    1.665s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode    0.585s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet    1.870s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role        2.074s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template    1.213s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation      1.452s [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/helper    [no test files]
?       github.com/hashicorp/terraform-provider-vault/internal/consts   [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/internal/identity/entity  2.706s [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/internal/identity/group   [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/internal/provider 0.866s [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/schema    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/testutil  2.125s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/util      1.582s [no tests to run]
=== RUN   TestAccIdentityEntityAlias
--- PASS: TestAccIdentityEntityAlias (52.89s)
=== RUN   TestAccIdentityEntityAliasDuplicateFlow
--- PASS: TestAccIdentityEntityAliasDuplicateFlow (134.86s)
=== RUN   TestAccIdentityEntityAlias_Update
--- PASS: TestAccIdentityEntityAlias_Update (52.59s)
=== RUN   TestAccIdentityEntityAlias_Metadata
--- PASS: TestAccIdentityEntityAlias_Metadata (52.21s)
PASS
ok      github.com/hashicorp/terraform-provider-vault/vault     293.958s

mbillow
mbillow commented Jun 28, 2022
View reviewed changes
vault/resource_identity_entity_alias.go
mountAccessor := data["mount_accessor"].(string)
aliases, err := entity.FindAliases(client, &entity.FindAliasParams{
Copy link
Contributor Author

@mbillow mbillow Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The only other use of this function is vault/resource_identity_entity_alias_test.go#L191. I'm not sure if it is better to just replace that reference with a more specific helper function or just leave it as is.

The current implementation of entity.FindAliases is only really dangerous when running against a server with a large number of entities, which won't be the case when running the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, we will take a closer look after your PR is merged!

@benashz benashz added this to the 3.8.0 milestone Jul 19, 2022
@benashz benashz self-assigned this Jul 19, 2022
benashz
benashz approved these changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

Thank you for your contribution to HashiCorp!

vault/resource_identity_entity_alias.go
@@ -3,6 +3,7 @@ package vault
import (
"context"
"fmt"
"github.com/hashicorp/vault/api"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we prefer the that the built-in imports are always grouped together, we can update this after merge however.

@benashz benashz merged commit 8cb55c1 into hashicorp:main Jul 25, 2022
@benashz benashz mentioned this pull request Jul 26, 2022
Merged
marcboudreau pushed a commit to marcboudreau/terraform-provider-vault that referenced this pull request Nov 6, 2022
@mbillow
Update entity alias creation to use entity lookup api (hashicorp#1517)
b3c17cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benashz benashz benashz approved these changes

Assignees

@benashz benashz

Labels
size/S
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@mbillow @benashz