Storage: Add mode-specific List implementations (#85814)

* Add List methods to mode 1 and 4
* Refactor errors

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 (

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")
+		klog.FromContext(ctx).Error(errDualWriterCreaterMissing, "legacy storage rest.Creater is missing")
-		return nil, errNoCreaterMethod
+		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)
+}

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"))
+}

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{})
-}

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)
+	}
+}

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)

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"))
+}

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 all DualWriterMode4 methods once we remove the generic DualWriter implementation
-// #TODO remove this once we remove the default 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{})
 }

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"))
+}

pkg/apiserver/rest/dualwriter_test.go

@@ -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"))
-}

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
+}