mantle/ore/aws: add ensure-public to manage AMI visibility#4562
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ensure-public command to the ore aws tool, designed to maintain the public accessibility of production RHCOS AMIs that AWS might automatically privatize due to deprecation. It also extends the AWS API client with methods for listing production images, checking public status, and restoring launch permissions. Feedback includes addressing potential nil pointer dereferences when accessing image metadata and refining the wording of deprecation date labels for clarity.
marmijo
force-pushed
the
add-ensure-public
branch
4 times, most recently
from
a9003a9 to
092316d
Compare
AWS enforces a 2-year deprecation date on all public AMIs and removes their public sharing permission after 6+ months of inactivity past the deprecation date. This silently breaks OpenShift customers on older versions who rely on those AMIs to scale cluster nodes. Add `ore aws ensure-public`, which scans all production AMIs (tagged production=true) in a region, checks their launch permission, and restores any that have gone private. A single AMI can also be targeted directly via --ami. Assisted-by: Claude <Sonnet 4.6>
marmijo
force-pushed
the
add-ensure-public
branch
from
092316d to
e1fe762
Compare
Rolv-Apneseth
left a comment
Rolv-Apneseth
left a comment
There was a problem hiding this comment.
Just some very minor things but otherwise LGTM
| hadError := false | ||
|
|
||
| for _, img := range images { | ||
| imgID := derefStr(img.ImageId) |
There was a problem hiding this comment.
If imgID is nil, shouldn't we just print an error and continue to the next image?
|
|
||
| // GetImageByIDIncludeDeprecated returns the AMI with the given ID owned by | ||
| // this account, including deprecated ones. | ||
| func (a *API) GetImageByIDIncludeDeprecated(imageID string) (*ec2types.Image, error) { |
There was a problem hiding this comment.
Thoughts dropping the IncludeDeprecated? I see the function above also includes deprecated images but doesn't have this in its name.
| if len(resp.Images) == 0 { | ||
| return nil, nil | ||
| } | ||
| return &resp.Images[0], nil |
There was a problem hiding this comment.
Is there any potential for multiple AMIs matching the same imageID here? i.e. should there be an error/warning if len > 1 or is that not possible?
2 participants
AWS enforces a 2-year deprecation date on all public AMIs and removes their public sharing permission after 6+ months of inactivity past the deprecation date. This silently breaks OpenShift customers on older versions who rely on those AMIs to scale cluster nodes.
Add
ore aws ensure-public, which scans all production AMIs (tagged production=true) in a region, checks their launch permission, and restores any that have gone private. A single AMI can also be targeted directly via--ami.See: https://redhat.atlassian.net/browse/COS-3842
Assisted-by: Claude <Sonnet 4.6>