From 6b2e4682fe883eebcaf1c1e43cf2957dde441174 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Fri, 1 Feb 2019 11:55:18 -0800 Subject: [PATCH] Minor fixes --- pkg/api/pod/util_test.go | 3 +- pkg/api/testing/defaulting_test.go | 1 + pkg/api/v1/persistentvolume/util_test.go | 2 + pkg/api/v1/pod/util_test.go | 3 +- .../pkg/apis/meta/v1/generated.proto | 10 ++ .../apimachinery/pkg/apis/meta/v1/meta.go | 19 +-- .../apimachinery/pkg/apis/meta/v1/types.go | 10 ++ .../pkg/endpoints/handlers/fieldmanager/BUILD | 1 + .../handlers/fieldmanager/fieldmanager.go | 34 +++-- .../fieldmanager/internal/managedfields.go | 15 +-- .../internal/managedfields_test.go | 8 +- .../fieldmanager/internal/pathelement_test.go | 46 +++---- .../fieldmanager/internal/typeconverter.go | 1 + .../internal/typeconverter_test.go | 77 ++++++++++- .../pkg/endpoints/handlers/rest_test.go | 3 - .../pkg/endpoints/patchhandler_test.go | 124 ------------------ .../apiserver/pkg/features/kube_features.go | 2 +- test/integration/auth/rbac_test.go | 77 +++++------ 18 files changed, 204 insertions(+), 232 deletions(-) diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index c2cdd1b9d89..0f0696c9cda 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -176,7 +176,8 @@ func collectResourcePaths(t *testing.T, resourcename string, path *field.Path, n case reflect.Ptr: resourcePaths.Insert(collectResourcePaths(t, resourcename, path, name, tp.Elem()).List()...) case reflect.Struct: - // Specifically skip ObjectMeta because it has recursive types + // ObjectMeta is generic and therefore should never have a field with a specific resource's name; + // it contains cycles so it's easiest to just skip it. if name == "ObjectMeta" { break } diff --git a/pkg/api/testing/defaulting_test.go b/pkg/api/testing/defaulting_test.go index dac954097dc..50453577272 100644 --- a/pkg/api/testing/defaulting_test.go +++ b/pkg/api/testing/defaulting_test.go @@ -49,6 +49,7 @@ func TestDefaulting(t *testing.T) { {Group: "", Version: "v1", Kind: "ConfigMap"}: {}, {Group: "", Version: "v1", Kind: "ConfigMapList"}: {}, {Group: "", Version: "v1", Kind: "Endpoints"}: {}, + {Group: "", Version: "v1", Kind: "EndpointsList"}: {}, {Group: "", Version: "v1", Kind: "Namespace"}: {}, {Group: "", Version: "v1", Kind: "NamespaceList"}: {}, {Group: "", Version: "v1", Kind: "Node"}: {}, diff --git a/pkg/api/v1/persistentvolume/util_test.go b/pkg/api/v1/persistentvolume/util_test.go index bf653821464..5a60dd1ce52 100644 --- a/pkg/api/v1/persistentvolume/util_test.go +++ b/pkg/api/v1/persistentvolume/util_test.go @@ -247,6 +247,8 @@ func collectSecretPaths(t *testing.T, path *field.Path, name string, tp reflect. case reflect.Ptr: secretPaths.Insert(collectSecretPaths(t, path, name, tp.Elem()).List()...) case reflect.Struct: + // ObjectMeta should not have any field with the word "secret" in it; + // it contains cycles so it's easiest to just skip it. if name == "ObjectMeta" { break } diff --git a/pkg/api/v1/pod/util_test.go b/pkg/api/v1/pod/util_test.go index b9f3544dc66..af05d73639b 100644 --- a/pkg/api/v1/pod/util_test.go +++ b/pkg/api/v1/pod/util_test.go @@ -340,7 +340,8 @@ func collectResourcePaths(t *testing.T, resourcename string, path *field.Path, n case reflect.Ptr: resourcePaths.Insert(collectResourcePaths(t, resourcename, path, name, tp.Elem()).List()...) case reflect.Struct: - // Specifically skip ObjectMeta because it has recursive types + // ObjectMeta is generic and therefore should never have a field with a specific resource's name; + // it contains cycles so it's easiest to just skip it. if name == "ObjectMeta" { break } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/generated.proto b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/generated.proto index ceb50f51223..70fbe897917 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/generated.proto +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/generated.proto @@ -206,6 +206,16 @@ message ExportOptions { // To understand how this is used, see: https://github.com/kubernetes-sigs/structured-merge-diff message Fields { // Map stores a set of fields in a data structure like a Trie. + // + // Each key is either a '.' representing the field itself, and will always map to an empty set, + // or a string representing a sub-field or item. The string will follow one of these four formats: + // 'f:', where is the name of a field in a struct, or key in a map + // 'v:', where is the exact json formatted value of a list item + // 'i:', where is position of a item in a list + // 'k:', where is a map of a list item's key fields to their unique values + // If a key maps to an empty Fields value, the field that key represents is part of the set. + // + // The exact format is defined in k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal map map = 1; } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go index 7eecbd912dc..05f07adf1b8 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/meta.go @@ -64,7 +64,7 @@ type Object interface { GetClusterName() string SetClusterName(clusterName string) GetManagedFields() []ManagedFieldsEntry - SetManagedFields(lastApplied []ManagedFieldsEntry) + SetManagedFields(managedFields []ManagedFieldsEntry) } // ListMetaAccessor retrieves the list interface from an object @@ -168,16 +168,9 @@ func (meta *ObjectMeta) GetOwnerReferences() []OwnerReference { return m func (meta *ObjectMeta) SetOwnerReferences(references []OwnerReference) { meta.OwnerReferences = references } -func (meta *ObjectMeta) GetClusterName() string { return meta.ClusterName } -func (meta *ObjectMeta) SetClusterName(clusterName string) { meta.ClusterName = clusterName } - -func (meta *ObjectMeta) GetManagedFields() []ManagedFieldsEntry { - return meta.ManagedFields -} - -func (meta *ObjectMeta) SetManagedFields(ManagedFields []ManagedFieldsEntry) { - meta.ManagedFields = make([]ManagedFieldsEntry, len(ManagedFields)) - for i, value := range ManagedFields { - meta.ManagedFields[i] = value - } +func (meta *ObjectMeta) GetClusterName() string { return meta.ClusterName } +func (meta *ObjectMeta) SetClusterName(clusterName string) { meta.ClusterName = clusterName } +func (meta *ObjectMeta) GetManagedFields() []ManagedFieldsEntry { return meta.ManagedFields } +func (meta *ObjectMeta) SetManagedFields(managedFields []ManagedFieldsEntry) { + meta.ManagedFields = managedFields } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go index 30c07984e0a..a85355a8333 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -1079,5 +1079,15 @@ const ( // To understand how this is used, see: https://github.com/kubernetes-sigs/structured-merge-diff type Fields struct { // Map stores a set of fields in a data structure like a Trie. + // + // Each key is either a '.' representing the field itself, and will always map to an empty set, + // or a string representing a sub-field or item. The string will follow one of these four formats: + // 'f:', where is the name of a field in a struct, or key in a map + // 'v:', where is the exact json formatted value of a list item + // 'i:', where is position of a item in a list + // 'k:', where is a map of a list item's key fields to their unique values + // If a key maps to an empty Fields value, the field that key represents is part of the set. + // + // The exact format is defined in k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal Map map[string]Fields `json:",inline" protobuf:"bytes,1,rep,name=map"` } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD index ce474dbbc45..fed358126e6 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD @@ -8,6 +8,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go index f4c1cdd3e2b..773063890cc 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go @@ -21,6 +21,7 @@ import ( "time" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -82,9 +83,19 @@ func NewCRDFieldManager(objectConverter runtime.ObjectConvertor, objectDefaulter // use-case), and simply updates the managed fields in the output // object. func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (runtime.Object, error) { + // If the object doesn't have metadata, we should just return without trying to + // set the managedFields at all, so creates/updates/patches will work normally. + if _, err := meta.Accessor(newObj); err != nil { + return newObj, nil + } + + // First try to decode the managed fields provided in the update, + // This is necessary to allow directly updating managed fields. managed, err := internal.DecodeObjectManagedFields(newObj) + // If the managed field is empty or we failed to decode it, - // let's try the live object + // let's try the live object. This is to prevent clients who + // don't understand managedFields from deleting it accidentally. if err != nil || len(managed) == 0 { managed, err = internal.DecodeObjectManagedFields(liveObj) if err != nil { @@ -99,13 +110,8 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r if err != nil { return nil, fmt.Errorf("failed to convert live object to proper version: %v", err) } - if err := internal.RemoveObjectManagedFields(liveObjVersioned); err != nil { - return nil, fmt.Errorf("failed to remove managed fields from live obj: %v", err) - } - if err := internal.RemoveObjectManagedFields(newObjVersioned); err != nil { - return nil, fmt.Errorf("failed to remove managed fields from new obj: %v", err) - } - + internal.RemoveObjectManagedFields(liveObjVersioned) + internal.RemoveObjectManagedFields(newObjVersioned) newObjTyped, err := f.typeConverter.ObjectToTyped(newObjVersioned) if err != nil { return nil, fmt.Errorf("failed to create typed new object: %v", err) @@ -135,19 +141,23 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r // Apply is used when server-side apply is called, as it merges the // object and update the managed fields. func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, force bool) (runtime.Object, error) { + // If the object doesn't have metadata, apply isn't allowed. + if _, err := meta.Accessor(liveObj); err != nil { + return nil, fmt.Errorf("couldn't get accessor: %v", err) + } + managed, err := internal.DecodeObjectManagedFields(liveObj) if err != nil { return nil, fmt.Errorf("failed to decode managed fields: %v", err) } // We can assume that patchObj is already on the proper version: // it shouldn't have to be converted so that it's not defaulted. + // TODO (jennybuckley): Explicitly checkt that patchObj is in the proper version. liveObjVersioned, err := f.toVersioned(liveObj) if err != nil { return nil, fmt.Errorf("failed to convert live object to proper version: %v", err) } - if err := internal.RemoveObjectManagedFields(liveObjVersioned); err != nil { - return nil, fmt.Errorf("failed to remove managed fields from live obj: %v", err) - } + internal.RemoveObjectManagedFields(liveObjVersioned) patchObjTyped, err := f.typeConverter.YAMLToTyped(patch) if err != nil { @@ -211,5 +221,5 @@ func (f *FieldManager) buildManagerInfo(prefix string, operation metav1.ManagedF if managerInfo.Manager == "" { managerInfo.Manager = "unknown" } - return internal.DecodeManager(&managerInfo) + return internal.BuildManagerIdentifier(&managerInfo) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go index b2f32646233..0c20d97b92b 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go @@ -30,13 +30,12 @@ import ( // RemoveObjectManagedFields removes the ManagedFields from the object // before we merge so that it doesn't appear in the ManagedFields // recursively. -func RemoveObjectManagedFields(obj runtime.Object) error { +func RemoveObjectManagedFields(obj runtime.Object) { accessor, err := meta.Accessor(obj) if err != nil { - return fmt.Errorf("couldn't get accessor: %v", err) + panic(fmt.Sprintf("couldn't get accessor: %v", err)) } accessor.SetManagedFields(nil) - return nil } // DecodeObjectManagedFields extracts and converts the objects ManagedFields into a fieldpath.ManagedFields. @@ -46,7 +45,7 @@ func DecodeObjectManagedFields(from runtime.Object) (fieldpath.ManagedFields, er } accessor, err := meta.Accessor(from) if err != nil { - return nil, fmt.Errorf("couldn't get accessor: %v", err) + panic(fmt.Sprintf("couldn't get accessor: %v", err)) } managed, err := decodeManagedFields(accessor.GetManagedFields()) @@ -60,7 +59,7 @@ func DecodeObjectManagedFields(from runtime.Object) (fieldpath.ManagedFields, er func EncodeObjectManagedFields(obj runtime.Object, fields fieldpath.ManagedFields) error { accessor, err := meta.Accessor(obj) if err != nil { - return fmt.Errorf("couldn't get accessor: %v", err) + panic(fmt.Sprintf("couldn't get accessor: %v", err)) } managed, err := encodeManagedFields(fields) @@ -77,7 +76,7 @@ func EncodeObjectManagedFields(obj runtime.Object, fields fieldpath.ManagedField func decodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (managedFields fieldpath.ManagedFields, err error) { managedFields = make(map[string]*fieldpath.VersionedSet, len(encodedManagedFields)) for _, encodedVersionedSet := range encodedManagedFields { - manager, err := DecodeManager(&encodedVersionedSet) + manager, err := BuildManagerIdentifier(&encodedVersionedSet) if err != nil { return nil, fmt.Errorf("error decoding manager from %v: %v", encodedVersionedSet, err) } @@ -89,8 +88,8 @@ func decodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (mana return managedFields, nil } -// DecodeManager creates a manager identifier string from a ManagedFieldsEntry -func DecodeManager(encodedManager *metav1.ManagedFieldsEntry) (manager string, err error) { +// BuildManagerIdentifier creates a manager identifier string from a ManagedFieldsEntry +func BuildManagerIdentifier(encodedManager *metav1.ManagedFieldsEntry) (manager string, err error) { encodedManagerCopy := *encodedManager // Never include the fields in the manager identifier diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go index 7f2d517f574..1712b3b23ae 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go @@ -25,8 +25,8 @@ import ( "sigs.k8s.io/yaml" ) -// TestRoundTripManagedFields will roundtrip ManagedFields from the format used by -// sigs.k8s.io/structured-merge-diff to the wire format (api format) and back +// TestRoundTripManagedFields will roundtrip ManagedFields from the wire format +// (api format) to the format used by sigs.k8s.io/structured-merge-diff and back func TestRoundTripManagedFields(t *testing.T) { tests := []string{ `- apiVersion: v1 @@ -153,7 +153,7 @@ func TestRoundTripManagedFields(t *testing.T) { } } -func TestDecodeManager(t *testing.T) { +func TestBuildManagerIdentifier(t *testing.T) { tests := []struct { managedFieldsEntry string expected string @@ -188,7 +188,7 @@ time: "2001-02-03T04:05:06Z" if err := yaml.Unmarshal([]byte(test.managedFieldsEntry), &unmarshaled); err != nil { t.Fatalf("did not expect yaml unmarshalling error but got: %v", err) } - decoded, err := DecodeManager(&unmarshaled) + decoded, err := BuildManagerIdentifier(&unmarshaled) if err != nil { t.Fatalf("did not expect decoding error but got: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go index 81b9dd41765..bd119e03c2e 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go @@ -20,20 +20,20 @@ import "testing" func TestPathElementRoundTrip(t *testing.T) { tests := []string{ - "i:0", - "i:1234", - "f:", - "f:spec", - "f:more-complicated-string", - "k:{\"name\":\"my-container\"}", - "k:{\"port\":\"8080\",\"protocol\":\"TCP\"}", - "k:{\"optionalField\":null}", - "k:{\"jsonField\":{\"A\":1,\"B\":null,\"C\":\"D\",\"E\":{\"F\":\"G\"}}}", - "k:{\"listField\":[\"1\",\"2\",\"3\"]}", - "v:null", - "v:\"some-string\"", - "v:1234", - "v:{\"some\":\"json\"}", + `i:0`, + `i:1234`, + `f:`, + `f:spec`, + `f:more-complicated-string`, + `k:{"name":"my-container"}`, + `k:{"port":"8080","protocol":"TCP"}`, + `k:{"optionalField":null}`, + `k:{"jsonField":{"A":1,"B":null,"C":"D","E":{"F":"G"}}}`, + `k:{"listField":["1","2","3"]}`, + `v:null`, + `v:"some-string"`, + `v:1234`, + `v:{"some":"json"}`, } for _, test := range tests { @@ -62,15 +62,15 @@ func TestPathElementIgnoreUnknown(t *testing.T) { func TestNewPathElementError(t *testing.T) { tests := []string{ - "", - "no-colon", - "i:index is not a number", - "i:1.23", - "i:", - "v:invalid json", - "v:", - "k:invalid json", - "k:{\"name\":invalid}", + ``, + `no-colon`, + `i:index is not a number`, + `i:1.23`, + `i:`, + `v:invalid json`, + `v:`, + `k:invalid json`, + `k:{"name":invalid}`, } for _, test := range tests { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go index b4974752dea..80ac84128ff 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go @@ -43,6 +43,7 @@ type TypeConverter interface { // // Note that this is not going to be sufficient for converting to/from // CRDs that have a schema defined (we don't support that schema yet). +// TODO(jennybuckley): Use the schema provided by a CRD if it exists. type DeducedTypeConverter struct{} var _ TypeConverter = DeducedTypeConverter{} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go index bced6d6328f..093ce64c945 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go @@ -17,8 +17,10 @@ limitations under the License. package internal_test import ( + "fmt" "path/filepath" "reflect" + "strings" "testing" "github.com/ghodss/yaml" @@ -30,7 +32,7 @@ import ( var fakeSchema = prototesting.Fake{ Path: filepath.Join( - "..", "..", "..", "..", "..", "..", "..", "..", "..", + strings.Repeat(".."+string(filepath.Separator), 9), "api", "openapi-spec", "swagger.json"), } @@ -49,7 +51,15 @@ func TestTypeConverter(t *testing.T) { t.Fatalf("Failed to build TypeConverter: %v", err) } - y := ` + dtc := internal.DeducedTypeConverter{} + + testCases := []struct { + name string + yaml string + }{ + { + name: "apps/v1.Deployment", + yaml: ` apiVersion: apps/v1 kind: Deployment metadata: @@ -69,8 +79,61 @@ spec: containers: - name: nginx image: nginx:1.15.4 -` +`, + }, { + name: "extensions/v1beta1.Deployment", + yaml: ` +apiVersion: extensions/v1beta1 +kind: Deployment +metadata: + name: nginx-deployment + labels: + app: nginx +spec: + replicas: 3 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: nginx:1.15.4 +`, + }, { + name: "v1.Pod", + yaml: ` +apiVersion: v1 +kind: Pod +metadata: + name: nginx-pod + labels: + app: nginx +spec: + containers: + - name: nginx + image: nginx:1.15.4 +`, + }, + } + for _, testCase := range testCases { + t.Run(fmt.Sprintf("%v ObjectToTyped with TypeConverter", testCase.name), func(t *testing.T) { + testObjectToTyped(t, tc, testCase.yaml) + }) + t.Run(fmt.Sprintf("%v YAMLToTyped with TypeConverter", testCase.name), func(t *testing.T) { + testYAMLToTyped(t, tc, testCase.yaml) + }) + t.Run(fmt.Sprintf("%v ObjectToTyped with DeducedTypeConverter", testCase.name), func(t *testing.T) { + testObjectToTyped(t, dtc, testCase.yaml) + }) + } +} + +func testObjectToTyped(t *testing.T, tc internal.TypeConverter, y string) { obj := &unstructured.Unstructured{Object: map[string]interface{}{}} if err := yaml.Unmarshal([]byte(y), &obj.Object); err != nil { t.Fatalf("Failed to parse yaml object: %v", err) @@ -90,12 +153,18 @@ Original object: Final object: %#v`, obj, newObj) } +} +func testYAMLToTyped(t *testing.T, tc internal.TypeConverter, y string) { + obj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(y), &obj.Object); err != nil { + t.Fatalf("Failed to parse yaml object: %v", err) + } yamlTyped, err := tc.YAMLToTyped([]byte(y)) if err != nil { t.Fatalf("Failed to convert yaml to typed: %v", err) } - newObj, err = tc.TypedToObject(yamlTyped) + newObj, err := tc.TypedToObject(yamlTyped) if err != nil { t.Fatalf("Failed to convert typed to object: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go index 04febe03a86..f6690f86950 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go @@ -795,9 +795,6 @@ func TestPatchWithVersionConflictThenAdmissionFailure(t *testing.T) { tc.Run(t) } -// TODO: Add test case for "apply with existing uid" verify it gives a conflict error, -// not a creation or an authz creation forbidden message - func TestHasUID(t *testing.T) { testcases := []struct { obj runtime.Object diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/patchhandler_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/patchhandler_test.go index f96c80c8bd4..f27a702dffd 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/patchhandler_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/patchhandler_test.go @@ -25,10 +25,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" genericapitesting "k8s.io/apiserver/pkg/endpoints/testing" - genericfeatures "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/rest" - utilfeature "k8s.io/apiserver/pkg/util/feature" - utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing" ) func TestPatch(t *testing.T) { @@ -152,124 +149,3 @@ func TestPatchRequiresMatchingName(t *testing.T) { t.Errorf("Unexpected response %#v", response) } } - -func TestPatchApply(t *testing.T) { - t.Skip("apply is being refactored") - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() - storage := map[string]rest.Storage{} - item := &genericapitesting.Simple{ - ObjectMeta: metav1.ObjectMeta{ - Name: "id", - Namespace: "", - UID: "uid", - }, - Other: "bar", - } - simpleStorage := SimpleRESTStorage{item: *item} - storage["simple"] = &simpleStorage - handler := handle(storage) - server := httptest.NewServer(handler) - defer server.Close() - - client := http.Client{} - request, err := http.NewRequest( - "PATCH", - server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/id", - bytes.NewReader([]byte(`{"metadata":{"name":"id"}, "labels": {"test": "yes"}}`)), - ) - request.Header.Set("Content-Type", "application/apply-patch+yaml") - response, err := client.Do(request) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if response.StatusCode != http.StatusOK { - t.Errorf("Unexpected response %#v", response) - } - if simpleStorage.updated.Labels["test"] != "yes" { - t.Errorf(`Expected labels to have "test": "yes", found %q`, simpleStorage.updated.Labels["test"]) - } - if simpleStorage.updated.Other != "bar" { - t.Errorf(`Merge should have kept initial "bar" value for Other: %v`, simpleStorage.updated.Other) - } - if len(simpleStorage.updated.ObjectMeta.ManagedFields) == 0 { - t.Errorf(`Expected managedFields field to be set, but is empty`) - } -} - -func TestApplyAddsGVK(t *testing.T) { - t.Skip("apply is being refactored") - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() - storage := map[string]rest.Storage{} - item := &genericapitesting.Simple{ - ObjectMeta: metav1.ObjectMeta{ - Name: "id", - Namespace: "", - UID: "uid", - }, - Other: "bar", - } - simpleStorage := SimpleRESTStorage{item: *item} - storage["simple"] = &simpleStorage - handler := handle(storage) - server := httptest.NewServer(handler) - defer server.Close() - - client := http.Client{} - request, err := http.NewRequest( - "PATCH", - server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/id", - bytes.NewReader([]byte(`{"metadata":{"name":"id"}, "labels": {"test": "yes"}}`)), - ) - request.Header.Set("Content-Type", "application/apply-patch+yaml") - response, err := client.Do(request) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if response.StatusCode != http.StatusOK { - t.Errorf("Unexpected response %#v", response) - } - // TODO: Need to fix this - expected := `{"apiVersion":"test.group/version","kind":"Simple","labels":{"test":"yes"},"metadata":{"name":"id"}}` - if simpleStorage.updated.ObjectMeta.ManagedFields[0].APIVersion != expected { - t.Errorf( - `Expected managedFields field to be %q, got %q`, - expected, - simpleStorage.updated.ObjectMeta.ManagedFields[0].APIVersion, - ) - } -} - -func TestApplyCreatesWithManagedFields(t *testing.T) { - t.Skip("apply is being refactored") - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() - storage := map[string]rest.Storage{} - simpleStorage := SimpleRESTStorage{} - storage["simple"] = &simpleStorage - handler := handle(storage) - server := httptest.NewServer(handler) - defer server.Close() - - client := http.Client{} - request, err := http.NewRequest( - "PATCH", - server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/simple/id", - bytes.NewReader([]byte(`{"metadata":{"name":"id"}, "labels": {"test": "yes"}}`)), - ) - request.Header.Set("Content-Type", "application/apply-patch+yaml") - response, err := client.Do(request) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if response.StatusCode != http.StatusOK { - t.Errorf("Unexpected response %#v", response) - } - // TODO: Need to fix this - expected := `{"apiVersion":"test.group/version","kind":"Simple","labels":{"test":"yes"},"metadata":{"name":"id"}}` - if simpleStorage.updated.ObjectMeta.ManagedFields[0].APIVersion != expected { - t.Errorf( - `Expected managedFields field to be %q, got %q`, - expected, - simpleStorage.updated.ObjectMeta.ManagedFields[0].APIVersion, - ) - } -} diff --git a/staging/src/k8s.io/apiserver/pkg/features/kube_features.go b/staging/src/k8s.io/apiserver/pkg/features/kube_features.go index 966e91ac97f..32a58fbad43 100644 --- a/staging/src/k8s.io/apiserver/pkg/features/kube_features.go +++ b/staging/src/k8s.io/apiserver/pkg/features/kube_features.go @@ -84,7 +84,7 @@ const ( DryRun utilfeature.Feature = "DryRun" // owner: @apelisse, @lavalamp - // alpha: v1.11 + // alpha: v1.14 // // Server-side apply. Merging happens on the server. ServerSideApply utilfeature.Feature = "ServerSideApply" diff --git a/test/integration/auth/rbac_test.go b/test/integration/auth/rbac_test.go index 963f872c86f..0d8403c32ab 100644 --- a/test/integration/auth/rbac_test.go +++ b/test/integration/auth/rbac_test.go @@ -30,6 +30,7 @@ import ( apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" "k8s.io/apiserver/pkg/authentication/request/bearertoken" "k8s.io/apiserver/pkg/authentication/token/tokenfile" @@ -482,40 +483,40 @@ func TestRBAC(t *testing.T) { {"limitrange-updater", "PUT", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK}, }, }, - // { - // bootstrapRoles: bootstrapRoles{ - // clusterRoles: []rbacapi.ClusterRole{ - // { - // ObjectMeta: metav1.ObjectMeta{Name: "allow-all"}, - // Rules: []rbacapi.PolicyRule{ruleAllowAll}, - // }, - // { - // ObjectMeta: metav1.ObjectMeta{Name: "patch-limitranges"}, - // Rules: []rbacapi.PolicyRule{ - // rbacapi.NewRule("patch").Groups("").Resources("limitranges").RuleOrDie(), - // }, - // }, - // }, - // clusterRoleBindings: []rbacapi.ClusterRoleBinding{ - // { - // ObjectMeta: metav1.ObjectMeta{Name: "patch-limitranges"}, - // Subjects: []rbacapi.Subject{ - // {Kind: "User", Name: "limitrange-patcher"}, - // }, - // RoleRef: rbacapi.RoleRef{Kind: "ClusterRole", Name: "patch-limitranges"}, - // }, - // }, - // }, - // requests: []request{ - // // Create the namespace used later in the test - // {superUser, "POST", "", "namespaces", "", "", limitRangeNamespace, http.StatusCreated}, + { + bootstrapRoles: bootstrapRoles{ + clusterRoles: []rbacapi.ClusterRole{ + { + ObjectMeta: metav1.ObjectMeta{Name: "allow-all"}, + Rules: []rbacapi.PolicyRule{ruleAllowAll}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "patch-limitranges"}, + Rules: []rbacapi.PolicyRule{ + rbacapi.NewRule("patch").Groups("").Resources("limitranges").RuleOrDie(), + }, + }, + }, + clusterRoleBindings: []rbacapi.ClusterRoleBinding{ + { + ObjectMeta: metav1.ObjectMeta{Name: "patch-limitranges"}, + Subjects: []rbacapi.Subject{ + {Kind: "User", Name: "limitrange-patcher"}, + }, + RoleRef: rbacapi.RoleRef{Kind: "ClusterRole", Name: "patch-limitranges"}, + }, + }, + }, + requests: []request{ + // Create the namespace used later in the test + {superUser, "POST", "", "namespaces", "", "", limitRangeNamespace, http.StatusCreated}, - // {"limitrange-patcher", "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusForbidden}, - // {superUser, "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusCreated}, - // {superUser, "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK}, - // {"limitrange-patcher", "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK}, - // }, - // }, + {"limitrange-patcher", "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusForbidden}, + {superUser, "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusCreated}, + {superUser, "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK}, + {"limitrange-patcher", "PATCH", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK}, + }, + }, } for i, tc := range tests { @@ -535,6 +536,7 @@ func TestRBAC(t *testing.T) { "limitrange-patcher": {Name: "limitrange-patcher"}, "user-with-no-permissions": {Name: "user-with-no-permissions"}, })) + masterConfig.GenericConfig.OpenAPIConfig = framework.DefaultOpenAPIConfig() _, s, closeFn := framework.RunAMaster(masterConfig) defer closeFn() @@ -569,11 +571,10 @@ func TestRBAC(t *testing.T) { } req, err := http.NewRequest(r.verb, s.URL+path, body) - // TODO: Un-comment this when Apply works again - // if r.verb == "PATCH" { - // // For patch operations, use the apply content type - // req.Header.Add("Content-Type", string(types.ApplyPatchType)) - // } + if r.verb == "PATCH" { + // For patch operations, use the apply content type + req.Header.Add("Content-Type", string(types.ApplyPatchType)) + } if err != nil { t.Fatalf("failed to create request: %v", err)