Merge pull request #78713 from liggitt/crd-status-conversion-error

Set expected in-memory version when decoding unstructured objects from etcd
This commit is contained in:
Kubernetes Prow Robot 2019-06-06 10:48:13 -07:00 committed by GitHub
commit f3f2b4066b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 299 additions and 41 deletions

View File

@ -235,6 +235,8 @@ func testPrinter(t *testing.T, printer printers.ResourcePrinter, unmarshalFunc f
TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Pod"},
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
}
// our decoder defaults, so we should default our expected object as well
legacyscheme.Scheme.Default(obj)
buf.Reset()
printer.PrintObj(obj, buf)
var objOut v1.Pod

View File

@ -177,12 +177,19 @@ type StatusREST struct {
var _ = rest.Patcher(&StatusREST{})
func (r *StatusREST) New() runtime.Object {
return &unstructured.Unstructured{}
return r.store.New()
}
// Get retrieves the object from the storage. It is required to support Patch.
func (r *StatusREST) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
return r.store.Get(ctx, name, options)
o, err := r.store.Get(ctx, name, options)
if err != nil {
return nil, err
}
if u, ok := o.(*unstructured.Unstructured); ok {
shallowCopyObjectMeta(u)
}
return o, nil
}
// Update alters the status subset of an object.

View File

@ -58,15 +58,18 @@ func checks(checkers ...Checker) []Checker {
return checkers
}
func TestWebhookConverter(t *testing.T) {
testWebhookConverter(t, false)
func TestWebhookConverterWithWatchCache(t *testing.T) {
testWebhookConverter(t, false, true)
}
func TestWebhookConverterWithoutWatchCache(t *testing.T) {
testWebhookConverter(t, false, false)
}
func TestWebhookConverterWithDefaulting(t *testing.T) {
testWebhookConverter(t, true)
testWebhookConverter(t, true, true)
}
func testWebhookConverter(t *testing.T, defaulting bool) {
func testWebhookConverter(t *testing.T, defaulting, watchCache bool) {
tests := []struct {
group string
handler http.Handler
@ -115,7 +118,7 @@ func testWebhookConverter(t *testing.T, defaulting bool) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CustomResourceDefaulting, true)()
}
tearDown, config, options, err := fixtures.StartDefaultServer(t)
tearDown, config, options, err := fixtures.StartDefaultServer(t, fmt.Sprintf("--watch-cache=%v", watchCache))
if err != nil {
t.Fatal(err)
}
@ -320,6 +323,28 @@ func validateNonTrivialConverted(t *testing.T, ctc *conversionTestContext) {
}
verifyMultiVersionObject(t, getVersion.Name, obj)
}
// send a non-trivial patch to the main resource to verify the oldObject is in the right version
if _, err := client.Patch(name, types.MergePatchType, []byte(`{"metadata":{"annotations":{"main":"true"}}}`), metav1.PatchOptions{}); err != nil {
t.Fatal(err)
}
// verify that the right, pruned version is in storage
obj, err = ctc.etcdObjectReader.GetStoredCustomResource(ns, name)
if err != nil {
t.Fatal(err)
}
verifyMultiVersionObject(t, "v1beta1", obj)
// send a non-trivial patch to the status subresource to verify the oldObject is in the right version
if _, err := client.Patch(name, types.MergePatchType, []byte(`{"metadata":{"annotations":{"status":"true"}}}`), metav1.PatchOptions{}, "status"); err != nil {
t.Fatal(err)
}
// verify that the right, pruned version is in storage
obj, err = ctc.etcdObjectReader.GetStoredCustomResource(ns, name)
if err != nil {
t.Fatal(err)
}
verifyMultiVersionObject(t, "v1beta1", obj)
})
}
}

View File

