diff --git a/duck/client.go b/duck/client.go index 0cb4466..90da17b 100644 --- a/duck/client.go +++ b/duck/client.go @@ -81,9 +81,19 @@ func NewDuckAwareClientWrapper(client client.Client) client.Client { } } +func NewDangerousDuckAwareClientWrapper(client client.Client) client.Client { + return &duckAwareClientWrapper{ + Reader: NewDuckAwareAPIReaderWrapper(client, client), + client: client, + allowDangerousRequests: true, + } +} + type duckAwareClientWrapper struct { client.Reader client client.Client + + allowDangerousRequests bool } func (c *duckAwareClientWrapper) Watch(ctx context.Context, list client.ObjectList, opts ...client.ListOption) (watch.Interface, error) { @@ -116,7 +126,19 @@ func (c *duckAwareClientWrapper) Create(ctx context.Context, obj client.Object, return c.client.Create(ctx, obj, opts...) } - return fmt.Errorf("Create is not supported for the duck typed objects") + if !c.allowDangerousRequests { + return fmt.Errorf("Create is not supported for the duck typed objects") + } + + uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return err + } + u := &unstructured.Unstructured{Object: uObj} + if err := c.client.Create(ctx, obj, opts...); err != nil { + return err + } + return runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, obj) } func (c *duckAwareClientWrapper) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { @@ -124,7 +146,19 @@ func (c *duckAwareClientWrapper) Update(ctx context.Context, obj client.Object, return c.client.Update(ctx, obj, opts...) } - return fmt.Errorf("Update is not supported for the duck typed objects, use Patch instead") + if !c.allowDangerousRequests { + return fmt.Errorf("Update is not supported for the duck typed objects, use Patch instead") + } + + uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return err + } + u := &unstructured.Unstructured{Object: uObj} + if err := c.client.Update(ctx, obj, opts...); err != nil { + return err + } + return runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, obj) } func (c *duckAwareClientWrapper) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { diff --git a/reconcilers/child.go b/reconcilers/child.go index dc56a1c..a063421 100644 --- a/reconcilers/child.go +++ b/reconcilers/child.go @@ -16,11 +16,13 @@ import ( corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/vmware-labs/reconciler-runtime/duck" "github.com/vmware-labs/reconciler-runtime/internal" ) @@ -149,6 +151,16 @@ type ChildReconciler[Type, ChildType client.Object, ChildListType client.ObjectL // +optional Sanitize func(child ChildType) interface{} + // DangerouslyAllowDuckTypedChildren allows the ChildType to be a duck typed resource. This is + // dangerous because duck types typically represent a subset of the target resource and may + // cause data loss if the resource's server representation contains fields that do not exist on + // the duck typed object. + // + // Use of this setting should be limited to when the author is certain the duck type is able to + // represent the resource with full fidelity, or when data loss for unrepresented fields is + // acceptable. + DangerouslyAllowDuckTypedChildren bool + stamp *ResourceManager[ChildType] lazyInit sync.Once } @@ -197,10 +209,16 @@ func (r *ChildReconciler[T, CT, CLT]) SetupWithManager(ctx context.Context, mgr return err } + var ct client.Object = r.ChildType + if duck.IsDuck(ct, mgr.GetScheme()) { + gvk := ct.GetObjectKind().GroupVersionKind() + ct = &unstructured.Unstructured{} + ct.GetObjectKind().SetGroupVersionKind(gvk) + } if r.SkipOwnerReference { - bldr.Watches(r.ChildType, EnqueueTracked(ctx)) + bldr.Watches(ct, EnqueueTracked(ctx)) } else { - bldr.Owns(r.ChildType) + bldr.Owns(ct) } if r.Setup == nil { @@ -210,6 +228,8 @@ func (r *ChildReconciler[T, CT, CLT]) SetupWithManager(ctx context.Context, mgr } func (r *ChildReconciler[T, CT, CLT]) validate(ctx context.Context) error { + c := RetrieveConfigOrDie(ctx) + // default implicit values if r.Finalizer != "" { r.SkipOwnerReference = true @@ -235,6 +255,11 @@ func (r *ChildReconciler[T, CT, CLT]) validate(ctx context.Context) error { return fmt.Errorf("ChildReconciler %q must implement MergeBeforeUpdate", r.Name) } + // require DangerouslyAllowDuckTypedChildren for duck types + if !r.DangerouslyAllowDuckTypedChildren && duck.IsDuck(r.ChildType, c.Scheme()) { + return fmt.Errorf("ChildReconciler %q must enable DangerouslyAllowDuckTypedChildren to use a child duck type", r.Name) + } + return nil } @@ -249,6 +274,10 @@ func (r *ChildReconciler[T, CT, CLT]) Reconcile(ctx context.Context, resource T) r.init() c := RetrieveConfigOrDie(ctx) + if r.DangerouslyAllowDuckTypedChildren { + c = c.WithDangerousDuckClientOperations() + ctx = StashConfig(ctx, c) + } log := logr.FromContextOrDiscard(ctx). WithName(r.Name). diff --git a/reconcilers/childset.go b/reconcilers/childset.go index 2020572..01a8cdc 100644 --- a/reconcilers/childset.go +++ b/reconcilers/childset.go @@ -12,6 +12,7 @@ import ( "sync" "github.com/go-logr/logr" + "github.com/vmware-labs/reconciler-runtime/duck" "github.com/vmware-labs/reconciler-runtime/internal" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" @@ -151,6 +152,16 @@ type ChildSetReconciler[Type, ChildType client.Object, ChildListType client.Obje // +optional Sanitize func(child ChildType) interface{} + // DangerouslyAllowDuckTypedChildren allows the ChildType to be a duck typed resource. This is + // dangerous because duck types typically represent a subset of the target resource and may + // cause data loss if the resource's server representation contains fields that do not exist on + // the duck typed object. + // + // Use of this setting should be limited to when the author is certain the duck type is able to + // represent the resource with full fidelity, or when data loss for unrepresented fields is + // acceptable. + DangerouslyAllowDuckTypedChildren bool + stamp *ResourceManager[ChildType] lazyInit sync.Once voidReconciler *ChildReconciler[Type, ChildType, ChildListType] @@ -232,11 +243,14 @@ func (r *ChildSetReconciler[T, CT, CLT]) childReconcilerFor(desired CT, desiredE } return void || id == r.IdentifyChild(child) }, - Sanitize: r.Sanitize, + Sanitize: r.Sanitize, + DangerouslyAllowDuckTypedChildren: r.DangerouslyAllowDuckTypedChildren, } } func (r *ChildSetReconciler[T, CT, CLT]) validate(ctx context.Context) error { + c := RetrieveConfigOrDie(ctx) + // default implicit values if r.Finalizer != "" { r.SkipOwnerReference = true @@ -262,6 +276,11 @@ func (r *ChildSetReconciler[T, CT, CLT]) validate(ctx context.Context) error { return fmt.Errorf("ChildSetReconciler %q must implement IdentifyChild", r.Name) } + // require DangerouslyAllowDuckTypedChildren for duck types + if !r.DangerouslyAllowDuckTypedChildren && duck.IsDuck(r.ChildType, c.Scheme()) { + return fmt.Errorf("ChildSetReconciler %q must enable DangerouslyAllowDuckTypedChildren to use a child duck type", r.Name) + } + return nil } diff --git a/reconcilers/config.go b/reconcilers/config.go index b9d265b..05c0987 100644 --- a/reconcilers/config.go +++ b/reconcilers/config.go @@ -49,6 +49,21 @@ func (c Config) WithCluster(cluster cluster.Cluster) Config { } } +// WithDangerousDuckClientOperations returns a new Config with client Create and Update methods for +// duck typed objects enabled. +// +// This is dangerous because duck types typically represent a subset of the target resource and may +// cause data loss if the resource's server representation contains fields that do not exist on the +// duck typed object. +func (c Config) WithDangerousDuckClientOperations() Config { + return Config{ + Client: duck.NewDangerousDuckAwareClientWrapper(c.Client), + APIReader: c.APIReader, + Recorder: c.Recorder, + Tracker: c.Tracker, + } +} + // TrackAndGet tracks the resources for changes and returns the current value. The track is // registered even when the resource does not exists so that its creation can be tracked. // diff --git a/reconcilers/validate_test.go b/reconcilers/validate_test.go index ac8b4ef..5576b60 100644 --- a/reconcilers/validate_test.go +++ b/reconcilers/validate_test.go @@ -14,11 +14,13 @@ import ( "github.com/vmware-labs/reconciler-runtime/internal/resources" "github.com/vmware-labs/reconciler-runtime/tracker" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -365,6 +367,8 @@ func TestSyncReconciler_validate(t *testing.T) { } func TestChildReconciler_validate(t *testing.T) { + scheme := runtime.NewScheme() + tests := []struct { name string parent *corev1.ConfigMap @@ -505,18 +509,82 @@ func TestChildReconciler_validate(t *testing.T) { }, } - for _, c := range tests { - t.Run(c.name, func(t *testing.T) { - ctx := StashResourceType(context.TODO(), c.parent) - err := c.reconciler.validate(ctx) - if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) { - t.Errorf("validate() error = %q, shouldErr %q", err.Error(), c.shouldErr) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := Config{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + } + ctx := context.TODO() + ctx = StashResourceType(ctx, tc.parent) + ctx = StashConfig(ctx, c) + err := tc.reconciler.validate(ctx) + if (err != nil) != (tc.shouldErr != "") || (tc.shouldErr != "" && tc.shouldErr != err.Error()) { + t.Errorf("validate() error = %q, shouldErr %q", err.Error(), tc.shouldErr) + } + }) + } +} + +func TestChildReconciler_validate_DuckTypedChild(t *testing.T) { + scheme := runtime.NewScheme() + + tests := []struct { + name string + parent *corev1.ConfigMap + reconciler *ChildReconciler[*corev1.ConfigMap, *resources.TestDuck, *resources.TestDuckList] + shouldErr string + }{ + { + name: "DangerouslyAllowDuckTypedChildren disabled", + parent: &corev1.ConfigMap{}, + reconciler: &ChildReconciler[*corev1.ConfigMap, *resources.TestDuck, *resources.TestDuckList]{ + Name: "DangerouslyAllowDuckTypedChildren disabled", + ChildType: &resources.TestDuck{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResource"}}, + ChildListType: &resources.TestDuckList{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResourceList"}}, + DesiredChild: func(ctx context.Context, parent *corev1.ConfigMap) (*resources.TestDuck, error) { return nil, nil }, + ReflectChildStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, child *resources.TestDuck, err error) {}, + MergeBeforeUpdate: func(current, desired *resources.TestDuck) {}, + OurChild: func(parent *corev1.ConfigMap, child *resources.TestDuck) bool { return false }, + + DangerouslyAllowDuckTypedChildren: false, + }, + shouldErr: `ChildReconciler "DangerouslyAllowDuckTypedChildren disabled" must enable DangerouslyAllowDuckTypedChildren to use a child duck type`, + }, + { + name: "DangerouslyAllowDuckTypedChildren enabled", + parent: &corev1.ConfigMap{}, + reconciler: &ChildReconciler[*corev1.ConfigMap, *resources.TestDuck, *resources.TestDuckList]{ + ChildType: &resources.TestDuck{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResource"}}, + ChildListType: &resources.TestDuckList{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResourceList"}}, + DesiredChild: func(ctx context.Context, parent *corev1.ConfigMap) (*resources.TestDuck, error) { return nil, nil }, + ReflectChildStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, child *resources.TestDuck, err error) {}, + MergeBeforeUpdate: func(current, desired *resources.TestDuck) {}, + OurChild: func(parent *corev1.ConfigMap, child *resources.TestDuck) bool { return false }, + + DangerouslyAllowDuckTypedChildren: true, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := Config{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + } + ctx := context.TODO() + ctx = StashResourceType(ctx, tc.parent) + ctx = StashConfig(ctx, c) + err := tc.reconciler.validate(ctx) + if (err != nil) != (tc.shouldErr != "") || (tc.shouldErr != "" && tc.shouldErr != err.Error()) { + t.Errorf("validate() error = %q, shouldErr %q", err.Error(), tc.shouldErr) } }) } } func TestChildSetReconciler_validate(t *testing.T) { + scheme := runtime.NewScheme() + tests := []struct { name string parent *corev1.ConfigMap @@ -667,12 +735,76 @@ func TestChildSetReconciler_validate(t *testing.T) { }, } - for _, c := range tests { - t.Run(c.name, func(t *testing.T) { - ctx := StashResourceType(context.TODO(), c.parent) - err := c.reconciler.validate(ctx) - if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) { - t.Errorf("validate() error = %q, shouldErr %q", err.Error(), c.shouldErr) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := Config{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + } + ctx := context.TODO() + ctx = StashResourceType(ctx, tc.parent) + ctx = StashConfig(ctx, c) + err := tc.reconciler.validate(ctx) + if (err != nil) != (tc.shouldErr != "") || (tc.shouldErr != "" && tc.shouldErr != err.Error()) { + t.Errorf("validate() error = %q, shouldErr %q", err.Error(), tc.shouldErr) + } + }) + } +} + +func TestChildSetReconciler_validate_DuckTypedChild(t *testing.T) { + scheme := runtime.NewScheme() + + tests := []struct { + name string + parent *corev1.ConfigMap + reconciler *ChildSetReconciler[*corev1.ConfigMap, *resources.TestDuck, *resources.TestDuckList] + shouldErr string + }{ + { + name: "DangerouslyAllowDuckTypedChildren disabled", + parent: &corev1.ConfigMap{}, + reconciler: &ChildSetReconciler[*corev1.ConfigMap, *resources.TestDuck, *resources.TestDuckList]{ + Name: "DangerouslyAllowDuckTypedChildren disabled", + ChildType: &resources.TestDuck{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResource"}}, + ChildListType: &resources.TestDuckList{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResourceList"}}, + DesiredChildren: func(ctx context.Context, resource *corev1.ConfigMap) ([]*resources.TestDuck, error) { return nil, nil }, + ReflectChildrenStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, result ChildSetResult[*resources.TestDuck]) {}, + MergeBeforeUpdate: func(current, desired *resources.TestDuck) {}, + OurChild: func(parent *corev1.ConfigMap, child *resources.TestDuck) bool { return false }, + IdentifyChild: func(child *resources.TestDuck) string { return "" }, + + DangerouslyAllowDuckTypedChildren: false, + }, + shouldErr: `ChildSetReconciler "DangerouslyAllowDuckTypedChildren disabled" must enable DangerouslyAllowDuckTypedChildren to use a child duck type`, + }, + { + name: "DangerouslyAllowDuckTypedChildren enabled", + parent: &corev1.ConfigMap{}, + reconciler: &ChildSetReconciler[*corev1.ConfigMap, *resources.TestDuck, *resources.TestDuckList]{ + ChildType: &resources.TestDuck{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResource"}}, + ChildListType: &resources.TestDuckList{TypeMeta: metav1.TypeMeta{APIVersion: resources.GroupVersion.String(), Kind: "TestResourceList"}}, + DesiredChildren: func(ctx context.Context, resource *corev1.ConfigMap) ([]*resources.TestDuck, error) { return nil, nil }, + ReflectChildrenStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, result ChildSetResult[*resources.TestDuck]) {}, + MergeBeforeUpdate: func(current, desired *resources.TestDuck) {}, + OurChild: func(parent *corev1.ConfigMap, child *resources.TestDuck) bool { return false }, + IdentifyChild: func(child *resources.TestDuck) string { return "" }, + + DangerouslyAllowDuckTypedChildren: true, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := Config{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + } + ctx := context.TODO() + ctx = StashResourceType(ctx, tc.parent) + ctx = StashConfig(ctx, c) + err := tc.reconciler.validate(ctx) + if (err != nil) != (tc.shouldErr != "") || (tc.shouldErr != "" && tc.shouldErr != err.Error()) { + t.Errorf("validate() error = %q, shouldErr %q", err.Error(), tc.shouldErr) } }) }