Re-introduce access list read rate limiting#62670
Conversation
This PR re-introduces access list read rate limiting removed in #62649. The PR had merge enabled and was merged without addressing the feedback. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
tigrato
added
the
no-changelog
| err = clientutils.IterateResources(ctx, a.Cache.ListAccessLists, func(al *accesslist.AccessList) error { | ||
| if !al.IsReviewable() { | ||
| return nil | ||
| var accessListsPageKey string |
There was a problem hiding this comment.
WDYT about keeping IterateResources and just putting a time.Sleep at the end of the closure? It's only 5ms, so if the context is canceled it won't take us long to detect.
I'm just weary of continuing to hand-write pagination code given the dozens of pagination bugs we've had. Now that we've got a well-tested and uniform tool, I think we should use it wherever possible.
There was a problem hiding this comment.
The reason I removed it in the first place is that if the sleep is inside the closure, it will sleep 20× more than intended. That’s because the IterateResources closure is invoked per item, not per page.
I could technically sleep for 1/20 of the time, but I think it’s better to keep the code as it is.
I could also add another iterator that operates on pages instead, but that would likely make the code more confusing.
public-teleport-github-review-bot
bot
removed request for
doggydogworld and
fspmarshall
3 participants
This PR re-introduces access list read rate limiting removed in #62649. The PR had merge enabled and was merged without addressing the feedback.