diff --git a/api/v1beta2/awsmachine_types.go b/api/v1beta2/awsmachine_types.go index 39a649a0e5..afa7c297a2 100644 --- 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 index 50af4f2211..7239384987 100644 --- 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 index 83fad02e00..2503796357 100644 --- 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 index d19a44e6f9..ca8b41df6d 100644 --- 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 index 7b2f94fd87..91fc0e3607 100644 --- 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 index 275d0b0c61..7cc420697c 100644 --- 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 index 47514135c6..5b6c087fdf 100644 --- 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 index 6dc2fd10b2..87831c5366 100644 --- 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 index 464b90f538..7ec328dd0d 100644 --- 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 index aca1b85a12..bdd3ccadab 100644 --- 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 index cc645df829..3a6e949cdc 100644 --- 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)}, + }) } }