-
Notifications
You must be signed in to change notification settings - Fork 519
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: added EC2 image validation #2983
Conversation
#[derive(Debug, Deserialize)] | ||
pub(crate) struct ValidationConfig { | ||
/// Vec of paths to JSON files containing expected metadata (image ids and SSM parameters) | ||
expected_metadata_lists: Vec<PathBuf>, |
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.
Slightly confused. These are paths to additional configuration files? If so, be specific about whether absolute paths are required. Or, if relative paths are allowed, what are they relative to? i.e. would paths relative to this aggregating config file work?
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.
They can be relative paths, but they are relative to the pwd of the caller. I'll add that to the comments.
/// Structure of the validation configuration file | ||
#[derive(Debug, Deserialize)] | ||
pub(crate) struct ValidationConfig { | ||
/// Vec of paths to JSON files containing expected metadata (image ids and SSM parameters) |
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.
Is this comment a paste error? Are metadata
, image_ids
and parameters
the right nouns here?
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.
This is correct, because the input files are the same as for the SSM parameter validation. The parameters don't necessarily have to be there, but the file should follow the structure of region->image_id->{}.
let mut images = HashMap::new(); | ||
|
||
// Send the request | ||
let mut get_future = client |
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.
This is applicable to the implementation in your previous PR as well. I can't quite tell from reading this if things are happening truly in parallel? Can you tell from your tests how the performance will scale to 30 regions with 100s of AMIs in each?
I think the key question is whether the send()
function is returning a unordered collection of futures.
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.
The requests are sent completely in parallel. These image requests are fast so the difference isn't really noticeable. For the SSM parameters, before they were parallelized, the total duration was over 3 minutes. After parallelizing, it took 1 minute (which is how long it takes to get the parameters from the region with the highest latency, even if that region is the only one to be queried).
If there were 30 regions with 100s of AMIs, the runtime would be the same as for a single region with the highest latency of those 30.
/// Represent the possible status of an EC2 image validation | ||
#[derive(Debug, Eq, Hash, PartialEq, Serialize, Deserialize)] | ||
pub(crate) enum AmiValidationResultStatus { | ||
/// The expected value was equal to the actual value |
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.
In the case of AMIs can we be more specific than value
? In this case do we actually mean:
/// The AMI was found and is public
pub(crate) image_id: String, | ||
|
||
/// The expected value of the image | ||
pub(crate) expected_value: ImageDef, |
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.
This naming seems to be a holdover from SSM? Should this field be called expected_image_def
? Should the next field be called actual_image_def
?
// Determine the validation status based on equality, presence, and absence of expected and | ||
// actual image values | ||
let status = match (&expected_value, &actual_value) { | ||
(expected_value, Some(actual_value)) if actual_value.eq(expected_value) => { |
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.
slightly more idiomatic to use the operator
(expected_value, Some(actual_value)) if actual_value.eq(expected_value) => { | |
(expected_value, Some(actual_value)) if actual_value. == expected_value => { |
Added a `validate-ami` subcommand to pubsys to validate EC2 images, given a config file with regions and paths to files containing image ids.
38c79e1
to
dae703f
Compare
^ Rebased after #2969 got merged. |
Added a
validate-ami
subcommand to pubsys to validate EC2 images, given a config file with regions and paths to files containing image ids.Description of changes:
Added a validate_ami mod to the pubsys crate. This mod adds a subcommand validate-ami with the following signature:
Each expected_metadata_list should have the following structure:
The parameters inside the image-id objects are irrelevant, but this same file structure is used for SSM validation (#2969) so this allows both validations to use the same input files.
The command outputs a tabled summary of the validation results. This table will look like this (unless the
--json
flag is passed, in which case a JSON object will be printed):The meaning of the different columns is this:
The validation will check the following 3 fields with their expected values:
Public
: trueEnaSupport
: trueSriovNetSupport
:simple
Testing done:
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.