From bad6068c48306da32a6035f2a82ac5a18adf379b Mon Sep 17 00:00:00 2001 From: Thomas Lavocat Date: Tue, 9 Jan 2024 16:56:34 +0100 Subject: [PATCH] aws: allow the user to customize their AMI name In the frontend a new field is allowing user to setup customized names for their images (see https://github.com/RedHatInsights/image-builder-frontend/pull/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. --- cmd/osbuild-upload-aws/main.go | 2 +- cmd/osbuild-worker/jobimpl-osbuild.go | 7 ++++++- internal/boot/aws.go | 2 +- internal/cloud/awscloud/awscloud.go | 29 ++++++++++++++++++--------- internal/cloudapi/v2/imagerequest.go | 20 ++++++++++-------- internal/target/aws_target.go | 3 +++ test/cases/api/aws.sh | 2 +- 7 files changed, 44 insertions(+), 21 deletions(-) diff --git a/cmd/osbuild-upload-aws/main.go b/cmd/osbuild-upload-aws/main.go index 61e86f1fac1..36425fb554d 100644 --- a/cmd/osbuild-upload-aws/main.go +++ b/cmd/osbuild-upload-aws/main.go @@ -58,7 +58,7 @@ func main() { bootModePtr = &bootMode } - ami, err := a.Register(imageName, bucketName, keyName, share, arch, bootModePtr) + ami, err := a.Register(imageName, bucketName, keyName, imageName, share, arch, bootModePtr) if err != nil { println(err.Error()) return diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go index 5aadc73624d..14f077d6958 100644 --- a/cmd/osbuild-worker/jobimpl-osbuild.go +++ b/cmd/osbuild-worker/jobimpl-osbuild.go @@ -636,7 +636,12 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error { break } - ami, err := a.Register(jobTarget.ImageName, bucket, targetOptions.Key, targetOptions.ShareWithAccounts, arch.Current().String(), targetOptions.BootMode) + // giving a Name tag value is optional, default to ImageName if missing. + tagName := jobTarget.ImageName + if targetOptions.TagName != "" { + tagName = targetOptions.TagName + } + ami, err := a.Register(jobTarget.ImageName, bucket, targetOptions.Key, tagName, targetOptions.ShareWithAccounts, arch.Current().String(), targetOptions.BootMode) if err != nil { targetResult.TargetError = clienterrors.WorkerClientError(clienterrors.ErrorImportingImage, err.Error(), nil) break diff --git a/internal/boot/aws.go b/internal/boot/aws.go index 4b155510efb..2d39a534252 100644 --- a/internal/boot/aws.go +++ b/internal/boot/aws.go @@ -99,7 +99,7 @@ func UploadImageToAWS(c *awsCredentials, imagePath string, imageName string) err if err != nil { return fmt.Errorf("cannot upload the image: %v", err) } - _, err = uploader.Register(imageName, c.Bucket, imageName, nil, arch.Current().String(), nil) + _, err = uploader.Register(imageName, c.Bucket, imageName, imageName, nil, arch.Current().String(), nil) if err != nil { return fmt.Errorf("cannot register the image: %v", err) } diff --git a/internal/cloud/awscloud/awscloud.go b/internal/cloud/awscloud/awscloud.go index 419d0b4dc7e..8cc555fe90b 100644 --- a/internal/cloud/awscloud/awscloud.go +++ b/internal/cloud/awscloud/awscloud.go @@ -212,7 +212,7 @@ func WaitUntilImportSnapshotTaskCompletedWithContext(c *ec2.EC2, ctx aws.Context // The caller can optionally specify the boot mode of the AMI. If the boot // mode is not specified, then the instances launched from this AMI use the // default boot mode value of the instance type. -func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch string, bootMode *string) (*string, error) { +func (a *AWS) Register(imageName string, bucket string, s3key string, tagName string, shareWith []string, rpmArch string, bootMode *string) (*string, error) { rpmArchToEC2Arch := map[string]string{ "x86_64": "x86_64", "aarch64": "arm64", @@ -229,15 +229,18 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str } } - logrus.Infof("[AWS] 📥 Importing snapshot from image: %s/%s", bucket, key) - snapshotDescription := fmt.Sprintf("Image Builder AWS Import of %s", name) + // Note that the key value is only used in the upload transition phase in S3. It's deleted once the snapshot is done + // importing. So it's content has not much impact. The key is not conveyed anymore, compute a unique one based on + // the nameTag's value. + logrus.Infof("[AWS] 📥 Importing snapshot from image: %s/%s", bucket, s3key) + snapshotDescription := fmt.Sprintf("Image Builder AWS Import of %s", imageName) importTaskOutput, err := a.ec2.ImportSnapshot( &ec2.ImportSnapshotInput{ Description: aws.String(snapshotDescription), DiskContainer: &ec2.SnapshotDiskContainer{ UserBucket: &ec2.UserBucket{ S3Bucket: aws.String(bucket), - S3Key: aws.String(key), + S3Key: aws.String(s3key), }, }, }, @@ -261,10 +264,10 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str } // we no longer need the object in s3, let's just delete it - logrus.Infof("[AWS] 🧹 Deleting image from S3: %s/%s", bucket, key) + logrus.Infof("[AWS] 🧹 Deleting image from S3: %s/%s", bucket, s3key) _, err = a.s3.DeleteObject(&s3.DeleteObjectInput{ Bucket: aws.String(bucket), - Key: aws.String(key), + Key: aws.String(s3key), }) if err != nil { return nil, err @@ -290,7 +293,11 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str Tags: []*ec2.Tag{ { Key: aws.String("Name"), - Value: aws.String(name), + Value: aws.String(tagName), + }, + { + Key: aws.String("Build-by"), + Value: aws.String("osbuild-composer"), }, }, }, @@ -306,7 +313,7 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str Architecture: aws.String(ec2Arch), BootMode: bootMode, VirtualizationType: aws.String("hvm"), - Name: aws.String(name), + Name: aws.String(imageName), RootDeviceName: aws.String("/dev/sda1"), EnaSupport: aws.Bool(true), BlockDeviceMappings: []*ec2.BlockDeviceMapping{ @@ -332,7 +339,11 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str Tags: []*ec2.Tag{ { Key: aws.String("Name"), - Value: aws.String(name), + Value: aws.String(tagName), + }, + { + Key: aws.String("Build-by"), + Value: aws.String("osbuild-composer"), }, }, }, diff --git a/internal/cloudapi/v2/imagerequest.go b/internal/cloudapi/v2/imagerequest.go index fdfaac346f6..e8ae6b03ad1 100644 --- a/internal/cloudapi/v2/imagerequest.go +++ b/internal/cloudapi/v2/imagerequest.go @@ -47,11 +47,8 @@ func newAWSTarget(options UploadOptions, imageType distro.ImageType) (*target.Ta return nil, HTTPError(ErrorJSONUnMarshallingError) } - // 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, - // an extra tag should be added. - key := fmt.Sprintf("composer-api-%s", uuid.New().String()) + targetUuid := uuid.New() + key := fmt.Sprintf("composer-api-%s", targetUuid.String()) var amiBootMode *string switch imageType.BootMode() { @@ -63,16 +60,23 @@ func newAWSTarget(options UploadOptions, imageType distro.ImageType) (*target.Ta amiBootMode = common.ToPtr(ec2.BootModeValuesLegacyBios) } + // tag name is optional, not providing one will make the Name tag default to the ImageName + tagName := "" + if awsUploadOptions.SnapshotName != nil { + tagName = *awsUploadOptions.SnapshotName + } + t := target.NewAWSTarget(&target.AWSTargetOptions{ Region: awsUploadOptions.Region, Key: key, + TagName: tagName, ShareWithAccounts: awsUploadOptions.ShareWithAccounts, BootMode: amiBootMode, }) + t.ImageName = key if awsUploadOptions.SnapshotName != nil { - t.ImageName = *awsUploadOptions.SnapshotName - } else { - t.ImageName = key + // When registering an image the ImageName is used to set the ami's name. AMIs names must be unique. + t.ImageName = fmt.Sprintf("%s-%s", *awsUploadOptions.SnapshotName, targetUuid.String()) } t.OsbuildArtifact.ExportFilename = imageType.Filename() return t, nil diff --git a/internal/target/aws_target.go b/internal/target/aws_target.go index b6921a1a4be..06979151125 100644 --- a/internal/target/aws_target.go +++ b/internal/target/aws_target.go @@ -14,6 +14,9 @@ type AWSTargetOptions struct { Key string `json:"key"` ShareWithAccounts []string `json:"shareWithAccounts"` + // Optional tag value if not provided will default to the ImageName's value. + TagName string `json:"tag_name,omitempty"` + // Boot mode of the AMI (optional) // Supported values: // - ec2.BootModeValuesLegacyBios diff --git a/test/cases/api/aws.sh b/test/cases/api/aws.sh index 1a3a30bfe44..cfead3b8d01 100644 --- a/test/cases/api/aws.sh +++ b/test/cases/api/aws.sh @@ -103,7 +103,7 @@ function checkUploadStatusOptions() { function verify() { $AWS_CMD ec2 describe-images \ --owners self \ - --filters Name=name,Values="$AWS_SNAPSHOT_NAME" \ + --filters Name=name,Values="$AWS_SNAPSHOT_NAME-*" \ > "$WORKDIR/ami.json" AMI_IMAGE_ID=$(jq -r '.Images[].ImageId' "$WORKDIR/ami.json")