Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3829 (2022-11-08 14:38:06)

 Change AMI name generation based on new format in image-builder
**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
The old AMIs were generated with following pattern: capa-ami-amazon-2-1.25.2-00-1664536077. From the recent releases, the new AMIs are getting generated as capa-ami-amazon-2-v1.25.3-1665727783(observe prefix v to the k8s version)). Because of this mismatch, the newly generated AMIs are not getting published in S3.
This PR fixes the generation of AMI names such that it respects both the above formats, such that all the new AMIs are also published to the S3 amis.json.
[Ref](https://kubernetes.slack.com/archives/C01E0Q35A8J/p1666969997721009) slack thread.
Changes in image-builder that is now appending AMIs with k8s semver: https://github.com/kubernetes-sigs/image-builder/commit/05eff6b6dcc75e158b51c26d5628ecf4de34152e

<!-- Enter a description of the change and why this change is needed -->

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [x] squashed commits
- [ ] includes documentation
- [x] adds unit tests
- [ ] adds or updates e2e tests


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from ankitasw by writing `/assign @ankitasw` in a comment. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["ankitasw"]} -->
k8s-ci-robot: @Ankitasw: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-test | 7bbec9c3f0c8eefd863e43746a5a2ab6efa9b185 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3829/pull-cluster-api-provider-aws-test/1589991518825877504) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=3829). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AAnkitasw). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
Ankitasw: Everytime I have confidence whenever I open this AMI PR, but later it keeps me doubtful about that implementation 😮‍💨 
Ankitasw: /test pull-cluster-api-provider-aws-e2e-blocking
Ankitasw: /hold
until tests passes
Ankitasw: /test pull-cluster-api-provider-aws-e2e
Ankitasw: the current functionality works fine with these changes, only thing to be tested post PR merge is new format should be respected and AMIs should be pushed to S3.
Ankitasw: Arghh!! One more change got missed in the commit, will push it and run tests again.
Ankitasw: /test pull-cluster-api-provider-aws-e2e-blocking
Ankitasw: /priority critical-urgent
k8s-ci-robot: @Ankitasw: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-e2e-blocking | bca280d74c65f9185e436895f3d8b6225719ef38 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3829/pull-cluster-api-provider-aws-e2e-blocking/1592806207591026688) | false | `/test pull-cluster-api-provider-aws-e2e-blocking`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=3829). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AAnkitasw). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
Ankitasw: /retest
Ankitasw: /unhold
as tests are passing
Ankitasw: @richardcase since I reverted the change, not running e2e tests again.

