From f279314dc79a90d6c866472728c98a70377f8125 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Fri, 1 Feb 2019 11:55:18 -0800 Subject: [PATCH] Minor fixes Kubernetes-commit: 6b2e4682fe883eebcaf1c1e43cf2957dde441174 --- .../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 ------------------ pkg/features/kube_features.go | 2 +- 9 files changed, 131 insertions(+), 179 deletions(-) diff --git a/pkg/endpoints/handlers/fieldmanager/fieldmanager.go b/pkg/endpoints/handlers/fieldmanager/fieldmanager.go index f4c1cdd3e..773063890 100644 --- a/pkg/endpoints/handlers/fieldmanager/fieldmanager.go +++ b/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/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go b/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go index b2f326462..0c20d97b9 100644 --- a/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go +++ b/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/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go b/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go index 7f2d517f5..1712b3b23 100644 --- a/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go +++ b/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/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go b/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go index 81b9dd417..bd119e03c 100644 --- a/pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go +++ b/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/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go b/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go index b4974752d..80ac84128 100644 --- a/pkg/endpoints/handlers/fieldmanager/internal/typeconverter.go +++ b/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/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go b/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go index bced6d632..093ce64c9 100644 --- a/pkg/endpoints/handlers/fieldmanager/internal/typeconverter_test.go +++ b/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/pkg/endpoints/handlers/rest_test.go b/pkg/endpoints/handlers/rest_test.go index 04febe03a..f6690f869 100644 --- a/pkg/endpoints/handlers/rest_test.go +++ b/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/pkg/endpoints/patchhandler_test.go b/pkg/endpoints/patchhandler_test.go index f96c80c8b..f27a702df 100644 --- a/pkg/endpoints/patchhandler_test.go +++ b/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/pkg/features/kube_features.go b/pkg/features/kube_features.go index 966e91ac9..32a58fbad 100644 --- a/pkg/features/kube_features.go +++ b/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"