aws: allow the user to customize their AMI name #3886

lavocatt · 2024-01-09T16:51:48Z

In the frontend a new field is allowing user to setup customized names
for their images (see
osbuild/image-builder-frontend#1136).

To allow the customization to effectively take place, this commit is
slightly changing how EC2 images are activated.

The Name tag and the AMI Name are not always sharing the same value
exactly anymore. If the user provides a SnaphsotName, the raw value is
set in the Name tag and the AMI name is set to $SnaphsotName-$UUID
(to make it unique).

This means that the Name tag can't be used anymore for the maintenance
tooling. The tooling will have to use the new Build-By tag to identify
the images built by osbuild-composer.

Example
The request:

Ended up producing this AMI:

This pull request includes:

adequate testing for the new functionality or fixed issue
adequate documentation informing people about the change such as
- submit a PR for the guides repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

https://issues.redhat.com/browse/HMS-2959

lavocatt · 2024-01-09T16:53:04Z

What kind of test would be adequate for this functionality do you think? I could change the aws test to add a snapshotName in the request and see if I can get access to the proper name after upload? Wdyt?

internal/cloudapi/v2/imagerequest.go

bcl · 2024-01-10T00:23:54Z

What benefit is there in using a new UUID to make the AMI Name unique? Why not use the UUID of the image so that no matter which field you look at it is clear (ha!) that it is the same thing?

lavocatt · 2024-01-10T09:21:07Z

What benefit is there in using a new UUID to make the AMI Name unique? Why not use the UUID of the image so that no matter which field you look at it is clear (ha!) that it is the same thing?

Thanks for this input, I hadn't thought about it this way. I'll make the two UUIDs the same.

lavocatt · 2024-01-10T09:48:48Z

One thing I'm noticing is that the AMI Id seems to reference the content of the Name tag too, and I don't know why. Or is it the name of the snapshot in the parenthesis?

internal/cloud/awscloud/awscloud.go

internal/cloudapi/v2/imagerequest.go

lavocatt · 2024-01-12T10:48:29Z

The tests are failing and I'm starting to understand what's happening.

We decided to reuse the snapshot_name because we thought that this field in the request wasn't really used anywhere. But it turns out that this field is being used in the tests:

osbuild-composer/test/cases/api/aws.sh

Lines 69 to 79 in 6735e74

    
             "image_request": { 
        
                 "architecture": "$ARCH", 
        
                 "image_type": "${IMAGE_TYPE}", 
        
                 "repositories": $(jq ".\"$ARCH\"" /usr/share/tests/osbuild-composer/repositories/"$DISTRO".json), 
        
                 "upload_options": { 
        
                   "region": "${AWS_REGION}", 
        
                   "snapshot_name": "${AWS_SNAPSHOT_NAME}", 
        
                   "share_with_accounts": ["${AWS_API_TEST_SHARE_ACCOUNT}"] 
        
               } 
        
             } 
        
           }

osbuild-composer/test/cases/api/aws.sh

