x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting fixes #41583

chrisberkhout · 2024-11-11T12:26:17Z

Proposed commit message

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting fixes

- Separate rate limits by endpoint.
- Stop requests until reset when `x-rate-limit-remaining: 0`.

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.

Discussion

Can be reviewed commit-by-commit.

The reason a new rate.Limiter needs to be created when x-rate-limit-remaining: 0 is that while rate.Limiter allows updating of the rate and burst, it doesn't allow clearing of existing tokens.

Disruptive User Impact

None.

Related issues

Relates x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting hangs #40106

elasticmachine · 2024-11-11T12:26:19Z

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

mergify · 2024-11-11T12:26:51Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @chrisberkhout? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

mergify · 2024-11-11T12:26:52Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

efd6 · 2024-11-11T21:03:52Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go

+		endpoint = "/api/v1/users/{user}"
+		path = strings.Replace(endpoint, "{user}", user, 1)


Can we not just use path.Join here rather than a search/replace?

We do also need endpoint now, without the ID inserted, for the rate limiter.

I think it's good that the final path is derived from the endpoint pattern. If there's a change they're less likely to diverge.

I don't particularly like it, but OK.

Ok. Leaving it like that.

efd6 · 2024-11-11T21:04:53Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go

+	var endpoint string
+	var path string


var endpoint, path string

efd6 · 2024-11-11T21:07:22Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go

 	u := &url.URL{
 		Scheme: "https",
 		Host:   host,
-		Path:   path.Join(endpoint, user, "factors"),
+		Path:   path,


I think keeping the use if path.Join would be better (also below).

As above. Leaving it.

efd6 · 2024-11-11T21:12:55Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta_test.go

 			}
