enhancement httpjson rate-limit early-limit #28513

hinchliff · 2021-10-18T18:46:07Z

Enhancement - Add configuration to httpjson to stop requests before the rate-limit is reached #28475

What does this PR do?

In Filebeat, the httpjson module respects the rate-limiting headers provided from the server, and will stop making requests when the remaining value reaches 0.

This enhancement would add a configuration option to indicate that the requests should be stopped/paused before fully using the specified rate-limit / remainder.

The default for httpjson would be to maintain the current behavior, but the default for okta would be to use a new "early" limit of 1, to avoid Okta violations.

Why is it important?

For sane servers, respecting the provided rate-limit values is acceptable and preferred. As discussed in #23023 (comment) and https://discuss.elastic.co/t/okta-module-causes-rate-limit-violations/286721, the Okta logs servers consider the full usage of the rate-limits as a Violation of their rate-limit policies.

Being able to configure an earlier stopping point, either at a fixed value (e.g. when remainder is 1) or at some percentage of the rate-limit (e.g. prior to the 90% warning level) would allow the use of the Filebeat Okta (and httpjson) modules without causing rate-limit Violations in Okta.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

How to test this PR locally

Unit tests have been added that cover all possible ranges of values for early-limit.

Related issues

Closes Add configuration to httpjson to stop requests before the rate-limit is reached #28475

Use cases

The default for httpjson would be to maintain the current behavior, but the default for okta would be to use a new "early" limit of 1, to avoid Okta violations.

Screenshots

Logs

cla-checker-service · 2021-10-18T18:46:11Z

💚 CLA has been signed

mergify · 2021-10-18T18:46:16Z

This pull request does not have a backport label. Could you fix it @hinchliff? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2021-10-18T18:52:33Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-10-27T13:17:52.355+0000
Duration: 101 min 47 sec
Commit: 64f52ca

Test stats 🧪

Test	Results
Failed	0
Passed	14995
Skipped	2324
Total	17319

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.

elasticmachine · 2021-10-19T11:44:28Z

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

mergify · 2021-10-19T14:11:49Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 28475-enhancement-httpjson-rate-limit-early-limit upstream/28475-enhancement-httpjson-rate-limit-early-limit
git merge upstream/master
git push upstream 28475-enhancement-httpjson-rate-limit-early-limit

…-httpjson-rate-limit-early-limit

hinchliff · 2021-10-19T17:59:15Z

I should be covered under my employer's CCLA. There is a CCLA with Optiv Security that includes me.

gtback · 2021-10-20T18:40:10Z

cla/check

marc-gr · 2021-10-21T08:13:04Z

/test

marc-gr · 2021-10-21T11:08:39Z

thanks for this! in order for the CI to pass, you need to cd x-pack/filebeat; mage fmt update and the commit the changes. LMK if you have any issues doing that or if you prefer us to do it :)

marc-gr · 2021-10-21T12:21:23Z

/test

marc-gr · 2021-10-25T10:35:10Z

It looks good in general. Just a couple of questions:

Is it safe to assume that Remaining value will always be present? If so, I'd document this assumption, since it seems if is not present and limit is reached, would behave as if no limit is in place. Otherwise maybe would be nice to have an extra option as a fallback (like a predetermined wait time in case a limit is reached and no remaining is found).
You mention okta suggested 90%, then maybe would be nice to add the limit to okta module as 0.9 instead.
Could you change the variables naming from snake_case to camelCase, for consistency?

Thanks for the work!

hinchliff · 2021-10-25T12:58:50Z

Is it safe to assume that Remaining value will always be present? If so, I'd document this assumption, since it seems if is not present and limit is reached, would behave as if no limit is in place. Otherwise maybe would be nice to have an extra option as a fallback (like a predetermined wait time in case a limit is reached and no remaining is found).

Yes, the default behavior before this PR would be that no rate-limiting would occur if the Remaining header were missing. I added some documentation to that affect.

I also added another test and some more documentation about the (new) behavior if the Limit header were missing. (This is only a factor in the behavior when using a percentage based rate-limit.)

