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.

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

feat: extend support for rate based statement rules #81

Closed
wants to merge 11 commits into from

Conversation

GuusDeGraeve
Copy link

@GuusDeGraeve GuusDeGraeve commented May 23, 2024

what

  • Adds support for scope_down_statement (including and statements) in rate_based_statement_rules
  • Adds support for custom_response in rate_based_statement_rules block action
  • Adds support for evaluation_window_sec in rate_based_statement_rules

why

  • There was no way to set rate limiting rules for specific hostnames, paths or http methods
  • There was no way to set custom responses for rate limiting rules when requests are blocked
  • There was no way to set the evaluation window for rate limit measuring

references

closes #63

@GuusDeGraeve GuusDeGraeve requested review from a team as code owners May 23, 2024 10:04
@GuusDeGraeve GuusDeGraeve requested review from Gowiem and jamengual May 23, 2024 10:04
@mergify mergify bot added the triage Needs triage label May 23, 2024
statement = any
rule_label = optional(list(string), null)
statement = any
custom_response_code = optional(number, null)
Copy link
Member

Choose a reason for hiding this comment

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

This should be added to the description

@osterman
Copy link
Member

/terratest

for_each = length(lookup(scope_down_statement.value, "and", [])) > 0 ? [scope_down_statement.value.and] : []
content {
dynamic "statement" {
for_each = and_statement.value
Copy link
Member

Choose a reason for hiding this comment

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

@GuusDeGraeve the PR looks good, thank you.

Some questions about this code

 for_each = length(lookup(scope_down_statement.value, "and", [])) > 0 ? [scope_down_statement.value.and] : []
                  content {
                    dynamic "statement" {
                      for_each = and_statement.value

Looks like scope_down_statement.value.and should be a list b/c you check its length.
But then you put it into another list in [scope_down_statement.value.and].
Should it be just ? scope_down_statement.value.and : [] ?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @aknysh, yes that is correct, the reason is because the add statement is a list of multiple statements. Which we loop over within the next dynamic block dynamic "statement" {.

I know it looks quite confusing when reading the code, but it is definitely on purpose.

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@GuusDeGraeve thanks for the PR, please see comments

Copy link

mergify bot commented Jun 26, 2024

💥 This pull request now has conflicts. Could you fix it @GuusDeGraeve? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Jun 26, 2024
@mergify mergify bot closed this Jun 26, 2024
Copy link

mergify bot commented Jun 26, 2024

This PR was closed due to inactivity and merge conflicts. 😭
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot removed conflict This PR has conflicts triage Needs triage labels Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support of evaluation_window_sec argument
3 participants