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

Don't overwrite vault_namespace path with response value. #396

Merged
merged 3 commits into from
May 7, 2019
Merged

Don't overwrite vault_namespace path with response value. #396

merged 3 commits into from
May 7, 2019

Conversation

williams-brian
Copy link
Contributor

@williams-brian williams-brian commented Apr 16, 2019

Overwriting the path with the one returned by Vault causes an invalid namespace to be set when using nested namespaces.
ex: namespace-b which is inside namespace-a will have the path overwritten with namespace-a/namespace-b/ when the correct value is namespace-b/

@ghost ghost added the size/XS label Apr 16, 2019
@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Apr 29, 2019
@tyrannosaurus-becks
Copy link
Contributor

Hi @williams-brian , thanks for working on this!

First question, would you be willing to add a test that duplicates the issue, and then shows that this code solves it?

Also, @petems , would you want to take a look at this one too?

@ghost ghost added size/M and removed size/XS labels May 1, 2019
@williams-brian
Copy link
Contributor Author

@tyrannosaurus-becks I have attempted to add a test case which demonstrates the issue, but I am not sure how to run the tests with Vault enterprise, so I haven't been able to verify that it works correctly.

I hope my explanation of the issue is clear. After running terraform apply the namespace path that appears in the Terraform state is different from the one which is set on the terraform resource. Subsequent runs of terraform apply then fail. My test case attempts to check that the path inside the terraform state is the same as what is on the "vault_namespace" resource.

@ghost ghost added size/S and removed size/M labels May 1, 2019
Overwriting the path with the one returned by Vault causes an invalid namespace to be set when using nested namespaces.
ex: namespace-b which is inside namespace-a will have the path overwritten with namespace-a/namespace-b/ when the correct value is namespace-b/
@williams-brian
Copy link
Contributor Author

williams-brian commented May 2, 2019

@tyrannosaurus-becks, I figured out how to run Vault Enterprise locally and use that for acceptance tests. The new test case should properly illustrate the issue.

Running the test before this change:

$ go test -v github.com/terraform-providers/terraform-provider-vault/vault -run TestNamespace_basic
=== RUN   TestNamespace_basic
--- FAIL: TestNamespace_basic (0.06s)
    testing.go:538: Step 2 error: Check failed: expected path to be child-namespace-5645055735786586850, got test-namespace-7756255336919402260/child-namespace-5645055735786586850
    testing.go:599: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Error refreshing: 1 error(s) occurred:
        
        * vault_namespace.test_child: 1 error(s) occurred:
        
        * vault_namespace.test_child: vault_namespace.test_child: error reading from Vault: Error making API request.
        
        URL: GET http://localhost:8200/v1/sys/namespaces/test-namespace-7756255336919402260/child-namespace-5645055735786586850
        Code: 400. Errors:
        
        * "/" is not allowed in namespace names
        
        State: <nil>
FAIL
FAIL	github.com/terraform-providers/terraform-provider-vault/vault	0.082s

Running the test after this change:

$ go test -v github.com/terraform-providers/terraform-provider-vault/vault -run TestNamespace_basic
=== RUN   TestNamespace_basic
--- PASS: TestNamespace_basic (0.08s)
PASS
ok  	github.com/terraform-providers/terraform-provider-vault/vault	0.098s

@petems
Copy link
Contributor

petems commented May 2, 2019

Good catch, I forgot to remove the logic around removing slashes after we added the validation function, which would break nested namespaces.

This solution makes sense to me! 😄

@williams-brian williams-brian changed the title Don't overwrite path with response value. Don't overwrite vault_namespace path with response value. May 2, 2019
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

@williams-brian wonderful! Thank you!

@tyrannosaurus-becks tyrannosaurus-becks merged commit e362b97 into hashicorp:master May 7, 2019
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Don't overwrite vault_namespace path with response value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants