diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go index 3b44b5ec2a..14f8d423bb 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go +++ b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go @@ -284,6 +284,7 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument { Action: iamv1.Actions{ "s3:CreateBucket", "s3:DeleteBucket", + "s3:GetObject", "s3:PutObject", "s3:DeleteObject", "s3:PutBucketPolicy", diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml index 9344f79aa0..39bd20ef2c 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml @@ -289,6 +289,7 @@ Resources: - Action: - s3:CreateBucket - s3:DeleteBucket + - s3:GetObject - s3:PutObject - s3:DeleteObject - s3:PutBucketPolicy diff --git a/controllers/awsmachine_controller.go b/controllers/awsmachine_controller.go index 0074293529..bec9ccaf44 100644 --- a/controllers/awsmachine_controller.go +++ b/controllers/awsmachine_controller.go @@ -850,8 +850,6 @@ func (r *AWSMachineReconciler) deleteIgnitionBootstrapDataFromS3(machineScope *s return nil } - machineScope.Info("Deleting unneeded entry from AWS S3", "secretPrefix", machineScope.GetSecretPrefix()) - if err := objectStoreSvc.Delete(machineScope); err != nil { return errors.Wrap(err, "deleting bootstrap data object") } diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go index 44e7ef3058..c330f5190b 100644 --- a/controllers/awsmachine_controller_unit_test.go +++ b/controllers/awsmachine_controller_unit_test.go @@ -1136,7 +1136,6 @@ func TestAWSMachineReconciler(t *testing.T) { _, err := reconciler.reconcileDelete(ms, cs, cs, cs, cs) g.Expect(err).To(BeNil()) - g.Expect(buf.String()).To(ContainSubstring("Deleting unneeded entry from AWS S3")) }) }) diff --git a/pkg/cloud/services/s3/s3.go b/pkg/cloud/services/s3/s3.go index cde883d64b..4bccb63072 100644 --- a/pkg/cloud/services/s3/s3.go +++ b/pkg/cloud/services/s3/s3.go @@ -173,27 +173,52 @@ func (s *Service) Delete(m *scope.MachineScope) error { bucket := s.bucketName() key := s.bootstrapDataKey(m) - s.scope.Info("Deleting object", "bucket_name", bucket, "key", key) - - _, err := s.S3Client.DeleteObject(&s3.DeleteObjectInput{ + _, err := s.S3Client.HeadObject(&s3.HeadObjectInput{ Bucket: aws.String(bucket), Key: aws.String(key), }) - if err == nil { - return nil - } - - aerr, ok := err.(awserr.Error) - if !ok { + if err != nil { + if aerr, ok := err.(awserr.Error); ok { + switch aerr.Code() { + case "Forbidden": + // In the case that the IAM policy does not have sufficient + // permissions to get the object, we will attempt to delete it + // anyway for backwards compatibility reasons. + s.scope.Debug("Received 403 forbidden from S3 HeadObject call. If GetObject permission has been granted to the controller but not ListBucket, object is already deleted. Attempting deletion anyway in case GetObject permission hasn't been granted to the controller but DeleteObject has.", "bucket", bucket, "key", key) + + _, err = s.S3Client.DeleteObject(&s3.DeleteObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + }) + if err != nil { + return errors.Wrap(err, "deleting S3 object") + } + + s.scope.Debug("Delete object call succeeded despite missing GetObject permission", "bucket", bucket, "key", key) + + return nil + case s3.ErrCodeNoSuchKey: + s.scope.Debug("Object already deleted", "bucket", bucket, "key", key) + return nil + case s3.ErrCodeNoSuchBucket: + s.scope.Debug("Bucket does not exist", "bucket", bucket) + return nil + default: + return errors.Wrap(aerr, "deleting S3 object") + } + } + } + + s.scope.Info("Deleting S3 object", "bucket", bucket, "key", key) + + _, err = s.S3Client.DeleteObject(&s3.DeleteObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + }) + if err != nil { return errors.Wrap(err, "deleting S3 object") } - switch aerr.Code() { - case s3.ErrCodeNoSuchBucket: - default: - return errors.Wrap(aerr, "deleting S3 object") - } - return nil } diff --git a/pkg/cloud/services/s3/s3_test.go b/pkg/cloud/services/s3/s3_test.go index 76e43691ec..0f4483522d 100644 --- a/pkg/cloud/services/s3/s3_test.go +++ b/pkg/cloud/services/s3/s3_test.go @@ -631,6 +631,8 @@ func TestDeleteObject(t *testing.T) { }, } + s3Mock.EXPECT().HeadObject(gomock.Any()) + s3Mock.EXPECT().DeleteObject(gomock.Any()).Do(func(deleteObjectInput *s3svc.DeleteObjectInput) { t.Run("use_configured_bucket_name_on_cluster_level", func(t *testing.T) { t.Parallel() @@ -672,7 +674,7 @@ func TestDeleteObject(t *testing.T) { }, } - s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil)).Times(1) + s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil)) if err := svc.Delete(machineScope); err != nil { t.Fatalf("Unexpected error, got: %v", err) @@ -696,6 +698,7 @@ func TestDeleteObject(t *testing.T) { }, } + s3Mock.EXPECT().HeadObject(gomock.Any()) s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, errors.New("foo")).Times(1) if err := svc.Delete(machineScope); err == nil { @@ -747,6 +750,7 @@ func TestDeleteObject(t *testing.T) { }, } + s3Mock.EXPECT().HeadObject(gomock.Any()).Times(2) s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, nil).Times(2) if err := svc.Delete(machineScope); err != nil {