Add Waf RateBasedRule support #1606
Conversation
Ninir
added
the
enhancement
There was a problem hiding this comment.
Hey @bekbulatov
thanks for the PR - I left you some comments/questions there.
Let me know what you think.
| errors = append(errors, fmt.Errorf("%q must be only %s", k, waf.RateKeyIp)) | ||
| } | ||
| return | ||
| }, |
There was a problem hiding this comment.
I'm not sure it's wise to have the validation here for this field. It looks like Amazon may have some plans for expansion, otherwise they wouldn't be introducing a field with a single valid value.
The downside of the static validation is that once AWS introduces a new rate key, we'll have to loose the validation or remove it and folks will be waiting for a new release (unable to use the new key) merely because of tight static validation.
It's not a big deal - I'm just trying to think ahead.
There was a problem hiding this comment.
Agreed, anyway the value will be validated by AWS.
There was a problem hiding this comment.
Yep, I think the AWS API validation is the best choice here.
| }, | ||
| "data_id": &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| Optional: true, |
There was a problem hiding this comment.
This field seems to be required in per the API docs: http://docs.aws.amazon.com/waf/latest/APIReference/API_Predicate.html
There was a problem hiding this comment.
Sure, I will update the schema for Rule as well
| }, | ||
| "rate_key": &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| Required: true, |
There was a problem hiding this comment.
It seems that this field isn't updatable (based on API reference), shouldn't it be marked as ForceNew then?
There was a problem hiding this comment.
Rate_key could be only IP now. Why do you think it should be recreated if changed?
There was a problem hiding this comment.
well, you're right that there's no way to change it, but if the user is unaware they might try and at the moment we'd just ignore the field (in Update) and support user's assumption that the field was successfully changed.
If we instead delegate the validation work to Create then it will fail and user will also realize during plan that this can't be changed.
| return nil | ||
| } | ||
|
|
||
| func updateWafRateBasedRuleResource(id string, oldP, newP []interface{}, rateLimit *int64, conn *waf.WAF) error { |
There was a problem hiding this comment.
It might be more convenient to accept rateLimit int here instead and do all the pointer magic inside the function to reduce the repetition, but no biggie.
| } | ||
| return | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Is there any particular reason why this change should be included in this PR? How is it related to WAF Rate Based Rule?
There was a problem hiding this comment.
I mentioned this in #1285. By default, WebACL works with REGULAR rules and you will get an error if you pass rule_id of a RATE_BASED rule. This is documented in https://github.com/aws/aws-sdk-go/blob/master/service/waf/api.go#L5708-L5713
There was a problem hiding this comment.
I see, but I think we can't just introduce a Required field - that would break it for users with existing configs which don't have this field there.
This applies to your other change in aws/resource_aws_waf_rule.go too.
There was a problem hiding this comment.
Do you mind changing it to Optional?
There was a problem hiding this comment.
btw. you might be able to avoid changing the updateWebAclResource if you also add the current default value as Default - that way it should just work the same way as before, yet allow users to specify type = "RATE_BASED" if they need to.
There was a problem hiding this comment.
@radeksimko Thank you for the review. I believe the Required change for aws/resource_aws_waf_rule.go is fine, as we just duplicate AWS validation, it's not a breaking change.
I made type optional and default to REGULAR as recommended.
There was a problem hiding this comment.
You're right - just checked the API docs - it was probably just overlooked.
Thanks.
radeksimko
added
the
waiting-response
radeksimko
removed
the
waiting-response
Add Waf RateBasedRule support
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Updated version of #1285.
I had to add the required
typefield to the rules block underaws_waf_rule. The value could be eitherREGULARorRATE_BASEDregarding which type of rule you are using.The docs are updated as well.