diff --git a/internal/controller/bucket/acl.go b/internal/controller/bucket/acl.go index ec5372b7..de2a1382 100644 --- a/internal/controller/bucket/acl.go +++ b/internal/controller/bucket/acl.go @@ -40,6 +40,15 @@ func (l *ACLClient) Observe(ctx context.Context, bucket *v1alpha1.Bucket, backen for _, backendName := range backendNames { beName := backendName go func() { + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. + // The backend may be down for some time and we do not want to block Create/Update/Delete + // calls on other backends. By returning NoAction here, we would never pass the Observe + // phase until the backend becomes Healthy or Disabled. + observationChan <- NoAction + + return + } observationChan <- l.observeBackend(bucket, beName) }() } @@ -57,14 +66,6 @@ func (l *ACLClient) Observe(ctx context.Context, bucket *v1alpha1.Bucket, backen func (l *ACLClient) observeBackend(bucket *v1alpha1.Bucket, backendName string) ResourceStatus { l.log.Debug("Observing subresource acl on backend", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName) - if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { - // If a backend is marked as unhealthy, we can ignore it for now by returning Updated. - // The backend may be down for some time and we do not want to block Create/Update/Delete - // calls on other backends. By returning NeedsUpdate here, we would never pass the Observe - // phase until the backend becomes Healthy or Disabled. - return Updated - } - // If your bucket uses the bucket owner enforced setting for S3 Object // Ownership, ACLs are disabled and no longer affect permissions. if s3types.ObjectOwnership(aws.ToString(bucket.Spec.ForProvider.ObjectOwnership)) == s3types.ObjectOwnershipBucketOwnerEnforced { @@ -92,6 +93,12 @@ func (l *ACLClient) Handle(ctx context.Context, b *v1alpha1.Bucket, backendName ctx, span := otel.Tracer("").Start(ctx, "bucket.ACLClient.Handle") defer span.End() + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + traces.SetAndRecordError(span, errUnhealthyBackend) + + return errUnhealthyBackend + } + switch l.observeBackend(b, backendName) { case NoAction, Updated: return nil diff --git a/internal/controller/bucket/consts.go b/internal/controller/bucket/consts.go index e58f52ad..3373fb86 100644 --- a/internal/controller/bucket/consts.go +++ b/internal/controller/bucket/consts.go @@ -1,5 +1,9 @@ package bucket +import "github.com/crossplane/crossplane-runtime/pkg/errors" + +var errUnhealthyBackend = errors.New("backend marked as unhealthy in backendstore") + const ( // k8s error messages. errNotBucket = "managed resource is not a Bucket custom resource" diff --git a/internal/controller/bucket/lifecycleconfiguration.go b/internal/controller/bucket/lifecycleconfiguration.go index 92e46757..83be4944 100644 --- a/internal/controller/bucket/lifecycleconfiguration.go +++ b/internal/controller/bucket/lifecycleconfiguration.go @@ -47,6 +47,16 @@ func (l *LifecycleConfigurationClient) Observe(ctx context.Context, bucket *v1al for _, backendName := range backendNames { beName := backendName go func() { + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. + // The backend may be down for some time and we do not want to block Create/Update/Delete + // calls on other backends. By returning NoAction here, we would never pass the Observe + // phase until the backend becomes Healthy or Disabled. + observationChan <- NoAction + + return + } + observation, err := l.observeBackend(ctx, bucket, beName) if err != nil { errChan <- err @@ -80,18 +90,9 @@ func (l *LifecycleConfigurationClient) Observe(ctx context.Context, bucket *v1al return Updated, nil } -//nolint:gocyclo,cyclop // Function requires multiple checks. func (l *LifecycleConfigurationClient) observeBackend(ctx context.Context, bucket *v1alpha1.Bucket, backendName string) (ResourceStatus, error) { l.log.Debug("Observing subresource lifecycle configuration on backend", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName) - if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { - // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. - // The backend may be down for some time and we do not want to block Create/Update/Delete - // calls on other backends. By returning NeedsUpdate here, we would never pass the Observe - // phase until the backend becomes Healthy or Disabled. - return NoAction, nil - } - s3Client, err := l.s3ClientHandler.GetS3Client(ctx, bucket, backendName) if err != nil { return NeedsUpdate, err @@ -149,6 +150,12 @@ func (l *LifecycleConfigurationClient) Handle(ctx context.Context, b *v1alpha1.B ctx, span := otel.Tracer("").Start(ctx, "bucket.LifecycleConfigurationClient.Handle") defer span.End() + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + traces.SetAndRecordError(span, errUnhealthyBackend) + + return errUnhealthyBackend + } + observation, err := l.observeBackend(ctx, b, backendName) if err != nil { err = errors.Wrap(err, errHandleLifecycleConfig) diff --git a/internal/controller/bucket/lifecycleconfiguration_test.go b/internal/controller/bucket/lifecycleconfiguration_test.go index 1df333a8..3818a56b 100644 --- a/internal/controller/bucket/lifecycleconfiguration_test.go +++ b/internal/controller/bucket/lifecycleconfiguration_test.go @@ -488,6 +488,31 @@ func TestHandle(t *testing.T) { args args want want }{ + "Unhealthy backend": { + fields: fields{ + backendStore: func() *backendstore.BackendStore { + fake := backendstorefakes.FakeS3Client{} + bs := backendstore.NewBackendStore() + bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusUnhealthy) + + return bs + }(), + }, + args: args{ + bucket: &v1alpha1.Bucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: bucketName, + }, + Spec: v1alpha1.BucketSpec{ + LifecycleConfigurationDisabled: false, + }, + }, + backendName: beName, + }, + want: want{ + err: errUnhealthyBackend, + }, + }, "Lifecycle config deletes successfully": { fields: fields{ backendStore: func() *backendstore.BackendStore { diff --git a/internal/controller/bucket/objectlockconfiguration.go b/internal/controller/bucket/objectlockconfiguration.go index bd61cbd2..8680f739 100644 --- a/internal/controller/bucket/objectlockconfiguration.go +++ b/internal/controller/bucket/objectlockconfiguration.go @@ -52,6 +52,16 @@ func (l *ObjectLockConfigurationClient) Observe(ctx context.Context, bucket *v1a for _, backendName := range backendNames { beName := backendName go func() { + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. + // The backend may be down for some time and we do not want to block Create/Update/Delete + // calls on other backends. By returning NoAction here, we would never pass the Observe + // phase until the backend becomes Healthy or Disabled. + observationChan <- NoAction + + return + } + observation, err := l.observeBackend(ctx, bucket, beName) if err != nil { errChan <- err @@ -88,14 +98,6 @@ func (l *ObjectLockConfigurationClient) Observe(ctx context.Context, bucket *v1a func (l *ObjectLockConfigurationClient) observeBackend(ctx context.Context, bucket *v1alpha1.Bucket, backendName string) (ResourceStatus, error) { l.log.Debug("Observing subresource object lock configuration on backend", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName) - if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { - // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. - // The backend may be down for some time and we do not want to block Create/Update/Delete - // calls on other backends. By returning NeedsUpdate here, we would never pass the Observe - // phase until the backend becomes Healthy or Disabled. - return NoAction, nil - } - s3Client, err := l.s3ClientHandler.GetS3Client(ctx, bucket, backendName) if err != nil { return NeedsUpdate, err @@ -129,6 +131,12 @@ func (l *ObjectLockConfigurationClient) Handle(ctx context.Context, b *v1alpha1. return nil } + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + traces.SetAndRecordError(span, errUnhealthyBackend) + + return errUnhealthyBackend + } + observation, err := l.observeBackend(ctx, b, backendName) if err != nil { err = errors.Wrap(err, errHandleVersioningConfig) diff --git a/internal/controller/bucket/objectlockconfiguration_test.go b/internal/controller/bucket/objectlockconfiguration_test.go index 77b3d7c5..08bc9547 100644 --- a/internal/controller/bucket/objectlockconfiguration_test.go +++ b/internal/controller/bucket/objectlockconfiguration_test.go @@ -61,37 +61,6 @@ func TestObjectLockConfigObserveBackend(t *testing.T) { args args want want }{ - "Attempt to observe object lock config on unhealthy backend (consider it NoAction to unblock)": { - fields: fields{ - backendStore: func() *backendstore.BackendStore { - fake := backendstorefakes.FakeS3Client{} - - bs := backendstore.NewBackendStore() - bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusUnhealthy) - - return bs - }(), - }, - args: args{ - bucket: &v1alpha1.Bucket{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bucket", - }, - Spec: v1alpha1.BucketSpec{ - ForProvider: v1alpha1.BucketParameters{ - ObjectLockConfiguration: &v1alpha1.ObjectLockConfiguration{ - ObjectLockEnabled: &objLockEnabled, - }, - }, - }, - }, - backendName: "s3-backend-1", - }, - want: want{ - status: NoAction, - err: nil, - }, - }, "External error getting object lock": { fields: fields{ backendStore: func() *backendstore.BackendStore { @@ -265,6 +234,41 @@ func TestObjectLockConfigurationHandle(t *testing.T) { args args want want }{ + "Unhealthy backend": { + fields: fields{ + backendStore: func() *backendstore.BackendStore { + fake := backendstorefakes.FakeS3Client{} + bs := backendstore.NewBackendStore() + bs.AddOrUpdateBackend(beName, &fake, nil, true, apisv1alpha1.HealthStatusUnhealthy) + + return bs + }(), + }, + args: args{ + bucket: &v1alpha1.Bucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: bucketName, + }, + Spec: v1alpha1.BucketSpec{ + ForProvider: v1alpha1.BucketParameters{ + ObjectLockEnabledForBucket: &enabledTrue, + ObjectLockConfiguration: &v1alpha1.ObjectLockConfiguration{ + ObjectLockEnabled: &objLockEnabled, + Rule: &v1alpha1.ObjectLockRule{ + DefaultRetention: &v1alpha1.DefaultRetention{ + Mode: v1alpha1.ModeGovernance, + }, + }, + }, + }, + }, + }, + backendName: beName, + }, + want: want{ + err: errUnhealthyBackend, + }, + }, "Object lock is not enabled for Bucket CR - nil value": { fields: fields{ backendStore: func() *backendstore.BackendStore { diff --git a/internal/controller/bucket/policy.go b/internal/controller/bucket/policy.go index 56c159cf..646d2719 100644 --- a/internal/controller/bucket/policy.go +++ b/internal/controller/bucket/policy.go @@ -41,6 +41,16 @@ func (p *PolicyClient) Observe(ctx context.Context, bucket *v1alpha1.Bucket, bac for _, backendName := range backendNames { beName := backendName go func() { + if p.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. + // The backend may be down for some time and we do not want to block Create/Update/Delete + // calls on other backends. By returning NoAction here, we would never pass the Observe + // phase until the backend becomes Healthy or Disabled. + observationChan <- NoAction + + return + } + observation, err := p.observeBackend(ctx, bucket, beName) if err != nil { errChan <- err @@ -77,14 +87,6 @@ func (p *PolicyClient) Observe(ctx context.Context, bucket *v1alpha1.Bucket, bac func (p *PolicyClient) observeBackend(ctx context.Context, bucket *v1alpha1.Bucket, backendName string) (ResourceStatus, error) { p.log.Debug("Observing subresource policy on backend", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName) - if p.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { - // If a backend is marked as unhealthy, we can ignore it for now by returning Updated. - // The backend may be down for some time and we do not want to block Create/Update/Delete - // calls on other backends. By returning NeedsUpdate here, we would never pass the Observe - // phase until the backend becomes Healthy or Disabled. - return Updated, nil - } - s3Client, err := p.s3ClientHandler.GetS3Client(ctx, bucket, backendName) if err != nil { return NeedsUpdate, err @@ -131,6 +133,12 @@ func (p *PolicyClient) Handle(ctx context.Context, b *v1alpha1.Bucket, backendNa ctx, span := otel.Tracer("").Start(ctx, "bucket.PolicyClient.Handle") defer span.End() + if p.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + traces.SetAndRecordError(span, errUnhealthyBackend) + + return errUnhealthyBackend + } + observation, err := p.observeBackend(ctx, b, backendName) if err != nil { err = errors.Wrap(err, errHandlePolicy) diff --git a/internal/controller/bucket/policy_test.go b/internal/controller/bucket/policy_test.go index 558162bc..17a39b43 100644 --- a/internal/controller/bucket/policy_test.go +++ b/internal/controller/bucket/policy_test.go @@ -71,7 +71,6 @@ func TestPolicyObserveBackend(t *testing.T) { status: Updated, }, }, - "s3 error": { fields: fields{ backendStore: func() *backendstore.BackendStore { diff --git a/internal/controller/bucket/versioningconfiguration.go b/internal/controller/bucket/versioningconfiguration.go index a1268f64..d8148a1a 100644 --- a/internal/controller/bucket/versioningconfiguration.go +++ b/internal/controller/bucket/versioningconfiguration.go @@ -47,6 +47,16 @@ func (l *VersioningConfigurationClient) Observe(ctx context.Context, bucket *v1a for _, backendName := range backendNames { beName := backendName go func() { + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. + // The backend may be down for some time and we do not want to block Create/Update/Delete + // calls on other backends. By returning NoAction here, we would never pass the Observe + // phase until the backend becomes Healthy or Disabled. + observationChan <- NoAction + + return + } + observation, err := l.observeBackend(ctx, bucket, beName) if err != nil { errChan <- err @@ -83,18 +93,11 @@ func (l *VersioningConfigurationClient) Observe(ctx context.Context, bucket *v1a func (l *VersioningConfigurationClient) observeBackend(ctx context.Context, bucket *v1alpha1.Bucket, backendName string) (ResourceStatus, error) { l.log.Debug("Observing subresource versioning configuration on backend", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName) - if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { - // If a backend is marked as unhealthy, we can ignore it for now by returning NoAction. - // The backend may be down for some time and we do not want to block Create/Update/Delete - // calls on other backends. By returning NeedsUpdate here, we would never pass the Observe - // phase until the backend becomes Healthy or Disabled. - return NoAction, nil - } - s3Client, err := l.s3ClientHandler.GetS3Client(ctx, bucket, backendName) if err != nil { return NeedsUpdate, err } + response, err := rgw.GetBucketVersioning(ctx, s3Client, aws.String(bucket.Name)) if err != nil { return NeedsUpdate, err @@ -144,6 +147,12 @@ func (l *VersioningConfigurationClient) Handle(ctx context.Context, b *v1alpha1. ctx, span := otel.Tracer("").Start(ctx, "bucket.VersioningConfigurationClient.Handle") defer span.End() + if l.backendStore.GetBackendHealthStatus(backendName) == apisv1alpha1.HealthStatusUnhealthy { + traces.SetAndRecordError(span, errUnhealthyBackend) + + return errUnhealthyBackend + } + observation, err := l.observeBackend(ctx, b, backendName) if err != nil { err = errors.Wrap(err, errHandleVersioningConfig) diff --git a/internal/controller/bucket/versioningconfiguration_test.go b/internal/controller/bucket/versioningconfiguration_test.go index d963f509..af34fe08 100644 --- a/internal/controller/bucket/versioningconfiguration_test.go +++ b/internal/controller/bucket/versioningconfiguration_test.go @@ -343,6 +343,33 @@ func TestVersioningConfigurationHandle(t *testing.T) { args args want want }{ + "Unhealthy backend": { + fields: fields{ + backendStore: func() *backendstore.BackendStore { + fake := backendstorefakes.FakeS3Client{} + bs := backendstore.NewBackendStore() + bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusUnhealthy) + + return bs + }(), + }, + args: args{ + bucket: &v1alpha1.Bucket{ + ObjectMeta: metav1.ObjectMeta{ + Name: bucketName, + }, + Spec: v1alpha1.BucketSpec{ + ForProvider: v1alpha1.BucketParameters{ + ObjectLockEnabledForBucket: &enabledTrue, + }, + }, + }, + backendName: beName, + }, + want: want{ + err: errUnhealthyBackend, + }, + }, "Object lock enabled for bucket but no versioning config so set default enabled versioning": { fields: fields{ backendStore: func() *backendstore.BackendStore {