Conversation
blotus
left a comment
blotus
left a comment
There was a problem hiding this comment.
Thanks for the PR !
It looks good overall, I just have a few comments/requests.
| LockToken: aws.String(token), | ||
| Id: aws.String(id), | ||
| Rules: []wafv2types.Rule{}, | ||
| VisibilityConfig: &wafv2types.VisibilityConfig{ |
There was a problem hiding this comment.
This overwrites whatever the visibility config was for the rule group. You should use the values that were already set in the RG instead (you can get the values from the object returned by w.GetRuleGroup that is called just before this function.
| } | ||
|
|
||
| err = w.CreateRuleGroup(ctx, w.config.RuleGroupName) | ||
| if !w.config.DelegateAclManagement { |
There was a problem hiding this comment.
We should probably check the if the rule group specified in the config exists when in delegated mode, so that a clean error can be returned immediately in case of misconfiguration
| return fmt.Errorf("failed to cleanup: %w", err) | ||
| } | ||
|
|
||
| w.aclsInfo, w.setsInfos, w.ruleGroupsInfos, err = w.ListResources(ctx) |
There was a problem hiding this comment.
If the goal is to limit the IAM permissions required, I think we could go a step further and also remove the read calls for the webacl when in delegated mode ? This would also make web_acl_name optional in this mode.
| @@ -429,9 +448,16 @@ | |||
|
|
|||
| w.Logger.Debugf("Deleting RuleGroup %s", w.config.RuleGroupName) | |||
There was a problem hiding this comment.
nit: the debug log should be different when delegate is true
| CloudWatchMetricName string `yaml:"cloudwatch_metric_name"` | ||
| SampleRequests bool `yaml:"sample_requests"` | ||
| CleanOnStart bool `yaml:"remove_sets_on_start"` | ||
| DelegateAclManagement bool `yaml:"delegate_acl_management"` |
There was a problem hiding this comment.
I'm not sure about delegate_acl_management: we don't really delegate anything ? Maybe use_existing_rule_group or something a bit more descriptive ?
paibok
force-pushed
the
dev/argar/v2
branch
5 times, most recently
from
282ce24 to
4d5818c
Compare
|
Hi @blotus, thanks for reviewing my PR and for the feedback. I've integrated most of your suggestions and just pushed the fixes. Let me know what you think! :) |
2 participants
Summary
wafv2:UpdateWebACLpermissions on the global WebACL managing CloudFront distributions.RuleGroupresources, adhering to the principle of least privilege.Changes
pkg/cfg/config.godelegate_acl_management(boolean) to the configuration structure.pkg/waf/waf.goWebACLobject itself.RuleGroup.