diff --git a/pkg/apiserver/rest/dualwriter.go b/pkg/apiserver/rest/dualwriter.go index 61e5e6d3348..269fc60847b 100644 --- a/pkg/apiserver/rest/dualwriter.go +++ b/pkg/apiserver/rest/dualwriter.go @@ -2,6 +2,7 @@ package rest import ( "context" + "errors" "k8s.io/apimachinery/pkg/api/meta" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" @@ -65,6 +66,9 @@ type DualWriter struct { Legacy LegacyStorage } +var errDualWriterCreaterMissing = errors.New("legacy storage rest.Creater is missing") +var errDualWriterListerMissing = errors.New("legacy storage rest.Lister is missing") + type DualWriterMode int const ( diff --git a/pkg/apiserver/rest/dualwriter_mode1.go b/pkg/apiserver/rest/dualwriter_mode1.go index 58dd0e0aede..0caf3a7aed2 100644 --- a/pkg/apiserver/rest/dualwriter_mode1.go +++ b/pkg/apiserver/rest/dualwriter_mode1.go @@ -2,8 +2,8 @@ package rest import ( "context" - "errors" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/registry/rest" @@ -14,20 +14,18 @@ type DualWriterMode1 struct { DualWriter } -var errNoCreaterMethod = errors.New("legacy storage rest.Creater is missing") - // NewDualWriterMode1 returns a new DualWriter in mode 1. // Mode 1 represents writing to and reading from LegacyStorage. func NewDualWriterMode1(legacy LegacyStorage, storage Storage) *DualWriterMode1 { return &DualWriterMode1{*NewDualWriter(legacy, storage)} } -// Create overrides the default behavior of the DualWriter and writes only to LegacyStorage. +// Create overrides the behavior of the generic DualWriter and writes only to LegacyStorage. func (d *DualWriterMode1) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { legacy, ok := d.Legacy.(rest.Creater) if !ok { - klog.FromContext(ctx).Error(errNoCreaterMethod, "legacy storage rest.Creater is missing") - return nil, errNoCreaterMethod + klog.FromContext(ctx).Error(errDualWriterCreaterMissing, "legacy storage rest.Creater is missing") + return nil, errDualWriterCreaterMissing } return legacy.Create(ctx, obj, createValidation, options) @@ -37,3 +35,13 @@ func (d *DualWriterMode1) Create(ctx context.Context, obj runtime.Object, create func (d *DualWriterMode1) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { return d.Legacy.Get(ctx, name, options) } + +// List overrides the behavior of the generic DualWriter and reads only from LegacyStorage. +func (d *DualWriterMode1) List(ctx context.Context, options *metainternalversion.ListOptions) (runtime.Object, error) { + legacy, ok := d.Legacy.(rest.Lister) + if !ok { + return nil, errDualWriterListerMissing + } + + return legacy.List(ctx, options) +} diff --git a/pkg/apiserver/rest/dualwriter_mode1_test.go b/pkg/apiserver/rest/dualwriter_mode1_test.go new file mode 100644 index 00000000000..2f0e58510bc --- /dev/null +++ b/pkg/apiserver/rest/dualwriter_mode1_test.go @@ -0,0 +1,33 @@ +package rest + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +const kind = "dummy" + +func TestMode1(t *testing.T) { + var ls = (LegacyStorage)(nil) + var s = (Storage)(nil) + lsSpy := NewLegacyStorageSpyClient(ls) + sSpy := NewStorageSpyClient(s) + + dw := NewDualWriterMode1(lsSpy, sSpy) + + // Get: it should use the Legacy Get implementation + _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) + assert.NoError(t, err) + assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Get")) + assert.Equal(t, 0, sSpy.Counts("Storage.Get")) + + // List: it should use the Legacy List implementation + _, err = dw.List(context.Background(), &metainternalversion.ListOptions{}) + assert.NoError(t, err) + assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.List")) + assert.Equal(t, 0, sSpy.Counts("Storage.List")) +} diff --git a/pkg/apiserver/rest/dualwriter_mode2.go b/pkg/apiserver/rest/dualwriter_mode2.go index ecbce6b7589..cb5c5a5879d 100644 --- a/pkg/apiserver/rest/dualwriter_mode2.go +++ b/pkg/apiserver/rest/dualwriter_mode2.go @@ -2,9 +2,9 @@ package rest import ( "context" - "fmt" "k8s.io/apimachinery/pkg/api/meta" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/registry/rest" @@ -21,11 +21,11 @@ func NewDualWriterMode2(legacy LegacyStorage, storage Storage) *DualWriterMode2 return &DualWriterMode2{*NewDualWriter(legacy, storage)} } -// Create overrides the default behavior of the DualWriter and writes to LegacyStorage and Storage. +// Create overrides the behavior of the generic DualWriter and writes to LegacyStorage and Storage. func (d *DualWriterMode2) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { legacy, ok := d.Legacy.(rest.Creater) if !ok { - return nil, fmt.Errorf("legacy storage rest.Creater is missing") + return nil, errDualWriterCreaterMissing } created, err := legacy.Create(ctx, obj, createValidation, options) @@ -55,6 +55,63 @@ func (d *DualWriterMode2) Create(ctx context.Context, obj runtime.Object, create return rsp, err } +// Get overrides the behavior of the generic DualWriter and retrieves an object from LegacyStorage. +func (d *DualWriterMode2) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { + return d.Legacy.Get(ctx, name, &metav1.GetOptions{}) +} + +// List overrides the behavior of the generic DualWriter. +// It returns Storage entries if possible and falls back to LegacyStorage entries if not. +func (d *DualWriterMode2) List(ctx context.Context, options *metainternalversion.ListOptions) (runtime.Object, error) { + legacy, ok := d.Legacy.(rest.Lister) + if !ok { + return nil, errDualWriterListerMissing + } + + ll, err := legacy.List(ctx, options) + if err != nil { + return nil, err + } + legacyList, err := meta.ExtractList(ll) + if err != nil { + return nil, err + } + + sl, err := d.Storage.List(ctx, options) + if err != nil { + return nil, err + } + storageList, err := meta.ExtractList(sl) + if err != nil { + return nil, err + } + + m := map[string]int{} + for i, obj := range storageList { + accessor, err := meta.Accessor(obj) + if err != nil { + return nil, err + } + m[accessor.GetName()] = i + } + + for i, obj := range legacyList { + accessor, err := meta.Accessor(obj) + if err != nil { + return nil, err + } + // Replace the LegacyStorage object if there's a corresponding entry in Storage. + if index, ok := m[accessor.GetName()]; ok { + legacyList[i] = storageList[index] + } + } + + if err = meta.SetList(ll, legacyList); err != nil { + return nil, err + } + return ll, nil +} + func enrichObject(orig, copy runtime.Object) (runtime.Object, error) { accessorC, err := meta.Accessor(copy) if err != nil { @@ -80,8 +137,3 @@ func enrichObject(orig, copy runtime.Object) (runtime.Object, error) { return copy, nil } - -// Get overrides the default behavior of the Storage and retrieves an object from LegacyStorage -func (d *DualWriterMode2) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { - return d.Legacy.Get(ctx, name, &metav1.GetOptions{}) -} diff --git a/pkg/apiserver/rest/dualwriter_mode2_test.go b/pkg/apiserver/rest/dualwriter_mode2_test.go new file mode 100644 index 00000000000..12fa392deb5 --- /dev/null +++ b/pkg/apiserver/rest/dualwriter_mode2_test.go @@ -0,0 +1,56 @@ +package rest + +import ( + "context" + "testing" + + "github.com/zeebo/assert" + "k8s.io/apimachinery/pkg/api/meta" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestMode2(t *testing.T) { + var ls = (LegacyStorage)(nil) + var s = (Storage)(nil) + lsSpy := NewLegacyStorageSpyClient(ls) + sSpy := NewStorageSpyClient(s) + + dw := NewDualWriterMode2(lsSpy, sSpy) + + // Get: it should use the Legacy Get implementation + _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) + assert.NoError(t, err) + assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Get")) + assert.Equal(t, 0, sSpy.Counts("Storage.Get")) + + // List: it should use call both Legacy and Storage List methods + res, err := dw.List(context.Background(), &metainternalversion.ListOptions{}) + assert.NoError(t, err) + assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.List")) + assert.Equal(t, 1, sSpy.Counts("Storage.List")) + + resList, err := meta.ExtractList(res) + assert.NoError(t, err) + + expectedItems := map[string]string{ + // Item 1: Storage should override Legacy + "Item 1": "Storage field 1", + // Item 2 shouldn't be included because it's not in Storage + // Item 3 should because it's in Legacy + "Item 3": "Legacy field 3", + } + + assert.Equal(t, len(expectedItems), len(resList)) + + for _, obj := range resList { + v, ok := obj.(*dummyObject) + assert.True(t, ok) + accessor, err := meta.Accessor(v) + assert.NoError(t, err) + + k, ok := expectedItems[accessor.GetName()] + assert.True(t, ok) + assert.Equal(t, k, v.Foo) + } +} diff --git a/pkg/apiserver/rest/dualwriter_mode3.go b/pkg/apiserver/rest/dualwriter_mode3.go index 517ea89d09b..911d0f8668f 100644 --- a/pkg/apiserver/rest/dualwriter_mode3.go +++ b/pkg/apiserver/rest/dualwriter_mode3.go @@ -21,11 +21,11 @@ func NewDualWriterMode3(legacy LegacyStorage, storage Storage) *DualWriterMode3 return &DualWriterMode3{*NewDualWriter(legacy, storage)} } -// Create overrides the default behavior of the DualWriter and writes to LegacyStorage and Storage. +// Create overrides the behavior of the generic DualWriter and writes to LegacyStorage and Storage. func (d *DualWriterMode3) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { legacy, ok := d.Legacy.(rest.Creater) if !ok { - return nil, fmt.Errorf("legacy storage rest.Creater is missing") + return nil, errDualWriterCreaterMissing } created, err := d.Storage.Create(ctx, obj, createValidation, options) diff --git a/pkg/apiserver/rest/dualwriter_mode3_test.go b/pkg/apiserver/rest/dualwriter_mode3_test.go new file mode 100644 index 00000000000..bfd79723804 --- /dev/null +++ b/pkg/apiserver/rest/dualwriter_mode3_test.go @@ -0,0 +1,31 @@ +package rest + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestMode3(t *testing.T) { + var ls = (LegacyStorage)(nil) + var s = (Storage)(nil) + lsSpy := NewLegacyStorageSpyClient(ls) + sSpy := NewStorageSpyClient(s) + + dw := NewDualWriterMode3(lsSpy, sSpy) + + // Get: it should use the Storage Get implementation + _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) + assert.NoError(t, err) + assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get")) + assert.Equal(t, 1, sSpy.Counts("Storage.Get")) + + // List: it should use the Storage List implementation + _, err = dw.List(context.Background(), &metainternalversion.ListOptions{}) + assert.NoError(t, err) + assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.List")) + assert.Equal(t, 1, sSpy.Counts("Storage.List")) +} diff --git a/pkg/apiserver/rest/dualwriter_mode4.go b/pkg/apiserver/rest/dualwriter_mode4.go index 6dc4b281192..fe62b988261 100644 --- a/pkg/apiserver/rest/dualwriter_mode4.go +++ b/pkg/apiserver/rest/dualwriter_mode4.go @@ -18,13 +18,14 @@ func NewDualWriterMode4(legacy LegacyStorage, storage Storage) *DualWriterMode4 return &DualWriterMode4{*NewDualWriter(legacy, storage)} } -// Create overrides the default behavior of the DualWriter and writes only to Storage. -// #TODO remove this once we remove the default DualWriter implementation +// #TODO remove all DualWriterMode4 methods once we remove the generic DualWriter implementation + +// Create overrides the behavior of the generic DualWriter and writes only to Storage. func (d *DualWriterMode4) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { return d.Storage.Create(ctx, obj, createValidation, options) } -// Get overrides the default behavior of the Storage and retrieves an object from Unified Storage +// Get overrides the behavior of the generic DualWriter and retrieves an object from Unified Storage. func (d *DualWriterMode4) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { return d.Storage.Get(ctx, name, &metav1.GetOptions{}) } diff --git a/pkg/apiserver/rest/dualwriter_mode4_test.go b/pkg/apiserver/rest/dualwriter_mode4_test.go new file mode 100644 index 00000000000..09c9882b853 --- /dev/null +++ b/pkg/apiserver/rest/dualwriter_mode4_test.go @@ -0,0 +1,31 @@ +package rest + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestMode4(t *testing.T) { + var ls = (LegacyStorage)(nil) + var s = (Storage)(nil) + lsSpy := NewLegacyStorageSpyClient(ls) + sSpy := NewStorageSpyClient(s) + + dw := NewDualWriterMode4(lsSpy, sSpy) + + // Get: it should use the Storage Get implementation + _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) + assert.NoError(t, err) + assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get")) + assert.Equal(t, 1, sSpy.Counts("Storage.Get")) + + // List: it should use the Storage Get implementation + _, err = dw.List(context.Background(), &metainternalversion.ListOptions{}) + assert.NoError(t, err) + assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.List")) + assert.Equal(t, 1, sSpy.Counts("Storage.List")) +} diff --git a/pkg/apiserver/rest/dualwriter_test.go b/pkg/apiserver/rest/dualwriter_test.go deleted file mode 100644 index f6d5a264803..00000000000 --- a/pkg/apiserver/rest/dualwriter_test.go +++ /dev/null @@ -1,75 +0,0 @@ -package rest - -import ( - "context" - "testing" - - "github.com/zeebo/assert" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -const kind = "dummy" - -func Test_Mode1(t *testing.T) { - var ls = (LegacyStorage)(nil) - var s = (Storage)(nil) - lsSpy := NewLegacyStorageSpyClient(ls) - sSpy := NewStorageSpyClient(s) - - dw := NewDualWriterMode1(lsSpy, sSpy) - - _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) - assert.NoError(t, err) - - // it should use the Legacy Get implementation - assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Get")) - assert.Equal(t, 0, sSpy.Counts("Storage.Get")) -} - -func Test_Mode2(t *testing.T) { - var ls = (LegacyStorage)(nil) - var s = (Storage)(nil) - lsSpy := NewLegacyStorageSpyClient(ls) - sSpy := NewStorageSpyClient(s) - - dw := NewDualWriterMode2(lsSpy, sSpy) - - _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) - assert.NoError(t, err) - - // it should use the Legacy Get implementation - assert.Equal(t, 1, lsSpy.Counts("LegacyStorage.Get")) - assert.Equal(t, 0, sSpy.Counts("Storage.Get")) -} - -func Mode3_Test(t *testing.T) { - var ls = (LegacyStorage)(nil) - var s = (Storage)(nil) - lsSpy := NewLegacyStorageSpyClient(ls) - sSpy := NewStorageSpyClient(s) - - dw := NewDualWriterMode3(lsSpy, sSpy) - - _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) - assert.NoError(t, err) - - // it should use the Unified Storage Get implementation - assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get")) - assert.Equal(t, 1, sSpy.Counts("Storage.Get")) -} - -func Test_Mode4(t *testing.T) { - var ls = (LegacyStorage)(nil) - var s = (Storage)(nil) - lsSpy := NewLegacyStorageSpyClient(ls) - sSpy := NewStorageSpyClient(s) - - dw := NewDualWriterMode3(lsSpy, sSpy) - - _, err := dw.Get(context.Background(), kind, &metav1.GetOptions{}) - assert.NoError(t, err) - - // it should use the Unified Storage Get implementation - assert.Equal(t, 0, lsSpy.Counts("LegacyStorage.Get")) - assert.Equal(t, 1, sSpy.Counts("Storage.Get")) -} diff --git a/pkg/apiserver/rest/spy_client.go b/pkg/apiserver/rest/spy_client.go index 3646f1d471f..9c80350c584 100644 --- a/pkg/apiserver/rest/spy_client.go +++ b/pkg/apiserver/rest/spy_client.go @@ -3,9 +3,11 @@ package rest import ( "context" + "k8s.io/apimachinery/pkg/api/meta" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/klog/v2" ) @@ -69,7 +71,22 @@ func (c *spyStorageClient) Get(ctx context.Context, name string, options *metav1 func (c *spyStorageClient) List(ctx context.Context, options *metainternalversion.ListOptions) (runtime.Object, error) { c.spy.record("Storage.List") klog.Info("method: Storage.List") - return nil, nil + + i1 := dummyObject{Foo: "Storage field 1"} + accessor, err := meta.Accessor(&i1) + if err != nil { + return nil, err + } + accessor.SetName("Item 1") + + i2 := dummyObject{Foo: "Storage field 2"} + accessor, err = meta.Accessor(&i2) + if err != nil { + return nil, err + } + accessor.SetName("Item 2") + + return &dummyList{Items: []dummyObject{i1, i2}}, nil } func (c *spyStorageClient) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { @@ -129,10 +146,30 @@ func (c *spyLegacyStorageClient) Get(ctx context.Context, name string, options * return nil, nil } +func (c *spyLegacyStorageClient) NewList() runtime.Object { + // stub for now so that spyLegacyStorageClient implements rest.Lister + return nil +} + func (c *spyLegacyStorageClient) List(ctx context.Context, options *metainternalversion.ListOptions) (runtime.Object, error) { c.spy.record("LegacyStorage.List") klog.Info("method: LegacyStorage.List") - return nil, nil + + i1 := dummyObject{Foo: "Legacy field 1"} + accessor, err := meta.Accessor(&i1) + if err != nil { + return nil, err + } + accessor.SetName("Item 1") + + i3 := dummyObject{Foo: "Legacy field 3"} + accessor, err = meta.Accessor(&i3) + if err != nil { + return nil, err + } + accessor.SetName("Item 3") + + return &dummyList{Items: []dummyObject{i1, i3}}, nil } func (c *spyLegacyStorageClient) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { @@ -146,3 +183,31 @@ func (c *spyLegacyStorageClient) Delete(ctx context.Context, name string, delete klog.Info("method: LegacyStorage.Delete") return nil, false, nil } + +type dummyList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []dummyObject `json:"items,omitempty"` +} + +type dummyObject struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + Foo string +} + +func (d *dummyList) GetObjectKind() schema.ObjectKind { + return nil +} + +func (d *dummyList) DeepCopyObject() runtime.Object { + return nil +} + +func (d *dummyObject) GetObjectKind() schema.ObjectKind { + return nil +} + +func (d *dummyObject) DeepCopyObject() runtime.Object { + return nil +}