Merge pull request kubernetes#128273 from benluddy/cbor-apply

KEP-4222: Support CBOR encoding for apply requests.

Author: k8s-ci-robot

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -325,7 +325,10 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	supportedTypes := []string{
 		string(types.JSONPatchType),
 		string(types.MergePatchType),
-		string(types.ApplyPatchType),
+		string(types.ApplyYAMLPatchType),
+	}
+	if utilfeature.TestOnlyFeatureGate.Enabled(features.TestOnlyCBORServingAndStorage) {
+		supportedTypes = append(supportedTypes, string(types.ApplyCBORPatchType))
 	}
 
 	var handlerFunc http.HandlerFunc

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go

@@ -186,15 +186,16 @@ func ValidateUpdateOptions(options *metav1.UpdateOptions) field.ErrorList {
 
 func ValidatePatchOptions(options *metav1.PatchOptions, patchType types.PatchType) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if patchType != types.ApplyPatchType {
-		if options.Force != nil {
-			allErrs = append(allErrs, field.Forbidden(field.NewPath("force"), "may not be specified for non-apply patch"))
-		}
-	} else {
+	switch patchType {
+	case types.ApplyYAMLPatchType, types.ApplyCBORPatchType:
 		if options.FieldManager == "" {
 			// This field is defaulted to "kubectl" by kubectl, but HAS TO be explicitly set by controllers.
 			allErrs = append(allErrs, field.Required(field.NewPath("fieldManager"), "is required for apply patch"))
 		}
+	default:
+		if options.Force != nil {
+			allErrs = append(allErrs, field.Forbidden(field.NewPath("force"), "may not be specified for non-apply patch"))
+		}
 	}
 	allErrs = append(allErrs, ValidateFieldManager(options.FieldManager, field.NewPath("fieldManager"))...)
 	allErrs = append(allErrs, ValidateDryRun(field.NewPath("dryRun"), options.DryRun)...)

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go

@@ -141,12 +141,23 @@ func TestValidPatchOptions(t *testing.T) {
 			Force:        boolPtr(true),
 			FieldManager: "kubectl",
 		},
-		patchType: types.ApplyPatchType,
+		patchType: types.ApplyYAMLPatchType,
 	}, {
 		opts: metav1.PatchOptions{
 			FieldManager: "kubectl",
 		},
-		patchType: types.ApplyPatchType,
+		patchType: types.ApplyYAMLPatchType,
+	}, {
+		opts: metav1.PatchOptions{
+			Force:        boolPtr(true),
+			FieldManager: "kubectl",
+		},
+		patchType: types.ApplyCBORPatchType,
+	}, {
+		opts: metav1.PatchOptions{
+			FieldManager: "kubectl",
+		},
+		patchType: types.ApplyCBORPatchType,
 	}, {
 		opts:      metav1.PatchOptions{},
 		patchType: types.MergePatchType,
@@ -175,7 +186,12 @@ func TestInvalidPatchOptions(t *testing.T) {
 		// missing manager
 		{
 			opts:      metav1.PatchOptions{},
-			patchType: types.ApplyPatchType,
+			patchType: types.ApplyYAMLPatchType,
+		},
+		// missing manager
+		{
+			opts:      metav1.PatchOptions{},
+			patchType: types.ApplyCBORPatchType,
 		},
 		// force on non-apply
 		{

staging/src/k8s.io/apimachinery/pkg/types/patch.go

@@ -25,5 +25,7 @@ const (
 	JSONPatchType           PatchType = "application/json-patch+json"
 	MergePatchType          PatchType = "application/merge-patch+json"
 	StrategicMergePatchType PatchType = "application/strategic-merge-patch+json"
-	ApplyPatchType          PatchType = "application/apply-patch+yaml"
+	ApplyPatchType          PatchType = ApplyYAMLPatchType
+	ApplyYAMLPatchType      PatchType = "application/apply-patch+yaml"
+	ApplyCBORPatchType      PatchType = "application/apply-patch+cbor"
 )

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -35,9 +35,11 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	cbor "k8s.io/apimachinery/pkg/runtime/serializer/cbor/direct"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/managedfields"
 	"k8s.io/apimachinery/pkg/util/mergepatch"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -50,8 +52,10 @@ import (
 	requestmetrics "k8s.io/apiserver/pkg/endpoints/handlers/metrics"
 	"k8s.io/apiserver/pkg/endpoints/handlers/negotiation"
 	"k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/apiserver/pkg/features"
 	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/apiserver/pkg/util/dryrun"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/component-base/tracing"
 )
 
@@ -129,10 +133,25 @@ func PatchResource(r rest.Patcher, scope *RequestScope, admit admission.Interfac
 		audit.LogRequestPatch(req.Context(), patchBytes)
 		span.AddEvent("Recorded the audit event")
 
-		baseContentType := runtime.ContentTypeJSON
-		if patchType == types.ApplyPatchType {
+		var baseContentType string
+		switch patchType {
+		case types.ApplyYAMLPatchType:
 			baseContentType = runtime.ContentTypeYAML
+		case types.ApplyCBORPatchType:
+			if !utilfeature.TestOnlyFeatureGate.Enabled(features.TestOnlyCBORServingAndStorage) {
+				// This request should have already been rejected by the
+				// Content-Type allowlist check. Return 500 because assumptions are
+				// already broken and the feature is not GA.
+				utilruntime.HandleErrorWithContext(req.Context(), nil, "The patch content-type allowlist check should have made this unreachable.")
+				scope.err(errors.NewInternalError(errors.NewInternalError(fmt.Errorf("unexpected patch type: %v", patchType))), w, req)
+				return
+			}
+
+			baseContentType = runtime.ContentTypeCBOR
+		default:
+			baseContentType = runtime.ContentTypeJSON
 		}
+
 		s, ok := runtime.SerializerInfoForMediaType(scope.Serializer.SupportedMediaTypes(), baseContentType)
 		if !ok {
 			scope.err(fmt.Errorf("no serializer defined for %v", baseContentType), w, req)
@@ -452,6 +471,20 @@ func (p *smpPatcher) createNewObject(_ context.Context) (runtime.Object, error)
 	return nil, errors.NewNotFound(p.resource.GroupResource(), p.name)
 }
 
+func newApplyPatcher(p *patcher, fieldManager *managedfields.FieldManager, unmarshalFn, unmarshalStrictFn func([]byte, interface{}) error) *applyPatcher {
+	return &applyPatcher{
+		fieldManager:        fieldManager,
+		patch:               p.patchBytes,
+		options:             p.options,
+		creater:             p.creater,
+		kind:                p.kind,
+		userAgent:           p.userAgent,
+		validationDirective: p.validationDirective,
+		unmarshalFn:         unmarshalFn,
+		unmarshalStrictFn:   unmarshalStrictFn,
+	}
+}
+
 type applyPatcher struct {
 	patch               []byte
 	options             *metav1.PatchOptions
@@ -460,6 +493,8 @@ type applyPatcher struct {
 	fieldManager        *managedfields.FieldManager
 	userAgent           string
 	validationDirective string
+	unmarshalFn         func(data []byte, v interface{}) error
+	unmarshalStrictFn   func(data []byte, v interface{}) error
 }
 
 func (p *applyPatcher) applyPatchToCurrentObject(requestContext context.Context, obj runtime.Object) (runtime.Object, error) {
@@ -472,7 +507,7 @@ func (p *applyPatcher) applyPatchToCurrentObject(requestContext context.Context,
 	}
 
 	patchObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
-	if err := yaml.Unmarshal(p.patch, &patchObj.Object); err != nil {
+	if err := p.unmarshalFn(p.patch, &patchObj.Object); err != nil {
 		return nil, errors.NewBadRequest(fmt.Sprintf("error decoding YAML: %v", err))
 	}
 
@@ -484,7 +519,7 @@ func (p *applyPatcher) applyPatchToCurrentObject(requestContext context.Context,
 	// TODO: spawn something to track deciding whether a fieldValidation=Strict
 	// fatal error should return before an error from the apply operation
 	if p.validationDirective == metav1.FieldValidationStrict || p.validationDirective == metav1.FieldValidationWarn {
-		if err := yaml.UnmarshalStrict(p.patch, &map[string]interface{}{}); err != nil {
+		if err := p.unmarshalStrictFn(p.patch, &map[string]interface{}{}); err != nil {
 			if p.validationDirective == metav1.FieldValidationStrict {
 				return nil, errors.NewBadRequest(fmt.Sprintf("error strict decoding YAML: %v", err))
 			}
@@ -634,16 +669,21 @@ func (p *patcher) patchResource(ctx context.Context, scope *RequestScope) (runti
 			fieldManager:       scope.FieldManager,
 		}
 	// this case is unreachable if ServerSideApply is not enabled because we will have already rejected the content type
-	case types.ApplyPatchType:
-		p.mechanism = &applyPatcher{
-			fieldManager:        scope.FieldManager,
-			patch:               p.patchBytes,
-			options:             p.options,
-			creater:             p.creater,
-			kind:                p.kind,
-			userAgent:           p.userAgent,
-			validationDirective: p.validationDirective,
-		}
+	case types.ApplyYAMLPatchType:
+		p.mechanism = newApplyPatcher(p, scope.FieldManager, yaml.Unmarshal, yaml.UnmarshalStrict)
+		p.forceAllowCreate = true
+	case types.ApplyCBORPatchType:
+		if !utilfeature.TestOnlyFeatureGate.Enabled(features.TestOnlyCBORServingAndStorage) {
+			utilruntime.HandleErrorWithContext(context.TODO(), nil, "CBOR apply requests should be rejected before reaching this point unless the feature gate is enabled.")
+			return nil, false, fmt.Errorf("%v: unimplemented patch type", p.patchType)
+		}
+
+		// The strict and non-strict funcs are the same here because any CBOR map with
+		// duplicate keys is invalid and always rejected outright regardless of strictness
+		// mode, and unknown field errors can't occur in practice because the type of the
+		// destination value for unmarshaling an apply configuration is always
+		// "unstructured".
+		p.mechanism = newApplyPatcher(p, scope.FieldManager, cbor.Unmarshal, cbor.Unmarshal)
 		p.forceAllowCreate = true
 	default:
 		return nil, false, fmt.Errorf("%v: unimplemented patch type", p.patchType)
@@ -670,7 +710,7 @@ func (p *patcher) patchResource(ctx context.Context, scope *RequestScope) (runti
 		result, err := requestFunc()
 		// If the object wasn't committed to storage because it's serialized size was too large,
 		// it is safe to remove managedFields (which can be large) and try again.
-		if isTooLargeError(err) && p.patchType != types.ApplyPatchType {
+		if isTooLargeError(err) && p.patchType != types.ApplyYAMLPatchType && p.patchType != types.ApplyCBORPatchType {
 			if _, accessorErr := meta.Accessor(p.restPatcher.New()); accessorErr == nil {
 				p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil,
 					p.applyPatch,

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

@@ -875,7 +875,10 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 				string(types.JSONPatchType),
 				string(types.MergePatchType),
 				string(types.StrategicMergePatchType),
-				string(types.ApplyPatchType),
+				string(types.ApplyYAMLPatchType),
+			}
+			if utilfeature.TestOnlyFeatureGate.Enabled(features.TestOnlyCBORServingAndStorage) {
+				supportedTypes = append(supportedTypes, string(types.ApplyCBORPatchType))
 			}
 			handler := metrics.InstrumentRouteFunc(action.Verb, group, version, resource, subresource, requestScope, metrics.APIServerComponent, deprecated, removedRelease, restfulPatchResource(patcher, reqScope, admit, supportedTypes))
 			handler = utilwarning.AddWarningsHandler(handler, warnings)

staging/src/k8s.io/client-go/dynamic/golden_test.go

@@ -207,7 +207,7 @@ func TestGoldenRequest(t *testing.T) {
 				if err != nil {
 					t.Fatalf("failed to load fixture: %v", err)
 				}
-				if diff := cmp.Diff(got, want); diff != "" {
+				if diff := cmp.Diff(want, got); diff != "" {
 					t.Errorf("unexpected difference from expected bytes:\n%s", diff)
 				}
 			}))

staging/src/k8s.io/client-go/dynamic/simple.go

@@ -25,12 +25,12 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/features"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/util/apply"
 	"k8s.io/client-go/util/consistencydetector"
 	"k8s.io/client-go/util/watchlist"
 	"k8s.io/klog/v2"
@@ -340,10 +340,6 @@ func (c *dynamicResourceClient) Apply(ctx context.Context, name string, obj *uns
 	if err := validateNamespaceWithOptionalName(c.namespace, name); err != nil {
 		return nil, err
 	}
-	outBytes, err := runtime.Encode(unstructured.UnstructuredJSONScheme, obj)
-	if err != nil {
-		return nil, err
-	}
 	accessor, err := meta.Accessor(obj)
 	if err != nil {
 		return nil, err
@@ -355,21 +351,16 @@ func (c *dynamicResourceClient) Apply(ctx context.Context, name string, obj *uns
 	}
 	patchOpts := opts.ToPatchOptions()
 
-	result := c.client.client.
-		Patch(types.ApplyPatchType).
-		AbsPath(append(c.makeURLSegments(name), subresources...)...).
-		Body(outBytes).
-		SpecificallyVersionedParams(&patchOpts, dynamicParameterCodec, versionV1).
-		Do(ctx)
-	if err := result.Error(); err != nil {
-		return nil, err
-	}
-	retBytes, err := result.Raw()
+	request, err := apply.NewRequest(c.client.client, obj.Object)
 	if err != nil {
 		return nil, err
 	}
+
 	var out unstructured.Unstructured
-	if err := runtime.DecodeInto(unstructured.UnstructuredJSONScheme, retBytes, &out); err != nil {
+	if err := request.
+		AbsPath(append(c.makeURLSegments(name), subresources...)...).
+		SpecificallyVersionedParams(&patchOpts, dynamicParameterCodec, versionV1).
+		Do(ctx).Into(&out); err != nil {
 		return nil, err
 	}
 	return &out, nil

staging/src/k8s.io/client-go/dynamic/testdata/TestGoldenRequest/apply

@@ -2,8 +2,8 @@ PATCH /apis/flops/v1alpha1/namespaces/mops/flips/mips/fin?force=true HTTP/1.1
 Host: example.com
 Accept: application/json
 Accept-Encoding: gzip
-Content-Length: 29
+Content-Length: 28
 Content-Type: application/apply-patch+yaml
 User-Agent: TestGoldenRequest
 
-{"metadata":{"name":"mips"}}
+{"metadata":{"name":"mips"}}

staging/src/k8s.io/client-go/dynamic/testdata/TestGoldenRequest/applystatus

@@ -2,8 +2,8 @@ PATCH /apis/flops/v1alpha1/namespaces/mops/flips/mips/status?force=true HTTP/1.1
 Host: example.com
 Accept: application/json
 Accept-Encoding: gzip
-Content-Length: 29
+Content-Length: 28
 Content-Type: application/apply-patch+yaml
 User-Agent: TestGoldenRequest
 
-{"metadata":{"name":"mips"}}
+{"metadata":{"name":"mips"}}