Minor fixes

Kubernetes-commit: 6b2e4682fe883eebcaf1c1e43cf2957dde441174

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 {
+	internal.RemoveObjectManagedFields(liveObjVersioned)
-		return nil, fmt.Errorf("failed to remove managed fields from live obj: %v", err)
+	internal.RemoveObjectManagedFields(newObjVersioned)
-	}
-	if err := internal.RemoveObjectManagedFields(newObjVersioned); err != nil {
-		return nil, fmt.Errorf("failed to remove managed fields from new obj: %v", err)
-	}
-
 	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 {
+	internal.RemoveObjectManagedFields(liveObjVersioned)
-		return nil, fmt.Errorf("failed to remove managed fields from live obj: %v", err)
-	}
 
 	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)
 }

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
+// BuildManagerIdentifier creates a manager identifier string from a ManagedFieldsEntry
-func DecodeManager(encodedManager *metav1.ManagedFieldsEntry) (manager string, err error) {
+func BuildManagerIdentifier(encodedManager *metav1.ManagedFieldsEntry) (manager string, err error) {
 	encodedManagerCopy := *encodedManager
 
 	// Never include the fields in the manager identifier

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
+// TestRoundTripManagedFields will roundtrip ManagedFields from the wire format
-// sigs.k8s.io/structured-merge-diff to the wire format (api format) and back
+// (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)
 			}

pkg/endpoints/handlers/fieldmanager/internal/pathelement_test.go

@@ -20,20 +20,20 @@ import "testing"
 
 func TestPathElementRoundTrip(t *testing.T) {
 	tests := []string{
-		"i:0",
+		`i:0`,
-		"i:1234",
+		`i:1234`,
-		"f:",
+		`f:`,
-		"f:spec",
+		`f:spec`,
-		"f:more-complicated-string",
+		`f:more-complicated-string`,
-		"k:{\"name\":\"my-container\"}",
+		`k:{"name":"my-container"}`,
-		"k:{\"port\":\"8080\",\"protocol\":\"TCP\"}",
+		`k:{"port":"8080","protocol":"TCP"}`,
-		"k:{\"optionalField\":null}",
+		`k:{"optionalField":null}`,
-		"k:{\"jsonField\":{\"A\":1,\"B\":null,\"C\":\"D\",\"E\":{\"F\":\"G\"}}}",
+		`k:{"jsonField":{"A":1,"B":null,"C":"D","E":{"F":"G"}}}`,
-		"k:{\"listField\":[\"1\",\"2\",\"3\"]}",
+		`k:{"listField":["1","2","3"]}`,
-		"v:null",
+		`v:null`,
-		"v:\"some-string\"",
+		`v:"some-string"`,
-		"v:1234",
+		`v:1234`,
-		"v:{\"some\":\"json\"}",
+		`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",
+		`no-colon`,
-		"i:index is not a number",
+		`i:index is not a number`,
-		"i:1.23",
+		`i:1.23`,
-		"i:",
+		`i:`,
-		"v:invalid json",
+		`v:invalid json`,
-		"v:",
+		`v:`,
-		"k:invalid json",
+		`k:invalid json`,
-		"k:{\"name\":invalid}",
+		`k:{"name":invalid}`,
 	}
 
 	for _, test := range tests {

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

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

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

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

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"