Skip to content

Commit

Permalink
TestFieldValidationPatchTyped
Browse files Browse the repository at this point in the history
  • Loading branch information
@kevindelgado
kevindelgado committed Oct 28, 2021
1 parent 9bd487a commit dd91910
Show file tree
Hide file tree
Showing 3 changed files with 175 additions and 15 deletions.
1 change: 1 addition & 0 deletions staging/src/k8s.io/apiserver/go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ require (
k8s.io/kube-openapi v0.0.0-20210817084001-7fbd8d59e5b8
k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.23
sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6
sigs.k8s.io/structured-merge-diff/v4 v4.1.2
sigs.k8s.io/yaml v1.2.0
)
Expand Down
52 changes: 37 additions & 15 deletions staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"strings"
"time"

kjson "sigs.k8s.io/json"

jsonpatch "github.com/evanphx/json-patch"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -144,10 +146,14 @@ func PatchResource(r rest.Patcher, scope *RequestScope, admit admission.Interfac
if fv := req.URL.Query()["fieldValidation"]; len(fv) > 0 {
validationDirective = fv[0]
}
decodeSerializer := s.Serializer
if validationDirective == metav1.FieldValidationWarn || validationDirective == metav1.FieldValidationStrict {
decodeSerializer = s.StrictSerializer
}

codec := runtime.NewCodec(
scope.Serializer.EncoderForVersion(s.Serializer, gv),
scope.Serializer.DecoderToVersion(s.Serializer, scope.HubGroupVersion),
scope.Serializer.DecoderToVersion(decodeSerializer, scope.HubGroupVersion),
)

userInfo, _ := request.UserFrom(ctx)
Expand Down Expand Up @@ -316,21 +322,34 @@ func (p *jsonPatcher) applyPatchToCurrentObject(requestContext context.Context,
}

// Apply the patch.
patchedObjJS, err := p.applyJSPatch(currentObjJS)
patchedObjJS, applyStrictJSONErrs, err := p.applyJSPatch(currentObjJS)
if err != nil {
return nil, err
}

// Construct the resulting typed, unversioned object.
objToUpdate := p.restPatcher.New()
if err := runtime.DecodeInto(p.codec, patchedObjJS, objToUpdate); err != nil {
if !runtime.IsStrictDecodingError(err) || p.validationDirective == metav1.FieldValidationStrict {
if !runtime.IsStrictDecodingError(err) {
return nil, errors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{
field.Invalid(field.NewPath("patch"), string(patchedObjJS), err.Error()),
})
}
decodeStrictJSONErr, ok := runtime.AsStrictDecodingError(err)
if !ok {
// not possible
return nil, errors.NewInternalError(fmt.Errorf("decode error both is and is not a strict decoding error: %v", err))
}
// combine the strict decoding errors from applyJSPatch and DecodeInto so that we report all of them
strictDecodingErr := runtime.NewStrictDecodingError(append(applyStrictJSONErrs, decodeStrictJSONErr.Errors()...))
if p.validationDirective == metav1.FieldValidationStrict {
return nil, errors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{
field.Invalid(field.NewPath("patch"), string(patchedObjJS), strictDecodingErr.Error()),
})

}
if p.validationDirective == metav1.FieldValidationWarn {
addStrictDecodingWarnings(requestContext, err)
addStrictDecodingWarnings(requestContext, strictDecodingErr)
}
}