-			if limiter.lim.Burst() != 1 {
-				t.Errorf("unexpected burst: got:%d want:1", limiter.lim.Burst())
+			for _, l := range *limiter {


I think this needs a comment, or preferably since we have constructed the system here, use the key that we expect to exist using limiter[<key>].

I went with the comment.

efd6 · 2024-11-11T21:14:34Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter.go

-type RateLimiter struct {
-	lim *rate.Limiter
-}
+type RateLimiter map[string]*rate.Limiter

 func NewRateLimiter() *RateLimiter {


This does not need to be on a pointer.

Changed to:

func NewRateLimiter() RateLimiter

efd6 · 2024-11-11T21:22:59Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta_test.go

@@ -444,7 +444,7 @@ var nextTests = []struct {
 func TestNext(t *testing.T) {
 	for i, test := range nextTests {
 		got, err := Next(test.header)
-		if err != test.wantErr {
+		if !errors.Is(err, test.wantErr) {


This is not right. The two cases are nil and io.EOF and the latter is never wrapped.

efd6 · 2024-11-11T21:23:12Z

x-pack/filebeat/input/entityanalytics/provider/okta/okta.go

@@ -478,7 +478,7 @@ func (p *oktaInput) doFetchUsers(ctx context.Context, state *stateStore, fullSyn

 		next, err := okta.Next(h)
 		if err != nil {
-			if err == io.EOF {
+			if errors.Is(err, io.EOF) {


Same comment here and below.

Reverted 3 cases.

efd6 · 2024-11-11T21:24:26Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter_test.go

-		r.Update(endpoint, headers, window, logp.L())
+		err := r.Update(endpoint, headers, window, logp.L())
+		if err != nil {
+			t.Errorf("Update returned error: %v", err)


t.Errorf("unexpected error from Update(): %v", err)

efd6 · 2024-11-11T21:31:12Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go

+		if _, copyErr := io.Copy(io.Discard, resp.Body); copyErr != nil {
+			err = errors.Join(err, copyErr)
+		}


If I were going to do this, I would have it as

_, cerr := io.Copy(io.Discard, resp.Body) return nil, nil, errors.Join(err, cerr)

but we are just discarding the the body to recover the conn's usability, so this is really only an optimisation and as such should not change the behaviour qualitatively; I would leave this as it was originally.

Reverted it.

efd6 · 2024-11-11T21:34:42Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter.go

+	return &r
+}
+
+func (r RateLimiter) get(path string) *rate.Limiter {


Suggested change

func (r RateLimiter) get(path string) *rate.Limiter {

func (r RateLimiter) Limiter(path string) *rate.Limiter {

with necessary changes elsewhere.

Or unexported? I think this should only be used internally and for testing.

Sure, unexported is fine. If this becomes something we need to share later, we can export it then.

mergify · 2024-11-18T14:43:38Z

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 okta-rate-limiting-logic upstream/okta-rate-limiting-logic
git merge upstream/main
git push upstream okta-rate-limiting-logic

chrisberkhout added bugfix Team:Security-Service Integrations Security Service Integrations Team labels Nov 11, 2024

chrisberkhout self-assigned this Nov 11, 2024

chrisberkhout requested a review from a team as a code owner November 11, 2024 12:26

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 11, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Nov 11, 2024

chrisberkhout mentioned this pull request Nov 11, 2024

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting hangs #40106

Open

chrisberkhout force-pushed the okta-rate-limiting-logic branch from 368f547 to b57da7b Compare November 11, 2024 13:33

chrisberkhout added backport-8.15 Automated backport to the 8.15 branch with mergify backport-8.16 Automated backport with mergify labels Nov 11, 2024

chrisberkhout force-pushed the okta-rate-limiting-logic branch 3 times, most recently from 3c56801 to 317c45e Compare November 11, 2024 17:27

efd6 reviewed Nov 11, 2024

View reviewed changes

chrisberkhout added 11 commits December 4, 2024 14:39

Extract existing functionality to okta.RateLimiter.

81faf96

Move related logging into okta.RateLimiter.

033fd48

Use a separate rate.Limiter per endpoint.

b13d02e

Stop requests until reset when x-rate-limit-remaining is zero.

1ee6a58

Test RateLimiter.

4648f7e

Add CHANGELOG.next.asciidoc entry.

41d4ec3

Fix lint errors.

b55b47e

For lint error: check instead of defer.

c55ade6

Address some feedback.

1d65a09

Rename unexported method get() to limiter().

c667ab9

Revert a change that was for the linter.

ab7232a

chrisberkhout added 4 commits December 4, 2024 14:39

Reverted another change that was for the linter.

5066f31

Revert some more changes made for the linter.

350cf46

Use okta.RateLimiter rather than *okta.RateLimiter.

25ac97a

Resolve rebase error.

13a79f3

chrisberkhout force-pushed the okta-rate-limiting-logic branch from 23a565a to 13a79f3 Compare December 4, 2024 13:42

chrisberkhout requested a review from efd6 December 4, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting fixes #41583

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting fixes #41583

chrisberkhout commented Nov 11, 2024 •

edited

Loading

elasticmachine commented Nov 11, 2024

mergify bot commented Nov 11, 2024

mergify bot commented Nov 11, 2024

efd6 Nov 11, 2024

chrisberkhout Nov 12, 2024

efd6 Nov 12, 2024

chrisberkhout Nov 18, 2024

efd6 Nov 11, 2024

chrisberkhout Nov 18, 2024

efd6 Nov 11, 2024

chrisberkhout Nov 18, 2024

efd6 Nov 11, 2024

chrisberkhout Nov 18, 2024

efd6 Nov 11, 2024

chrisberkhout Dec 4, 2024

efd6 Nov 11, 2024

chrisberkhout Dec 4, 2024

efd6 Nov 11, 2024

chrisberkhout Dec 4, 2024

efd6 Nov 11, 2024

chrisberkhout Nov 18, 2024

efd6 Nov 11, 2024 •

edited

Loading

chrisberkhout Dec 4, 2024

efd6 Nov 11, 2024

chrisberkhout Nov 12, 2024

efd6 Nov 12, 2024

chrisberkhout Nov 18, 2024

mergify bot commented Nov 18, 2024

		endpoint = "/api/v1/users/{user}"
		path = strings.Replace(endpoint, "{user}", user, 1)

	func (r RateLimiter) get(path string) *rate.Limiter {
	func (r RateLimiter) Limiter(path string) *rate.Limiter {

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting fixes #41583

Are you sure you want to change the base?

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting fixes #41583

Conversation

chrisberkhout commented Nov 11, 2024 • edited Loading

Proposed commit message

Checklist

Discussion

Disruptive User Impact

Related issues

elasticmachine commented Nov 11, 2024

mergify bot commented Nov 11, 2024

mergify bot commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efd6 Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Nov 18, 2024

chrisberkhout commented Nov 11, 2024 •

edited

Loading

efd6 Nov 11, 2024 •

edited

Loading