Merge pull request openshift#8466 from r4f4/capi-aws-custom-ami-fix

OCPBUGS-33508: capi/aws: fix setting custom AMI

Author: openshift-merge-bot[bot]

pkg/infrastructure/aws/clusterapi/ami.go

@@ -10,23 +10,15 @@ import (
 	"github.com/sirupsen/logrus"
 
 	"github.com/openshift/installer/pkg/asset/installconfig"
+	"github.com/openshift/installer/pkg/asset/rhcos"
 )
 
 // copyAMIToRegion copies the AMI to the region configured in the installConfig if needed.
-func copyAMIToRegion(ctx context.Context, installConfig *installconfig.InstallConfig, infraID, rhcosImage string) (string, error) {
-	osImage := strings.SplitN(rhcosImage, ",", 2)
-	amiID := osImage[0]
-	amiRegion := installConfig.Config.AWS.Region
-	if len(osImage) > 1 {
-		amiRegion = osImage[1]
-	}
-
-	// Already in target region, nothing to do
-	if amiRegion == installConfig.Config.AWS.Region {
-		return amiID, nil
-	}
+func copyAMIToRegion(ctx context.Context, installConfig *installconfig.InstallConfig, infraID string, rhcosImage *rhcos.Image) (string, error) {
+	osImage := strings.SplitN(string(*rhcosImage), ",", 2)
+	amiID, amiRegion := osImage[0], osImage[1]
 
-	logrus.Infof("Copying AMI to region %s", installConfig.AWS.Region)
+	logrus.Infof("Copying AMI %s to region %s", amiID, installConfig.AWS.Region)
 
 	session, err := installConfig.AWS.Session(ctx)
 	if err != nil {

pkg/infrastructure/aws/clusterapi/aws.go

@@ -45,13 +45,29 @@ func (*Provider) PreProvision(ctx context.Context, in clusterapi.PreProvisionInp
 		return fmt.Errorf("failed to create IAM roles: %w", err)
 	}
 
-	amiID, err := copyAMIToRegion(ctx, in.InstallConfig, in.InfraID, string(*in.RhcosImage))
+	// The AWSMachine manifests might already have the AMI ID set from the machine pool which takes into account the
+	// ways in which the AMI can be specified: the default rhcos if already in the target region, a custom AMI ID set in
+	// platform.aws.amiID, and a custom AMI ID specified in the controlPlane stanza. So we just get the value from the
+	// first awsmachine manifest we find, instead of duplicating all the inheriting logic here.
+	for i := range in.MachineManifests {
+		if awsMachine, ok := in.MachineManifests[i].(*capa.AWSMachine); ok {
+			// Default/custom AMI already in target region, nothing else to do
+			if ptr.Deref(awsMachine.Spec.AMI.ID, "") != "" {
+				return nil
+			}
+		}
+	}
+
+	// Notice that we have to use the default RHCOS value because we set the AMI.ID to empty if the default RHCOS is not
+	// in the target region and it needs to be copied over. See pkg/asset/machines/clusterapi.go
+	amiID, err := copyAMIToRegion(ctx, in.InstallConfig, in.InfraID, in.RhcosImage)
 	if err != nil {
 		return fmt.Errorf("failed to copy AMI: %w", err)
 	}
+	// Update manifests with the new ID
 	for i := range in.MachineManifests {
 		if awsMachine, ok := in.MachineManifests[i].(*capa.AWSMachine); ok {
-			awsMachine.Spec.AMI = capa.AMIReference{ID: ptr.To(amiID)}
+			awsMachine.Spec.AMI.ID = ptr.To(amiID)
 		}
 	}
 	return nil