[release-2.4] 🐛 fix: s3: fix bucket object not found (#4907)

* 🐛 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.

---------

Co-authored-by: Rafael Fonseca <[email protected]>

Author: k8s-infra-cherrypick-robot

pkg/cloud/services/s3/s3.go

@@ -204,17 +204,19 @@ 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)
 				return nil
 			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)

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) {