Patch:
diff --git a/cmd/clusterawsadm/ami/helper.go b/cmd/clusterawsadm/ami/helper.go
index 0ca0e4c70..27b05a8f5 100644
--- a/cmd/clusterawsadm/ami/helper.go
+++ b/cmd/clusterawsadm/ami/helper.go
@@ -216,7 +216,11 @@ func getAllImages(ec2Client ec2iface.EC2API, ownerID string) (map[string][]*ec2.
 	imagesMap := make(map[string][]*ec2.Image)
 	for _, image := range out.Images {
 		arr := strings.Split(aws.StringValue(image.Name), "-")
-		arr = arr[:len(arr)-2]
+		if arr[len(arr)-2] == "00" {
+			arr = arr[:len(arr)-2]
+		} else {
+			arr = arr[:len(arr)-1]
+		}
 		name := strings.Join(arr, "-")
 		images, ok := imagesMap[name]
 		if !ok {
@@ -230,18 +234,30 @@ func getAllImages(ec2Client ec2iface.EC2API, ownerID string) (map[string][]*ec2.
 
 func findAMI(imagesMap map[string][]*ec2.Image, baseOS, kubernetesVersion string) (*ec2.Image, error) {
 	amiNameFormat := "capa-ami-{{.BaseOS}}-{{.K8sVersion}}"
+	// Support new AMI format capa-ami-<os-version>-?<k8s-version>-*
 	amiName, err := ec2service.GenerateAmiName(amiNameFormat, baseOS, kubernetesVersion)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to process ami format: %q", amiNameFormat)
 	}
-
 	if val, ok := imagesMap[amiName]; ok && val != nil {
-		latestImage, err := ec2service.GetLatestImage(val)
+		return latestAMI(val)
+	} else {
+		amiName, err = ec2service.GenerateAmiName(amiNameFormat, baseOS, strings.TrimPrefix(kubernetesVersion, "v"))
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrapf(err, "failed to process ami format: %q", amiNameFormat)
+		}
+		if val, ok = imagesMap[amiName]; ok && val != nil {
+			return latestAMI(val)
 		}
-		return latestImage, nil
 	}
 
 	return nil, nil
 }
+
+func latestAMI(val []*ec2.Image) (*ec2.Image, error) {
+	latestImage, err := ec2service.GetLatestImage(val)
+	if err != nil {
+		return nil, err
+	}
+	return latestImage, nil
+}
diff --git a/pkg/cloud/services/ec2/ami.go b/pkg/cloud/services/ec2/ami.go
index 8f46c97dd..5c70e7f93 100644
--- a/pkg/cloud/services/ec2/ami.go
+++ b/pkg/cloud/services/ec2/ami.go
@@ -77,7 +77,7 @@ type AMILookup struct {
 
 // GenerateAmiName will generate an AMI name.
 func GenerateAmiName(amiNameFormat, baseOS, kubernetesVersion string) (string, error) {
-	amiNameParameters := AMILookup{baseOS, strings.TrimPrefix(kubernetesVersion, "v")}
+	amiNameParameters := AMILookup{baseOS, kubernetesVersion}
 	// revert to default if not specified
 	if amiNameFormat == "" {
 		amiNameFormat = DefaultAmiNameFormat
@@ -106,7 +106,7 @@ func DefaultAMILookup(ec2Client ec2iface.EC2API, ownerID, baseOS, kubernetesVers
 		baseOS = defaultMachineAMILookupBaseOS
 	}
 
-	amiName, err := GenerateAmiName(amiNameFormat, baseOS, kubernetesVersion)
+	amiName, err := GenerateAmiName(amiNameFormat, baseOS, strings.TrimPrefix(kubernetesVersion, "v"))
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to process ami format: %q", amiNameFormat)
 	}


Test patch:
diff --git a/pkg/cloud/services/ec2/ami_test.go b/pkg/cloud/services/ec2/ami_test.go
index eec279a88..189cbb016 100644
--- a/pkg/cloud/services/ec2/ami_test.go
+++ b/pkg/cloud/services/ec2/ami_test.go
@@ -263,7 +263,7 @@ func TestGenerateAmiName(t *testing.T) {
 		{
 			name: "Should return image name even if OS and amiNameFormat is empty",
 			args: args{
-				kubernetesVersion: "v1.23.3",
+				kubernetesVersion: "1.23.3",
 			},
 			want: "capa-ami--?1.23.3-*",
 		},
@@ -285,6 +285,15 @@ func TestGenerateAmiName(t *testing.T) {
 			},
 			want: "random-centos-7-?1.23.3-*",
 		},
+		{
+			name: "Should return valid amiName if new AMI name format passed",
+			args: args{
+				amiNameFormat:     "random-{{.BaseOS}}-{{.K8sVersion}}",
+				baseOS:            "centos-7",
+				kubernetesVersion: "v1.23.3",
+			},
+			want: "random-centos-7-v1.23.3",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go
index dcd23ce2b..9ebbd2d1a 100644
--- a/pkg/cloud/services/ec2/instances_test.go
+++ b/pkg/cloud/services/ec2/instances_test.go
@@ -557,7 +557,7 @@ func TestCreateInstance(t *testing.T) {
 				},
 			},
 			expect: func(m *mocks.MockEC2APIMockRecorder) {
-				amiName, err := GenerateAmiName("capa-ami-{{.BaseOS}}-?{{.K8sVersion}}-*", "ubuntu-18.04", "v1.16.1")
+				amiName, err := GenerateAmiName("capa-ami-{{.BaseOS}}-?{{.K8sVersion}}-*", "ubuntu-18.04", "1.16.1")
 				if err != nil {
 					t.Fatalf("Failed to process ami format: %v", err)
 				}
@@ -687,7 +687,7 @@ func TestCreateInstance(t *testing.T) {
 				},
 			},
 			expect: func(m *mocks.MockEC2APIMockRecorder) {
-				amiName, err := GenerateAmiName("capa-ami-{{.BaseOS}}-?{{.K8sVersion}}-*", "ubuntu-18.04", "v1.16.1")
+				amiName, err := GenerateAmiName("capa-ami-{{.BaseOS}}-?{{.K8sVersion}}-*", "ubuntu-18.04", "1.16.1")
 				if err != nil {
 					t.Fatalf("Failed to process ami format: %v", err)
 				}
@@ -818,7 +818,7 @@ func TestCreateInstance(t *testing.T) {
 				},
 			},
 			expect: func(m *mocks.MockEC2APIMockRecorder) {
-				amiName, err := GenerateAmiName("capa-ami-{{.BaseOS}}-?{{.K8sVersion}}-*", "ubuntu-18.04", "v1.16.1")
+				amiName, err := GenerateAmiName("capa-ami-{{.BaseOS}}-?{{.K8sVersion}}-*", "ubuntu-18.04", "1.16.1")
 				if err != nil {
 					t.Fatalf("Failed to process ami format: %v", err)
 				}