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

Audit log missing entries #353

Open
lyricnz opened this issue Jun 12, 2024 · 4 comments
Open

Audit log missing entries #353

lyricnz opened this issue Jun 12, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@lyricnz
Copy link
Contributor

lyricnz commented Jun 12, 2024

The audit log from a run last night appears to be missing entries:

  • revolver ran at 6pm AEST (0600Z), and stopped a bunch of EC2 in one account, which can be seen from audit-console log.
image
  • but these are missing completely from the audit.log in S3 at the same time

Suspect race-condition in appending to the S3 audit log? Multiple accounts are running at the same time, and the S3 file writer doesn't have native/atomic file append, so fakes this by reading contents + appending + writing, which could result in lost-writes.

image
@lyricnz
Copy link
Contributor Author

lyricnz commented Jun 13, 2024

Technically this can happen with files too, not just S3, since fs.writeFile() is not atomic.

@lyricnz lyricnz added the bug Something isn't working label Jun 13, 2024
@lyricnz
Copy link
Contributor Author

lyricnz commented Jun 13, 2024

@abukharov thoughts?

  • a mutex (on output filename)
  • buffering all the output and writing at the end
  • saying "just don't do that" and making users configure unique output files for each account+region? Which makes for ugly+fragmented audit logs:
    • audit.aws-whatever-account-name-nonprod.ap-southeast-2.csv
    • audit.aws-whatever-account-name-nonprod.ap-southeast-4.csv
    • audit.aws-whatever-account-name-nonprod.ap-somewhere-2.csv
    • ...

@lyricnz
Copy link
Contributor Author

lyricnz commented Jun 27, 2024

For emergency fix, I split the files to be unique by account+region, but this makes for rather messy output directory/S3 bucket.

@lyricnz
Copy link
Contributor Author

lyricnz commented Jul 3, 2024

As discussed with Alex: rather than implementing any locking/mutex in AccountRevolver, the logging could be deferred until all the accounts are completed, then merged, and emitted at the global scope. I'm going to delay implementing this for now, as the workaround (per-account output files) is OK for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant