From 154a82584a708445b85ad527474313f76a485f8f Mon Sep 17 00:00:00 2001
From: Rafael F <r4f4rfs@gmail.com>
Date: Wed, 27 Mar 2024 07:26:59 +0100
Subject: [PATCH] :bug: fix: s3: fix bucket object not found (#4879)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 🐛 fix: s3: do not ignore non-aws errors when deleting object

If any error of non awserr.Error type happens when trying to list a
bootstrap data object, it would be silently ignored.

* 🐛fix: s3: ignore "NotFound" errors

The `s3.HeadObject` API call can return "NotFound" when either the
bucket or the object does not exist (as opposed to the more descriptive
`s3.ErrCodeNoSuchKey` or `s3.ErrCodeNoSuchBucket`).

This would cause the machine controller to loop indefinitely trying to
delete an already deleted object but failing:

```
E0316 16:37:08.973942     366 awsmachine_controller.go:307] "unable to delete machine" err=<
	deleting bootstrap data object: deleting S3 object: NotFound: Not Found
		status code: 404, request id: 5Z101DW1KN380WTY, host id: tYlSi9K38lBkIsr2DNf/xFfgDuFaVfeUmpscXdljiMZC5iRxPIDuXSLwHJwdFnosYCfi7Bih25GaDpVAbSq4ZA==
 >
```

* 🌱s3: add unit test for already deleted s3 object.
---
 pkg/cloud/services/s3/s3.go      |  6 +++--
 pkg/cloud/services/s3/s3_test.go | 39 ++++++++++++++++++++++++++------
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/pkg/cloud/services/s3/s3.go b/pkg/cloud/services/s3/s3.go
index b6695fd006..7cccebc0f7 100644
--- a/pkg/cloud/services/s3/s3.go
+++ b/pkg/cloud/services/s3/s3.go
@@ -204,6 +204,9 @@ func (s *Service) Delete(m *scope.MachineScope) error {
 
 				s.scope.Debug("Delete object call succeeded despite missing GetObject permission", "bucket", bucket, "key", key)
 
+				return nil
+			case "NotFound":
+				s.scope.Debug("Either bucket or object does not exist", "bucket", bucket, "key", key)
 				return nil
 			case s3.ErrCodeNoSuchKey:
 				s.scope.Debug("Object already deleted", "bucket", bucket, "key", key)
@@ -211,10 +214,9 @@ func (s *Service) Delete(m *scope.MachineScope) error {
 			case s3.ErrCodeNoSuchBucket:
 				s.scope.Debug("Bucket does not exist", "bucket", bucket)
 				return nil
-			default:
-				return errors.Wrap(aerr, "deleting S3 object")
 			}
 		}
+		return errors.Wrap(err, "deleting S3 object")
 	}
 
 	s.scope.Info("Deleting S3 object", "bucket", bucket, "key", key)
diff --git a/pkg/cloud/services/s3/s3_test.go b/pkg/cloud/services/s3/s3_test.go
index baa44ff875..80422ffc4e 100644
--- a/pkg/cloud/services/s3/s3_test.go
+++ b/pkg/cloud/services/s3/s3_test.go
@@ -694,11 +694,9 @@ func TestDeleteObject(t *testing.T) {
 		}
 	})
 
-	t.Run("succeeds_when_bucket_has_already_been_removed", func(t *testing.T) {
+	t.Run("succeeds_when", func(t *testing.T) {
 		t.Parallel()
 
-		svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
-
 		machineScope := &scope.MachineScope{
 			Machine: &clusterv1.Machine{},
 			AWSMachine: &infrav1.AWSMachine{
@@ -708,11 +706,38 @@ func TestDeleteObject(t *testing.T) {
 			},
 		}
 
-		s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchBucket, "", nil))
+		t.Run("bucket_has_already_been_removed", func(t *testing.T) {
+			t.Parallel()
 
-		if err := svc.Delete(machineScope); err != nil {
-			t.Fatalf("Unexpected error, got: %v", err)
-		}
+			svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
+			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)
+			}
+		})
+
+		t.Run("object_has_already_been_removed", func(t *testing.T) {
+			t.Parallel()
+
+			svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
+			s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, awserr.New(s3svc.ErrCodeNoSuchKey, "", nil))
+
+			if err := svc.Delete(machineScope); err != nil {
+				t.Fatalf("Unexpected error, got: %v", err)
+			}
+		})
+
+		t.Run("bucket_or_object_not_found", func(t *testing.T) {
+			t.Parallel()
+
+			svc, s3Mock := testService(t, &testServiceInput{Bucket: &infrav1.S3Bucket{}})
+			s3Mock.EXPECT().HeadObject(gomock.Any()).Return(nil, awserr.New("NotFound", "Not found", nil))
+
+			if err := svc.Delete(machineScope); err != nil {
+				t.Fatalf("Unexpected error, got: %v", err)
+			}
+		})
 	})
 
 	t.Run("returns_error_when", func(t *testing.T) {