Merge pull request #1040 from hebelsan/bastion-rework

[GEP-27] Use cloud profile bastion machine and image

pkg/controller/bastion/actuator_reconcile.go

@@ -157,7 +157,7 @@ func ensureSecurityGroup(ctx context.Context, logger logr.Logger, bastion *exten
 		return "", err
 	}
 
-	permsToDelete := []ec2types.IpPermission{}
+	var permsToDelete []ec2types.IpPermission
 	for i, perm := range group.IpPermissionsEgress {
 		if !ipPermissionsEqual(perm, egressPermission) {
 			permsToDelete = append(permsToDelete, group.IpPermissionsEgress[i])

pkg/controller/bastion/options.go

@@ -7,19 +7,18 @@ package bastion
 import (
 	"context"
 	"fmt"
-	"regexp"
+	"slices"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/ec2"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
+	extensionsbastion "github.com/gardener/gardener/extensions/pkg/bastion"
 	"github.com/gardener/gardener/extensions/pkg/controller"
 	gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1"
 	extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1"
-	"github.com/gardener/gardener/pkg/client/kubernetes"
-	"github.com/gardener/gardener/pkg/extensions"
-	"k8s.io/apimachinery/pkg/util/sets"
 
-	awsv1alpha1 "github.com/gardener/gardener-extension-provider-aws/pkg/apis/aws/v1alpha1"
+	api "github.com/gardener/gardener-extension-provider-aws/pkg/apis/aws"
+	"github.com/gardener/gardener-extension-provider-aws/pkg/apis/aws/helper"
 	awsclient "github.com/gardener/gardener-extension-provider-aws/pkg/aws/client"
 )
 
@@ -47,6 +46,7 @@ type Options struct {
 // function does not create any IaaS resources.
 func DetermineOptions(ctx context.Context, bastion *extensionsv1alpha1.Bastion, cluster *controller.Cluster, awsClient *awsclient.Client) (*Options, error) {
 	name := cluster.ObjectMeta.Name
+	region := cluster.Shoot.Spec.Region
 	subnetName := name + "-public-utility-z0"
 	instanceName := fmt.Sprintf("%s-%s-bastion", name, bastion.Name)
 
@@ -68,19 +68,24 @@ func DetermineOptions(ctx context.Context, bastion *extensionsv1alpha1.Bastion,
 		return nil, fmt.Errorf("security group for worker node does not exist yet")
 	}
 
-	cloudProfileConfig, err := getCloudProfileConfig(cluster)
+	vmDetails, err := extensionsbastion.GetMachineSpecFromCloudProfile(cluster.CloudProfile)
+	if err != nil {
+		return nil, fmt.Errorf("failed to determine VM details for bastion host: %w", err)
+	}
+
+	cloudProfileConfig, err := helper.CloudProfileConfigFromCluster(cluster)
 	if err != nil {
 		return nil, fmt.Errorf("failed to extract cloud provider config from cluster: %w", err)
 	}
 
-	imageID, err := determineImageID(cluster.Shoot, cloudProfileConfig)
+	machineImageVersion, err := getProviderSpecificImage(cloudProfileConfig.MachineImages, vmDetails)
 	if err != nil {
-		return nil, fmt.Errorf("failed to determine OS image for bastion host: %w", err)
+		return nil, fmt.Errorf("failed to extract image from provider config: %w", err)
 	}
 
-	instanceType, err := determineInstanceType(ctx, imageID, awsClient)
+	ami, err := findImageAMIByRegion(machineImageVersion, vmDetails, region)
 	if err != nil {
-		return nil, fmt.Errorf("failed to determine instance type: %w", err)
+		return nil, fmt.Errorf("failed to find image AMI by region: %w", err)
 	}
 
 	return &Options{
@@ -91,8 +96,8 @@ func DetermineOptions(ctx context.Context, bastion *extensionsv1alpha1.Bastion,
 		WorkerSecurityGroupName:  workerSecurityGroupName,
 		WorkerSecurityGroupID:    *workerSecurityGroup.GroupId,
 		InstanceName:             instanceName,
-		InstanceType:             instanceType,
-		ImageID:                  imageID,
+		InstanceType:             vmDetails.MachineTypeName,
+		ImageID:                  ami,
 	}, nil
 }
 
@@ -122,131 +127,40 @@ func resolveSubnetName(ctx context.Context, awsClient *awsclient.Client, subnetN
 	return
 }
 
-func getCloudProfileConfig(cluster *extensions.Cluster) (*awsv1alpha1.CloudProfileConfig, error) {
-	if cluster.CloudProfile.Spec.ProviderConfig.Raw == nil {
-		return nil, fmt.Errorf("no cloud provider config set in cluster's CloudProfile")
-	}
-
-	var (
-		cloudProfileConfig = &awsv1alpha1.CloudProfileConfig{}
-		decoder            = kubernetes.GardenCodec.UniversalDeserializer()
-	)
-
-	if _, _, err := decoder.Decode(cluster.CloudProfile.Spec.ProviderConfig.Raw, nil, cloudProfileConfig); err != nil {
-		return nil, err
-	}
-
-	return cloudProfileConfig, nil
-}
-
-// determineImageID finds the first AMI that is configured for the same region as the shoot cluster.
-// If no image is found, an error is returned.
-func determineImageID(shoot *gardencorev1beta1.Shoot, providerConfig *awsv1alpha1.CloudProfileConfig) (string, error) {
-	// TODO(hebelsan): remove version hack after bastion image is well defined, e.g. in cloudProfile
-	// only allow garden linux versions 1312.x.x because they have ssh enabled by default
-	re := regexp.MustCompile(`^1312\.\d+\.\d+$`)
-	for _, image := range providerConfig.MachineImages {
-		for _, version := range image.Versions {
-			if image.Name == "gardenlinux" && !re.MatchString(version.Version) {
-				continue
-			}
-			for _, region := range version.Regions {
-				if region.Name == shoot.Spec.Region {
-					return region.AMI, nil
-				}
-			}
-		}
-	}
-
-	return "", fmt.Errorf("found no suitable AMI for machines in region %q", shoot.Spec.Region)
-}
-
-func determineInstanceType(ctx context.Context, imageID string, awsClient *awsclient.Client) (string, error) {
-	var preferredType string
-	imageInfo, err := getImages(ctx, imageID, awsClient)
-	if err != nil {
-		return "", err
-	}
-
-	imageArchitecture := imageInfo.Architecture
-
-	// default instance type
-	switch imageArchitecture {
-	case ec2types.ArchitectureValuesX8664:
-		preferredType = "t2.nano"
-	case ec2types.ArchitectureValuesArm64:
-		preferredType = "t4g.nano"
-	default:
-		return "", fmt.Errorf("image architecture not supported")
-	}
-
-	exist, err := getInstanceTypeOfferings(ctx, preferredType, awsClient)
-	if err != nil {
-		return "", err
-	}
-
-	if len(exist.InstanceTypeOfferings) != 0 {
-		return preferredType, nil
-	}
-
-	// filter t type instance
-	tTypes, err := getInstanceTypeOfferings(ctx, "t*", awsClient)
-	if err != nil {
-		return "", err
-	}
-
-	if len(tTypes.InstanceTypeOfferings) == 0 {
-		return "", fmt.Errorf("no t* instance type offerings available")
-	}
+// getProviderSpecificImage returns the provider specific MachineImageVersion that matches with the given VmDetails
+func getProviderSpecificImage(images []api.MachineImages, vm extensionsbastion.MachineSpec) (api.MachineImageVersion, error) {
+	imageIndex := slices.IndexFunc(images, func(image api.MachineImages) bool {
+		return image.Name == vm.ImageBaseName
+	})
 
-	tTypeSet := sets.Set[ec2types.InstanceType]{}
-	for _, t := range tTypes.InstanceTypeOfferings {
-		tTypeSet.Insert(t.InstanceType)
+	if imageIndex == -1 {
+		return api.MachineImageVersion{},
+			fmt.Errorf("machine image with name %s not found in cloudProfileConfig", vm.ImageBaseName)
 	}
 
-	result, err := awsClient.EC2.DescribeInstanceTypes(ctx, &ec2.DescribeInstanceTypesInput{
-		InstanceTypes: tTypeSet.UnsortedList(),
-		Filters: []ec2types.Filter{
-			{
-				Name:   aws.String("processor-info.supported-architecture"),
-				Values: []string{string(imageArchitecture)},
-			},
-		},
+	versions := images[imageIndex].Versions
+	versionIndex := slices.IndexFunc(versions, func(version api.MachineImageVersion) bool {
+		return version.Version == vm.ImageVersion
 	})
 
-	if err != nil {
-		return "", err
+	if versionIndex == -1 {
+		return api.MachineImageVersion{},
+			fmt.Errorf("version %s for arch %s of image %s not found in cloudProfileConfig",
+				vm.ImageVersion, vm.Architecture, vm.ImageBaseName)
 	}
 
-	if len(result.InstanceTypes) == 0 {
-		return "", fmt.Errorf("no instance types returned for architecture %s and instance types list %v", imageArchitecture, tTypeSet.UnsortedList())
-	}
-
-	return string(result.InstanceTypes[0].InstanceType), nil
+	return versions[versionIndex], nil
 }
 
-func getImages(ctx context.Context, ami string, awsClient *awsclient.Client) (*ec2types.Image, error) {
-	imageInfo, err := awsClient.EC2.DescribeImages(ctx, &ec2.DescribeImagesInput{
-		ImageIds: []string{ami},
+func findImageAMIByRegion(image api.MachineImageVersion, vmDetails extensionsbastion.MachineSpec, region string) (string, error) {
+	regionIndex := slices.IndexFunc(image.Regions, func(RegionAMIMapping api.RegionAMIMapping) bool {
+		return RegionAMIMapping.Name == region && RegionAMIMapping.Architecture != nil && *RegionAMIMapping.Architecture == vmDetails.Architecture
 	})
 
-	if err != nil {
-		return nil, fmt.Errorf("failed to get Images Info: %w", err)
-	}
-
-	if len(imageInfo.Images) == 0 {
-		return nil, fmt.Errorf("images info not found: %w", err)
+	if regionIndex == -1 {
+		return "", fmt.Errorf("image '%s' with version '%s' and architecture '%s' not found in region '%s'",
+			vmDetails.ImageBaseName, image.Version, vmDetails.Architecture, region)
 	}
-	return &imageInfo.Images[0], nil
-}
 
-func getInstanceTypeOfferings(ctx context.Context, filter string, awsClient *awsclient.Client) (*ec2.DescribeInstanceTypeOfferingsOutput, error) {
-	return awsClient.EC2.DescribeInstanceTypeOfferings(ctx, &ec2.DescribeInstanceTypeOfferingsInput{
-		Filters: []ec2types.Filter{
-			{
-				Name:   aws.String("instance-type"),
-				Values: []string{filter},
-			},
-		},
-	})
+	return image.Regions[regionIndex].AMI, nil
 }

pkg/controller/bastion/options_test.go

@@ -5,68 +5,88 @@
 package bastion
 
 import (
-	"github.com/gardener/gardener/pkg/apis/core/v1beta1"
+	extensionsbastion "github.com/gardener/gardener/extensions/pkg/bastion"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"k8s.io/utils/ptr"
 
-	apisaws "github.com/gardener/gardener-extension-provider-aws/pkg/apis/aws/v1alpha1"
+	api "github.com/gardener/gardener-extension-provider-aws/pkg/apis/aws"
 )
 
 var _ = Describe("Bastion Options", func() {
 	var region = "test-region"
 	var image = "gardenlinux"
+	var version = "1.0.0"
 	var ami = "test-ami"
-	var cloudProfileConfig *apisaws.CloudProfileConfig
-	var shoot *v1beta1.Shoot
+	var machineName = "test-machine"
+	var architecture = "amd64"
+	var amiMapping []api.RegionAMIMapping
+	var machineImageVersion api.MachineImageVersion
+	var machineImages []api.MachineImages
+	var vmDetails extensionsbastion.MachineSpec
 
 	BeforeEach(func() {
-		cloudProfileConfig = &apisaws.CloudProfileConfig{
-			MachineImages: []apisaws.MachineImages{
-				{
-					Name: image,
-					Versions: []apisaws.MachineImageVersion{
-						{
-							Regions: []apisaws.RegionAMIMapping{
-								{
-									Name: region,
-									AMI:  ami,
-								},
-							},
-						},
-					},
-				},
+		amiMapping = []api.RegionAMIMapping{
+			{
+				Name:         region,
+				AMI:          ami,
+				Architecture: ptr.To(architecture),
 			},
 		}
-		shoot = &v1beta1.Shoot{
-			Spec: v1beta1.ShootSpec{
-				Region: region,
+		machineImageVersion = api.MachineImageVersion{
+			Version: version,
+			Regions: amiMapping,
+		}
+		machineImages = []api.MachineImages{
+			{
+				Name:     image,
+				Versions: []api.MachineImageVersion{machineImageVersion},
 			},
 		}
+		vmDetails = extensionsbastion.MachineSpec{
+			MachineTypeName: machineName,
+			Architecture:    architecture,
+			ImageBaseName:   image,
+			ImageVersion:    version,
+		}
 	})
 
-	Context("determineImageID", func() {
-		var supportedGardenLinuxVersion = "1312.2.0"
-		var unsupportedGardenLinuxVersion = "1443.1.0"
-
-		It("should find imageID", func() {
-			cloudProfileConfig.MachineImages[0].Versions[0].Version = supportedGardenLinuxVersion
-			foundAmi, err := determineImageID(shoot, cloudProfileConfig)
+	Context("getProviderSpecificImage", func() {
+		It("should succeed for existing image and version", func() {
+			machineImageVersion, err := getProviderSpecificImage(machineImages, vmDetails)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(foundAmi).To(Equal(ami))
+			Expect(machineImageVersion).To(Equal(machineImageVersion))
 		})
 
-		It("should fail for unsupported image version", func() {
-			cloudProfileConfig.MachineImages[0].Versions[0].Version = unsupportedGardenLinuxVersion
-			_, err := determineImageID(shoot, cloudProfileConfig)
+		It("fail if image name does not exist", func() {
+			vmDetails.ImageBaseName = "unknown"
+			_, err := getProviderSpecificImage(machineImages, vmDetails)
 			Expect(err).To(HaveOccurred())
 		})
 
-		It("unsupported image version should pass for none gardenlinux images", func() {
-			cloudProfileConfig.MachineImages[0].Versions[0].Version = unsupportedGardenLinuxVersion
-			cloudProfileConfig.MachineImages[0].Name = "ubuntu"
-			foundAmi, err := determineImageID(shoot, cloudProfileConfig)
+		It("fail if image version does not exist", func() {
+			vmDetails.ImageVersion = "6.6.6"
+			_, err := getProviderSpecificImage(machineImages, vmDetails)
+			Expect(err).To(HaveOccurred())
+		})
+	})
+
+	Context("findImageAMIByRegion", func() {
+		It("should find image AMI by region", func() {
+			imageAmi, err := findImageAMIByRegion(machineImageVersion, vmDetails, region)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(foundAmi).To(Equal(ami))
+			Expect(imageAmi).To(Equal(ami))
+		})
+
+		It("fail if region does not match", func() {
+			_, err := findImageAMIByRegion(machineImageVersion, vmDetails, "unknown")
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("fail if architecture does not match", func() {
+			vmDetails.Architecture = "x86"
+			_, err := findImageAMIByRegion(machineImageVersion, vmDetails, region)
+			Expect(err).To(HaveOccurred())
 		})
 	})
 })