Lines 103 to 110 in 6735e74

    
           function verify() { 
        
             $AWS_CMD ec2 describe-images \ 
        
               --owners self \ 
        
               --filters Name=name,Values="$AWS_SNAPSHOT_NAME" \ 
        
               > "$WORKDIR/ami.json" 
        
             AMI_IMAGE_ID=$(jq -r '.Images[].ImageId' "$WORKDIR/ami.json") 
        
             AWS_SNAPSHOT_ID=$(jq -r '.Images[].BlockDeviceMappings[].Ebs.SnapshotId' "$WORKDIR/ami.json")

The test suite is using this field to recover the ID of the image that was just built. Adding a UUID to the string broke that logic. There's an easy fix which implies to simply change how the test suite is finding the image by updating the filter from being equal to containing. For that I can just add a * to the filter.

Here's a few opened questions:

Since there's no way to name a snapshot, maybe the snapshot_name field should be renamed to something else.
This field was originally made to have an easy way to find an image. Should we keep the functionality as it was originally (without changing the filter)?

I think it's reasonable to could keep the broader filter (the one with a *). But I feel like renaming the field would be a nice addition to this PR. Maybe something like CustomName instead of SnaphsotName ?

What do you think?

internal/cloud/awscloud/awscloud.go

bcl

The consistent UUID part of things looks good.

internal/cloud/awscloud/awscloud.go

internal/cloudapi/v2/imagerequest.go

internal/cloud/awscloud/awscloud.go

In the frontend a new field is allowing user to setup customized names for their images (see osbuild/image-builder-frontend#1136). To allow the customization to effectively take place, this commit is slightly changing how EC2 images are activated. The Name tag and the AMI Name are not always sharing the same value exactly anymore. If the user provides a SnaphsotName, the raw value is set in the `Name` tag and the AMI name is set to `$SnaphsotName-$UUID` (to make it unique). This means that the `Name` tag can't be used anymore for the maintenance tooling. The tooling will have to use the new Build-By tag to identify the images built by osbuild-composer.

thozza

The change looks reasonable, but I'm wondering if we can't instead extend the maintenance service to use additional / different tag for filtering images? This would allow us to use the same value for the Name tag and as image name and would reduce the complexity and required changes in osbuild-composer a lot. 🤔

thozza · 2024-02-19T13:46:58Z

internal/cloudapi/v2/imagerequest.go

-	// For service maintenance, images are discovered by the "Name:composer-api-*"
-	// tag filter. Currently all image names in the service are generated, so they're
-	// guaranteed to be unique as well. If users are ever allowed to name their images,


I think that part of this comment should be preserved. Especially the information that the maintenance service uses "Name:composer-api-*" to discover images is very useful.

thozza · 2024-02-19T13:49:09Z

internal/target/aws_target.go

+	// Optional tag value if not provided will default to the ImageName's value.
+	TagName string `json:"tag_name,omitempty"`


tiny nitpick: I would suggest to rename this to NameTag instead. The TagName indicates (at least to me) that it is a name (a.k.a. the key) of a tag, not a value of a tag named Name.

thozza · 2024-02-19T14:03:17Z

internal/cloud/awscloud/awscloud.go

+				{
+					Key:   aws.String("Build-by"),
+					Value: aws.String("osbuild-composer"),


I'm wondering if the maintenance service could not leverage this new tag instead of the image name for filtering? This would allow us to keep the image name and the value of the Name tag consistent. Since you are appending an UUID to the user-provided image name, it will be unique and usable as the single image name.

@croissanne WDYT about using this new tag for the maintenance service?

croissanne · 2024-02-22T11:55:50Z

The change looks reasonable, but I'm wondering if we can't instead extend the maintenance service to use additional / different tag for filtering images? This would allow us to use the same value for the Name tag and as image name and would reduce the complexity and required changes in osbuild-composer a lot. 🤔

I think that's already implied in the description. Might be useful to do this in the same PR though, would just require a tiny change.

github-actions · 2024-03-24T04:08:32Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2024-03-31T04:10:02Z

This PR was closed because it has been stalled for 30+7 days with no activity.

lavocatt requested review from achilleas-k and croissanne January 9, 2024 16:51

achilleas-k reviewed Jan 9, 2024

View reviewed changes

internal/cloudapi/v2/imagerequest.go Outdated Show resolved Hide resolved

lavocatt requested a review from bcl January 10, 2024 09:26

lavocatt force-pushed the hms-2959 branch from 23dbfcd to 02a56b1 Compare January 10, 2024 09:44

mgold1234 reviewed Jan 10, 2024

View reviewed changes

internal/cloud/awscloud/awscloud.go Outdated Show resolved Hide resolved

internal/cloud/awscloud/awscloud.go Outdated Show resolved Hide resolved

internal/cloudapi/v2/imagerequest.go Outdated Show resolved Hide resolved

lavocatt force-pushed the hms-2959 branch from 02a56b1 to e68b632 Compare January 10, 2024 09:59

lavocatt force-pushed the hms-2959 branch 3 times, most recently from 9eb8f17 to ec07fd1 Compare January 15, 2024 10:38

lavocatt requested a review from achilleas-k January 15, 2024 10:55

croissanne reviewed Jan 15, 2024

View reviewed changes

internal/cloud/awscloud/awscloud.go Outdated Show resolved Hide resolved

lavocatt force-pushed the hms-2959 branch from ec07fd1 to 9507d3b Compare January 17, 2024 16:08

lavocatt requested a review from croissanne January 17, 2024 16:09

lavocatt force-pushed the hms-2959 branch from 9507d3b to e1719aa Compare January 17, 2024 16:10

bcl reviewed Jan 18, 2024

View reviewed changes

lavocatt commented Jan 18, 2024

View reviewed changes

internal/cloud/awscloud/awscloud.go Outdated Show resolved Hide resolved

internal/cloudapi/v2/imagerequest.go Outdated Show resolved Hide resolved

lavocatt force-pushed the hms-2959 branch 8 times, most recently from 7dba54d to 4d6188e Compare January 23, 2024 11:25

thozza reviewed Jan 23, 2024

View reviewed changes

internal/cloud/awscloud/awscloud.go Outdated Show resolved Hide resolved

lavocatt force-pushed the hms-2959 branch 2 times, most recently from 0d9c3b2 to bad6068 Compare February 2, 2024 13:46

lavocatt force-pushed the hms-2959 branch 2 times, most recently from 3454043 to 88c82f2 Compare February 12, 2024 09:03

lavocatt force-pushed the hms-2959 branch from 88c82f2 to 643be1c Compare February 12, 2024 13:15

mcattamoredhat mentioned this pull request Feb 13, 2024

edge-commit CS9 failure: ostree-remount.service fails to remount /sysroot and /var mountpoints #3942

Open

thozza reviewed Feb 19, 2024

View reviewed changes

github-actions bot added the Stale label Mar 24, 2024

github-actions bot closed this Mar 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws: allow the user to customize their AMI name #3886

aws: allow the user to customize their AMI name #3886

lavocatt commented Jan 9, 2024 •

edited

Loading

lavocatt commented Jan 9, 2024

bcl commented Jan 10, 2024

lavocatt commented Jan 10, 2024 •

edited

Loading

lavocatt commented Jan 10, 2024 •

edited

Loading

lavocatt commented Jan 12, 2024 •

edited

Loading

bcl left a comment

thozza left a comment

thozza Feb 19, 2024

thozza Feb 19, 2024

thozza Feb 19, 2024

croissanne Feb 22, 2024

croissanne commented Feb 22, 2024

github-actions bot commented Mar 24, 2024

github-actions bot commented Mar 31, 2024

		// Optional tag value if not provided will default to the ImageName's value.
		TagName string `json:"tag_name,omitempty"`

aws: allow the user to customize their AMI name #3886

aws: allow the user to customize their AMI name #3886

Conversation

lavocatt commented Jan 9, 2024 • edited Loading

lavocatt commented Jan 9, 2024

bcl commented Jan 10, 2024

lavocatt commented Jan 10, 2024 • edited Loading

lavocatt commented Jan 10, 2024 • edited Loading

lavocatt commented Jan 12, 2024 • edited Loading

bcl left a comment

Choose a reason for hiding this comment

thozza left a comment

Choose a reason for hiding this comment

thozza Feb 19, 2024

Choose a reason for hiding this comment

thozza Feb 19, 2024

Choose a reason for hiding this comment

thozza Feb 19, 2024

Choose a reason for hiding this comment

croissanne Feb 22, 2024

Choose a reason for hiding this comment

croissanne commented Feb 22, 2024

github-actions bot commented Mar 24, 2024

github-actions bot commented Mar 31, 2024

lavocatt commented Jan 9, 2024 •

edited

Loading

lavocatt commented Jan 10, 2024 •

edited

Loading

lavocatt commented Jan 10, 2024 •

edited

Loading

lavocatt commented Jan 12, 2024 •

edited

Loading