pubsys: added ami validation #3020

mjsterckx · 2023-04-14T19:05:33Z

Description of changes:

Added a validate-ami subcommand to the pubsys tool. This command has the following signature:

pubsys-validate-ami 0.1.0
Validates EC2 images

USAGE:
    pubsys --infra-config-path <infra-config-path> validate-ami [FLAGS] [OPTIONS] --expected-amis-path <expected-amis-path>

FLAGS:
        --json       If this argument is given, print the validation results summary as a JSON object instead of a
                     plaintext table
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
        --expected-amis-path <expected-amis-path>           File holding the expected amis
        --write-results-path <write-results-path>           Optional path where the validation results should be written
        --write-results-filter <write-results-filter>...
            Optional filter to only write validation results with these statuses to the above path The available
            statuses are: `Correct`, `Incorrect`, `Missing`
        --log-level <log-level>
            How much detail to log; from least to most: ERROR, WARN, INFO, DEBUG, TRACE [default: INFO]

The function takes a path to a file containing an image object per region. An example file looks like this:

{
  "us-west-2": {
    "id": "ami-012345678",
    "name": "bottlerocket-aws-k8s-1.24-x86_64-v1.14.0-abcdef-dirty",
    "public": true,
    "launch_permissions": [
      {
        "group": "all"
      }
    ]
  }
}

The function will call the describe-images function on each image in the file to ensure that the retrieved id, name, and publicity match the expected values. If public is false, then the specific launch_permissions are retrieved using describe-image-attribute and included in the comparison. Aside from publicity and launch_permissions, the function will check that sriov_net_support is simple and ena_support is true.

The result of the function looks like this:

+-----------+---------+-----------+---------+------------+
| String    | correct | incorrect | missing | unreachable |
+-----------+---------+-----------+---------+------------+
| us-west-2 | 0       | 0         | 1       | 0          |
+-----------+---------+-----------+---------+------------+

correct indicates that all retrieved values match the expected values
incorrect indicates that at least one value does not match the expected value
missing indicates that the specific image failed to be retrieved
unreachable indicates that the images in this region could not be retrieved because the region could not be reached

--write-results-path and --write-results-filter allow the user to write the results of the ami validation to a JSON file. The filter determines which statuses will be written to this file. The file looks like this:

[
  {
    "id": "ami-012345678",
    "expected_image_def": {
      "id": "ami-012345678",
      "name": "bottlerocket-aws-k8s-1.24-x86_64-v1.14.0-abcdef-dirty",
      "public": true,
      "launch_permissions": null,
      "ena_support": true,
      "sriov_net_support": "simple"
    },
    "actual_image_def": null,
    "region": "us-west-2",
    "status": "Missing"
  }
]

Modified the SSM validation results to represent the new Unreachable status as well.

Testing done:

Unit tests
Ran the program on an unregistered ami (results seen above) and on registered public ones, which showed as correct.

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.

cbgbt

Nice work! Most of these are just nitpicks or tips. The thing that I care the most to see changed is to add a test for the AMI file parsing code. The function is a pure function and the logic inside is complex enough to warrant it.

cbgbt · 2023-04-14T20:44:40Z

tools/pubsys/src/aws/validate_ami/mod.rs

+    write_results_path: Option<PathBuf>,
+
+    #[structopt(long, requires = "write-results-path")]
+    /// Optional filter to only write validation results with these statuses to the above path


nit, but the help text looks funky without this.

Suggested change

/// Optional filter to only write validation results with these statuses to the above path

/// Optional filter to only write validation results with these statuses to the above path.

cbgbt · 2023-04-14T21:04:13Z

tools/pubsys/src/aws/validate_ami/ami.rs

+
+pub(crate) async fn describe_images<'a>(


Could use a docstring here to explain why we need this.

cbgbt · 2023-04-14T21:10:31Z

tools/pubsys/src/aws/validate_ami/ami.rs

+    region: &Region,
+    client: &Ec2Client,
+    image_ids: Vec<String>,
+    expected_image_public: &HashMap<String, bool>,


I'd document what the purpose of this parameter is in the docstring, since it's not obvious from the function signature.

cbgbt · 2023-04-14T21:44:14Z

tools/pubsys/src/aws/validate_ami/results.rs

+    correct: i32,
+    incorrect: i32,
+    missing: i32,
+    accessible: bool,


