-
Notifications
You must be signed in to change notification settings - Fork 127
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
Custom Block Response #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the contribution!
I have a request if you can make the changes to what you have done so far.
Also, if you can please run the make commands to validate, add to changelog and docs, i'd appreciate that too!
main.tf
Outdated
content {} | ||
content { | ||
custom_response { | ||
response_code = var.custom_response_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response_code = var.custom_response_code | |
response_code = lookup(rule.custom_response, 403) |
Instead of using a variable, a lookup function should be used since these arguments are filled in via the rules variable. This would eliminate the need to create a standalone variable for this.
Also, from the docs you sent I can see that there are two other arguments that can be used here based on terraform docs:
"Block": {
"CustomResponse": {
"CustomResponseBodyKey": "CustomResponseBodyKey1",
"ResponseCode": 404,
"ResponseHeaders": [
{
"Name": "BlockActionHeader1Name",
"Value": "BlockActionHeader1Value"
}
]
}
Can you please add these too?
Okay, added the ability to define the custom body and headers as well. Certainly more robust now. 👍🏼 However, I am unable to run the make commands. I also do not see any documentation on them? Edit: ah I see... tucked under the |
Make commands ran ✅ |
variables.tf
Outdated
variable "enable_custom_response" { | ||
type = bool | ||
description = "Set to `true` to define custom responses (status code, body, and/or headers) for non managed rule group statements block actions." | ||
default = false | ||
} | ||
|
||
variable "custom_response_body" { | ||
type = map(string) | ||
description = "A custom response body to be referenced on a per rule basis." | ||
default = {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw this PR! Looking to solve the same problem, WDYT about an array of custom response bodies so we are not limited to just one? Please see https://github.com/umotif-public/terraform-aws-waf-webaclv2/pull/57/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naseemkullah have you tested that? I was not able to get it to work with an array input for custom_response_body
. I like your use of object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just updated my PR with the inclusion of your purposed custom_response_bodies
. 👍🏼 Good call.
…tions. Dropped the enabled variable as not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really appreciate the examples!
I just have a few comments to make sure the examples will work.
output "custom_ip_set_arn" { | ||
description = "The ARN of the Custom IP Set" | ||
value = aws_wafv2_ip_set.custom_ip_set.arn | ||
} | ||
|
||
output "block_ip_set_arn" { | ||
description = "The ARN of the Block IP Set" | ||
value = aws_wafv2_ip_set.block_ip_set.arn | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these resources that you're referencing here in the outputs?
This example with these outputs.
Similar comment to the other outputs in the other examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Good catch... got overzealous with my copy pasta.
} | ||
] | ||
|
||
rules = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
visibility_config
will be required in both of the rules - see here: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/wafv2_web_acl#visibility_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch here as well... nuked the visibility_config
from the copy of my personal block because had local variables I didn't want to bother with here. But just added the block ✔️ .
custom_response = true | ||
custom_response_code = 412 | ||
custom_response_key = "custom_response_body_1" | ||
custom_response_headers = [ | ||
{ | ||
name = "X-Custom-Header" | ||
value = "You are not authorized to access this resource." | ||
}, | ||
{ | ||
name = "X-Custom-Header-2" | ||
value = "You are still not authorized to access this resource." | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
custom_response = true | |
custom_response_code = 412 | |
custom_response_key = "custom_response_body_1" | |
custom_response_headers = [ | |
{ | |
name = "X-Custom-Header" | |
value = "You are not authorized to access this resource." | |
}, | |
{ | |
name = "X-Custom-Header-2" | |
value = "You are still not authorized to access this resource." | |
} | |
] | |
custom_response = { | |
enabled = true | |
code = 412 | |
body_key = "custom_response_body_1" | |
headers = [ | |
{ | |
name = "X-Custom-Header" | |
value = "You are not authorized to access this resource." | |
}, | |
{ | |
name = "X-Custom-Header-2" | |
value = "You are still not authorized to access this resource." | |
} | |
] | |
} |
nit: nested instead of flat
} | ||
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Was working on this very change. :)
Thanks for all the feedback @Ohid25 and @naseemkullah. I think this is ready to go. Took me a bit to grasp the super slick syntax for dynamic
Very clever! |
|
||
custom_response = { | ||
response_code = 412 | ||
response_key = "default_1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response_key = "default_1", | |
custom_response_body_key = "default_1", |
Just a better exposure of the underlying api imho (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-webacl-customresponse.html)
main.tf
Outdated
dynamic "custom_response" { | ||
for_each = length(lookup(rule.value, "custom_response", [])) == 0 ? [] : [lookup(rule.value, "custom_response", {})] | ||
content { | ||
custom_response_body_key = lookup(custom_response.value, "response_key", null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
custom_response_body_key = lookup(custom_response.value, "response_key", null) | |
custom_response_body_key = lookup(custom_response.value, "custom_response_body_key", null) |
Just better exposure of underlying api
lgtm, as mentioned optional nit: would just change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Thanks for the contribution! 🎉 . This is now released as a part of 3.8.0 |
Description
I added the capability to define custom responses for block actions. More info on custom responses can be found here and here.
Note that this does not apply to managed rule group statements per AWS design.