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

Add challenge and js_challenge rate-limit modes #172

Merged
merged 9 commits into from
Jan 7, 2019
Merged

Add challenge and js_challenge rate-limit modes #172

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
77b98a3
Add challenge and js_challenge rate-limit modes
Nov 30, 2018
5aa27db
Attempt to raise errors on incorrectly defined ratelimit
Dec 7, 2018
c6f5e0a
Apply @jacobbednarz's patch, modified to stop timeouts being provided…
Dec 7, 2018
9b635c5
Simplify if statement
Dec 7, 2018
5e44796
Remove rogue ValidateAction
Dec 7, 2018
0b222ec
Add acceptance tests
Dec 7, 2018
11eb7ea
Fix formatting
Dec 10, 2018
9890792
Remove errant log message
Dec 11, 2018
e2f16c4
Address short timeout periods and mispelling of 'timeout'
Jan 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 41 additions & 9 deletions cloudflare/resource_cloudflare_rate_limit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ func resourceCloudflareRateLimit() *schema.Resource {
"mode": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.StringInSlice([]string{"simulate", "ban"}, true),
ValidateFunc: validation.StringInSlice([]string{"simulate", "ban", "challenge", "js_challenge"}, true),
},

"timeout": {
Type: schema.TypeInt,
Required: true,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

... 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Optional: true,
ValidateFunc: validation.IntBetween(1, 86400),
},

Expand Down Expand Up @@ -198,10 +198,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)
Expand All @@ -224,6 +229,12 @@ func resourceCloudflareRateLimitCreate(d *schema.ResourceData, meta interface{})

newRateLimit.Correlate, _ = expandRateLimitCorrelate(d)

newRateLimitAction, err := expandRateLimitAction(d)
if err != nil {
return err
}
newRateLimit.Action = newRateLimitAction

zoneName := d.Get("zone").(string)
zoneId, err := client.ZoneIDByName(zoneName)
if err != nil {
Expand Down Expand Up @@ -256,11 +267,22 @@ func resourceCloudflareRateLimitUpdate(d *schema.ResourceData, meta interface{})
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,
}

newRateLimitAction, err := expandRateLimitAction(d)
if err != nil {
return err
}
updatedRateLimit.Action = newRateLimitAction

newRateLimitMatch, err := expandRateLimitTrafficMatcher(d)
if err != nil {
Expand Down Expand Up @@ -331,6 +353,7 @@ func expandRateLimitTrafficMatcher(d *schema.ResourceData) (matcher cloudflare.R
}
responseMatcher.Statuses = statuses
}

if originIface, ok := matchResp["origin_traffic"]; ok {
originTraffic := originIface.(bool)
responseMatcher.OriginTraffic = &originTraffic
Expand All @@ -340,15 +363,24 @@ func expandRateLimitTrafficMatcher(d *schema.ResourceData) (matcher cloudflare.R
return
}

func expandRateLimitAction(d *schema.ResourceData) cloudflare.RateLimitAction {
func expandRateLimitAction(d *schema.ResourceData) (action cloudflare.RateLimitAction, err 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{})

action := cloudflare.RateLimitAction{
Mode: tfAction["mode"].(string),
Timeout: tfAction["timeout"].(int),
mode := tfAction["mode"].(string)
timeout := tfAction["timeout"].(int)

if timeout == 0 {
if mode == "simulate" || mode == "ban" {
return action, fmt.Errorf("rate limit timeout must be set if the 'mode' is simulate or ban")
}
} else if mode == "challenge" || mode == "js_challenge" {
return action, fmt.Errorf("rate limit timeout must not be set if the 'mode' is challenge or js_challenge")
}

action.Mode = mode
action.Timeout = timeout

if _, ok := tfAction["response"]; ok && len(tfAction["response"].([]interface{})) > 0 {
log.Printf("[DEBUG] Cloudflare Rate Limit specified action: %+v \n", tfAction)
tfActionResponse := tfAction["response"].([]interface{})[0].(map[string]interface{})
Expand All @@ -358,7 +390,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) {
Expand Down
108 changes: 108 additions & 0 deletions cloudflare/resource_cloudflare_rate_limit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,41 @@ func TestAccCloudflareRateLimit_Basic(t *testing.T) {
})
}

func TestAccCloudflareRateLimitChallenge_Basic(t *testing.T) {
t.Parallel()
var rateLimit cloudflare.RateLimit
zone := os.Getenv("CLOUDFLARE_DOMAIN")
rnd := acctest.RandString(10)
name := "cloudflare_rate_limit." + rnd

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudflareRateLimitDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckCloudflareRateLimitChallengeConfigBasic(zone, rnd),
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudflareRateLimitExists(name, &rateLimit),
testAccCheckCloudflareRateLimitIDIsValid(name, zone),
// check that the action challenge mode has been set
resource.TestCheckResourceAttr(name, "action.0.mode", "challenge"),
resource.TestCheckResourceAttr(name, "action.0.response.#", "0"),
resource.TestCheckResourceAttr(name, "bypass_url_patterns.#", "0"),
resource.TestCheckResourceAttr(name, "match.0.response.0.statuses.#", "0"),
resource.TestCheckResourceAttr(name, "disabled", "false"),
resource.TestCheckResourceAttr(name, "match.#", "1"),
resource.TestCheckResourceAttr(name, "match.0.request.#", "1"),
resource.TestCheckResourceAttr(name, "match.0.request.0.schemes.#", "1"),
resource.TestCheckResourceAttr(name, "match.0.request.0.url_pattern", "*"),
resource.TestCheckResourceAttr(name, "match.0.response.#", "1"),
resource.TestCheckResourceAttr(name, "match.0.response.0.origin_traffic", "true"),
),
},
},
})
}