I don't have strong feelings about rewriting this, but I'm realizing that because accessible removes meaning from the other values here, I think a more maintainable way to write this would be to:

Change AmiValidationRegionSummary to an enum of something like

enum AmiValiationRegionSummary { Results { correct: u64, // etc }, Unreachable,

Modify display() to render this by copying it to a private implementation which uses the marker values, or n/a or something similar in the table.

It's currently implemented the same way in the ssm-validation so I will address those together in a different PR.

cbgbt · 2023-04-14T23:55:49Z

tools/pubsys/src/aws/validate_ami/mod.rs

+                }),
+            )
+        })
+        .collect::<HashMap<Region, ami::Result<HashSet<AmiValidationResult>>>>();


Bit of a gnarly turbofish that probably isn't needed, since there's a type specifier on the variable binding.

Suggested change

.collect::<HashMap<Region, ami::Result<HashSet<AmiValidationResult>>>>();

.collect();

cbgbt · 2023-04-14T23:57:30Z

tools/pubsys/src/aws/validate_ami/mod.rs

+        })
+        .collect::<HashMap<Region, ami::Result<HashSet<AmiValidationResult>>>>();
+
+    let validation_results = AmiValidationResults::new(results);


nit, but usually new isn't appropriate when you're initializing something that's computation heavy like that. A better name would be from_result_map or evaluate_results() or something.

Suggested change

let validation_results = AmiValidationResults::new(results);

let validation_results = AmiValidationResults::new(results);

cbgbt · 2023-04-15T00:06:46Z

tools/pubsys/src/aws/validate_ami/mod.rs

