Refactor capi logic out from NodePool controller

This moves the CAPI related logic into their own file and add test coverage particularly for the reconcile function. Additional refactor of the business logic itself is left out intentionally for now to contain the scope of the refactor and avoid backward compatibility issues.
openshift · Sep 25, 2024 · bc26251 · bc26251
1 parent c34a1f6
commit bc26251
Show file tree

Hide file tree

Showing 13 changed files with 3,058 additions and 2,262 deletions.
diff --git a/hypershift-operator/controllers/nodepool/aws.go b/hypershift-operator/controllers/nodepool/aws.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/support/releaseinfo"
 	k8sutilspointer "k8s.io/utils/pointer"
 	capiaws "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 )
@@ -20,7 +21,21 @@ func awsClusterCloudProviderTagKey(id string) string {
 	return fmt.Sprintf("kubernetes.io/cluster/%s", id)
 }
 
-func awsMachineTemplateSpec(infraName, ami string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, defaultSG bool) (*capiaws.AWSMachineTemplateSpec, error) {
+func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, defaultSG bool, releaseImage *releaseinfo.ReleaseImage) (*capiaws.AWSMachineTemplateSpec, error) {
+
+	var ami string
+	region := hostedCluster.Spec.Platform.AWS.Region
+	arch := nodePool.Spec.Arch
+	if nodePool.Spec.Platform.AWS.AMI != "" {
+		ami = nodePool.Spec.Platform.AWS.AMI
+	} else {
+		// TODO: Should the region be included in the NodePool platform information?
+		var err error
+		ami, err = defaultNodePoolAMI(region, arch, releaseImage)
+		if err != nil {
+			return nil, fmt.Errorf("couldn't discover an AMI for release image: %w", err)
+		}
+	}
 
 	subnet := &capiaws.AWSResourceReference{}
 	subnet.ID = nodePool.Spec.Platform.AWS.Subnet.ID

diff --git a/hypershift-operator/controllers/nodepool/aws_test.go b/hypershift-operator/controllers/nodepool/aws_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/support/releaseinfo"
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sutilspointer "k8s.io/utils/pointer"
@@ -48,6 +49,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 					Type: hyperv1.AWSPlatform,
 					AWS: &hyperv1.AWSNodePoolPlatform{
 						RootVolume: &volume,
+						AMI:        amiName,
 					},
 				},
 				Release: hyperv1.Release{},
@@ -61,6 +63,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 				ResourceTags: []hyperv1.AWSResourceTag{
 					{Key: "key", Value: "value"},
 				},
+				AMI: amiName,
 			}}},
 
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
@@ -74,7 +77,9 @@ func TestAWSMachineTemplate(t *testing.T) {
 					{Key: "key", Value: "value"},
 				},
 			}}},
-			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{}}},
+			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{
+				AMI: amiName,
+			}}},
 
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
 				tmpl.Spec.Template.Spec.AdditionalTags["key"] = "value"
@@ -93,6 +98,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 					{Key: "nodepool-only", Value: "value"},
 					{Key: "cluster-and-nodepool", Value: "nodepool"},
 				},
+				AMI: amiName,
 			}}},
 
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
@@ -104,6 +110,9 @@ func TestAWSMachineTemplate(t *testing.T) {
 		{
 			name:          "Cluster default sg is used when none specified",
 			clusterStatus: &hyperv1.HostedClusterStatus{Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{DefaultWorkerSecurityGroupID: "cluster-default"}}},
+			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{
+				AMI: amiName,
+			}}},
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
 				tmpl.Spec.Template.Spec.AdditionalSecurityGroups = []capiaws.AWSResourceReference{{ID: k8sutilspointer.String("cluster-default")}}
 			}),
@@ -112,6 +121,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 			name: "NodePool sg is used in addition to cluster default",
 			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{
 				SecurityGroups: []hyperv1.AWSResourceReference{{ID: k8sutilspointer.String("nodepool-specific")}},
+				AMI:            amiName,
 			}}},
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
 				tmpl.Spec.Template.Spec.AdditionalSecurityGroups = []capiaws.AWSResourceReference{{ID: k8sutilspointer.String("nodepool-specific")}, {ID: defaultSG[0].ID}}
@@ -126,10 +136,15 @@ func TestAWSMachineTemplate(t *testing.T) {
 					t.Errorf("did not get expected NotReady error")
 				}
 			},
+			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{
+				AMI: amiName,
+			}}},
 		},
 		{
-			name:     "NodePool has ec2-http-tokens annotation with 'required' as a value",
-			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{}}},
+			name: "NodePool has ec2-http-tokens annotation with 'required' as a value",
+			nodePool: hyperv1.NodePoolSpec{Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{
+				AMI: amiName,
+			}}},
 			nodePoolAnnotations: map[string]string{
 				ec2InstanceMetadataHTTPTokensAnnotation: "required",
 			},
@@ -150,15 +165,17 @@ func TestAWSMachineTemplate(t *testing.T) {
 			if tc.clusterStatus != nil {
 				clusterStatus = *tc.clusterStatus
 			}
-			result, err := awsMachineTemplateSpec(infraName, amiName,
+			result, err := awsMachineTemplateSpec(infraName,
 				&hyperv1.HostedCluster{Spec: tc.cluster, Status: clusterStatus},
 				&hyperv1.NodePool{
 					ObjectMeta: metav1.ObjectMeta{
 						Annotations: tc.nodePoolAnnotations,
 					},
 					Spec: tc.nodePool,
 				},
-				true)
+				true,
+				&releaseinfo.ReleaseImage{},
+			)
 			if tc.checkError != nil {
 				tc.checkError(t, err)
 			} else {