[v18] Fix memory leak in access list reminder notifications#62663
[v18] Fix memory leak in access list reminder notifications#62663tigrato merged 4 commits intobranch/v18from
Conversation
Fixes a memory leak caused by variable shadowing where `nextKey` was redeclared in the pagination loop instead of being assigned. This caused the loop to always pass an empty pagination token, fetching the same page repeatedly and never terminating for tenants with more than 1000 pending access list review notifications. The fix renames the loop variable to `notificationsPageKey` to avoid shadowing and properly updates it with the next page token. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
rosstimothy
left a comment
rosstimothy
left a comment
There was a problem hiding this comment.
Can we restore the rate limiting that was removed. It looks like Zac's comment was ignored on the original PR.
We read ALs from the cache. All access lists are read from the cache much more frequently, including on every login, so I don’t see an issue here. |
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>
It wasn't obvious to me that you'd seen the comment, thought about it, or had rationale for removing it since the comment wasn't replied to. I'm not sure why the throttling was added in the first place. Let's give @fspmarshall a chance to chime in before we remove it entirely. |
cherry-picked from #62670 |
I didn't saw it. the PR had auto-merge enabled and since zac approved it, it merged |
public-teleport-github-review-bot
bot
removed the request for review
from kopiczko
4 participants
Backport #62649 to branch/v18
changelog: Fixed a memory leak in access list reminder notifications affecting clusters with more than 1000 pending Access List reviews.