+    // If the value for each Region is a JSON object instead of an array, wrap it in an array
+    let vectored_images = expected_amis
+        .into_iter()
+        .map(|(region, value)| {
+            let image_values = match value.as_array() {


Can we write a test for this behavior? As an alternative tip, you can get Serde to parse this for you "for free" via #[serde(untagged)] on an enum.

enum ImageDef { Image(Image), ImageList(Vec<Image>), } impl ImageDef { fn images(&self) -> Vec<Image> { match self { // etc } } }

ecpullen

Still need to check validate_ami/results.rs

ecpullen · 2023-04-17T21:25:06Z

tools/pubsys/src/aws/validate_ami/ami.rs

+    let mut requests = Vec::with_capacity(clients.len());
+    for region in clients.keys() {
+        trace!("Requesting images in {}", region);
+        let ec2_client: &Ec2Client = &clients[region];


Suggested change

let ec2_client: &Ec2Client = &clients[region];

let ec2_client: &Ec2Client = &clients.get(region).context()...;

Indexing with [] can panic.

ecpullen · 2023-04-17T21:31:02Z

tools/pubsys/src/aws/validate_ami/ami.rs

+) -> HashMap<&'a Region, Result<HashMap<String, ImageDef>>> {
+    // Build requests for images; we have to request with a regional client so we split them by
+    // region
+    let mut requests = Vec::with_capacity(clients.len());


Using an iterator for this function could simplify some of the logic below.

ecpullen · 2023-04-17T21:46:30Z

tools/pubsys/src/aws/validate_ami/ami.rs

+                region.as_ref()
+            );
+            let mut image_def = ImageDef::from(image.to_owned());
+            if !*expected_public {


Can this go above 141 and then make ImageDef::from() take in the bool so image_def doesn't need to be mut?

ecpullen · 2023-04-17T21:48:58Z

tools/pubsys/src/aws/validate_ami/mod.rs

+    let infra_config = InfraConfig::from_path_or_lock(&args.infra_config_path, false)
+        .context(error::ConfigSnafu)?;
+
+    let aws = infra_config.aws.clone().unwrap_or_default();


Does this need .clone()

ecpullen · 2023-04-17T21:54:43Z

tools/pubsys/src/aws/validate_ami/mod.rs

+    info!("Parsed expected ami file");
+
+    // Create a HashMap of AmiClients, one for each region where validation should happen
+    let base_region = &Region::new(aws.regions[0].clone());


Suggested change

let base_region = &Region::new(aws.regions[0].clone());

let base_region = &Region::new(aws.regions.get(0).context()...);

[] can panic

ecpullen · 2023-04-17T22:17:35Z

tools/pubsys/src/aws/validate_ami/mod.rs

+/// Validates EC2 images in a single region, based on a Vec (ImageDef) of expected images
+/// and a HashMap (AmiId, ImageDef) of actual retrieved images. Returns a HashSet of


Super nit

Suggested change

/// Validates EC2 images in a single region, based on a Vec (ImageDef) of expected images

/// and a HashMap (AmiId, ImageDef) of actual retrieved images. Returns a HashSet of

/// Validates EC2 images in a single region, based on a `Vec<ImageDef>` of expected images

/// and a `HashMap<AmiId, ImageDef>` of actual retrieved images. Returns a `HashSet<AmiValidationResult>` containing ...

ecpullen · 2023-04-17T22:20:10Z

tools/pubsys/src/aws/validate_ami/mod.rs

+    expected_images: Vec<ImageDef>,
+    actual_images: &HashMap<AmiId, ImageDef>,


Is there a reason 1 is a reference and the other is taking ownership?

ecpullen · 2023-04-17T22:21:19Z

tools/pubsys/src/aws/validate_ami/mod.rs

+        if image.public {
+            image.launch_permissions = None;
+        }


Why isn't image.launch_permissions set to None when the ImageDef is created? If that's not possible can launch_permissions be a function?

ecpullen · 2023-04-17T22:22:36Z

tools/pubsys/src/aws/validate_ami/mod.rs

+            image.id.clone(),
+            image.clone(),


Do we need to duplicate the image.id?

ecpullen · 2023-04-17T22:25:02Z

tools/pubsys/src/aws/validate_ami/mod.rs

+    )
+    .context(error::ParseExpectedImagesFileSnafu)?;
+
+    // If the value for each Region is a JSON object instead of an array, wrap it in an array


What would cause the value to be an object instead of an array? This is a file that another pubsys command is writing right?

Correct, this takes a file that is written by pubsys ami where each region has a single AMI object. However, I wanted to add functionality where the command can take a file containing an array of AMI objects per region as well.

ecpullen

+1 to the suggested AmiValidationRegionSummary refactor. Otherwise just a few small things.

ecpullen · 2023-04-17T22:32:33Z

tools/pubsys/src/aws/validate_ami/results.rs

+impl Display for AmiValidationResultStatus {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            AmiValidationResultStatus::Correct => write!(f, "Correct"),
+            AmiValidationResultStatus::Incorrect => write!(f, "Incorrect"),
+            AmiValidationResultStatus::Missing => write!(f, "Missing"),
+        }
+    }
+}


Suggested change

impl Display for AmiValidationResultStatus {

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {

match self {

AmiValidationResultStatus::Correct => write!(f, "Correct"),

AmiValidationResultStatus::Incorrect => write!(f, "Incorrect"),

AmiValidationResultStatus::Missing => write!(f, "Missing"),

}

}

}

derive_display_from_serialize!(AmiValidationResultStatus);

That will have the same effect. And expand if new variants of the enum are added.

ecpullen · 2023-04-17T22:40:26Z

tools/pubsys/src/aws/validate_ami/results.rs

+impl FromStr for AmiValidationResultStatus {
+    type Err = super::Error;
+
+    fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
+        match s {
+            "Correct" => Ok(Self::Correct),
+            "Incorrect" => Ok(Self::Incorrect),
+            "Missing" => Ok(Self::Missing),
+            filter => Err(Self::Err::InvalidStatusFilter {
+                filter: filter.to_string(),
+            }),
+        }
+    }
+}


Suggested change

impl FromStr for AmiValidationResultStatus {

type Err = super::Error;

fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {

match s {

"Correct" => Ok(Self::Correct),

"Incorrect" => Ok(Self::Incorrect),

"Missing" => Ok(Self::Missing),

filter => Err(Self::Err::InvalidStatusFilter {

filter: filter.to_string(),

}),

}

}

}

derive_fromstr_from_deserialize!(AmiValidationResultStatus)

ecpullen · 2023-04-17T22:42:45Z

tools/pubsys/src/aws/validate_ami/results.rs

+    pub(crate) region: Region,
+
+    /// The validation status of the image
+    pub(crate) status: AmiValidationResultStatus,


This should just be a function since it is described by other fields of this enum.

I think I suggested this be added as a field in a prior revision, since it's otherwise not really a Result being computed, but rather a validation computation being created which is later used to compute the result.

I guess if we renamed this AmiValidation with a method AmiValidation::get_result() or something, I'd be ok with that.

But aren't these serialized? Do we want to keep the status so that the computation result is specifically kept?

etungsten · 2023-04-18T18:03:34Z

tools/pubsys/src/aws/validate_ami/mod.rs

+                        expected_images
+                            .get(region)
+                            .map(|e| e.to_owned())
+                            .unwrap_or(vec![]),


nit:

Suggested change

.unwrap_or(vec![]),

.unwrap_or_default();,

etungsten · 2023-04-18T18:11:47Z

tools/pubsys/src/aws/validate_ami/mod.rs

+
+    #[derive(Debug, Snafu)]
+    #[snafu(visibility(pub(super)))]
+    pub(crate) enum Error {


Please check the error enum variants and ensure they're being used. I see a few that aren't being used anywhere. e.g. ReadValidationConfig, ParseValidationConfig, MissingField etc.

Please check the error enum variants and ensure they're being used. I see a few that aren't being used anywhere. e.g. ReadValidationConfig, ParseValidationConfig, MissingField etc.

As an aside, the reason you have to check is because the code that Snafu generates "uses" each enum variant so the compiler can't warn you about unused variants.

etungsten · 2023-04-18T18:14:42Z

tools/pubsys/src/aws/validate_ami/results.rs

+/// Represents a single EC2 image validation result
+#[derive(Debug, Eq, Hash, PartialEq, Serialize)]
+pub(crate) struct AmiValidationResult {
+    /// The id of the image


nit: proper capitalization

Suggested change

/// The id of the image

/// The ID of the image

and where ever else this may apply

etungsten · 2023-04-18T18:15:50Z

tools/pubsys/src/aws/validate_ami/results.rs

+    /// The id of the image
+    pub(crate) id: String,
+
+    /// ImageDef containing expected values for the image


nit:

Suggested change

/// ImageDef containing expected values for the image

/// `ImageDef` containing expected values for the image

and where ever else this may apply.

webern

I love the testing.

webern · 2023-04-19T16:29:44Z

tools/pubsys/src/aws/validate_ami/mod.rs

+use std::path::PathBuf;
+use structopt::{clap, StructOpt};
+
+/// Validates EC2 images


Describe this in more detail. What's the input, what's the output, etc.

webern · 2023-04-19T16:33:13Z

tools/pubsys/src/aws/validate_ami/mod.rs

+
+    let validation_results = AmiValidationResults::new(results);
+
+    // If a path was given to write the results to, write the results


Writing nit

Suggested change

// If a path was given to write the results to, write the results

// If a path was given, write the results

webern · 2023-04-19T16:34:13Z

tools/pubsys/src/aws/validate_ami/mod.rs

+
+    #[derive(Debug, Snafu)]
+    #[snafu(visibility(pub(super)))]
+    pub(crate) enum Error {


Please check the error enum variants and ensure they're being used. I see a few that aren't being used anywhere. e.g. ReadValidationConfig, ParseValidationConfig, MissingField etc.

As an aside, the reason you have to check is because the code that Snafu generates "uses" each enum variant so the compiler can't warn you about unused variants.

etungsten · 2023-04-19T20:00:43Z

tools/pubsys/src/aws/validate_ami/ami.rs

+        #[snafu(display("Failed to describe images in {}: {}", region, source))]
+        DescribeImages {
+            region: String,
+            source: SdkError<DescribeImagesError>,
+        },


Where-ever we're using a SdkError as source we need to do this to get the full error message:

Suggested change

#[snafu(display("Failed to describe images in {}: {}", region, source))]

DescribeImages {

region: String,

source: SdkError<DescribeImagesError>,

},

use aws_smithy_types::error::display::DisplayErrorContext;

#[snafu(display("Failed to describe images in {}: {}", region, DisplayErrorContext(source)))]

DescribeImages {

region: String,

source: SdkError<DescribeImagesError>,

},

See https://docs.rs/aws-smithy-client/latest/aws_smithy_client/enum.SdkError.html

When logging an error from the SDK, it is recommended that you either wrap the error in DisplayErrorContext, use another error reporter library that visits the error’s cause/source chain, or call Error::source for more details about the underlying cause.

Note: This needs to be a tree wide change, but might as well start here.

Please create an issue for the other places if you're willing.

cbgbt

Looks good!

Sorry to pile on more feedback -- this is a substantial change! Just some tidying up.

cbgbt · 2023-04-19T22:58:24Z

tools/pubsys/src/aws/validate_ami/ami.rs

+    }
+}
+
+/// Fetches all images with IDs in `image_ids`. The map `expected_image_public` is used to


It looks like the docstrings for this and describe_images_in_region may not have been updated for some subsequent changes to the code. I would fix this before merging.

cbgbt · 2023-04-19T23:38:32Z

tools/pubsys/src/aws/validate_ami/mod.rs

+                match result {
+                    Ok(result) => Ok(result),
+                    Err(_) => Err(error::Error::UnreachableRegion {


nit/tip (don't feel compelled to change this): I think this is the same as

result.map_err(|_| error::Error::UnreachableRegion { region: region.to_string() })

match on Result and Option when the operation to be carried out is already expressed as a method on those structs tends to result in deep nesting.

cbgbt · 2023-04-19T23:43:28Z

tools/pubsys/src/aws/validate_ssm/results.rs

    incorrect: i32,
    missing: i32,
    unexpected: i32,
-    accessible: bool,
+    unreachable: i32,


nit: these should probably be unsigned.

cbgbt · 2023-04-19T23:50:06Z

tools/pubsys/src/aws/validate_ssm/mod.rs

+                    Err(_) => Err(error::Error::UnreachableRegion {
+                        region: region.to_string(),
+                    }),


Does it make sense for this to hold onto the original error?

I think we just want to make sure the reason for the unavailability can be surfaced. Ensuring that it is logged somewhere at ERROR level probably suffices, since it's not like the deeper result information is stored in our serialized format.

mjsterckx requested review from cbgbt, rpkelly and webern April 14, 2023 19:05

cbgbt reviewed Apr 15, 2023

View reviewed changes

ecpullen reviewed Apr 17, 2023

View reviewed changes

etungsten reviewed Apr 18, 2023

View reviewed changes

webern reviewed Apr 19, 2023

View reviewed changes

etungsten reviewed Apr 19, 2023

View reviewed changes

mjsterckx force-pushed the pubsys-ami-validation branch from 57dc749 to 0e83ef4 Compare April 19, 2023 20:54

cbgbt reviewed Apr 19, 2023

View reviewed changes

mjsterckx added 2 commits April 20, 2023 00:13

pubsys: added ami validation

78fb74e

pubsys: added 'Unreachable' status to SsmValidationResultStatus

2e8b67d

mjsterckx force-pushed the pubsys-ami-validation branch from 0e83ef4 to 2e8b67d Compare April 20, 2023 00:14

cbgbt approved these changes Apr 20, 2023

View reviewed changes

etungsten approved these changes Apr 20, 2023

View reviewed changes

mjsterckx merged commit 99b5bf1 into bottlerocket-os:develop Apr 20, 2023

mjsterckx deleted the pubsys-ami-validation branch April 20, 2023 02:55

mjsterckx mentioned this pull request Apr 21, 2023

Makefile: added SSM and AMI validation targets #3043

Merged

	/// Optional filter to only write validation results with these statuses to the above path
	/// Optional filter to only write validation results with these statuses to the above path.

	.collect::<HashMap<Region, ami::Result<HashSet<AmiValidationResult>>>>();
	.collect();

	let validation_results = AmiValidationResults::new(results);
	let validation_results = AmiValidationResults::new(results);

	let ec2_client: &Ec2Client = &clients[region];
	let ec2_client: &Ec2Client = &clients.get(region).context()...;

	let base_region = &Region::new(aws.regions[0].clone());
	let base_region = &Region::new(aws.regions.get(0).context()...);

		/// Validates EC2 images in a single region, based on a Vec (ImageDef) of expected images
		/// and a HashMap (AmiId, ImageDef) of actual retrieved images. Returns a HashSet of

		expected_images: Vec<ImageDef>,
		actual_images: &HashMap<AmiId, ImageDef>,

	/// ImageDef containing expected values for the image
	/// `ImageDef` containing expected values for the image


		let validation_results = AmiValidationResults::new(results);

		// If a path was given to write the results to, write the results

	// If a path was given to write the results to, write the results
	// If a path was given, write the results

pubsys: added ami validation #3020

pubsys: added ami validation #3020

Conversation

mjsterckx commented Apr 14, 2023 • edited Loading

cbgbt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecpullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecpullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

mjsterckx commented Apr 14, 2023 •

edited

Loading

cbgbt Apr 19, 2023 •

edited

Loading

cbgbt Apr 19, 2023 •

edited

Loading