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

Add vault_quota_rate_limit resource #825

Merged
merged 7 commits into from
Sep 10, 2020
Merged

Add vault_quota_rate_limit resource #825

merged 7 commits into from
Sep 10, 2020

Conversation

lawliet89
Copy link
Contributor

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

Fixes #824

Release note for CHANGELOG:

add `vault_quota_rate_limit` resource to manage resource quota limit

Output from acceptance testing:

$ VAULT_ADDR='http://127.0.0.1:8200' VAULT_TOKEN=12345 make testacc TESTARGS='-run=TestQuotaRateLimit'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestQuotaRateLimit -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/generate    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/codegen (cached) [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/generated       [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/decode  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/encode  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/alphabet  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/role      (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/template  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/transformation    (cached) [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/schema  [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/util    (cached) [no tests to run]
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.42s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   (cached)

...

@lawliet89
Copy link
Contributor Author

Test failure is unrelated:

=== RUN   TestAccIdentityGroupMemberEntityIdsNonExclusive

--- FAIL: TestAccIdentityGroupMemberEntityIdsNonExclusive (0.48s)

    testing.go:640: Step 2 error: Check failed: unexpected member entity id 98b9b473-e3de-fd10-16f5-1cd8266bfa07 in group 8583f549-15c7-a891-6d1a-147c76fcec72

@kalafut kalafut requested a review from catsby August 11, 2020 03:47
catsby
catsby suggested changes Aug 25, 2020
View reviewed changes
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 good so far! I had some small requests and I just noticed the included test doesn't pass, at least for me, can you check it out?

[terraform-provider-vault][quota-rate-limit](4)$ VAULT_TOKEN=root make testacc TEST=./vault/... TESTARGS="-run=TestQuotaRateLimit -count=1"                                                                                                              ⏎ ✹ ✭
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./vault/... -v -run=TestQuotaRateLimit -count=1 -timeout 120m
=== RUN   TestQuotaRateLimit
    testing.go:701: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.

        Error: Check failed: Resource Quota Rate Limit tf-test-7029932848308157165 still exists

        State: <no state>
--- FAIL: TestQuotaRateLimit (0.23s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-vault/vault   0.498s
FAIL
make: *** [testacc] Error 1

website/docs/r/quota_rate_limit.md Outdated Show resolved Hide resolved
vault/resource_quota_rate_limit.go Outdated Show resolved Hide resolved
vault/resource_quota_rate_limit.go Outdated Show resolved Hide resolved
@catsby catsby added this to the Future milestone Aug 26, 2020
@lawliet89
Copy link
Contributor Author

@catsby Sorry for the silence. Let me look into your comments ASAP.

@ghost ghost added size/L and removed size/XL labels Sep 1, 2020
@lawliet89
Copy link
Contributor Author

lawliet89 commented Sep 1, 2020 •
edited
Loading

@catsby I have made the changes you suggested.

I cannot replicate your test failure. What version of Vault are you running the tests against? The test passes on CI and it's running against the latest tag and I am running against 1.5.3.

CI Test failure is due to a persistently flaky test.

@catsby
Copy link
Contributor

catsby commented Sep 2, 2020

Hey @lawliet89 - I've been running the tests with a Vault server compiled from the master branch of Vault, which could be part of my issues. I did try with 1.5.3 and got the test to pass, however I still got an error if I ran with count=2:

[terraform-provider-vault][quota-rate-limit](4)$ VAULT_TOKEN=root make testacc TEST=./vault/... TESTARGS="-run=TestQuota -count=2" 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./vault/... -v -run=TestQuota -count=2 -timeout 120m
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.21s)
=== RUN   TestQuotaRateLimit
    testing.go:640: Step 0 error: errors during apply:

        Error: Error creating Resource Rate Limit Quota tf-test-2918301973751561055: Error making API request.

        URL: PUT http://127.0.0.1:8200/v1/sys/quotas/rate-limit/tf-test-2918301973751561055
        Code: 400. Errors:

        * quota rule with similar properties exists under the name "tf-test-9053389438201676050"

          on /var/folders/9s/yrf0vw2j2xbfgktx9x16tk6c0000gp/T/tf-test001805183/main.tf line 2:
          (source code not available)


--- FAIL: TestQuotaRateLimit (0.06s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-vault/vault   0.547s
FAIL
make: *** [testacc] Error 1

Can you try running that command? I'm running Vault 1.5.3 with vault server -dev -dev-root-token-id=root

@lawliet89
Copy link
Contributor Author

lawliet89 commented Sep 3, 2020 •
edited
Loading

Vault doesn't like it when you have multiple quotas for the same path.

I can add another separate test to support running the tests in parallel for different secret mounts but I also do want to have a test for "". The test for "" cannot be run simultaneously it seems. What do you think?

@ghost ghost removed the waiting-response label Sep 3, 2020
@catsby
Copy link
Contributor

catsby commented Sep 3, 2020

Well, I was assuming that after the test completed the first time, all quotas would be removed before the second test ran. That would mean any previous quota at that path would have been deleted, so there shouldn't be a conflict. Am I misunderstanding something here?

@lawliet89
Copy link
Contributor Author

I modified the tests and made the attribute path be ForceNew. That seemed to have fixed the issues.

$ VAULT_ADDR='http://127.0.0.1:8200' VAULT_TOKEN=12345 make testacc TESTARGS="-run=TestQuota -count 10"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestQuota -count 10 -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/generate    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/codegen 0.296s [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/generated       [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/decode  0.825s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/encode  0.542s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/alphabet  0.484s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/role      0.549s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/template  0.265s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/transformation    0.419s [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/schema  [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/util    0.229s [no tests to run]
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.50s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.47s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.46s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.48s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.43s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.44s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.92s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.56s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.78s)
=== RUN   TestQuotaRateLimit
--- PASS: TestQuotaRateLimit (0.46s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   5.530s

@ghost ghost added size/XL and removed size/L labels Sep 4, 2020
catsby
catsby approved these changes Sep 10, 2020
View reviewed changes
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 thanks!

@catsby catsby merged commit 1b4dda4 into hashicorp:master Sep 10, 2020
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
@lawliet89
Add vault_quota_rate_limit resource (hashicorp#825)
7c7ece4 
* Add `vault_quota_rate_limit` resource

* Fix tab

* Fix documentation

* Remove unused functionemove unused function

* Add validation function

* Make `path` `ForceNew`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catsby catsby catsby approved these changes

Assignees
No one assigned
Labels
documentation new-resource size/XL
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.

[feature request] Support Resource Quota
2 participants
@lawliet89 @catsby