athena audit logs - query rate limiter by tobiaszheller · Pull Request #24918 · gravitational/teleport

tobiaszheller · 2023-04-20T18:06:49Z

Part of https://github.com/gravitational/teleport.e/issues/894
RFD: https://github.com/gravitational/teleport/blob/master/rfd/0118-scalable-audit-logs.md

This PR adds rate limit feature to athena audit logs on searching events.
Rate limit is needed to allow throttling of tenants in Cloud.

tobiaszheller · 2023-04-24T13:17:51Z

@smallinsky @rosstimothy in dfc304c I have reworked it to use wrap logger inside limiter. It turned out quite a lot of additional code. Let me know what do you think?

rosstimothy · 2023-04-24T13:30:13Z

+	if cfg.LimiterBurst <= 0 {
+		return trace.BadParameter("limiterBurst cannot be less or equal to 0")
+	}
+	if math.Abs(cfg.LimiterRate) < 1e-9 {


Why 1e-9?

Wonder if we can't just change the cfg.LimiterRate type to uint.

sorry for missing docs for that config. LimiterRate defines how many tokens and added per seconds.
I am checking it with 1e-9 just to have very small delta when comparing float against 0. On one hand it should be equal to 0 because if it's not present in url params, it will be default, so 0. @rosstimothy do you think I should just better document it?

@smallinsky i don't think you can define rps using uint, someone may want to set it 0.5 for example. And defining it in miliseconds will be even more confusing.

One thing that we can do is use helper fn from rate package Every. You define interval (time.Duration) at which token is filled. Do you think it will be easier to understand?

0.5 RPS or 10.1 RPS values as rate limit should be quite rate but I don't have strong about it so please free to ignore my comment.

I don't thing that we can do is use helper fn from rate package Every. You define interval (time.Duration) at which token is filled. Do you think it will be easier to understand?

I think that current behaviour with 1s time unit is fine and readable. It should be quite easy to extend in the future but if we don't have case for Athena we should skip it for now and keep it simple.

smallinsky · 2023-04-24T14:58:11Z

+	if cfg.LimiterBurst <= 0 {
+		return trace.BadParameter("limiterBurst cannot be less or equal to 0")
+	}
+	if math.Abs(cfg.LimiterRate) < 1e-9 {


Wonder if we can't just change the cfg.LimiterRate type to uint.

smallinsky · 2023-04-24T14:59:05Z

+}
+
+type SearchEventsLimiterConfig struct {
+	LimiterRate  float64


We we need float64 type ? Making ing uint will allow to simplify the CheckAndSetDefaults logic

tobiaszheller · 2023-04-24T18:40:56Z

@rosstimothy 4fa09ec
addresses all comments from PR. I think only #24918 (comment) remains open and would like to hear your opinions.

tobiaszheller · 2023-04-26T12:19:18Z

@smallinsky @rosstimothy in 26b6d79
I have reworked config, it accepts now 3 params:

	// RefillTime is amount of time between refills.
	RefillTime time.Duration
	// RefillAmount is the number of tokens that are added to the bucket during a refill.
	RefillAmount int
	// Burst defines number of available tokens. It's initially full and refilled
	// based on refillAmount and refillDuration.
	Burst int

similary to how our per IP limiter does: https://github.com/gravitational/oxy/blob/c59990dc8c641ec79a7f6eee48a20c7e8f4c69c6/ratelimit/tokenlimiter.go#L31, I just used more descriptive names.

Let me know what do you think?

rosstimothy

LGTM but seems like there is now a broken test case:

=== RUN   TestAthenaAuditLogSetup/config_with_rate_limit_-_should_use_events.SearchEventsLimiter
{"caller":"athena/consumer.go:506","component":"athena","error":"operation error SQS: ReceiveMessage, failed to resolve service endpoint, an AWS region is required, but was not found","level":"error","message":"Failure processing SQS messages","timestamp":"2023-04-27T10:16:33Z"}
    service_test.go:322: 
        	Error Trace:	/__w/teleport/teleport/lib/service/service_test.go:322
        	            				/__w/teleport/teleport/lib/service/service_test.go:338
        	Error:      	Received unexpected error:
        	            	LimiterRefillAmount must be greater than 0 if LimiterBurst is used
        	Test:       	TestAthenaAuditLogSetup/config_with_rate_limit_-_should_use_events.SearchEventsLimiter
{"caller":"athena/consumer.go:506","component":"athena","error":"operation error SQS: ReceiveMessage, failed to resolve service endpoint, an AWS region is required, but was not found","level":"error","message":"Failure processing SQS messages","timestamp":"2023-04-27T10:16:33Z"}
--- FAIL: TestAthenaAuditLogSetup/config_with_rate_limit_-_should_use_events.SearchEventsLimiter (0.00s)

tobiaszheller · 2023-04-28T13:13:09Z

LGTM but seems like there is now a broken test case:

=== RUN   TestAthenaAuditLogSetup/config_with_rate_limit_-_should_use_events.SearchEventsLimiter
{"caller":"athena/consumer.go:506","component":"athena","error":"operation error SQS: ReceiveMessage, failed to resolve service endpoint, an AWS region is required, but was not found","level":"error","message":"Failure processing SQS messages","timestamp":"2023-04-27T10:16:33Z"}
    service_test.go:322: 
        	Error Trace:	/__w/teleport/teleport/lib/service/service_test.go:322
        	            				/__w/teleport/teleport/lib/service/service_test.go:338
        	Error:      	Received unexpected error:
        	            	LimiterRefillAmount must be greater than 0 if LimiterBurst is used
        	Test:       	TestAthenaAuditLogSetup/config_with_rate_limit_-_should_use_events.SearchEventsLimiter
{"caller":"athena/consumer.go:506","component":"athena","error":"operation error SQS: ReceiveMessage, failed to resolve service endpoint, an AWS region is required, but was not found","level":"error","message":"Failure processing SQS messages","timestamp":"2023-04-27T10:16:33Z"}
--- FAIL: TestAthenaAuditLogSetup/config_with_rate_limit_-_should_use_events.SearchEventsLimiter (0.00s)

fixed in d7e491e

public-teleport-github-review-bot · 2023-04-28T14:03:58Z

@tobiaszheller See the table below for backport results.

Branch	Result
branch/v13	Failed

public-teleport-github-review-bot · 2023-05-15T10:46:39Z

@tobiaszheller See the table below for backport results.

Branch	Result
branch/v13	Create PR

github-actions Bot requested review from lxea and smallinsky April 20, 2023 18:07

github-actions Bot added audit-log Issues related to Teleports Audit Log size/md labels Apr 20, 2023

tobiaszheller requested a review from rosstimothy April 20, 2023 18:10

tobiaszheller added the backport/branch/v13 label Apr 21, 2023

smallinsky reviewed Apr 21, 2023

View reviewed changes

Comment thread lib/events/athena/querier.go Outdated

Comment thread lib/events/athena/querier.go Outdated

Comment thread lib/events/athena/querier.go Outdated

Comment thread lib/events/athena/querier.go Outdated

smallinsky self-requested a review April 21, 2023 08:37

smallinsky approved these changes Apr 21, 2023

View reviewed changes

rosstimothy reviewed Apr 21, 2023

View reviewed changes

Comment thread lib/events/athena/querier.go Outdated

Comment thread lib/events/athena/querier_test.go Outdated

tobiaszheller requested a review from rosstimothy April 24, 2023 13:18

rosstimothy reviewed Apr 24, 2023

View reviewed changes

smallinsky reviewed Apr 24, 2023

View reviewed changes

tobiaszheller requested a review from rosstimothy April 24, 2023 18:41

rosstimothy reviewed Apr 26, 2023

View reviewed changes

Comment thread lib/events/search_limiter.go Outdated

Comment thread lib/events/search_limiter.go Outdated

Comment thread lib/events/athena/athena.go Outdated

tobiaszheller requested review from rosstimothy and smallinsky April 27, 2023 10:13

rosstimothy reviewed Apr 28, 2023

View reviewed changes

tobiaszheller requested a review from rosstimothy April 28, 2023 13:13

smallinsky approved these changes Apr 28, 2023

View reviewed changes

rosstimothy approved these changes Apr 28, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from lxea April 28, 2023 13:23

athena audit logs - query rate limiter

d6c775c

tobiaszheller force-pushed the tobiaszheller/auditevents-athena-ratelimit-query branch from d7e491e to d6c775c Compare April 28, 2023 13:30

tobiaszheller enabled auto-merge April 28, 2023 13:30

tobiaszheller added this pull request to the merge queue Apr 28, 2023

Merged via the queue into master with commit 29497f5 Apr 28, 2023

tobiaszheller deleted the tobiaszheller/auditevents-athena-ratelimit-query branch April 28, 2023 14:02

tobiaszheller mentioned this pull request May 15, 2023

[v13] athena audit logs - query rate limiter #26221

Merged

Conversation

tobiaszheller commented Apr 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiaszheller commented Apr 24, 2023

Uh oh!

Uh oh!

Uh oh!

rosstimothy Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

smallinsky Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

tobiaszheller Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

smallinsky Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smallinsky Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

smallinsky Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

tobiaszheller commented Apr 24, 2023

Uh oh!

tobiaszheller commented Apr 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy left a comment

Choose a reason for hiding this comment

Uh oh!

tobiaszheller commented Apr 28, 2023

Uh oh!

public-teleport-github-review-bot Bot commented Apr 28, 2023

Uh oh!

public-teleport-github-review-bot Bot commented May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants