pubsys: write SSM parameters and AMI permissions to output files #2987

mjsterckx · 2023-04-06T22:03:18Z

Description of changes:

Modified the pubsys ssm and pubsys promote-ssm commands to allow the parameters to be written to a file. Modified the pubsys ami and pubsys publish-ami commands to have them write AMI launch permissions and publicity info to the output file.

pubsys ssm has one new argument: ssm_parameter_output. If set, the rendered parameters will be written to the file at this path with the following structure:

{
  "<region-name>": {
    "<parameter-name>": "<parameter-value>",
    ...
  }
}

pubsys promote-ssm has one new argument: ssm_parameter_output. The newly promoted parameters will be combined with the parameters already present in the file and written to it.
pubsys ami now writes public and launch_permissions to the output file along with id and name. The public field is set to false by default when registering the AMI, but if the AMI ID has already been registered before, then the field is set to the publicity of that AMI. The launch_permissions field gets an empty vec by default, but if the AMI is already registered, then the launch permissions for that AMI are retrieved and stored in that field.
pubsys publish-ami updates the public and launch_permissions fields in the input file if anything has changed after the publishing has finished.

Testing done:

Unit tests
Ran cargo make, cargo make ami, cargo make ssm with the new argument, and got a file like this:

{
  "us-west-2": {
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_id": "ami-012345678",
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_version": "1.14.0-f782c96d-dirty"
  }
}

The AMI file looked like this:

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

Ran cargo make promote-ssm with the above file as input and keep_original_parameters false, and got a file like this:

{
  "us-west-2": {
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0/image_version": "1.14.0-f782c96d-dirty",
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0/image_id": "ami-012345678",
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_id": "ami-012345678",
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_version": "1.14.0-f782c96d-dirty"
  }
}

Ran cargo make ami-public and the amis.json file looked like this afterwards:

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

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.

bcressey · 2023-04-06T22:49:04Z

{
  "us-west-2": {
    "ami-012345678": {
      "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_id": "ami-012345678",
      "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_version": "1.14.0-f782c96d-dirty"
    }
  }
}

Why not always write the file, the way that pubsys ami always writes amis.json? That would have these benefits:

cargo make ssm and cargo make promote-ssm would just work and keep ssm-parameters.json up to date
the input / output / keep original distinctions could go away, since the input and output would be the same
overall the behavior would be more like pubsys ami which reduces developer surprise

I am also not convinced that everything should go under an AMI key like ami-012345678. The image_version field is unrelated to the AMI, and the image_id field duplicates that information.

A flattened map like this seems more intuitive:

{
  "us-west-2": {
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_id": "ami-012345678",
    "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_version": "1.14.0-f782c96d-dirty",
    "/msterckx/aws-k8s-1.24/x86_64/latest/image_id": "ami-012345678",
    "/msterckx/aws-k8s-1.24/x86_64/latest/image_version": "1.14.0-f782c96d-dirty"
  }
}

mjsterckx · 2023-04-06T22:55:53Z