You mention okta suggested 90%, then maybe would be nice to add the limit to okta module as 0.9 instead.

I wasn't sure about making such a "big" change to the defaults in the Okta module. Using a new default of 1 should avoid Okta "Violations", while still being very close to the original default behavior. Using a value of 90% would avoid Violations, but I believe would still cause "Warnings" from Okta.

Personally, I had been planning on using a value of 89% to avoid both Violations and Warnings. I have updated the Okta module to use this value as the default, and I added some documentation to that module about the new default and overriding the rate-limiting of httpjson.

Could you change the variables naming from snake_case to camelCase, for consistency?

Done. thanks.

marc-gr · 2021-10-26T08:34:10Z

/test

hinchliff · 2021-10-26T12:24:33Z

mage fmt update doesn't show any changes. I reviewed the output in Jenkins, and I think this is a failure within Jenkins.

marc-gr · 2021-10-27T07:36:16Z

/test

marc-gr · 2021-10-27T08:29:12Z

/test

marc-gr

Just a small comment, after it is fixed, should be good to go 👍

x-pack/filebeat/input/httpjson/rate_limiter_test.go

marc-gr · 2021-10-27T13:17:18Z

/test

marc-gr

LGTM! Thanks for the work! 👍

* elastic#28475 enhancement filebeat httpjson rate-limit early * elastic#28475 okta should default to early limit to avoid violations * adding changelog entry * lint fix * additional comments and tests on behavior with missing headers * changing default okta-rate limit to 89%. adding docs in Okta module * Re-generate okta docs * removing unused lines from test Co-authored-by: Marc Guasch <[email protected]>

hinchliff · 2021-11-03T16:10:33Z

@marc-gr -- do you know if i need to do anything specific to make sure that this gets into (any) 7.x release?

I looked around, but I couldn't tell which new releases might get commits to master like from this PR.

* #28475 enhancement filebeat httpjson rate-limit early * #28475 okta should default to early limit to avoid violations * adding changelog entry * lint fix * additional comments and tests on behavior with missing headers * changing default okta-rate limit to 89%. adding docs in Okta module * Re-generate okta docs * removing unused lines from test Co-authored-by: Marc Guasch <[email protected]> (cherry picked from commit c77bf19) # Conflicts: # x-pack/filebeat/input/httpjson/rate_limiter.go

* #28475 enhancement filebeat httpjson rate-limit early * #28475 okta should default to early limit to avoid violations * adding changelog entry * lint fix * additional comments and tests on behavior with missing headers * changing default okta-rate limit to 89%. adding docs in Okta module * Re-generate okta docs * removing unused lines from test Co-authored-by: Marc Guasch <[email protected]> (cherry picked from commit c77bf19)

* #28475 enhancement filebeat httpjson rate-limit early * #28475 okta should default to early limit to avoid violations * adding changelog entry * lint fix * additional comments and tests on behavior with missing headers * changing default okta-rate limit to 89%. adding docs in Okta module * Re-generate okta docs * removing unused lines from test Co-authored-by: Marc Guasch <[email protected]> (cherry picked from commit c77bf19) Co-authored-by: Alan Hinchliff <[email protected]>

