Skip to content

Commit

Permalink
🐛 fix: s3: fix bucket object not found (kubernetes-sigs#4879)
Browse files Browse the repository at this point in the history
* 🐛 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.
  • Loading branch information
r4f4 authored and fad3t committed Oct 8, 2024
1 parent d59c205 commit 154a825
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 9 deletions.
6 changes: 4 additions & 2 deletions pkg/cloud/services/s3/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
39 changes: 32 additions & 7 deletions pkg/cloud/services/s3/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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) {
Expand Down

0 comments on commit 154a825

Please sign in to comment.