Add challenge and js_challenge rate-limit modes #172

jdarley · 2018-11-30T13:28:30Z

Minor update to add challenge and js_challenge support to cloudflare_rate_limit resource.

This has been manually tested and I can confirm it will create a rate limit with a challenge or js_challenge action as expected.

This PR related to issue 171

jacobbednarz · 2018-12-02T21:41:58Z

cloudflare/resource_cloudflare_rate_limit.go

 						},

 						"timeout": {
 							Type:         schema.TypeInt,
-							Required:     true,


As this is now optional based on the mode, we probably want to add a check in the Create/Update that if the mode is simulate or ban, we check the value is defined to prevent people scratching their heads with unintended behaviour.

I completely agree. The Cloudflare API does give reasonable error messages around this though it should really be caught by the provider before the API is called.

I'm just working on it at the moment, though this is the first time I've written any Go so you may have to forgive my non-idiomatic code.

After grappling with attempting to insert validation into expandRateLimitAction, I've realised that this won't behave quite as expected. I've attempted to implement a ValidateFunc into the action section of the schema itself (where I'll have access to both mode and timeout), but it appears that this is not supported on lists or sets at this point in time.

I'd appreciate any pointers as to how to proceed with this.

... but it appears that this is not supported on lists or sets at this point in time.

Yep, the ValidateFunc is only supported on primitive types which throws it out a bit.

I'd appreciate any pointers as to how to proceed with this.

Sure, I think this might do the job. You were on the right track with using expandRateLimitAction as it's used in both the Create and Update. I haven't thoroughly vetted this though.

