Adapt Ignition user data S3 support to upstream PR (#614)

AndiDog · web-flow · commit bf5022362fef · 2025-01-23T12:19:08.000+01:00
See changes made in kubernetes-sigs#5172
diff --git a/api/v1beta2/awsmachine_types.go b/api/v1beta2/awsmachine_types.go
@@ -30,6 +30,21 @@ const (
 
 	// DefaultIgnitionVersion represents default Ignition version generated for machine userdata.
 	DefaultIgnitionVersion = "2.3"
+
+	// DefaultIgnitionStorageType represents the default storage type of Ignition userdata
+	DefaultIgnitionStorageType = IgnitionStorageTypeOptionClusterObjectStore
+
+	// DefaultMachinePoolIgnitionStorageType represents the default storage type of Ignition userdata for machine pools.
+	//
+	// This is only different from DefaultIgnitionStorageType because of backward compatibility. Machine pools used to
+	// default to store Ignition user data directly on the EC2 instance. Since the choice between remote storage (S3)
+	// and direct storage was introduced, the default was kept, but might change in newer API versions.
+	//
+	// GIANT SWARM CUSTOMIZED!!!: We already have clusters without explicit "ClusterObjectStore" storage type, so they
+	// should keep defaulting to this setting even if in the upstream PR, which merged later, the default became
+	// `IgnitionStorageTypeOptionUnencryptedUserData`. After upgrading all clusters to the latest cluster-aws version
+	// that sets the field explicitly, we can revert this to be equal to upstream's default:
+	DefaultMachinePoolIgnitionStorageType = IgnitionStorageTypeOptionClusterObjectStore
 )
 
 // SecretBackend defines variants for backend secret storage.
diff --git a/api/v1beta2/awsmachine_webhook.go b/api/v1beta2/awsmachine_webhook.go
@@ -399,12 +399,11 @@ func (r *AWSMachine) Default() {
 	}
 
 	if r.ignitionEnabled() && r.Spec.Ignition.Version == "" {
-		if r.Spec.Ignition == nil {
-			r.Spec.Ignition = &Ignition{}
-		}
-
 		r.Spec.Ignition.Version = DefaultIgnitionVersion
 	}
+	if r.ignitionEnabled() && r.Spec.Ignition.StorageType == "" {
+		r.Spec.Ignition.StorageType = DefaultIgnitionStorageType
+	}
 }
 
 func (r *AWSMachine) validateAdditionalSecurityGroups() field.ErrorList {
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go
@@ -295,8 +295,8 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument {
 			Action: iamv1.Actions{
 				"s3:CreateBucket",
 				"s3:DeleteBucket",
-				"s3:GetObject",
 				"s3:DeleteObject",
+				"s3:GetObject",
 				"s3:ListBucket",
 				"s3:PutBucketPolicy",
 				"s3:PutBucketTagging",
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml
@@ -301,8 +301,8 @@ Resources:
         - Action:
           - s3:CreateBucket
           - s3:DeleteBucket
-          - s3:GetObject
           - s3:DeleteObject
+          - s3:GetObject
           - s3:ListBucket
           - s3:PutBucketPolicy
           - s3:PutBucketTagging
diff --git a/controllers/awsmachine_controller.go b/controllers/awsmachine_controller.go
@@ -739,7 +739,7 @@ func (r *AWSMachineReconciler) resolveUserData(machineScope *scope.MachineScope,
 	if machineScope.UseIgnition(userDataFormat) {
 		var ignitionStorageType infrav1.IgnitionStorageTypeOption
 		if machineScope.AWSMachine.Spec.Ignition == nil {
-			ignitionStorageType = infrav1.IgnitionStorageTypeOptionClusterObjectStore
+			ignitionStorageType = infrav1.DefaultIgnitionStorageType
 		} else {
 			ignitionStorageType = machineScope.AWSMachine.Spec.Ignition.StorageType
 		}
@@ -795,8 +795,8 @@ func (r *AWSMachineReconciler) cloudInitUserData(machineScope *scope.MachineScop
 // then returns the config to instruct ignition on how to pull the user data from the bucket.
 func (r *AWSMachineReconciler) generateIgnitionWithRemoteStorage(scope *scope.MachineScope, objectStoreSvc services.ObjectStoreInterface, userData []byte) ([]byte, error) {
 	if objectStoreSvc == nil {
-		return nil, errors.New("using Ignition by default requires a cluster wide object storage configured at `AWSCluster.Spec.Ignition.S3Bucket`. " +
-			"You must configure one or instruct Ignition to use EC2 user data instead, by setting `AWSMachine.Spec.Ignition.StorageType` to `UnencryptedUserData`")
+		return nil, errors.New("using Ignition by default requires a cluster wide object storage configured at `AWSCluster.spec.s3Bucket`. " +
+			"You must configure one or instruct Ignition to use EC2 user data instead, by setting `AWSMachine.spec.ignition.storageType` to `UnencryptedUserData`")
 	}
 
 	objectURL, err := objectStoreSvc.Create(scope, userData)
diff --git a/exp/api/v1beta2/awsmachinepool_webhook.go b/exp/api/v1beta2/awsmachinepool_webhook.go
@@ -254,4 +254,7 @@ func (r *AWSMachinePool) Default() {
 	if r.ignitionEnabled() && r.Spec.Ignition.Version == "" {
 		r.Spec.Ignition.Version = infrav1.DefaultIgnitionVersion
 	}
+	if r.ignitionEnabled() && r.Spec.Ignition.StorageType == "" {
+		r.Spec.Ignition.StorageType = infrav1.DefaultMachinePoolIgnitionStorageType
+	}
 }
diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go
@@ -418,7 +418,7 @@ func (r *AWSMachinePoolReconciler) reconcileDelete(machinePoolScope *scope.Machi
 	}
 
 	launchTemplateID := machinePoolScope.AWSMachinePool.Status.LaunchTemplateID
-	launchTemplate, _, _, _, err := ec2Svc.GetLaunchTemplate(machinePoolScope.LaunchTemplateName())
+	launchTemplate, _, _, _, err := ec2Svc.GetLaunchTemplate(machinePoolScope.LaunchTemplateName()) //nolint:dogsled
 	if err != nil {
 		return err
 	}
diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go
@@ -37,12 +37,14 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	apimachinerytypes "k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
+	utilfeature "k8s.io/component-base/featuregate/testing"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
+	"sigs.k8s.io/cluster-api-provider-aws/v2/feature"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services"
@@ -772,6 +774,8 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			})
 
 			t.Run("launch template and ASG exist, bootstrap data secret name changed, Ignition bootstrap data stored in S3", func(t *testing.T) {
+				defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)()
+
 				g := NewWithT(t)
 				setup(t, g)
 				reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
@@ -787,7 +791,9 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 
 				// Enable Ignition S3 storage
 				cs.AWSCluster.Spec.S3Bucket = &infrav1.S3Bucket{}
-				ms.AWSMachinePool.Spec.Ignition = &infrav1.Ignition{}
+				ms.AWSMachinePool.Spec.Ignition = &infrav1.Ignition{
+					StorageType: infrav1.IgnitionStorageTypeOptionClusterObjectStore,
+				}
 				ms.AWSMachinePool.Default() // simulate webhook that sets default ignition version
 
 				asgSvc.EXPECT().GetASGByName(gomock.Any()).DoAndReturn(func(scope *scope.MachinePoolScope) (*expinfrav1.AutoScalingGroup, error) {
diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go
@@ -92,8 +92,20 @@ func (s *Service) ReconcileLaunchTemplate(
 		return nil, err
 	}
 
+	var ignitionStorageType = infrav1.DefaultMachinePoolIgnitionStorageType
+	var ignitionVersion = infrav1.DefaultIgnitionVersion
+	if ignition := ignitionScope.Ignition(); ignition != nil {
+		ignitionStorageType = ignition.StorageType
+		ignitionVersion = ignition.Version
+	}
+
 	var userDataForLaunchTemplate []byte
-	if s3Scope.Bucket() != nil && bootstrapDataFormat == "ignition" && ignitionScope.Ignition() != nil {
+	if bootstrapDataFormat == "ignition" && ignitionStorageType == infrav1.IgnitionStorageTypeOptionClusterObjectStore {
+		if s3Scope.Bucket() == nil {
+			return nil, errors.New("using Ignition with `AWSMachinePool.spec.ignition.storageType=ClusterObjectStore` " +
+				"requires a cluster wide object storage configured at `AWSCluster.spec.s3Bucket`")
+		}
+
 		scope.Info("Using S3 bucket storage for Ignition format")
 
 		// S3 bucket storage enabled and Ignition format is used. Ignition supports reading large user data from S3,
@@ -108,10 +120,9 @@ func (s *Service) ReconcileLaunchTemplate(
 			return nil, err
 		}
 
-		ignVersion := ignitionScope.Ignition().Version
-		semver, err := semver.ParseTolerant(ignVersion)
+		semver, err := semver.ParseTolerant(ignitionVersion)
 		if err != nil {
-			err = errors.Wrapf(err, "failed to parse ignition version %q", ignVersion)
+			err = errors.Wrapf(err, "failed to parse ignition version %q", ignitionVersion)
 			conditions.MarkFalse(scope.GetSetter(), expinfrav1.LaunchTemplateReadyCondition, expinfrav1.LaunchTemplateReconcileFailedReason, clusterv1.ConditionSeverityError, err.Error())
 			return nil, err
 		}
@@ -159,7 +170,7 @@ func (s *Service) ReconcileLaunchTemplate(
 				return nil, err
 			}
 		default:
-			err = errors.Errorf("unsupported ignition version %q", ignVersion)
+			err = errors.Errorf("unsupported ignition version %q", ignitionVersion)
 			conditions.MarkFalse(scope.GetSetter(), expinfrav1.LaunchTemplateReadyCondition, expinfrav1.LaunchTemplateReconcileFailedReason, clusterv1.ConditionSeverityError, err.Error())
 			return nil, err
 		}
@@ -279,7 +290,8 @@ func (s *Service) ReconcileLaunchTemplate(
 			if err != nil {
 				return nil, err
 			}
-			if deletedLaunchTemplateVersionBootstrapDataHash != nil && s3Scope.Bucket() != nil && bootstrapDataFormat == "ignition" && ignitionScope.Ignition() != nil {
+
+			if deletedLaunchTemplateVersionBootstrapDataHash != nil && s3Scope.Bucket() != nil && bootstrapDataFormat == "ignition" && ignitionStorageType == infrav1.IgnitionStorageTypeOptionClusterObjectStore {
 				scope.Info("Deleting S3 object for deleted launch template version", "version", *deletedLaunchTemplateVersion.VersionNumber)
 
 				err = objectStoreSvc.DeleteForMachinePool(scope, *deletedLaunchTemplateVersionBootstrapDataHash)
diff --git a/pkg/cloud/services/ec2/launchtemplate_test.go b/pkg/cloud/services/ec2/launchtemplate_test.go
@@ -85,14 +85,16 @@ var testBootstrapDataHash = userdata.ComputeHash(testBootstrapData)
 
 func defaultEC2AndDataTags(name string, clusterName string, userDataSecretKey types.NamespacedName, bootstrapDataHash string) []*ec2.Tag {
 	tags := defaultEC2Tags(name, clusterName)
-	tags = append(tags, &ec2.Tag{
-		Key:   aws.String(infrav1.LaunchTemplateBootstrapDataSecret),
-		Value: aws.String(userDataSecretKey.String()),
-	})
-	tags = append(tags, &ec2.Tag{
-		Key:   aws.String(infrav1.LaunchTemplateBootstrapDataHash),
-		Value: aws.String(bootstrapDataHash),
-	})
+	tags = append(
+		tags,
+		&ec2.Tag{
+			Key:   aws.String(infrav1.LaunchTemplateBootstrapDataSecret),
+			Value: aws.String(userDataSecretKey.String()),
+		},
+		&ec2.Tag{
+			Key:   aws.String(infrav1.LaunchTemplateBootstrapDataHash),
+			Value: aws.String(bootstrapDataHash),
+		})
 	sortTags(tags)
 	return tags
 }
diff --git a/pkg/cloud/services/s3/s3.go b/pkg/cloud/services/s3/s3.go
@@ -219,6 +219,7 @@ func (s *Service) Create(m *scope.MachineScope, data []byte) (string, error) {
 	return objectURL.String(), nil
 }
 
+// CreateForMachinePool creates an object for machine pool related bootstrap data in the S3 bucket.
 func (s *Service) CreateForMachinePool(scope scope.LaunchTemplateScope, data []byte) (string, error) {
 	if !s.bucketManagementEnabled() {
 		return "", errors.New("requested object creation but bucket management is not enabled")
@@ -336,6 +337,7 @@ func (s *Service) deleteObject(bucket, key string) error {
 	return nil
 }
 
+// DeleteForMachinePool deletes the object for machine pool related bootstrap data from the S3 bucket.
 func (s *Service) DeleteForMachinePool(scope scope.LaunchTemplateScope, bootstrapDataHash string) error {
 	if !s.bucketManagementEnabled() {
 		return errors.New("requested object deletion but bucket management is not enabled")
@@ -440,6 +442,8 @@ func (s *Service) ensureBucketLifecycleConfiguration(bucketName string) error {
 						// The bootstrap token for new nodes to join the cluster is normally rotated regularly,
 						// such as in CAPI's `KubeadmConfig` reconciler. Therefore, the launch template user data
 						// stored in the S3 bucket only needs to live longer than the token TTL.
+						// This lifecycle policy is here as backup. Normally, CAPA should delete outdated S3 objects
+						// (see function `DeleteForMachinePool`).
 						Days: aws.Int64(1),
 					},
 					Filter: &s3.LifecycleRuleFilter{
@@ -537,24 +541,26 @@ func (s *Service) bucketPolicy(bucketName string) (string, error) {
 		}
 
 		for _, iamInstanceProfile := range bucket.NodesIAMInstanceProfiles {
-			statements = append(statements, iam.StatementEntry{
-				Sid:    iamInstanceProfile,
-				Effect: iam.EffectAllow,
-				Principal: map[iam.PrincipalType]iam.PrincipalID{
-					iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, iamInstanceProfile)},
-				},
-				Action:   []string{"s3:GetObject"},
-				Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/node/*", partition, bucketName)},
-			})
-			statements = append(statements, iam.StatementEntry{
-				Sid:    iamInstanceProfile,
-				Effect: iam.EffectAllow,
-				Principal: map[iam.PrincipalType]iam.PrincipalID{
-					iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, iamInstanceProfile)},
+			statements = append(
+				statements,
+				iam.StatementEntry{
+					Sid:    iamInstanceProfile,
+					Effect: iam.EffectAllow,
+					Principal: map[iam.PrincipalType]iam.PrincipalID{
+						iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, iamInstanceProfile)},
+					},
+					Action:   []string{"s3:GetObject"},
+					Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/node/*", partition, bucketName)},
 				},
-				Action:   []string{"s3:GetObject"},
-				Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/machine-pool/*", partition, bucketName)},
-			})
+				iam.StatementEntry{
+					Sid:    iamInstanceProfile,
+					Effect: iam.EffectAllow,
+					Principal: map[iam.PrincipalType]iam.PrincipalID{
+						iam.PrincipalAWS: []string{fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, *accountID.Account, iamInstanceProfile)},
+					},
+					Action:   []string{"s3:GetObject"},
+					Resource: []string{fmt.Sprintf("arn:%s:s3:::%s/machine-pool/*", partition, bucketName)},
+				})
 		}
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -399,12 +399,11 @@ func (r *AWSMachine) Default() {`
`399`	`399`	`}`
`400`	`400`
`401`	`401`	`if r.ignitionEnabled() && r.Spec.Ignition.Version == "" {`
`402`		`- if r.Spec.Ignition == nil {`
`403`		`- r.Spec.Ignition = &Ignition{}`
`404`		`- }`
`405`		`-`
`406`	`402`	`r.Spec.Ignition.Version = DefaultIgnitionVersion`
`407`	`403`	`}`
	`404`	`+ if r.ignitionEnabled() && r.Spec.Ignition.StorageType == "" {`
	`405`	`+ r.Spec.Ignition.StorageType = DefaultIgnitionStorageType`
	`406`	`+ }`
`408`	`407`	`}`
`409`	`408`
`410`	`409`	`func (r *AWSMachine) validateAdditionalSecurityGroups() field.ErrorList {`
Original file line number	Diff line number	Diff line change
`@@ -254,4 +254,7 @@ func (r *AWSMachinePool) Default() {`
`254`	`254`	`if r.ignitionEnabled() && r.Spec.Ignition.Version == "" {`
`255`	`255`	`r.Spec.Ignition.Version = infrav1.DefaultIgnitionVersion`
`256`	`256`	`}`
	`257`	`+ if r.ignitionEnabled() && r.Spec.Ignition.StorageType == "" {`
	`258`	`+ r.Spec.Ignition.StorageType = infrav1.DefaultMachinePoolIgnitionStorageType`
	`259`	`+ }`
`257`	`260`	`}`
Original file line number	Diff line number	Diff line change
`@@ -418,7 +418,7 @@ func (r AWSMachinePoolReconciler) reconcileDelete(machinePoolScope scope.Machi`
`418`	`418`	`}`
`419`	`419`
`420`	`420`	`launchTemplateID := machinePoolScope.AWSMachinePool.Status.LaunchTemplateID`
`421`		`- launchTemplate, _, _, _, err := ec2Svc.GetLaunchTemplate(machinePoolScope.LaunchTemplateName())`
	`421`	`+ launchTemplate, _, _, _, err := ec2Svc.GetLaunchTemplate(machinePoolScope.LaunchTemplateName()) //nolint:dogsled`
`422`	`422`	`if err != nil {`
`423`	`423`	`return err`
`424`	`424`	`}`