Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #4907 (2024-04-04 11:16:23)

[release-2.4] :bug: fix: s3: fix bucket object not found
This is an automated cherry-pick of #4879

/assign richardcase

```release-note
Fix a bug where the machine controller will keep trying to delete an already deleted s3 object.
```

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [justinsb](https://github.com/justinsb) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/release-2.4/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["justinsb"]} -->
damdo: /lgtm
Ankitasw: /approve
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4907#issuecomment-2039026085" title="Approved">Ankitasw</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/release-2.4/OWNERS)~~ [Ankitasw]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/cloud/services/s3/s3.go b/pkg/cloud/services/s3/s3.go
index b6695fd00..7cccebc0f 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)


Test patch:
diff --git a/pkg/cloud/services/s3/s3_test.go b/pkg/cloud/services/s3/s3_test.go
index baa44ff87..80422ffc4 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) {