-
Notifications
You must be signed in to change notification settings - Fork 522
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
pubsys: Do not allow private AMIs to be published to SSM parameters in the public namespace #2680
Conversation
ab22561
to
75c5cc8
Compare
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.
Looks good to me once the build failure is addressed. Great to have this!
tools/pubsys/src/aws/ssm/mod.rs
Outdated
@@ -156,8 +210,56 @@ pub(crate) async fn run(args: &Args, ssm_args: &SsmArgs) -> Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
// Determines the number of AMIs that can be checked for public permissions in parallel. | |||
const CONCURRENT_AMI_CHECKS: usize = 16; |
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.
Would it be better to set this at runtime based on the running host's number of cores?
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.
Since this is network-bound, I picked an arbitrarily "low" number of concurrent checks. I've found that you often hit throttle limits trying to hit AWS using all of your cores if your host is too beefy. This can be rectified with more careful throttling avoidance, but I opted for a simpler route here.
I should probably clarify why the number was chosen in a comment.
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.
Interesting, I was actually worried this number was too high. I was thinking something like Min(cpu_cores, 16)
, but even wondering if that should be more like 8
than 16
.
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.
8
is probably a safer bet!
d918b80
to
30b7f0d
Compare
|
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.
I'm still learning Rust for concurrency things, so I didn't review those specifics, but otherwise it looks good to me.
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.
Nice! Just have a few non-blocking questions and small suggestions.
90912b1
to
9679767
Compare
Addressed @etungsten's feedback
|
licensing issue with the governor
crate
9679767
to
a89fb03
Compare
|
Description of changes:
This change by default will disallow publishing SSM parameters to the public
/aws/
SSM namespace if they refer to private AMIs.Testing done:
/aws/service/doggierocket
namespace. ( 🐶 )--allow-private-images
passes this check (but then fails because we are trying to update parameters that don't exist)Originally I sought to use Traits and fakes to provide additional automated testing here, but it was pretty difficult to do it in a way that didn't require more significant refactoring.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.