…28864) * enhancement httpjson rate-limit early-limit (#28513) * #28475 enhancement filebeat httpjson rate-limit early * #28475 okta should default to early limit to avoid violations * adding changelog entry * lint fix * additional comments and tests on behavior with missing headers * changing default okta-rate limit to 89%. adding docs in Okta module * Re-generate okta docs * removing unused lines from test Co-authored-by: Marc Guasch <[email protected]> (cherry picked from commit c77bf19) # Conflicts: # x-pack/filebeat/input/httpjson/rate_limiter.go * fix merge conflicts Co-authored-by: Alan Hinchliff <[email protected]> Co-authored-by: Marc Guasch <[email protected]>

Alan Hinchliff added 2 commits October 18, 2021 14:30

elastic#28475 enhancement filebeat httpjson rate-limit early

38d2b4c

elastic#28475 okta should default to early limit to avoid violations

37be802

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 18, 2021

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Oct 18, 2021

adding changelog entry

c57a9c3

jamiehynds added the Team:Security-External Integrations label Oct 19, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 19, 2021

jamiehynds requested a review from marc-gr October 19, 2021 11:44

Merge remote-tracking branch 'upstream/master' into 28475-enhancement…

9540c37

…-httpjson-rate-limit-early-limit

lint fix

073ab32

Alan Hinchliff added 2 commits October 25, 2021 08:32

additional comments and tests on behavior with missing headers

1d27920

changing default okta-rate limit to 89%. adding docs in Okta module

98e2dc5

Re-generate okta docs

34eb997

marc-gr requested changes Oct 27, 2021

View reviewed changes

x-pack/filebeat/input/httpjson/rate_limiter_test.go Outdated Show resolved Hide resolved

removing unused lines from test

64f52ca

marc-gr approved these changes Oct 27, 2021

View reviewed changes

marc-gr added the needs_integration_sync Changes in this PR need synced to elastic/integrations. label Oct 27, 2021

marc-gr merged commit c77bf19 into elastic:master Oct 28, 2021

hinchliff deleted the 28475-enhancement-httpjson-rate-limit-early-limit branch October 28, 2021 13:15

marc-gr added backport-v7.16.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Nov 8, 2021

mergify bot mentioned this pull request Nov 8, 2021

[7.16](backport #28513) enhancement httpjson rate-limit early-limit #28864

Merged

marc-gr added the backport-v8.0.0 Automated backport with mergify label Nov 8, 2021

mergify bot mentioned this pull request Nov 8, 2021

[8.0](backport #28513) enhancement httpjson rate-limit early-limit #28865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement httpjson rate-limit early-limit #28513

enhancement httpjson rate-limit early-limit #28513

hinchliff commented Oct 18, 2021 •

edited

Loading

cla-checker-service bot commented Oct 18, 2021 •

edited

Loading

mergify bot commented Oct 18, 2021

elasticmachine commented Oct 18, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

elasticmachine commented Oct 19, 2021

mergify bot commented Oct 19, 2021

hinchliff commented Oct 19, 2021

gtback commented Oct 20, 2021

marc-gr commented Oct 21, 2021

marc-gr commented Oct 21, 2021

marc-gr commented Oct 21, 2021

marc-gr commented Oct 25, 2021

hinchliff commented Oct 25, 2021

marc-gr commented Oct 26, 2021

hinchliff commented Oct 26, 2021

marc-gr commented Oct 27, 2021

marc-gr commented Oct 27, 2021

marc-gr left a comment

marc-gr commented Oct 27, 2021

marc-gr left a comment

hinchliff commented Nov 3, 2021

enhancement httpjson rate-limit early-limit #28513

enhancement httpjson rate-limit early-limit #28513

Conversation

hinchliff commented Oct 18, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

cla-checker-service bot commented Oct 18, 2021 • edited Loading

mergify bot commented Oct 18, 2021

elasticmachine commented Oct 18, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Oct 19, 2021

mergify bot commented Oct 19, 2021

hinchliff commented Oct 19, 2021

gtback commented Oct 20, 2021

marc-gr commented Oct 21, 2021

marc-gr commented Oct 21, 2021

marc-gr commented Oct 21, 2021

marc-gr commented Oct 25, 2021

hinchliff commented Oct 25, 2021

marc-gr commented Oct 26, 2021

hinchliff commented Oct 26, 2021

marc-gr commented Oct 27, 2021

marc-gr commented Oct 27, 2021

marc-gr left a comment

Choose a reason for hiding this comment

marc-gr commented Oct 27, 2021

marc-gr left a comment

Choose a reason for hiding this comment

hinchliff commented Nov 3, 2021

hinchliff commented Oct 18, 2021 •

edited

Loading

cla-checker-service bot commented Oct 18, 2021 •

edited

Loading

elasticmachine commented Oct 18, 2021 •

edited by jenkins-beats-ci bot

Loading