func TestAccCloudflareRateLimit_FullySpecified(t *testing.T) {
t.Parallel()
var rateLimit cloudflare.RateLimit
Expand Down Expand Up @@ -167,6 +202,42 @@ func TestAccCloudflareRateLimit_CreateAfterManualDestroy(t *testing.T) {
})
}

func TestAccCloudflareRateLimit_WithoutTimeout(t *testing.T) {
t.Parallel()
zone := os.Getenv("CLOUDFLARE_DOMAIN")
rnd := acctest.RandString(10)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudflareRateLimitDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckCloudflareRateLimitConfigWithoutTimeout(zone, rnd),
ExpectError: regexp.MustCompile(regexp.QuoteMeta("rate limit timeout must be set if the 'mode' is simulate or ban")),
},
},
})
}

func TestAccCloudflareRateLimit_ChallengeWithTimeout(t *testing.T) {
t.Parallel()
zone := os.Getenv("CLOUDFLARE_DOMAIN")
rnd := acctest.RandString(10)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudflareRateLimitDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckCloudflareRateLimitChallengeConfigWithTimeout(zone, rnd),
ExpectError: regexp.MustCompile(regexp.QuoteMeta("rate limit timeout must not be set if the 'mode' is challenge or js_challenge")),
},
},
})
}

func testAccCheckCloudflareRateLimitDestroy(s *terraform.State) error {
client := testAccProvider.Meta().(*cloudflare.API)

Expand Down Expand Up @@ -314,3 +385,40 @@ resource "cloudflare_rate_limit" "%[1]s" {
bypass_url_patterns = ["%[2]s/bypass1","%[2]s/bypass2"]
}`, id, zone)
}

func testAccCheckCloudflareRateLimitChallengeConfigBasic(zone, id string) string {
return fmt.Sprintf(`
resource "cloudflare_rate_limit" "%[1]s" {
zone = "%[2]s"
threshold = 1000
period = 1
action {
mode = "challenge"
}
}`, id, zone)
}

func testAccCheckCloudflareRateLimitConfigWithoutTimeout(zone, id string) string {
return fmt.Sprintf(`
resource "cloudflare_rate_limit" "%[1]s" {
zone = "%[2]s"
threshold = 1000
period = 1
action {
mode = "simulate"
}
}`, id, zone)
}

func testAccCheckCloudflareRateLimitChallengeConfigWithTimeout(zone, id string) string {
return fmt.Sprintf(`
resource "cloudflare_rate_limit" "%[1]s" {
zone = "%[2]s"
threshold = 1000
period = 1
action {
mode = "challenge"
timeoute = 60
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
timeoute = 60
timeout = 60

}
}`, id, zone)
}
2 changes: 1 addition & 1 deletion website/docs/r/rate_limit.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ The **match.response** block supports:
The **action** block supports:

* `mode` - (Required) The type of action to perform. Allowable values are 'simulate' and 'ban'.
* `timeout` - (Required) The time in seconds as an integer to perform the mitigation action. Must be the same or greater than the period (min: 1, max: 86400).
* `timeout` - (Optional) The time in seconds as an integer to perform the mitigation action. This field is required if the `mode` is either `simulate` or `ban`. Must be the same or greater than the period (min: 1, max: 86400).
* `response` - (Optional) Custom content-type and body to return, this overrides the custom error for the zone. This field is not required. Omission will result in default HTML error page. Definition below.

The **action.response** block supports:
Expand Down