From 8c06a60222db5bb4beb77746e0a6eaf44de057d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sun, 24 May 2026 17:52:48 +0200 Subject: [PATCH] fix read-only resource extractors observing the empty desired base (#115) readResource fetched the cluster object into a deep copy that was then discarded; BaseResource.ExtractData re-deep-copied DesiredObject, so extractors on read-only resources only ever saw the inert base used to build the resource. Add concepts.ObservationRecorder, implement it on BaseResource, and have readResource invoke RecordObservation after Get so the fetched object is visible to subsequent extractors. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/component.md | 5 +- docs/custom-resource.md | 21 +++++--- pkg/component/concepts/observable.go | 21 ++++++++ pkg/component/read.go | 8 +++ pkg/component/read_test.go | 67 ++++++++++++++++++++++++++ pkg/component/zz_mock_resource_test.go | 9 ++++ pkg/generic/builder_base.go | 8 ++- pkg/generic/resource_base.go | 23 +++++++++ pkg/generic/resource_static_test.go | 50 +++++++++++++++++++ 9 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 pkg/component/concepts/observable.go diff --git a/docs/component.md b/docs/component.md index 90ed7149..b734b816 100644 --- a/docs/component.md +++ b/docs/component.md @@ -256,7 +256,10 @@ regardless of whether they are managed or read-only. For each resource: 2. The resource is either applied to the cluster (managed) or fetched from it (read-only). Managed resources use Server-Side Apply and get a controller owner reference pointing to the owner CRD, unless the resource is cluster-scoped and the owner is namespace-scoped (see [Cluster-Scoped Resources](#cluster-scoped-resources)). -3. If the resource implements `DataExtractable`, its data extractors run immediately. This makes extracted data +3. For read-only resources that implement `ObservationRecorder`, the framework records the fetched object back onto the + resource so that subsequent inspection observes the live cluster state. Resources built from `generic.BaseResource` + implement this automatically. +4. If the resource implements `DataExtractable`, its data extractors run immediately. This makes extracted data available to subsequent resources' guards and mutations within the same reconciliation cycle. This means a read-only resource registered before a managed resource can extract data that feeds into the managed diff --git a/docs/custom-resource.md b/docs/custom-resource.md index c1ddc735..ed3ef656 100644 --- a/docs/custom-resource.md +++ b/docs/custom-resource.md @@ -539,6 +539,7 @@ import ( // - concepts.Suspendable (DeleteOnSuspend, Suspend, SuspensionStatus) // - concepts.Guardable (GuardStatus) // - concepts.DataExtractable (ExtractData) +// - concepts.ObservationRecorder (RecordObservation) type Resource struct { base *generic.WorkloadResource[*examplev1.GameServer, *Mutator] } @@ -582,16 +583,24 @@ func (r *Resource) GuardStatus() (concepts.GuardStatusWithReason, error) { func (r *Resource) ExtractData() error { return r.base.ExtractData() } + +func (r *Resource) RecordObservation(observed client.Object) error { + return r.base.RecordObservation(observed) +} ``` +Forward `RecordObservation` whenever the resource may be registered as read-only with a data extractor. The framework +uses it to feed the fetched cluster object back to the resource before extraction runs; without it, the extractor would +see the inert base passed to the builder rather than live cluster state. + Which methods to include depends on your resource category: -| Category | Typical Methods | -| ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | -| Workload | `Identity`, `Object`, `Mutate`, `ConvergingStatus`, `GraceStatus`, `DeleteOnSuspend`, `Suspend`, `SuspensionStatus`, `GuardStatus`, `ExtractData` | -| Static | `Identity`, `Object`, `Mutate`, `GuardStatus`, `ExtractData` | -| Task | `Identity`, `Object`, `Mutate`, `ConvergingStatus`, `DeleteOnSuspend`, `Suspend`, `SuspensionStatus`, `GuardStatus`, `ExtractData` | -| Integration | `Identity`, `Object`, `Mutate`, `ConvergingStatus`, `GraceStatus`, `DeleteOnSuspend`, `Suspend`, `SuspensionStatus`, `GuardStatus`, `ExtractData` | +| Category | Typical Methods | +| ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Workload | `Identity`, `Object`, `Mutate`, `ConvergingStatus`, `GraceStatus`, `DeleteOnSuspend`, `Suspend`, `SuspensionStatus`, `GuardStatus`, `ExtractData`, `RecordObservation` | +| Static | `Identity`, `Object`, `Mutate`, `GuardStatus`, `ExtractData`, `RecordObservation` | +| Task | `Identity`, `Object`, `Mutate`, `ConvergingStatus`, `DeleteOnSuspend`, `Suspend`, `SuspensionStatus`, `GuardStatus`, `ExtractData`, `RecordObservation` | +| Integration | `Identity`, `Object`, `Mutate`, `ConvergingStatus`, `GraceStatus`, `DeleteOnSuspend`, `Suspend`, `SuspensionStatus`, `GuardStatus`, `ExtractData`, `RecordObservation` | ### 6. Define Feature Mutations diff --git a/pkg/component/concepts/observable.go b/pkg/component/concepts/observable.go new file mode 100644 index 00000000..7e5100be --- /dev/null +++ b/pkg/component/concepts/observable.go @@ -0,0 +1,21 @@ +package concepts + +import "sigs.k8s.io/controller-runtime/pkg/client" + +// ObservationRecorder defines the contract for resources whose internal state must be +// updated with what was just fetched from the cluster, before any subsequent inspection +// (most notably data extraction) takes place. +// +// Read-only resources are constructed with a base object that typically carries only +// enough identifying metadata for the framework to fetch the live object. The fetched +// object must then be recorded back onto the resource so that capabilities such as +// [DataExtractable] observe the cluster state rather than the inert base. +// +// Implementations should accept any object compatible with the resource's underlying +// type and return an error when the supplied object is not assignable to that type. +type ObservationRecorder interface { + // RecordObservation stores the supplied object as the resource's most recently + // observed cluster state. Subsequent calls to capabilities that inspect the + // resource's state (such as ExtractData) operate against this object. + RecordObservation(observed client.Object) error +} diff --git a/pkg/component/read.go b/pkg/component/read.go index e90711e0..5b5f7fc7 100644 --- a/pkg/component/read.go +++ b/pkg/component/read.go @@ -31,6 +31,14 @@ func readResource( ) } + if recorder, ok := resource.(concepts.ObservationRecorder); ok { + if err := recorder.RecordObservation(object); err != nil { + return nil, fmt.Errorf( + "failed to record observation for resource %s: %w", resource.Identity(), err, + ) + } + } + status, err := getConvergingStatus(resource, concepts.ConvergingOperationNone) if err != nil { return nil, fmt.Errorf( diff --git a/pkg/component/read_test.go b/pkg/component/read_test.go index ee837832..b358a36b 100644 --- a/pkg/component/read_test.go +++ b/pkg/component/read_test.go @@ -6,10 +6,12 @@ import ( "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -149,6 +151,71 @@ func TestReadResources(t *testing.T) { resource.AssertExpectations(t) }) + t.Run("should record the fetched object when the resource implements ObservationRecorder", func(t *testing.T) { + // Given a cluster Secret with populated data and a read-only resource + // constructed with an empty base; the framework must hand the fetched + // object to RecordObservation so that subsequent data extraction sees + // the cluster state, not the inert base. + clusterSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "observed-secret", + Namespace: namespace, + }, + Data: map[string][]byte{"token": []byte("from-cluster")}, + } + require.NoError(t, fakeClient.Create(ctx, clusterSecret)) + + baseObject := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "observed-secret", + Namespace: namespace, + }, + } + + resource := &MockObservationRecorderResource{} + resource.On("Object").Return(baseObject, nil) + resource.On("RecordObservation", mock.MatchedBy(func(obj client.Object) bool { + secret, ok := obj.(*corev1.Secret) + if !ok { + return false + } + return string(secret.Data["token"]) == "from-cluster" + })).Return(nil) + + // When + _, err := readResources(ctx, reconcileContext, []Resource{resource}) + + // Then + require.NoError(t, err) + resource.AssertExpectations(t) + }) + + t.Run("should propagate errors from RecordObservation", func(t *testing.T) { + clusterSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "record-fail-secret", + Namespace: namespace, + }, + } + require.NoError(t, fakeClient.Create(ctx, clusterSecret)) + + resource := &MockObservationRecorderResource{} + resource.On("Object").Return(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "record-fail-secret", + Namespace: namespace, + }, + }, nil) + resource.On("Identity").Return("v1/Secret/record-fail-secret") + resource.On("RecordObservation", mock.Anything).Return(errors.New("record failed")) + + _, err := readResources(ctx, reconcileContext, []Resource{resource}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "record failed") + assert.Contains(t, err.Error(), "v1/Secret/record-fail-secret") + }) + t.Run("should return error and update status if resource.ConvergingStatus() fails", func(t *testing.T) { // Given cm := &corev1.ConfigMap{ diff --git a/pkg/component/zz_mock_resource_test.go b/pkg/component/zz_mock_resource_test.go index 1b66890e..34452237 100644 --- a/pkg/component/zz_mock_resource_test.go +++ b/pkg/component/zz_mock_resource_test.go @@ -163,3 +163,12 @@ func (m *MockGuardableAliveResource) GuardStatus() (concepts.GuardStatusWithReas args := m.Called() return args.Get(0).(concepts.GuardStatusWithReason), args.Error(1) } + +type MockObservationRecorderResource struct { + MockResource +} + +func (m *MockObservationRecorderResource) RecordObservation(observed client.Object) error { + args := m.Called(observed) + return args.Error(0) +} diff --git a/pkg/generic/builder_base.go b/pkg/generic/builder_base.go index b27b7e29..19d0adfb 100644 --- a/pkg/generic/builder_base.go +++ b/pkg/generic/builder_base.go @@ -69,7 +69,13 @@ func (b *BaseBuilder[T, M]) WithGuard(handler func(T) (concepts.GuardStatusWithR b.BaseRes.GuardHandler = handler } -// WithDataExtractor registers a typed data extractor to run after successful reconciliation. +// WithDataExtractor registers a typed data extractor to run immediately after the +// resource has been processed during reconciliation. +// +// For managed resources, the extractor receives the object as it stands after feature +// mutations have been applied. For read-only resources, it receives the object as it +// was just fetched from the cluster. Extractors must be idempotent because they run on +// every reconcile pass. func (b *BaseBuilder[T, M]) WithDataExtractor(extractor func(T) error) { if extractor != nil { b.BaseRes.DataExtractors = append(b.BaseRes.DataExtractors, extractor) diff --git a/pkg/generic/resource_base.go b/pkg/generic/resource_base.go index ec406767..b19cd1f8 100644 --- a/pkg/generic/resource_base.go +++ b/pkg/generic/resource_base.go @@ -81,6 +81,10 @@ func (r *BaseResource[T, M]) PreviewObject() (T, error) { } // ExtractData runs all registered data extractors against a deep copy of the reconciled object. +// +// For managed resources the reconciled object is the desired state produced by Mutate. +// For read-only resources it is the object most recently supplied via RecordObservation, +// which the read flow invokes after fetching from the cluster. func (r *BaseResource[T, M]) ExtractData() error { copyObj, ok := r.DesiredObject.DeepCopyObject().(T) if !ok { @@ -99,6 +103,25 @@ func (r *BaseResource[T, M]) ExtractData() error { return nil } +// RecordObservation stores the supplied object as the resource's most recently observed +// cluster state. The framework invokes this on read-only resources immediately after +// fetching them, so that subsequent capabilities such as ExtractData observe the live +// object rather than the inert base used to construct the resource. +// +// RecordObservation returns an error when the supplied object is not assignable to the +// resource's underlying type. +func (r *BaseResource[T, M]) RecordObservation(observed client.Object) error { + typed, ok := observed.(T) + if !ok { + return fmt.Errorf( + "failed to record observation: expected object of type %T, got %T", + r.DesiredObject, observed, + ) + } + r.DesiredObject = typed + return nil +} + // GuardStatus evaluates the resource's guard precondition. // If no guard handler is configured, the resource is unconditionally unblocked. // The handler receives a deep copy of the desired object to prevent accidental mutations. diff --git a/pkg/generic/resource_static_test.go b/pkg/generic/resource_static_test.go index a8c18416..6ec99eab 100644 --- a/pkg/generic/resource_static_test.go +++ b/pkg/generic/resource_static_test.go @@ -90,6 +90,56 @@ func TestStaticResource(t *testing.T) { assert.EqualError(t, err, "extract error") }) + t.Run("RecordObservation makes the observed object visible to ExtractData", func(t *testing.T) { + base := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "base-cm", + Namespace: "default", + }, + } + readOnly := &StaticResource[*corev1.ConfigMap, *mockMutator]{ + BaseResource: BaseResource[*corev1.ConfigMap, *mockMutator]{ + DesiredObject: base, + IdentityFunc: identityFunc, + NewMutator: newMutator, + }, + } + + observed := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "base-cm", + Namespace: "default", + }, + Data: map[string]string{"foo": "from-cluster"}, + } + require.NoError(t, readOnly.RecordObservation(observed)) + + var seen string + readOnly.DataExtractors = []func(*corev1.ConfigMap) error{ + func(cm *corev1.ConfigMap) error { + seen = cm.Data["foo"] + return nil + }, + } + require.NoError(t, readOnly.ExtractData()) + assert.Equal(t, "from-cluster", seen, + "extractor must see the observed cluster object, not the empty desired base") + }) + + t.Run("RecordObservation rejects an object of the wrong type", func(t *testing.T) { + readOnly := &StaticResource[*corev1.ConfigMap, *mockMutator]{ + BaseResource: BaseResource[*corev1.ConfigMap, *mockMutator]{ + DesiredObject: &corev1.ConfigMap{}, + IdentityFunc: identityFunc, + NewMutator: newMutator, + }, + } + + err := readOnly.RecordObservation(&corev1.Secret{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "ConfigMap") + }) + t.Run("GuardStatus returns unblocked when no handler is set", func(t *testing.T) { res.GuardHandler = nil result, err := res.GuardStatus()