@ -17,6 +17,7 @@ limitations under the License.
package integration
import (
"fmt"
"strings"
"testing"
"time"
@ -29,6 +30,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/json"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/apimachinery/pkg/watch"
utilfeature "k8s.io/apiserver/pkg/util/feature"
utilfeaturetesting "k8s.io/component-base/featuregate/testing"
"k8s.io/utils/pointer"
@ -43,6 +45,18 @@ var defaultingFixture = &apiextensionsv1beta1.CustomResourceDefinition{
Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{
Group: "tests.apiextensions.k8s.io",
Version: "v1beta1",
Versions: []apiextensionsv1beta1.CustomResourceDefinitionVersion{
{
Name: "v1beta1",
Storage: false,
Served: true,
},
{
Name: "v1beta2",
Storage: true,
Served: false,
},
},
Names: apiextensionsv1beta1.CustomResourceDefinitionNames{
Plural: "foos",
Singular: "foo",
@ -57,7 +71,7 @@ var defaultingFixture = &apiextensionsv1beta1.CustomResourceDefinition{
},
}
const defaultingFooSchema = `
const defaultingFooV1beta1Schema = `
type: object
properties:
spec:
@ -69,6 +83,13 @@ properties:
b:
type: string
default: "B"
c:
type: string
v1beta1:
type: string
default: "v1beta1"
v1beta2:
type: string
status:
type: object
properties:
@ -78,20 +99,76 @@ properties:
b:
type: string
default: "B"
c:
type: string
v1beta1:
type: string
default: "v1beta1"
v1beta2:
type: string
`
func TestCustomResourceDefaulting(t *testing.T) {
const defaultingFooV1beta2Schema = `
type: object
properties:
spec:
type: object
properties:
a:
type: string
default: "A"
b:
type: string
default: "B"
c:
type: string
v1beta1:
type: string
v1beta2:
type: string
default: "v1beta2"
status:
type: object
properties:
a:
type: string
default: "A"
b:
type: string
default: "B"
c:
type: string
v1beta1:
type: string
v1beta2:
type: string
default: "v1beta2"
`
func TestCustomResourceDefaultingWithWatchCache(t *testing.T) {
testDefaulting(t, true)
}
func TestCustomResourceDefaultingWithoutWatchCache(t *testing.T) {
testDefaulting(t, false)
}
func testDefaulting(t *testing.T, watchCache bool) {
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CustomResourceDefaulting, true)()
tearDownFn, apiExtensionClient, dynamicClient, err := fixtures.StartDefaultServerWithClients(t)
tearDownFn, apiExtensionClient, dynamicClient, err := fixtures.StartDefaultServerWithClients(t, fmt.Sprintf("--watch-cache=%v", watchCache))
if err != nil {
t.Fatal(err)
}
defer tearDownFn()
crd := defaultingFixture.DeepCopy()
crd.Spec.Validation = &apiextensionsv1beta1.CustomResourceValidation{}
if err := yaml.Unmarshal([]byte(defaultingFooSchema), &crd.Spec.Validation.OpenAPIV3Schema); err != nil {
crd.Spec.Versions[0].Schema = &apiextensionsv1beta1.CustomResourceValidation{}
if err := yaml.Unmarshal([]byte(defaultingFooV1beta1Schema), &crd.Spec.Versions[0].Schema.OpenAPIV3Schema); err != nil {
t.Fatal(err)
}
crd.Spec.Versions[1].Schema = &apiextensionsv1beta1.CustomResourceValidation{}
if err := yaml.Unmarshal([]byte(defaultingFooV1beta2Schema), &crd.Spec.Versions[1].Schema.OpenAPIV3Schema); err != nil {
t.Fatal(err)
}
@ -101,13 +178,15 @@ func TestCustomResourceDefaulting(t *testing.T) {
}
mustExist := func(obj map[string]interface{}, pths [][]string) {
t.Helper()
for _, pth := range pths {
if _, found, _ := unstructured.NestedFieldNoCopy(obj, pth...); !found {
t.Errorf("Expected '%s' field exist", strings.Join(pth, "."))
t.Errorf("Expected '%s' field was missing", strings.Join(pth, "."))
}
}
}
mustNotExist := func(obj map[string]interface{}, pths [][]string) {
t.Helper()
for _, pth := range pths {
if fld, found, _ := unstructured.NestedFieldNoCopy(obj, pth...); found {
t.Errorf("Expected '%s' field to not exist, but it does: %v", strings.Join(pth, "."), fld)
@ -115,6 +194,7 @@ func TestCustomResourceDefaulting(t *testing.T) {
}
}
updateCRD := func(update func(*apiextensionsv1beta1.CustomResourceDefinition)) {
t.Helper()
var err error
for retry := 0; retry < 10; retry++ {
var obj *apiextensionsv1beta1.CustomResourceDefinition
@ -136,22 +216,34 @@ func TestCustomResourceDefaulting(t *testing.T) {
t.Fatal(err)
}
}
addDefault := func(key string, value interface{}) {
addDefault := func(version string, key string, value interface{}) {
t.Helper()
updateCRD(func(obj *apiextensionsv1beta1.CustomResourceDefinition) {
for _, root := range []string{"spec", "status"} {
obj.Spec.Validation.OpenAPIV3Schema.Properties[root].Properties[key] = apiextensionsv1beta1.JSONSchemaProps{
Type: "string",
Default: jsonPtr(value),
for i := range obj.Spec.Versions {
if obj.Spec.Versions[i].Name != version {
continue
}
obj.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties[root].Properties[key] = apiextensionsv1beta1.JSONSchemaProps{
Type: "string",
Default: jsonPtr(value),
}
}
}
})
}
removeDefault := func(key string) {
removeDefault := func(version string, key string) {
t.Helper()
updateCRD(func(obj *apiextensionsv1beta1.CustomResourceDefinition) {
for _, root := range []string{"spec", "status"} {
props := obj.Spec.Validation.OpenAPIV3Schema.Properties[root].Properties[key]
props.Default = nil
obj.Spec.Validation.OpenAPIV3Schema.Properties[root].Properties[key] = props
for i := range obj.Spec.Versions {
if obj.Spec.Versions[i].Name != version {
continue
}
props := obj.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties[root].Properties[key]
props.Default = nil
obj.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties[root].Properties[key] = props
}
}
})
}
@ -168,8 +260,12 @@ func TestCustomResourceDefaulting(t *testing.T) {
if err != nil {
t.Fatalf("Unable to create CR: %v", err)
}
initialResourceVersion := foo.GetResourceVersion()
t.Logf("CR created: %#v", foo.UnstructuredContent())
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}})
// spec.a and spec.b are defaulted in both versions
// spec.v1beta1 is defaulted when reading the incoming request
// spec.v1beta2 is defaulted when reading the storage response
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "v1beta1"}, {"spec", "v1beta2"}})
mustNotExist(foo.Object, [][]string{{"status"}})
t.Logf("Updating status and expecting 'a' and 'b' to show up.")
@ -179,8 +275,90 @@ func TestCustomResourceDefaulting(t *testing.T) {
}
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}})
t.Logf("Add 'c' default and wait until GET sees it in both status and spec")
addDefault("c", "C")
t.Logf("Add 'c' default to the storage version and wait until GET sees it in both status and spec")
addDefault("v1beta2", "c", "C")
t.Logf("wait until GET sees 'c' in both status and spec")
if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{})
if err != nil {
return false, err
}
if _, found, _ := unstructured.NestedString(obj.Object, "spec", "c"); !found {
t.Log("will retry, did not find spec.c in the object")
return false, nil
}
foo = obj
return true, nil
}); err != nil {
t.Fatal(err)
}
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
t.Logf("wait until GET sees 'c' in both status and spec of cached get")
if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{ResourceVersion: "0"})
if err != nil {
return false, err
}
if _, found, _ := unstructured.NestedString(obj.Object, "spec", "c"); !found {
t.Logf("will retry, did not find spec.c in the cached object")
return false, nil
}
foo = obj
return true, nil
}); err != nil {
t.Fatal(err)
}
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
t.Logf("verify LIST sees 'c' in both status and spec")
foos, err := fooClient.List(metav1.ListOptions{})
if err != nil {
t.Fatal(err)
}
for _, foo := range foos.Items {
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
}
t.Logf("verify LIST from cache sees 'c' in both status and spec")
foos, err = fooClient.List(metav1.ListOptions{ResourceVersion: "0"})
if err != nil {
t.Fatal(err)
}
for _, foo := range foos.Items {
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
}
// Omit this test when using the watch cache because changing the CRD resets the watch cache's minimum available resource version.
// The watch cache is populated by list and watch, which are both tested by this test.
// The contents of the watch cache are seen by list with rv=0, which is tested by this test.
if !watchCache {
t.Logf("verify WATCH sees 'c' in both status and spec")
w, err := fooClient.Watch(metav1.ListOptions{ResourceVersion: initialResourceVersion})
if err != nil {
t.Fatal(err)
}
select {
case event := <-w.ResultChan():
if event.Type != watch.Modified {
t.Fatalf("unexpected watch event: %v, %#v", event.Type, event.Object)
}
if e, a := "v1beta1", event.Object.GetObjectKind().GroupVersionKind().Version; e != a {
t.Errorf("watch event for v1beta1 API returned %v", a)
}
mustExist(
event.Object.(*unstructured.Unstructured).Object,
[][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}},
)
case <-time.After(wait.ForeverTestTimeout):
t.Fatal("timed out without getting watch event")
}
}
t.Logf("Add 'c' default to the REST version, remove it from the storage version, and wait until GET no longer sees it in both status and spec")
addDefault("v1beta1", "c", "C")
removeDefault("v1beta2", "c")
if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{})
if err != nil {
@ -188,22 +366,24 @@ func TestCustomResourceDefaulting(t *testing.T) {
}
_, found, _ := unstructured.NestedString(obj.Object, "spec", "c")
foo = obj
return found, nil
return !found, nil
}); err != nil {
t.Fatal(err)
}
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}})
mustNotExist(foo.Object, [][]string{{"spec", "c"}, {"status", "c"}})
t.Logf("Updating status, expecting 'c' to be set in spec and status")
t.Logf("Updating status, expecting 'c' to be set in status only")
if foo, err = fooClient.UpdateStatus(foo, metav1.UpdateOptions{}); err != nil {
t.Fatal(err)
}
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}, {"status", "c"}})
mustNotExist(foo.Object, [][]string{{"spec", "c"}})
t.Logf("Removing 'a', 'b' and `c` properties. Expecting that 'c' goes away in spec, but not in status. 'a' and 'b' were peristed.")
removeDefault("a")
removeDefault("b")
removeDefault("c")
t.Logf("Removing 'a', 'b' and `c` properties from the REST version. Expecting that 'c' goes away in spec, but not in status. 'a' and 'b' were presisted.")
removeDefault("v1beta1", "a")
removeDefault("v1beta1", "b")
removeDefault("v1beta1", "c")
if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{})
if err != nil {

View File

@ -127,6 +127,16 @@ func (u *Unstructured) UnmarshalJSON(b []byte) error {
return err
}
// NewEmptyInstance returns a new instance of the concrete type containing only kind/apiVersion and no other data.
// This should be called instead of reflect.New() for unstructured types because the go type alone does not preserve kind/apiVersion info.
func (in *Unstructured) NewEmptyInstance() runtime.Unstructured {
out := new(Unstructured)
if in != nil {
out.GetObjectKind().SetGroupVersionKind(in.GetObjectKind().GroupVersionKind())
}
return out
}
func (in *Unstructured) DeepCopy() *Unstructured {
if in == nil {
return nil

View File

@ -52,6 +52,16 @@ func (u *UnstructuredList) EachListItem(fn func(runtime.Object) error) error {
return nil
}
// NewEmptyInstance returns a new instance of the concrete type containing only kind/apiVersion and no other data.
// This should be called instead of reflect.New() for unstructured types because the go type alone does not preserve kind/apiVersion info.
func (u *UnstructuredList) NewEmptyInstance() runtime.Unstructured {
out := new(UnstructuredList)
if u != nil {
out.SetGroupVersionKind(u.GroupVersionKind())
}
return out
}
// UnstructuredContent returns a map contain an overlay of the Items field onto
// the Object field. Items always overwrites overlay.
func (u *UnstructuredList) UnstructuredContent() map[string]interface{} {

View File

@ -260,6 +260,9 @@ type Object interface {
// to JSON allowed.
type Unstructured interface {
Object
// NewEmptyInstance returns a new instance of the concrete type containing only kind/apiVersion and no other data.
// This should be called instead of reflect.New() for unstructured types because the go type alone does not preserve kind/apiVersion info.
NewEmptyInstance() Unstructured
// UnstructuredContent returns a non-nil map with this object's contents. Values may be
// []interface{}, map[string]interface{}, or any primitive type. Contents are typically serialized to
// and from JSON. SetUnstructuredContent should be used to mutate the contents.

View File

@ -113,13 +113,6 @@ func (c *codec) Decode(data []byte, defaultGVK *schema.GroupVersionKind, into ru
// if we specify a target, use generic conversion.
if into != nil {
if into == obj {
if isVersioned {
return versioned, gvk, nil
}
return into, gvk, nil
}
// perform defaulting if requested
if c.defaulter != nil {
// create a copy to ensure defaulting is not applied to the original versioned objects
@ -133,6 +126,14 @@ func (c *codec) Decode(data []byte, defaultGVK *schema.GroupVersionKind, into ru
}
}
// Short-circuit conversion if the into object is same object
if into == obj {
if isVersioned {
return versioned, gvk, nil
}
return into, gvk, nil
}
if err := c.convertor.Convert(obj, into, c.decodeVersion); err != nil {
return nil, gvk, err
}

View File

@ -261,6 +261,14 @@ func (obj *Unstructured) EachListItem(fn func(runtime.Object) error) error {
return nil
}
func (obj *Unstructured) NewEmptyInstance() runtime.Unstructured {
out := new(Unstructured)
if obj != nil {
out.SetGroupVersionKind(obj.GroupVersionKind())
}
return out
}
func (obj *Unstructured) UnstructuredContent() map[string]interface{} {
if obj.Object == nil {
return make(map[string]interface{})

View File

@ -20,6 +20,7 @@ go_test(
"//staging/src/k8s.io/apimachinery/pkg/api/apitesting:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/conversion:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",

View File

@ -685,9 +685,15 @@ func (s *store) watch(ctx context.Context, key string, rv string, pred storage.S
func (s *store) getState(getResp *clientv3.GetResponse, key string, v reflect.Value, ignoreNotFound bool) (*objState, error) {
state := &objState{
obj: reflect.New(v.Type()).Interface().(runtime.Object),
meta: &storage.ResponseMeta{},
}
if u, ok := v.Addr().Interface().(runtime.Unstructured); ok {
state.obj = u.NewEmptyInstance()
} else {
state.obj = reflect.New(v.Type()).Interface().(runtime.Object)
}
if len(getResp.Kvs) == 0 {
if !ignoreNotFound {
return nil, storage.NewKeyNotFoundError(key, 0)

View File

@ -35,6 +35,7 @@ import (
apitesting "k8s.io/apimachinery/pkg/api/apitesting"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/conversion"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
@ -1349,7 +1350,11 @@ func testSetup(t *testing.T) (context.Context, *store, *integration.ClusterV3) {
func testPropogateStore(ctx context.Context, t *testing.T, store *store, obj *example.Pod) (string, *example.Pod) {
// Setup store with a key and grab the output for returning.
key := "/testkey"
err := store.conditionalDelete(ctx, key, &example.Pod{}, reflect.ValueOf(example.Pod{}), nil, storage.ValidateAllObjectFunc)
v, err := conversion.EnforcePtr(obj)
if err != nil {
panic("unable to convert output object to pointer")
}
err = store.conditionalDelete(ctx, key, &example.Pod{}, v, nil, storage.ValidateAllObjectFunc)
if err != nil && !storage.IsNotFound(err) {
t.Fatalf("Cleanup failed: %v", err)
}