diff --git a/internal/controller/bucket/delete.go b/internal/controller/bucket/delete.go index 8473d840..fddb4d22 100644 --- a/internal/controller/bucket/delete.go +++ b/internal/controller/bucket/delete.go @@ -120,29 +120,36 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error { return err } - // If an error occurred during deletion, we must return for requeue. if deleteErr != nil { c.log.Info("Failed to delete bucket on one or more backends", "error", deleteErr.Error()) traces.SetAndRecordError(span, deleteErr) - // If the error is BucketNotEmpty error, the DeleteBucket operation should be failed - // and the client should be able to use the bucket with non-empty buckends. if errors.Is(deleteErr, rgw.ErrBucketNotEmpty) { + c.log.Info("Cannot deleted non-empty bucket - this error will not be requeued", consts.KeyBucketName, bucket.Name) + // An error occured attempting to delete the bucket because it is not empty. + // If this Delete operation was triggered because the Bucket CR was "Disabled", + // we need to unset this value so as not to continue attempting Delete. + // Otherwise we can return no error as we do not wish to requeue the Delete. + if !bucket.Spec.Disabled { + return nil + } if err := c.updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired { - c.log.Info("Change 'disabled' flag to false", consts.KeyBucketName, bucket.Name) + c.log.Info("Non-empty Buckets should not be disabled - setting 'disabled' flag to false", consts.KeyBucketName, bucket.Name) bucketLatest.Spec.Disabled = false return NeedsObjectUpdate }); err != nil { err = errors.Wrap(err, errUpdateBucketCR) - c.log.Info("Failed to change 'disabled' flag to false", consts.KeyBackendName, bucket.Name, "error", err.Error()) + c.log.Info("Failed to set 'disabled' flag to false", consts.KeyBackendName, bucket.Name, "error", err.Error()) traces.SetAndRecordError(span, err) return err } - } + return nil + } + // In all other cases we should return the deletion error for requeue. return deleteErr } diff --git a/internal/controller/bucket/delete_test.go b/internal/controller/bucket/delete_test.go index ca512b1b..5198f0bd 100644 --- a/internal/controller/bucket/delete_test.go +++ b/internal/controller/bucket/delete_test.go @@ -682,7 +682,111 @@ func TestDelete(t *testing.T) { }, }, want: want{ - err: rgw.ErrBucketNotEmpty, + err: nil, + statusDiff: func(t *testing.T, mg resource.Managed) { + t.Helper() + bucket, _ := mg.(*v1alpha1.Bucket) + + // s3-backend-1 failed so is stuck in Deleting status. + assert.True(t, + bucket.Status.AtProvider.Backends[s3Backend1].BucketCondition.Equal(xpv1.Deleting().WithMessage(fmt.Errorf("%w: %w", rgw.ErrBucketNotEmpty, rgw.BucketNotEmptyError{}).Error())), + "unexpected bucket condition on s3-backend-1") + + // s3-backend-2 was successfully deleted so was removed from status. + assert.False(t, + func(b v1alpha1.Backends) bool { + if _, ok := b[s3Backend2]; ok { + return true + } + + return false + }(bucket.Status.AtProvider.Backends), + "s3-backend-2 should not exist in backends") + + // If backend deletion fails due to BucketNotEmpty error, Disabled flag should be false. + assert.False(t, + bucket.Spec.Disabled, + "Disabled flag should be false", + ) + }, + finalizerDiff: func(t *testing.T, mg resource.Managed) { + t.Helper() + bucket, _ := mg.(*v1alpha1.Bucket) + + assert.Equal(t, + []string{v1alpha1.InUseFinalizer}, + bucket.Finalizers, + "unexpeceted finalizers", + ) + }, + }, + }, + "Error deleting disabled bucket because one specified bucket is not empty": { + fields: fields{ + backendStore: func() *backendstore.BackendStore { + // DeleteBucket first calls HeadBucket to establish + // if a bucket exists, so return a random error + // to mimic a failed delete. + fakeClient := &backendstorefakes.FakeS3Client{} + fakeClient.HeadBucketReturns( + &s3.HeadBucketOutput{}, + nil, + ) + fakeClient.DeleteBucketReturns( + nil, + rgw.BucketNotEmptyError{}, + ) + + // DeleteBucket first calls HeadBucket to establish + // if a bucket exists, so return not found + // error to short circuit a successful delete. + var notFoundError *s3types.NotFound + fakeClientOK := &backendstorefakes.FakeS3Client{} + fakeClientOK.HeadBucketReturns( + &s3.HeadBucketOutput{}, + notFoundError, + ) + + bs := backendstore.NewBackendStore() + bs.AddOrUpdateBackend(s3Backend1, fakeClient, nil, true, apisv1alpha1.HealthStatusHealthy) + bs.AddOrUpdateBackend(s3Backend2, fakeClientOK, nil, true, apisv1alpha1.HealthStatusHealthy) + + return bs + }(), + }, + args: args{ + mg: &v1alpha1.Bucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + Finalizers: []string{v1alpha1.InUseFinalizer}, + Labels: map[string]string{ + v1alpha1.BackendLabelPrefix + s3Backend1: "true", + v1alpha1.BackendLabelPrefix + s3Backend2: "true", + }, + }, + Spec: v1alpha1.BucketSpec{ + Providers: []string{ + s3Backend1, + s3Backend2, + }, + Disabled: true, + }, + Status: v1alpha1.BucketStatus{ + AtProvider: v1alpha1.BucketObservation{ + Backends: v1alpha1.Backends{ + s3Backend1: &v1alpha1.BackendInfo{ + BucketCondition: xpv1.Available(), + }, + s3Backend2: &v1alpha1.BackendInfo{ + BucketCondition: xpv1.Available(), + }, + }, + }, + }, + }, + }, + want: want{ + err: nil, statusDiff: func(t *testing.T, mg resource.Managed) { t.Helper() bucket, _ := mg.(*v1alpha1.Bucket)