Merge pull request #4904 from r4f4/preserve-ignition

✨ s3: allow best effort delete for objects

Author: k8s-ci-robot

api/v1beta1/zz_generated.conversion.go

@@ -309,6 +309,10 @@ type S3Bucket struct {
 	// +kubebuilder:validation:MaxLength:=63
 	// +kubebuilder:validation:Pattern=`^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$`
 	Name string `json:"name"`
+
+	// BestEffortDeleteObjects defines whether access/permission errors during object deletion should be ignored.
+	// +optional
+	BestEffortDeleteObjects *bool `json:"bestEffortDeleteObjects,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1beta2/awscluster_types.go

@@ -1666,6 +1666,10 @@ spec:
                   (https://coreos.github.io/ignition/) for bootstrapping (requires
                   BootstrapFormatIgnition feature flag to be enabled).
                 properties:
+                  bestEffortDeleteObjects:
+                    description: BestEffortDeleteObjects defines whether access/permission
+                      errors during object deletion should be ignored.
+                    type: boolean
                   controlPlaneIAMInstanceProfile:
                     description: |-
                       ControlPlaneIAMInstanceProfile is a name of the IAMInstanceProfile, which will be allowed

api/v1beta2/zz_generated.deepcopy.go

@@ -1266,6 +1266,10 @@ spec:
                           (https://coreos.github.io/ignition/) for bootstrapping (requires
                           BootstrapFormatIgnition feature flag to be enabled).
                         properties:
+                          bestEffortDeleteObjects:
+                            description: BestEffortDeleteObjects defines whether access/permission
+                              errors during object deletion should be ignored.
+                            type: boolean
                           controlPlaneIAMInstanceProfile:
                             description: |-
                               ControlPlaneIAMInstanceProfile is a name of the IAMInstanceProfile, which will be allowed

config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml

@@ -32,6 +32,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/sts"
 	"github.com/aws/aws-sdk-go/service/sts/stsiface"
 	"github.com/pkg/errors"
+	"k8s.io/utils/ptr"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	iam "sigs.k8s.io/cluster-api-provider-aws/v2/iam/api/v1beta1"
@@ -194,12 +195,8 @@ func (s *Service) Delete(m *scope.MachineScope) error {
 				// 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")
+				if err := s.deleteObject(bucket, key); err != nil {
+					return err
 				}
 
 				s.scope.Debug("Delete object call succeeded despite missing GetObject permission", "bucket", bucket, "key", key)
@@ -221,11 +218,23 @@ func (s *Service) Delete(m *scope.MachineScope) error {
 
 	s.scope.Info("Deleting S3 object", "bucket", bucket, "key", key)
 
-	_, err = s.S3Client.DeleteObject(&s3.DeleteObjectInput{
+	return s.deleteObject(bucket, key)
+}
+
+func (s *Service) deleteObject(bucket, key string) error {
+	if _, err := s.S3Client.DeleteObject(&s3.DeleteObjectInput{
 		Bucket: aws.String(bucket),
 		Key:    aws.String(key),
-	})
-	if err != nil {
+	}); err != nil {
+		if ptr.Deref(s.scope.Bucket().BestEffortDeleteObjects, false) {
+			if aerr, ok := err.(awserr.Error); ok {
+				switch aerr.Code() {
+				case "Forbidden", "AccessDenied":
+					s.scope.Debug("Ignoring deletion error", "bucket", bucket, "key", key, "error", aerr.Message())
+					return nil
+				}
+			}
+		}
 		return errors.Wrap(err, "deleting S3 object")
 	}
 

config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml

@@ -738,6 +738,18 @@ func TestDeleteObject(t *testing.T) {
 				t.Fatalf("Unexpected error, got: %v", err)
 			}
 		})
+
+		t.Run("object_access_denied_and_BestEffortDeleteObjects_is_on", func(t *testing.T) {
+			t.Parallel()
+
+			svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{BestEffortDeleteObjects: aws.Bool(true)}})
+			s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, nil)
+			s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, awserr.New("AccessDenied", "Access Denied", nil))
+
+			if err := svc.Delete(machineScope); err != nil {
+				t.Fatalf("Unexpected error, got: %v", err)
+			}
+		})
 	})
 
 	t.Run("returns_error_when", func(t *testing.T) {
@@ -793,6 +805,27 @@ func TestDeleteObject(t *testing.T) {
 				t.Fatalf("Expected error")
 			}
 		})
+
+		t.Run("object_access_denied_and_BestEffortDeleteObjects_is_off", func(t *testing.T) {
+			t.Parallel()
+
+			svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
+			s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, nil)
+			s3Mock.EXPECT().DeleteObject(gomock.Any()).Return(nil, awserr.New("AccessDenied", "Access Denied", nil))
+
+			machineScope := &scope.MachineScope{
+				Machine: &clusterv1.Machine{},
+				AWSMachine: &infrav1.AWSMachine{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: nodeName,
+					},
+				},
+			}
+
+			if err := svc.Delete(machineScope); err == nil {
+				t.Fatalf("Expected error")
+			}
+		})
 	})
 
 	t.Run("is_idempotent", func(t *testing.T) {