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

pubsys: improve stability of SSM parameter validation #348

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

cbgbt
Copy link
Contributor

@cbgbt cbgbt commented Aug 19, 2024

Issue number:

Closes #347
Closes #346

Description of changes:

    workspace: homogenize dependencies
    
    This changes helps prepare the workspace to use workspace dependencies
    by using the same version requirements and feature sets for common
    dependencies of crates in the workspace.
    workspace: move to workspace dependencies
    pubsys: implement Display for ValidateSsm error
    pubsys: rate-limit SSM GetParameter calls
    pubsys: retry SSM validation on any failure

Testing done:

  • published and validated AMIs with SSM parameters

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.

This changes helps prepare the workspace to use workspace dependencies
by using the same version requirements and feature sets for common
dependencies of crates in the workspace.
buildsys-config = { version = "0.1", path = "tools/buildsys-config" }
oci-cli-wrapper = { version = "0.1", path = "tools/oci-cli-wrapper" }
parse-datetime = { version = "0.1", path = "tools/parse-datetime" }
pipesys = { version = "0.1", path = "tools/pipesys", lib = true, artifact = [ "bin:pipesys" ] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does lib = true do here? It's sort of surprising since these are all binaries.

Can we move the artifact dependency stuff to twoliter/Cargo.toml and leave just the version and path here? It seems a little weird for cases where we have a cross crate dependency (like pubsys depends on buildsys).

Copy link
Contributor Author

@cbgbt cbgbt Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does lib = true do here? It's sort of surprising since these are all binaries.

buildsys depends on pipesys as a library, and you need to add lib = true if you use artifact dependencies to tell Cargo to build both.

   Compiling buildsys v0.1.0 (/home/ec2-user/twoliter/tools/buildsys)
error[E0433]: failed to resolve: use of undeclared crate or module `pipesys`
  --> tools/buildsys/src/builder.rs:20:5
   |
20 | use pipesys::server::Server as PipesysServer;
   |     ^^^^^^^ use of undeclared crate or module `pipesys`

Can we move the artifact dependency stuff to twoliter/Cargo.toml and leave just the version and path here?

I really wanted to do it this way, but Cargo wouldn't cooperate. e.g. if you move the artifact declaration for pubsys to twoliter/Cargo.toml, cargo build says:

cargo clippy --locked -- -D warnings --no-deps
warning: /home/ec2-user/twoliter/twoliter/Cargo.toml: unused manifest key: dependencies.pubsys.artifact
warning: twoliter v0.4.4 (/home/ec2-user/twoliter/twoliter) ignoring invalid dependency `pubsys` which is missing a lib target

... and then eventually

    Checking pubsys v0.1.0 (/home/ec2-user/twoliter/tools/pubsys)
error: environment variable `CARGO_BIN_FILE_PUBSYS` not defined at compile time
  --> twoliter/src/tools.rs:17:38
   |
17 | const PUBSYS: &[u8] = include_bytes!(env!("CARGO_BIN_FILE_PUBSYS"));
   |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: Cargo sets build script variables at run time. Use `std::env::var("CARGO_BIN_FILE_PUBSYS")` instead
   = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `twoliter` (bin "twoliter") due to 1 previous error
make: *** [Makefile:14: clippy] Error 101


/// Async throttling function to be called before calling SSM GetParameter* functions.
///
/// Returns when the rate limit is satisifed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
/// Returns when the rate limit is satisifed.
/// Returns when the rate limit is satisfied.

@cbgbt cbgbt merged commit 916dee8 into bottlerocket-os:develop Aug 21, 2024
1 check passed
@cbgbt cbgbt deleted the stabilize-ssm-validate branch August 21, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pubsys: SSM validation is flaky pubsys: Improve error message on SSM validation failure
3 participants