Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix access list reminder notifications for access lists overdue by more than 8 days #52820

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

rudream
Copy link
Contributor

@rudream rudream commented Mar 5, 2025

Purpose

This PR resolves #52808

This PR fixes a flaky test, as well as a bug that would have prevented access lists due by more than 8 days from triggering a notification.

Context

While implementing the feature, I added a memory optimization that would only process access lists that fall between our notification thresholds (due in less than 14 days<->overdue by more than 7 days) to avoid keeping in memory and processing irrelevant access lists. However the way the latter limit was implemented (by excluding those overdue by more than 8 days) was incorrect since technically an access list could be created with a review due date more than 8 days ago, for which we would want a "overdue by more than 7 days" notification.

This bug is very unlikely to ever affect anyone in a real cluster, since for a real access list to be overdue by more than 8 days, it must have been overdue by 7 days first, and then 8 days, and thus have triggered the notification previously. The only way to trigger this bug is by initially creating the access list with a hardcoded due date value already more than 8 days in the past, which happens to be what we do in the unit test.

I suspect the flakiness came from timing issues when running the tests in CI, since the test access list was created with a due date of exactly 8 days ago, and the exclusion threshold is also 8 days and would therefore exclude it if time passed between the time the access list was created and the threshold check.

@rudream rudream added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Mar 5, 2025
@rudream rudream requested a review from zmb3 March 5, 2025 20:48
@rudream rudream force-pushed the yassine/fix-notifications-reminder branch from ff5f72f to 923a903 Compare March 5, 2025 20:49
@github-actions github-actions bot requested review from avatus and Joerger March 5, 2025 20:49
@rudream rudream force-pushed the yassine/fix-notifications-reminder branch from 923a903 to 20471d9 Compare March 5, 2025 20:49
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from Joerger March 6, 2025 16:58
@rudream rudream added this pull request to the merge queue Mar 6, 2025
Merged via the queue into master with commit bdd25bc Mar 6, 2025
43 of 44 checks passed
@rudream rudream deleted the yassine/fix-notifications-reminder branch March 6, 2025 17:19
@public-teleport-github-review-bot

@rudream See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestCreateAccessListReminderNotifications flakiness
3 participants