Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop unrecognized fields before update #112

Merged
merged 1 commit into from
Jul 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions pkg/stores/proxy/proxy_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,20 @@ type Store struct {
// NewProxyStore returns a wrapped types.Store.
func NewProxyStore(clientGetter ClientGetter, notifier RelationshipNotifier, lookup accesscontrol.AccessSetLookup, namespaceCache corecontrollers.NamespaceCache) types.Store {
return &errorStore{
Store: &WatchRefresh{
Store: partition.NewStore(
&rbacPartitioner{
proxyStore: &Store{
clientGetter: clientGetter,
notifier: notifier,
Store: &unformatterStore{
Store: &WatchRefresh{
Store: partition.NewStore(
&rbacPartitioner{
proxyStore: &Store{
clientGetter: clientGetter,
notifier: notifier,
},
},
},
lookup,
namespaceCache,
),
asl: lookup,
lookup,
namespaceCache,
),
asl: lookup,
},
},
}
}
Expand Down
66 changes: 66 additions & 0 deletions pkg/stores/proxy/unformatter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package proxy

import (
"github.com/rancher/apiserver/pkg/types"
"github.com/rancher/wrangler/pkg/data"
"github.com/rancher/wrangler/pkg/data/convert"
)

// unformatterStore removes fields added by the formatter that kubernetes cannot recognize.
type unformatterStore struct {
types.Store
}

// ByID looks up a single object by its ID.
func (u *unformatterStore) ByID(apiOp *types.APIRequest, schema *types.APISchema, id string) (types.APIObject, error) {
return u.Store.ByID(apiOp, schema, id)
}

// List returns a list of resources.
func (u *unformatterStore) List(apiOp *types.APIRequest, schema *types.APISchema) (types.APIObjectList, error) {
return u.Store.List(apiOp, schema)
}

// Create creates a single object in the store.
func (u *unformatterStore) Create(apiOp *types.APIRequest, schema *types.APISchema, data types.APIObject) (types.APIObject, error) {
return u.Store.Create(apiOp, schema, data)
}

// Update updates a single object in the store.
func (u *unformatterStore) Update(apiOp *types.APIRequest, schema *types.APISchema, data types.APIObject, id string) (types.APIObject, error) {
data = unformat(data)
return u.Store.Update(apiOp, schema, data, id)
}

// Delete deletes an object from a store.
func (u *unformatterStore) Delete(apiOp *types.APIRequest, schema *types.APISchema, id string) (types.APIObject, error) {
return u.Store.Delete(apiOp, schema, id)

}

// Watch returns a channel of events for a list or resource.
func (u *unformatterStore) Watch(apiOp *types.APIRequest, schema *types.APISchema, wr types.WatchRequest) (chan types.APIEvent, error) {
return u.Store.Watch(apiOp, schema, wr)
}

func unformat(obj types.APIObject) types.APIObject {
unst, ok := obj.Object.(map[string]interface{})
if !ok {
return obj
}
data.RemoveValue(unst, "metadata", "fields")
data.RemoveValue(unst, "metadata", "relationships")
data.RemoveValue(unst, "metadata", "state")
conditions, ok := data.GetValue(unst, "status", "conditions")
if ok {
conditionsSlice := convert.ToMapSlice(conditions)
for i := range conditionsSlice {
data.RemoveValue(conditionsSlice[i], "error")
data.RemoveValue(conditionsSlice[i], "transitioning")
data.RemoveValue(conditionsSlice[i], "lastUpdateTime")
}
data.PutValue(unst, conditionsSlice, "status", "conditions")
}
obj.Object = unst
return obj
}
112 changes: 112 additions & 0 deletions pkg/stores/proxy/unformatter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package proxy

import (
"testing"

"github.com/rancher/apiserver/pkg/types"
"github.com/stretchr/testify/assert"
)

func Test_unformat(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better if this tested the exported method Update on the type instead?
unformat is just an implementation detail and might change, while we want to be certain that the public interface works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I don't find a lot of value in it. I would have to implement a mock store to hold the underlying types.Store, so it would be adding a lot of boilerplate to test the entire .Update() flow, when all that really matters is this one function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I just thought there might already be a simple mock store for testing...

tests := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always good to start a test with t.Parallel() and also run subtests in parallel, unless there is global state.

for _, test := range tests {
	test := test
	t.Run(test.name, func(t *testing.T) {
		t.Parallel()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the test itself can also be run in parallel:

Suggested change
tests := []struct {
func TestUnformat(t *testing.T) {
t.Parallel()

name string
obj types.APIObject
want types.APIObject
}{
{
name: "noop",
obj: types.APIObject{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"name": "noop",
},
},
},
want: types.APIObject{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"name": "noop",
},
},
},
},
{
name: "remove fields",
obj: types.APIObject{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"name": "foo",
"fields": []string{
"name",
"address",
"phonenumber",
},
"relationships": []map[string]interface{}{
{
"toId": "bar",
"rel": "uses",
},
},
"state": map[string]interface{}{
"error": false,
},
},
"status": map[string]interface{}{
"conditions": []map[string]interface{}{
{
"type": "Ready",
"status": "True",
"lastUpdateTime": "a minute ago",
"transitioning": false,
"error": false,
},
{
"type": "Initialized",
"status": "True",
"lastUpdateTime": "yesterday",
"transitioning": false,
"error": false,
},
},
},
},
},
want: types.APIObject{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"name": "foo",
},
"status": map[string]interface{}{
"conditions": []map[string]interface{}{
{
"type": "Ready",
"status": "True",
},
{
"type": "Initialized",
"status": "True",
},
},
},
},
},
},
{
name: "unrecognized object",
obj: types.APIObject{
Object: "object",
},
want: types.APIObject{
Object: "object",
},
},
}
for _, test := range tests {
test := test
t.Run(test.name, func(t *testing.T) {
t.Parallel()
got := unformat(test.obj)
assert.Equal(t, test.want, got)
})
}
}