-
Notifications
You must be signed in to change notification settings - Fork 127
Fix secret scope ACL deletion loop exiting early on NotFound #4212
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
base: main
Are you sure you want to change the base?
Conversation
| title "Manually delete one ACL to simulate race condition" | ||
| # This simulates a race condition where an ACL is deleted between when we | ||
| # list them and when bundle deploy tries to delete them | ||
| trace $CLI secrets delete-acl test-scope [email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can sequential call before "bundle deploy" simulate race condition?
If I understand correctly the race condition this refers too is the one in setACLs() function which indeed does list + delete,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out this was a red herring when investigating the flaky test. It happens exclusively with TF.
The fix in this PR is real but the acceptance test doesn't properly test the fix. It can be useful in general though.
I will remove the acceptance test from this PR and verify that it adds real coverage before submitting it again.
| # list them and when bundle deploy tries to delete them | ||
| trace $CLI secrets delete-acl test-scope [email protected] | ||
|
|
||
| trace $CLI bundle deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: add readplan variant to this test, it's a few lines of bash to make it test that this still works with deploy --plan:
% git grep -i readplan acceptance/bundle/resources/jobs/delete_task/
acceptance/bundle/resources/jobs/delete_task/script:$CLI bundle deploy $(readplanarg out.plan_create.direct.json)
acceptance/bundle/resources/jobs/delete_task/script:$CLI bundle deploy $(readplanarg out.plan_update.direct.json)
acceptance/bundle/resources/jobs/delete_task/test.toml:EnvMatrix.READPLAN = ["", "1"]
|
Commit: 9cc0aa3
40 interesting tests: 24 KNOWN, 11 FAIL, 4 flaky, 1 SKIP
Top 32 slowest tests (at least 2 minutes):
|
When deleting multiple secret scope ACLs, if one ACL returns NotFound (due to race conditions or eventual consistency), the code would return nil and exit the loop early. This prevented remaining ACLs from being deleted. Changed to continue to skip NotFound errors and proceed with remaining deletions.
9cc0aa3 to
ba3d55f
Compare
Changes
When deleting multiple secret scope ACLs, if one ACL returns NotFound (due to race conditions or eventual consistency), the code would
return niland exit the loop early. This prevented remaining ACLs from being deleted. Changed tocontinueto skip NotFound errors and proceed with remaining deletions.Tests
Only static analysis, no tests.