Skip to content

Commit

Permalink
Add extra NoAction ResourceStatus to handle subresources with conditions
Browse files Browse the repository at this point in the history
  • Loading branch information
nolancon committed Jul 2, 2024
1 parent fee05f8 commit b9275b4
Show file tree
Hide file tree
Showing 12 changed files with 52 additions and 32 deletions.
5 changes: 2 additions & 3 deletions internal/controller/bucket/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (l *ACLClient) Observe(ctx context.Context, bucket *v1alpha1.Bucket, backen

for i := 0; i < len(backendNames); i++ {
observation := <-observationChan
if observation != Updated {
if observation == NeedsUpdate || observation == NeedsDeletion {
return observation, nil
}
}
Expand Down Expand Up @@ -92,9 +92,8 @@ func (l *ACLClient) Handle(ctx context.Context, b *v1alpha1.Bucket, backendName
defer span.End()

switch l.observeBackend(b, backendName) {
case Updated:
case NoAction, Updated:
return nil

case NeedsUpdate, NeedsDeletion:
if err := l.createOrUpdate(ctx, b, backendName); err != nil {
err = errors.Wrap(err, errHandleAcl)
Expand Down
16 changes: 11 additions & 5 deletions internal/controller/bucket/lifecycleconfiguration.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (l *LifecycleConfigurationClient) Observe(ctx context.Context, bucket *v1al

return NeedsUpdate, err
case observation := <-observationChan:
if observation != Updated {
if observation == NeedsUpdate || observation == NeedsDeletion {
return observation, nil
}
case err := <-errChan:
Expand All @@ -85,11 +85,11 @@ func (l *LifecycleConfigurationClient) observeBackend(ctx context.Context, bucke
l.log.Info("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 Updated.
// 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 Updated, nil
return NoAction, nil
}

s3Client, err := l.s3ClientHandler.GetS3Client(ctx, bucket, backendName)
Expand All @@ -108,7 +108,7 @@ func (l *LifecycleConfigurationClient) observeBackend(ctx context.Context, bucke
// No lifecycle config found on this backend.
l.log.Info("No lifecycle configuration found on backend - no action required", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName)

return Updated, nil
return NoAction, nil
} else {
l.log.Info("Lifecycle configuration found on backend - requires deletion", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName)

Expand Down Expand Up @@ -158,8 +158,14 @@ func (l *LifecycleConfigurationClient) Handle(ctx context.Context, b *v1alpha1.B
}

switch observation {
case Updated:
case NoAction:
return nil
case Updated:
// The lifecycle config is updated, so we can consider this
// sub resource Available.
available := xpv1.Available()
bb.setLifecycleConfigCondition(b.Name, backendName, &available)

case NeedsDeletion:
if err := l.delete(ctx, b, backendName); err != nil {
err = errors.Wrap(err, errHandleLifecycleConfig)
Expand Down
12 changes: 6 additions & 6 deletions internal/controller/bucket/lifecycleconfiguration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func TestObserveBackend(t *testing.T) {
err: errExternal,
},
},
"Attempt to observe lifecycle config on unhealthy backend (consider it updated to unblock)": {
"Attempt to observe lifecycle config on unhealthy backend (consider it NoAction to unblock)": {
fields: fields{
backendStore: func() *backendstore.BackendStore {
fake := backendstorefakes.FakeS3Client{}
Expand All @@ -120,7 +120,7 @@ func TestObserveBackend(t *testing.T) {
backendName: "s3-backend-1",
},
want: want{
status: Updated,
status: NoAction,
err: nil,
},
},
Expand Down Expand Up @@ -162,7 +162,7 @@ func TestObserveBackend(t *testing.T) {
err: nil,
},
},
"Lifecycle config not specified in CR and does exists on backend so is Updated": {
"Lifecycle config not specified in CR and does exists on backend so NoAction": {
fields: fields{
backendStore: func() *backendstore.BackendStore {
fake := backendstorefakes.FakeS3Client{
Expand Down Expand Up @@ -192,7 +192,7 @@ func TestObserveBackend(t *testing.T) {
backendName: "s3-backend-1",
},
want: want{
status: Updated,
status: NoAction,
err: nil,
},
},
Expand Down Expand Up @@ -245,7 +245,7 @@ func TestObserveBackend(t *testing.T) {
err: nil,
},
},
"Lifecycle config specified in CR and disabled but does not exist on backend so is Updated": {
"Lifecycle config specified in CR and disabled but does not exist on backend so is NoAction": {
fields: fields{
backendStore: func() *backendstore.BackendStore {
fake := backendstorefakes.FakeS3Client{
Expand Down Expand Up @@ -286,7 +286,7 @@ func TestObserveBackend(t *testing.T) {
backendName: "s3-backend-1",
},
want: want{
status: Updated,
status: NoAction,
err: nil,
},
},
Expand Down
13 changes: 10 additions & 3 deletions internal/controller/bucket/objectlockconfiguration.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func (l *ObjectLockConfigurationClient) Observe(ctx context.Context, bucket *v1a

return NeedsUpdate, err
case observation := <-observationChan:
if observation != Updated {
if observation == NeedsUpdate || observation == NeedsDeletion {
return observation, nil
}
case err := <-errChan:
Expand All @@ -89,11 +89,11 @@ func (l *ObjectLockConfigurationClient) observeBackend(ctx context.Context, buck
l.log.Info("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 Updated.
// 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 Updated, nil
return NoAction, nil
}

s3Client, err := l.s3ClientHandler.GetS3Client(ctx, bucket, backendName)
Expand Down Expand Up @@ -138,7 +138,14 @@ func (l *ObjectLockConfigurationClient) Handle(ctx context.Context, b *v1alpha1.
}

switch observation {
case NoAction:
return nil
case Updated:
// The object lock config is updated, so we can consider this
// sub resource Available.
available := xpv1.Available()
bb.setObjectLockConfigCondition(b.Name, backendName, &available)

return nil
case NeedsDeletion:
// Object lock configuration, once enabled, cannot be disabled/deleted.
Expand Down
5 changes: 3 additions & 2 deletions internal/controller/bucket/objectlockconfiguration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func TestObjectLockConfigObserveBackend(t *testing.T) {
args args
want want
}{
"Attempt to observe object lock config on unhealthy backend (consider it updated to unblock)": {
"Attempt to observe object lock config on unhealthy backend (consider it NoAction to unblock)": {
fields: fields{
backendStore: func() *backendstore.BackendStore {
fake := backendstorefakes.FakeS3Client{}
Expand All @@ -88,7 +88,7 @@ func TestObjectLockConfigObserveBackend(t *testing.T) {
backendName: "s3-backend-1",
},
want: want{
status: Updated,
status: NoAction,
err: nil,
},
},
Expand Down Expand Up @@ -238,6 +238,7 @@ func TestObjectLockConfigObserveBackend(t *testing.T) {
}
}

//nolint:maintidx //Test with lots of cases.
func TestObjectLockConfigurationHandle(t *testing.T) {
t.Parallel()
bucketName := "bucket"
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/bucket/observe.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex

return managed.ExternalObservation{}, err
}
if obs != Updated {
if obs == NeedsUpdate || obs == NeedsDeletion {
return managed.ExternalObservation{
ResourceExists: true,
ResourceUpToDate: false,
Expand Down
4 changes: 2 additions & 2 deletions internal/controller/bucket/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (p *PolicyClient) Observe(ctx context.Context, bucket *v1alpha1.Bucket, bac

return NeedsUpdate, err
case observation := <-observationChan:
if observation != Updated {
if observation == NeedsUpdate || observation == NeedsDeletion {
return observation, nil
}
case err := <-errChan:
Expand Down Expand Up @@ -140,7 +140,7 @@ func (p *PolicyClient) Handle(ctx context.Context, b *v1alpha1.Bucket, backendNa
}

switch observation {
case Updated:
case NoAction, Updated:
return nil
case NeedsDeletion:
if err := p.delete(ctx, b, backendName); err != nil {
Expand Down
4 changes: 3 additions & 1 deletion internal/controller/bucket/subresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ func NewSubresourceClients(b *backendstore.BackendStore, h *s3clienthandler.Hand
type ResourceStatus int

const (
// NoAction is returned if the resource requires no action.
NoAction ResourceStatus = iota
// Updated is returned if the resource is updated.
Updated ResourceStatus = iota
Updated
// NeedsUpdate is returned if the resource required updating.
NeedsUpdate
// NeedsDeletion is returned if the resource needs to be deleted.
Expand Down
15 changes: 11 additions & 4 deletions internal/controller/bucket/versioningconfiguration.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (l *VersioningConfigurationClient) Observe(ctx context.Context, bucket *v1a

return NeedsUpdate, err
case observation := <-observationChan:
if observation != Updated {
if observation == NeedsUpdate || observation == NeedsDeletion {
return observation, nil
}
case err := <-errChan:
Expand All @@ -84,11 +84,11 @@ func (l *VersioningConfigurationClient) observeBackend(ctx context.Context, buck
l.log.Info("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 Updated.
// 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 Updated, nil
return NoAction, nil
}

s3Client, err := l.s3ClientHandler.GetS3Client(ctx, bucket, backendName)
Expand All @@ -112,7 +112,7 @@ func (l *VersioningConfigurationClient) observeBackend(ctx context.Context, buck
// considered Updated for the bucket and we do nothing.
l.log.Info("Versioning is not enabled for bucket on backend - no action required", consts.KeyBucketName, bucket.Name, consts.KeyBackendName, backendName)

return Updated, nil
return NoAction, nil
} else {
// A non-empty versioning configuration was returned from the backend, signifying
// that versioning was previously enabled for this bucket. A bucket cannot be un-versioned,
Expand Down Expand Up @@ -153,7 +153,14 @@ func (l *VersioningConfigurationClient) Handle(ctx context.Context, b *v1alpha1.
}

switch observation {
case NoAction:
return nil
case Updated:
// The versioning config is updated, so we can consider this
// sub resource Available.
available := xpv1.Available()
bb.setVersioningConfigCondition(b.Name, backendName, &available)

return nil
case NeedsDeletion:
// Versioning Configurations are not deleted, only suspended, which requires an update.
Expand Down
6 changes: 3 additions & 3 deletions internal/controller/bucket/versioningconfiguration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func TestVersioningConfigObserveBackend(t *testing.T) {
err: errExternal,
},
},
"Attempt to observe versioniong config on unhealthy backend (consider it updated to unblock)": {
"Attempt to observe versioniong config on unhealthy backend (consider it NoAction to unblock)": {
fields: fields{
backendStore: func() *backendstore.BackendStore {
fake := backendstorefakes.FakeS3Client{}
Expand All @@ -115,7 +115,7 @@ func TestVersioningConfigObserveBackend(t *testing.T) {
backendName: "s3-backend-1",
},
want: want{
status: Updated,
status: NoAction,
err: nil,
},
},
Expand Down Expand Up @@ -175,7 +175,7 @@ func TestVersioningConfigObserveBackend(t *testing.T) {
backendName: "s3-backend-1",
},
want: want{
status: Updated,
status: NoAction,
err: nil,
},
},
Expand Down
1 change: 0 additions & 1 deletion internal/rgw/objectlockconfiguration.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//nolint:dupl // Similarities with versioningconfiguration.go
package rgw

import (
Expand Down
1 change: 0 additions & 1 deletion internal/rgw/versioningconfiguration.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//nolint:dupl // Similarities with objectlockconfiguration.go
package rgw

import (
Expand Down

0 comments on commit b9275b4

Please sign in to comment.