diff --git a/cloudflare/resource_cloudflare_rate_limit.go b/cloudflare/resource_cloudflare_rate_limit.go index e67f1a4..dd0f170 100644 --- a/cloudflare/resource_cloudflare_rate_limit.go +++ b/cloudflare/resource_cloudflare_rate_limit.go @@ -197,11 +197,15 @@ func resourceCloudflareRateLimit() *schema.Resource { func resourceCloudflareRateLimitCreate(d *schema.ResourceData, meta interface{}) error { client := meta.(*cloudflare.API) + rateLimitAction, err := expandRateLimitAction(d) + if err != nil { + return errors.Wrap(err, "error expanding rate limit action") + } newRateLimit := cloudflare.RateLimit{ Threshold: d.Get("threshold").(int), Period: d.Get("period").(int), - Action: expandRateLimitAction(d), + Action: rateLimitAction, } newRateLimitMatch, err := expandRateLimitTrafficMatcher(d) @@ -255,11 +259,15 @@ func resourceCloudflareRateLimitUpdate(d *schema.ResourceData, meta interface{}) client := meta.(*cloudflare.API) zoneId := d.Get("zone_id").(string) rateLimitId := d.Id() + rateLimitAction, err := expandRateLimitAction(d) + if err != nil { + return errors.Wrap(err, "error expanding rate limit action") + } updatedRateLimit := cloudflare.RateLimit{ Threshold: d.Get("threshold").(int), Period: d.Get("period").(int), - Action: expandRateLimitAction(d), + Action: rateLimitAction, } newRateLimitMatch, err := expandRateLimitTrafficMatcher(d) @@ -341,13 +349,21 @@ func expandRateLimitTrafficMatcher(d *schema.ResourceData) (matcher cloudflare.R return } -func expandRateLimitAction(d *schema.ResourceData) cloudflare.RateLimitAction { +func expandRateLimitAction(d *schema.ResourceData) (cloudflare.RateLimitAction, error) { // dont need to guard for array length because MinItems is set **and** action is required tfAction := d.Get("action").([]interface{})[0].(map[string]interface{}) + mode := tfAction["mode"].(string) + timeout := tfAction["timeout"].(int) + + if mode == "simulate" || mode == "ban" { + if timeout == 0 { + return cloudflare.RateLimitAction{}, fmt.Errorf("rate limit timeout must be set if the 'mode' is simulate or ban") + } + } action := cloudflare.RateLimitAction{ - Mode: tfAction["mode"].(string), - Timeout: tfAction["timeout"].(int), + Mode: mode, + Timeout: timeout, } if _, ok := tfAction["response"]; ok && len(tfAction["response"].([]interface{})) > 0 { @@ -359,7 +375,7 @@ func expandRateLimitAction(d *schema.ResourceData) cloudflare.RateLimitAction { Body: tfActionResponse["body"].(string), } } - return action + return action, nil } func expandRateLimitCorrelate(d *schema.ResourceData) (correlate cloudflare.RateLimitCorrelate, err error) {

Do you want to try this and let me know how you go?

That's absolutely brilliant, thanks for your help. I'd used fmt.Errorf to raise errors, but hadn't wrapped them with errors.Wrap. This now raises errors correctly.

I expanded the logic to throw an error when timeout values are provided for challenge and js_challenge modes as the Cloudflare API won't accept them.

jdarley · 2018-12-07T10:36:08Z

The previous commit was just my current work in progress, unfortunately it didn't error during the plan or the apply phases and let the underlying error form the API bubble out.

I'm just in the process of applying the diff above and testing it.

… to challenge and js_challenge modes

jacobbednarz · 2018-12-20T23:54:19Z

@jdarley is this ready for another review?

jdarley · 2018-12-21T15:06:49Z

I think it is @jacobbednarz. I've tested it manually by attempting to create challenge responses with a timeout and block/simulate without a timeout.

I've introduced a couple of acceptance tests which I think will test this behaviour, though I've not yet sorted an environment to run the full suite of acceptance tests against.

jacobbednarz · 2018-12-23T22:39:16Z

cloudflare/resource_cloudflare_rate_limit_test.go

+  period = 1
+  action {
+    mode = "challenge"
+    timeoute = 60


Suggested change

timeoute = 60

timeout = 60

jacobbednarz · 2018-12-23T22:44:12Z

I know they weren't all your changes however the period attribute is too low for a standard enterprise account and throws an entitlement error in the tests. Let's set them to 10 as that gets all the tests passing. Here is a patch to make this faster for you.

diff --git a/cloudflare/resource_cloudflare_rate_limit_test.go b/cloudflare/resource_cloudflare_rate_limit_test.go
index 1a1ecbcad21e86c058420a5c9fdfb355064838b9..203142bb41028dc934a9501e265b88a1dd8a5d40 100644
--- a/cloudflare/resource_cloudflare_rate_limit_test.go
+++ b/cloudflare/resource_cloudflare_rate_limit_test.go
@@ -326,7 +326,7 @@ func testAccCheckCloudflareRateLimitConfigBasic(zone, id string) string {
 resource "cloudflare_rate_limit" "%[1]s" {
   zone = "%[2]s"
   threshold = 1000
-  period = 1
+  period = 10
   action {
     mode = "simulate"
     timeout = 86400
@@ -339,7 +339,7 @@ func testAccCheckCloudflareRateLimitConfigMatchingUrl(zone, id string) string {
 resource "cloudflare_rate_limit" "%[1]s" {
   zone = "%[2]s"
   threshold = 1000
-  period = 1
+  period = 10
   match {
     request {
       url_pattern = "%[2]s/tfacc-url-%[1]s"
@@ -357,7 +357,7 @@ func testAccCheckCloudflareRateLimitConfigFullySpecified(zone, id string) string
 resource "cloudflare_rate_limit" "%[1]s" {
   zone = "%[2]s"
   threshold = 2000
-  period = 2
+  period = 10
   match {
     request {
       url_pattern = "%[2]s/tfacc-full-%[1]s"
@@ -391,7 +391,7 @@ func testAccCheckCloudflareRateLimitChallengeConfigBasic(zone, id string) string
 resource "cloudflare_rate_limit" "%[1]s" {
   zone = "%[2]s"
   threshold = 1000
-  period = 1
+  period = 10
   action {
     mode = "challenge"
   }
@@ -403,7 +403,7 @@ func testAccCheckCloudflareRateLimitConfigWithoutTimeout(zone, id string) string
 resource "cloudflare_rate_limit" "%[1]s" {
   zone = "%[2]s"
   threshold = 1000
-  period = 1
+  period = 10
   action {
     mode = "simulate"
   }
@@ -415,7 +415,7 @@ func testAccCheckCloudflareRateLimitChallengeConfigWithTimeout(zone, id string)
 resource "cloudflare_rate_limit" "%[1]s" {
   zone = "%[2]s"
   threshold = 1000
-  period = 1
+  period = 10
   action {
     mode = "challenge"
     timeoute = 60

jacobbednarz · 2018-12-23T22:46:52Z

Very close @jdarley! Once we address the timeoute typo and the period changes, we're good to go! Here are the integration tests all passing with those two changes.

$ CLOUDFLARE_DOMAIN="$CF_DOMAIN" \
  CLOUDFLARE_EMAIL="$CF_EMAIL" \
  CLOUDFLARE_TOKEN="$CF_API_TOKEN" \
  TF_ACC=1 \
  go test -v github.com/terraform-providers/terraform-provider-cloudflare/cloudflare -run "TestAccCloudflareRateLimit_*" -timeout 120m

=== RUN   TestAccCloudflareRateLimit_Import
=== PAUSE TestAccCloudflareRateLimit_Import
=== RUN   TestAccCloudflareRateLimit_Basic
=== PAUSE TestAccCloudflareRateLimit_Basic
=== RUN   TestAccCloudflareRateLimitChallenge_Basic
=== PAUSE TestAccCloudflareRateLimitChallenge_Basic
=== RUN   TestAccCloudflareRateLimit_FullySpecified
=== PAUSE TestAccCloudflareRateLimit_FullySpecified
=== RUN   TestAccCloudflareRateLimit_Update
=== PAUSE TestAccCloudflareRateLimit_Update
=== RUN   TestAccCloudflareRateLimit_CreateAfterManualDestroy
=== PAUSE TestAccCloudflareRateLimit_CreateAfterManualDestroy
=== RUN   TestAccCloudflareRateLimit_WithoutTimeout
=== PAUSE TestAccCloudflareRateLimit_WithoutTimeout
=== RUN   TestAccCloudflareRateLimit_ChallengeWithTimeout
=== PAUSE TestAccCloudflareRateLimit_ChallengeWithTimeout
=== CONT  TestAccCloudflareRateLimit_Import
=== CONT  TestAccCloudflareRateLimit_WithoutTimeout
=== CONT  TestAccCloudflareRateLimit_CreateAfterManualDestroy
=== CONT  TestAccCloudflareRateLimit_FullySpecified
--- PASS: TestAccCloudflareRateLimit_WithoutTimeout (0.02s)
=== CONT  TestAccCloudflareRateLimit_Update
--- PASS: TestAccCloudflareRateLimit_FullySpecified (3.30s)
=== CONT  TestAccCloudflareRateLimitChallenge_Basic
--- PASS: TestAccCloudflareRateLimit_Import (3.58s)
=== CONT  TestAccCloudflareRateLimit_Basic
--- PASS: TestAccCloudflareRateLimit_Update (4.78s)
=== CONT  TestAccCloudflareRateLimit_ChallengeWithTimeout
--- PASS: TestAccCloudflareRateLimit_ChallengeWithTimeout (0.01s)
--- PASS: TestAccCloudflareRateLimitChallenge_Basic (2.93s)
--- PASS: TestAccCloudflareRateLimit_Basic (2.92s)
--- PASS: TestAccCloudflareRateLimit_CreateAfterManualDestroy (6.84s)
PASS
ok  	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	6.865s

jdarley · 2019-01-06T19:28:27Z

Apologies for not getting this sorted sooner @jacobbednarz - I've had a couple of weeks completely away from the screen. Hopefully this last commit will get it over the line!

jacobbednarz · 2019-01-06T20:51:04Z

No sweat @jdarley. Glad to hear you're taking time away from the screens 😄

jacobbednarz

LGTM 🚢

patryk · 2019-01-07T15:15:36Z

Thanks @jdarley!

Add challenge and js_challenge rate-limit modes

77b98a3

ghost added the size/S label Nov 30, 2018

jdarley mentioned this pull request Nov 30, 2018

challenge and js_challenge support for cloudflare_rate_limit #171

Closed

jacobbednarz reviewed Dec 2, 2018

View reviewed changes

Attempt to raise errors on incorrectly defined ratelimit

5aa27db

ghost added kind/documentation Categorizes issue or PR as related to documentation. size/M and removed size/S labels Dec 7, 2018

James Darley added 4 commits December 7, 2018 11:05

Apply @jacobbednarz's patch, modified to stop timeouts being provided…

c6f5e0a

… to challenge and js_challenge modes

Simplify if statement

9b635c5

Remove rogue ValidateAction

5e44796

Add acceptance tests

0b222ec

ghost added size/L and removed size/M labels Dec 7, 2018

James Darley added 2 commits December 10, 2018 14:24

Fix formatting

11eb7ea

Remove errant log message

9890792

jacobbednarz reviewed Dec 23, 2018

View reviewed changes

Address short timeout periods and mispelling of 'timeout'

e2f16c4

jacobbednarz approved these changes Jan 6, 2019

View reviewed changes

patryk merged commit 340950a into cloudflare:master Jan 7, 2019

matejvelikonja mentioned this pull request Jan 24, 2019

Optional timeout for cloudflare_rate_limit action.timeout VladRassokhin/intellij-hcl#189

Closed

jamfish mentioned this pull request Apr 22, 2019

Update Rate Limit Docs #292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add challenge and js_challenge rate-limit modes #172

Add challenge and js_challenge rate-limit modes #172

jdarley commented Nov 30, 2018 •

edited

Loading

jacobbednarz Dec 2, 2018

jdarley Dec 6, 2018

jdarley Dec 6, 2018

jacobbednarz Dec 6, 2018

jdarley Dec 7, 2018

jdarley commented Dec 7, 2018

jacobbednarz commented Dec 20, 2018

jdarley commented Dec 21, 2018

jacobbednarz Dec 23, 2018

jacobbednarz commented Dec 23, 2018

jacobbednarz commented Dec 23, 2018

jdarley commented Jan 6, 2019

jacobbednarz commented Jan 6, 2019

jacobbednarz left a comment

patryk commented Jan 7, 2019

Add challenge and js_challenge rate-limit modes #172

Add challenge and js_challenge rate-limit modes #172

Conversation

jdarley commented Nov 30, 2018 • edited Loading

jacobbednarz Dec 2, 2018

Choose a reason for hiding this comment

jdarley Dec 6, 2018

Choose a reason for hiding this comment

jdarley Dec 6, 2018

Choose a reason for hiding this comment

jacobbednarz Dec 6, 2018

Choose a reason for hiding this comment

jdarley Dec 7, 2018

Choose a reason for hiding this comment

jdarley commented Dec 7, 2018

jacobbednarz commented Dec 20, 2018

jdarley commented Dec 21, 2018

jacobbednarz Dec 23, 2018

Choose a reason for hiding this comment

jacobbednarz commented Dec 23, 2018

jacobbednarz commented Dec 23, 2018

jdarley commented Jan 6, 2019

jacobbednarz commented Jan 6, 2019

jacobbednarz left a comment

Choose a reason for hiding this comment

patryk commented Jan 7, 2019

jdarley commented Nov 30, 2018 •

edited

Loading