The goal of this code was to generate files that were easily consumed by the SSM validation and EC2 validation code (#2969 and #2983). I should've probably made that more clear in the description.

Doing it this way allows us to create a file for version 1.14.0. and a separate file for latest, which is what we use for the validation. Having the AMI id as a key lets the EC2 validation use the same files to check all of the images in there, without having to parse each parameter to see if it contains an image id.

bcressey · 2023-04-07T20:41:34Z

The goal of this code was to generate files that were easily consumed by the SSM validation and EC2 validation code (#2969 and #2983).

I don't think it makes sense for the SSM validation code to be organizing parameters under an AMI ID - the AMI ID is irrelevant there. All the SSM validation should care about is whether the current parameter values match the expected values.

For EC2 validation, it kind of does seem like the validator logic should be going through and finding the subset of parameters that refer to an AMI ID. But if you wanted something more robust than a suffix match, you could organize it slightly differently:

{
  "us-west-2": {
    "image_ids": {
      "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_id": "ami-012345678",
      "/msterckx/aws-k8s-1.24/x86_64/latest/image_id": "ami-012345678",
    },
    "image_versions": {
      "/msterckx/aws-k8s-1.24/x86_64/1.14.0-f782c96d-dirty/image_version": "1.14.0-f782c96d-dirty",
      "/msterckx/aws-k8s-1.24/x86_64/latest/image_version": "1.14.0-f782c96d-dirty"
    }
  }
}

Doing it this way allows us to create a file for version 1.14.0. and a separate file for latest, which is what we use for the validation.

For pubsys / makefile integration, having the ability to easily work with all published artifacts for a specific build ID (like 1.14.0-f782c96d-dirty) is valuable. If I make AMIs and publish SSM parameters, I should be able to run cargo make validate-ssm and have it validate everything. If I subsequently promote SSM parameters, I'd expect cargo make validate-ssm to validate all the SSM parameters I've promoted. I would not expect to need to pass different arguments or point these operations to different JSON files; this should all just be maintained for me in the JSON file that resides in build artifacts.

For other use cases, like an account-wide ssm-params.json covering all SSM parameters ever published, a single unified file also seems to make the most sense. It'd be pretty easy to generate that file by concatenating all the files together, overwriting fields that are also present in later files (like "latest" params). And then the code path for doing the validation would be the same as in the local developer case.

mjsterckx · 2023-04-12T23:46:00Z

^ Addressed @bcressey's suggestions and added the writing of AMI publicity info and launch permissions to the pubsys ami and pubsys publish-ami commands.

cbgbt · 2023-04-13T04:40:32Z

It would be a lot easier to review this if it were split into two commits, one for each feature. Would you mind resubmitting it?

mjsterckx · 2023-04-13T14:28:34Z

^ Split the one commit into two.

cbgbt · 2023-04-13T20:28:06Z

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

@@ -372,20 +418,63 @@ async fn _run(args: &Args, ami_args: &AmiArgs) -> Result<HashMap<String, Image>>
    Ok(amis)
 }

+#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq, Hash)]
+pub(crate) struct LaunchPermissionDef {


This seems like it should be an enum, something like:

#[derive(Serialize, Deserialize, etc)] #[serde(rename_all = "lowercase")] enum LaunchPermissionDef { Group(String), UserId(String), OrganizationArn(String), OrganizationalUnitArn(String), }

I'm pretty sure this still serializes to JSON the way you want.

cbgbt · 2023-04-13T20:42:12Z

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

+    fn from(launch_permission: LaunchPermission) -> Self {
+        Self {
+            group: launch_permission
+                .group()
+                .map(|group| group.as_str().to_owned()),
+            user_id: launch_permission
+                .user_id()


Supposing you do make this an enum, you could rewrite this pretty succinctly with a match:

let LaunchPermission { group, user_id, organization_arn, organizational_unit_arn } = launch_permission; match (group, user_id, organization_arn, organizational_unit_arn) { (Some(group), None, None, None) => LaunchPermissionDef::Group(group), (None, Some(user_id), None, None) => LaunchPermissionDef::UserId(user_id), (None, None, Some(organization_arn), None) => LaunchPermissionDef::OrganizationArn(organization_arn), (None, None, None, Some(organizational_unit_arn)) => LaunchPermissionDef::OrganizationalUnitArn(organizational_unit_arn), _ => // error! };

You would have to change this from impl From to impl TryFrom to handle the error case of all of the elements being None though.

cbgbt · 2023-04-13T20:46:14Z

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

+
+fn write_amis(ami_output: &PathBuf, amis: &HashMap<Region, Image>) -> Result<()> {
+    let file = File::create(ami_output).context(error::FileSnafu {
+        op: "write amis to file",
+        path: ami_output,


This function should probably also be used in pubsys ami command, right?

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

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

@@ -216,8 +231,86 @@ pub(crate) async fn run(args: &Args, promote_args: &PromoteArgs) -> Result<()> {
    Ok(())
 }

+/// Read parameters in given file, add newly promoted parameters, and write combined parameters to
+/// the given file
+async fn write_rendered_parameters(


I think append_rendered_parameters may be more telling, since write often carries a connotation of overwrite

cbgbt · 2023-04-13T21:15:17Z

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

+        "Writing promoted SSM parameters to {}",
+        ssm_parameters_output.display()
+    );
+    let parsed_parameters = parse_expected_parameters(&ssm_parameters_output.to_owned())


Should this be parse_existing_parameters or just parse_parameters? Seems like the name is a holdover from parsing it in the validation code, but we need the method more generically.

Suggested change

let parsed_parameters = parse_expected_parameters(&ssm_parameters_output.to_owned())

let parsed_parameters = parse_parameter_file(&ssm_parameters_output.to_owned())

cbgbt · 2023-04-13T21:16:42Z

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

+    let combined_parameters: HashMap<Region, HashMap<SsmKey, String>> =
+        combine_parameters(parsed_parameters, set_parameters, source_target_map);
+
+    serde_json::to_writer_pretty(


Should this be a function somewhere? We write out this parameter data structure in more than one place now.

cbgbt · 2023-04-13T21:17:37Z

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

+/// Return a HashMap of Region mapped to a HashMap of SsmKey, String pairs, representing the newly
+/// promoted parameters as well as the original parameters


I think the behavior is correct, but it's worth calling out that any parameter collisions take the promoted parameter value.

That case probably deserves a test too.

cbgbt · 2023-04-13T21:19:00Z

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

@@ -213,6 +223,31 @@ pub(crate) async fn run(args: &Args, ssm_args: &SsmArgs) -> Result<()> {
    Ok(())
 }

+/// Write rendered parameters to the file at `ssm_parameters_output`
+async fn write_rendered_parameters(


Oh, you do have one! Just need to share it.

cbgbt · 2023-04-13T21:22:43Z

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

+        .context(error::ParseExistingSsmParametersSnafu {
+            path: ssm_parameters_output,
+        })?;


Am I correct that this will fail if the file is empty or doesn't exist? Is it better to just write the new parameters in that case, rather than refusing to proceed?

That is correct, but I don't know if we just want to write the new parameters. One reason it could fail is that the syntax in the file is incorrect, in which case I think it's better to alert the user and halt the program than completely overwrite the content of the file.

We chatted offline and concluded that we could specially handle Error::ReadExpectedParameterFile.

cbgbt

Looks good to me other than the casing.

cbgbt · 2023-04-14T01:14:04Z

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

+#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq, Hash)]
+pub(crate) enum LaunchPermissionDef {


I think it would be more coherent with our other serialization code if you lowercased these:

Suggested change

#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq, Hash)]

pub(crate) enum LaunchPermissionDef {

#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq, Hash)]

#[serde(rename_all = "lowercase")]

pub(crate) enum LaunchPermissionDef {

Wouldn't it make more sense to use snake_case?

If I git grep for rename_all in Bottlerocket, it seems like we typically use kebab-case or lowercase

cbgbt · 2023-04-14T01:16:08Z

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

+            organization_arn,
+            organizational_unit_arn,
+            ..
+        } = launch_permission.clone();


I am surprised that you had to clone this.

cbgbt · 2023-04-14T01:16:53Z

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

+        .context(error::ParseExistingSsmParametersSnafu {
+            path: ssm_parameters_output,
+        })?;


We chatted offline and concluded that we could specially handle Error::ReadExpectedParameterFile.

mjsterckx requested review from cbgbt and webern April 6, 2023 22:03

This was referenced Apr 11, 2023

pubsys: added EC2 image validation #2983

Closed

pubsys: changed validate-ssm input to single file #3004

Merged

mjsterckx force-pushed the pubsys-write-parameters branch from 283a660 to 42a0ec8 Compare April 12, 2023 23:43

mjsterckx changed the title ~~pubsys: write SSM parameters to file~~ pubsys: write SSM parameters and AMI publicity info and launch permissions to output files Apr 12, 2023

mjsterckx changed the title ~~pubsys: write SSM parameters and AMI publicity info and launch permissions to output files~~ pubsys: write SSM parameters and AMI permissions to output files Apr 12, 2023

mjsterckx requested a review from rpkelly April 13, 2023 00:13

mjsterckx force-pushed the pubsys-write-parameters branch from 42a0ec8 to a6cacad Compare April 13, 2023 14:27

cbgbt reviewed Apr 13, 2023

View reviewed changes

webern approved these changes Apr 13, 2023

View reviewed changes

mjsterckx force-pushed the pubsys-write-parameters branch 2 times, most recently from d6b95a2 to d29cefa Compare April 13, 2023 23:40

cbgbt reviewed Apr 14, 2023

View reviewed changes

mjsterckx added 2 commits April 14, 2023 14:44

pubsys: write AMI publicity info to file

43c921f

pubsys: write SSM parameters to file

ec72838

mjsterckx force-pushed the pubsys-write-parameters branch from d29cefa to ec72838 Compare April 14, 2023 14:45

cbgbt approved these changes Apr 14, 2023

View reviewed changes

mjsterckx merged commit d2104ad into bottlerocket-os:develop Apr 14, 2023

mjsterckx deleted the pubsys-write-parameters branch April 14, 2023 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pubsys: write SSM parameters and AMI permissions to output files #2987

pubsys: write SSM parameters and AMI permissions to output files #2987

mjsterckx commented Apr 6, 2023 •

edited

Loading

bcressey commented Apr 6, 2023

mjsterckx commented Apr 6, 2023

bcressey commented Apr 7, 2023

mjsterckx commented Apr 12, 2023

cbgbt commented Apr 13, 2023

mjsterckx commented Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

cbgbt Apr 13, 2023

mjsterckx Apr 13, 2023

cbgbt Apr 14, 2023

cbgbt left a comment

cbgbt Apr 14, 2023

mjsterckx Apr 14, 2023

cbgbt Apr 14, 2023

cbgbt Apr 14, 2023

cbgbt Apr 14, 2023

	let parsed_parameters = parse_expected_parameters(&ssm_parameters_output.to_owned())
	let parsed_parameters = parse_parameter_file(&ssm_parameters_output.to_owned())

		/// Return a HashMap of Region mapped to a HashMap of SsmKey, String pairs, representing the newly
		/// promoted parameters as well as the original parameters

		#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq, Hash)]
		pub(crate) enum LaunchPermissionDef {

pubsys: write SSM parameters and AMI permissions to output files #2987

pubsys: write SSM parameters and AMI permissions to output files #2987

Conversation

mjsterckx commented Apr 6, 2023 • edited Loading

bcressey commented Apr 6, 2023

mjsterckx commented Apr 6, 2023

bcressey commented Apr 7, 2023

mjsterckx commented Apr 12, 2023

cbgbt commented Apr 13, 2023

mjsterckx commented Apr 13, 2023

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

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

mjsterckx commented Apr 6, 2023 •

edited

Loading