Expand All @@ -346,46 +365,49 @@ func (p *jsonPatcher) createNewObject() (runtime.Object, error) {

// applyJSPatch applies the patch. Input and output objects must both have
// the external version, since that is what the patch must have been constructed against.
func (p *jsonPatcher) applyJSPatch(versionedJS []byte) (patchedJS []byte, retErr error) {
func (p *jsonPatcher) applyJSPatch(versionedJS []byte) (patchedJS []byte, strictErrors []error, retErr error) {
switch p.patchType {
case types.JSONPatchType:
// sanity check potentially abusive patches
// TODO(liggitt): drop this once golang json parser limits stack depth (https://github.com/golang/go/issues/31789)
if len(p.patchBytes) > 1024*1024 {
v := []interface{}{}
if err := json.Unmarshal(p.patchBytes, &v); err != nil {
return nil, errors.NewBadRequest(fmt.Sprintf("error decoding patch: %v", err))
return nil, nil, errors.NewBadRequest(fmt.Sprintf("error decoding patch: %v", err))
}
}

patchObj, err := jsonpatch.DecodePatch(p.patchBytes)
if err != nil {
return nil, errors.NewBadRequest(err.Error())
return nil, nil, errors.NewBadRequest(err.Error())
}
if len(patchObj) > maxJSONPatchOperations {
return nil, errors.NewRequestEntityTooLargeError(
return nil, nil, errors.NewRequestEntityTooLargeError(
fmt.Sprintf("The allowed maximum operations in a JSON patch is %d, got %d",
maxJSONPatchOperations, len(patchObj)))
}
patchedJS, err := patchObj.Apply(versionedJS)
if err != nil {
return nil, errors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false)
return nil, nil, errors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false)
}
return patchedJS, nil
return patchedJS, nil, nil
case types.MergePatchType:
// sanity check potentially abusive patches
// TODO(liggitt): drop this once golang json parser limits stack depth (https://github.com/golang/go/issues/31789)
if len(p.patchBytes) > 1024*1024 {
if len(p.patchBytes) > 1024*1024 || p.validationDirective == metav1.FieldValidationStrict || p.validationDirective == metav1.FieldValidationWarn {
v := map[string]interface{}{}
if err := json.Unmarshal(p.patchBytes, &v); err != nil {
return nil, errors.NewBadRequest(fmt.Sprintf("error decoding patch: %v", err))
var err error
strictErrors, err = kjson.UnmarshalStrict(p.patchBytes, &v)
if err != nil {
return nil, nil, errors.NewBadRequest(fmt.Sprintf("error decoding patch: %v", err))
}
}

return jsonpatch.MergePatch(versionedJS, p.patchBytes)
patchedJS, retErr = jsonpatch.MergePatch(versionedJS, p.patchBytes)
return patchedJS, strictErrors, retErr
default:
// only here as a safety net - go-restful filters content-type
return nil, fmt.Errorf("unknown Content-Type header for patch: %v", p.patchType)
return nil, nil, fmt.Errorf("unknown Content-Type header for patch: %v", p.patchType)
}
}

Expand Down
137 changes: 137 additions & 0 deletions test/integration/apiserver/field_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,143 @@ func TestFieldValidationPut(t *testing.T) {
}
}

// TestFieldValidationPatchTyped tests mege-patch and json-patch requests containing unknown fields with
// strict and non-strict field validation for typed objects.
func TestFieldValidationPatchTyped(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictFieldValidation, true)()

_, client, closeFn := setup(t)
defer closeFn()

deployName := `"test-deployment"`
postBytes, err := os.ReadFile("./testdata/deploy-small.json")
if err != nil {
t.Fatalf("failed to read file: %v", err)
}
postBody := []byte(fmt.Sprintf(string(postBytes), deployName))

if _, err := client.CoreV1().RESTClient().Post().
AbsPath("/apis/apps/v1").
Namespace("default").
Resource("deployments").
Body(postBody).
DoRaw(context.TODO()); err != nil {
t.Fatalf("failed to create initial deployment: %v", err)
}

patchBody := `
{
"spec": {
"unknown1": "val1",
"unknownDupe": "valDupe",
"unknownDupe": "valDupe2",
"paused": true,
"paused": false,
"template": {
"spec": {
"containers": [{
"name": "nginx",
"unknownNested": "val1",
"dnsPolicy": "ClusterFirst",
"dnsPolicy": "None"
}]
}
}
}
}
`
var testcases = []struct {
name string
opts metav1.UpdateOptions
patchType types.PatchType
body string
strictDecodingErrors []string
strictDecodingWarnings []string
}{
{
name: "merge-patch-strict-validation",
opts: metav1.UpdateOptions{
FieldValidation: "Strict",
},
patchType: types.MergePatchType,
body: patchBody,
strictDecodingErrors: []string{
`duplicate field "unknownDupe"`,
`duplicate field "paused"`,
`duplicate field "dnsPolicy"`,
// TODO: why is dnsPolicy unknown too?
`unknown field "dnsPolicy"`,
`unknown field "unknownNested"`,
`unknown field "unknown1"`,
},
},
{
name: "merge-patch-warn-validation",
opts: metav1.UpdateOptions{
FieldValidation: "Warn",
},
patchType: types.MergePatchType,
body: patchBody,
strictDecodingWarnings: []string{
`duplicate field "unknownDupe"`,
`duplicate field "paused"`,
`duplicate field "dnsPolicy"`,
// TODO: why is dnsPolicy unknown too?
`unknown field "dnsPolicy"`,
`unknown field "unknownNested"`,
`unknown field "unknown1"`,
},
},
{
name: "merge-patch-ignore-validation",
opts: metav1.UpdateOptions{
FieldValidation: "Ignore",
},
patchType: types.MergePatchType,
body: patchBody,
},
{
name: "merge-patch-no-validation",
patchType: types.MergePatchType,
body: patchBody,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
req := client.CoreV1().RESTClient().Patch(tc.patchType).
AbsPath("/apis/apps/v1").
Namespace("default").
Resource("deployments").
Name("test-deployment").
VersionedParams(&tc.opts, metav1.ParameterCodec)
klog.Warningf("tc.name: %s", tc.name)
result := req.Body([]byte(tc.body)).Do(context.TODO())

if result.Error() == nil && len(tc.strictDecodingErrors) > 0 {
resObj, _ := result.Get()
klog.Warningf("resObj: %v", resObj)
t.Fatalf("unexpected patch succeeded")
}
for _, strictErr := range tc.strictDecodingErrors {
if !strings.Contains(result.Error().Error(), strictErr) {
t.Fatalf("missing strict decoding error: %s from error: %s", strictErr, result.Error().Error())
}
}

if len(result.Warnings()) == 0 && len(tc.strictDecodingWarnings) > 0 {
t.Fatalf("unexpected patch had no warnings")
}
for i, strictWarn := range tc.strictDecodingWarnings {
if strictWarn != result.Warnings()[i].Text {
t.Fatalf("expected warning: %s, got warning: %s", strictWarn, result.Warnings()[i].Text)
}

}
})
}
}

// Benchmark field validation for strict vs non-strict
func BenchmarkFieldValidationPostPut(b *testing.B) {
defer featuregatetesting.SetFeatureGateDuringTest(b, utilfeature.DefaultFeatureGate, features.StrictFieldValidation, true)()
Expand Down

0 comments on commit dd91910

Please sign in to comment.