pubsys: support publishing one AMI into multiple accounts per region#679
pubsys: support publishing one AMI into multiple accounts per region#679timjhh wants to merge 1 commit into
Conversation
| /// `roles` list, de-duplicated while preserving order (`role` first). If no roles are | ||
| /// configured, returns a single `None`, meaning "use the base/global credentials without | ||
| /// assuming a region-specific role". | ||
| pub fn all_roles(&self) -> Vec<Option<String>> { |
There was a problem hiding this comment.
This is a nit, but I think it's a bit more idiomatic for this to return an Option<Vec<String>>
There was a problem hiding this comment.
I still think this would be a good improvement if you get around to it!
There was a problem hiding this comment.
looked into it, now that this is an intermediary form of the accounts we are publishing to, we are using the Nonetype of the vector to fall back to use the globally configured roles.
If we used Option<Vec<String>> we would only be able to specify 'no roles arns can be used' or 'some role arns can be used' instead of the current semantic meaning of 'targets to publish to' of the data type
There was a problem hiding this comment.
actually nvm, I was able to do this!
|
|
||
| trace!("Current SSM parameters: {current_parameters:#?}"); | ||
| let mut any_changes = false; | ||
| for (account_id, account_parameters) in ¶meters_by_account { |
There was a problem hiding this comment.
If using a different AWS account per region, this will end up breaking parallelism while publishing SSM parameters to multiple regions. Instead, each region will now run in serial since they each use their own accounts.
Can we implement the multi-account support within get_parameters / set_parameters / validate_parameters instead, to preserve parallelism?
There was a problem hiding this comment.
This has been done in the new rev
timjhh
force-pushed
the
pubsys-multi-account
branch
4 times, most recently
from
8ba709b to
e04c06a
Compare
| ) | ||
| }) | ||
| .collect::<HashMap<&Region, Result<_>>>(); | ||
| .collect::<HashMap<Region, Result<_>>>(); |
There was a problem hiding this comment.
this needs to be a HashMap<RegionAccount, Result>> or else errors may be swallowed if there are multiple accounts in a region
There was a problem hiding this comment.
so true, threaded RegionAccount all the way through the workflow to address the issue
| /// `roles` list, de-duplicated while preserving order (`role` first). If no roles are | ||
| /// configured, returns a single `None`, meaning "use the base/global credentials without | ||
| /// assuming a region-specific role". | ||
| pub fn all_roles(&self) -> Vec<Option<String>> { |
There was a problem hiding this comment.
I still think this would be a good improvement if you get around to it!
timjhh
force-pushed
the
pubsys-multi-account
branch
from
e04c06a to
b9e4239
Compare
timjhh
force-pushed
the
pubsys-multi-account
branch
from
b9e4239 to
5647b0f
Compare
|
|
||
| let mut seen = std::collections::HashSet::new(); | ||
| Some( | ||
| roles |
There was a problem hiding this comment.
it's possible under the current configuration to specify two roles in the same account, which would lead to a confusing error when pubsys runs into a name collision - can we throw an error on deserialization if there are multiple roles using the same account in a region? might need a custom deserializer with #[serde(deserialize_with ...)]
| aws_sdk_configs.insert(region.clone(), client_config.clone()); | ||
| let role = role_for_account(&aws, &key.region, &key.account_id); | ||
| let client_config = | ||
| build_client_config_for_role(&key.region, &base_region, &aws, role.as_deref()) |
There was a problem hiding this comment.
there is a sneaky breaking change here: pubsys previously allowed you to publish SSM parameters in one account that reference AMIs in a different account, with the SSM account's role specified in the Infra.toml used to publish the SSM parameters. (this was a happy accident since there was a 1:1 region-AMI mapping, so there was no confusion on which AMI should be referenced by the parameter)
if I'm understanding correctly, as this is written, it will only search for a role that matches the account ID of the AMI. the SSM account role in the Infra.toml will be ignored and it will fall back to the base creds.
I'll think a bit about how we can handle this. we might need to introduce the concept of a "primary" image in a region or something like that.
3 participants
Issue number:
Closes #678
Description of changes:
pubsys: support publishing one AMI into multiple accounts per region
Previously pubsys assumed one account per region (a single role per
[aws.region.*]), so registering a single built AMI into separate
preprod/prod accounts in the same region required dynamically generated
per-account spec files.
This adds a
roleslist to [aws.region.*] alongside the existingrole. We register the AMI once in the base account, then copy + shareit into each additional target account (reusing the existing
modify_snapshots/modify_image + copy_image machinery), and publish a
separate set of SSM parameters per account.
The intermediate --ami-input JSON produced by
pubsys amiand consumedby
pubsys ssm/pubsys publish-amiis now nested by account(region -> account -> image). SSM get/set/validate run once per account
since each account has its own parameter store.
Testing done:
cargo testpasses for pubsys and pubsys-config packages.roletakes precedence overroles, we will allow an empty or missingrolesvector, duplicate roles across both fields are de-duplicated and accepted, and that the modified--ami-inputfield works across the ssm tool.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.