Skip to content

Commit

Permalink
Feedback 11/8: Pruning, converter, rest helpers
Browse files Browse the repository at this point in the history
  • Loading branch information
@kevindelgado
kevindelgado committed Nov 10, 2021
1 parent 38cfcf4 commit 43a17e1
Show file tree
Hide file tree
Showing 18 changed files with 284 additions and 448 deletions.
67 changes: 15 additions & 52 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"fmt"
"net/http"
"path"
"reflect"
"sort"
"strings"
"sync"
Expand Down Expand Up @@ -1229,18 +1228,14 @@ func (d schemaCoercingDecoder) Decode(data []byte, defaults *schema.GroupVersion
obj, gvk, err := d.delegate.Decode(data, defaults, into)
var decodingStrictErrs []error
if err != nil {
if !runtime.IsStrictDecodingError(err) {
return nil, gvk, err
}
decodeStrictJSONErr, ok := runtime.AsStrictDecodingError(err)
decodeStrictErr, ok := runtime.AsStrictDecodingError(err)
if !ok {
// unreachable
return nil, gvk, fmt.Errorf("error both is and is not a strict decoding error: %v", err)
return nil, gvk, err
}
decodingStrictErrs = decodeStrictJSONErr.Errors()
decodingStrictErrs = decodeStrictErr.Errors()
}
if u, ok := obj.(*unstructured.Unstructured); ok {
unknownFields, preserved, err := d.validator.apply(u)
unknownFields, err := d.validator.apply(u)
if err != nil {
return nil, gvk, err
}
Expand All @@ -1249,16 +1244,6 @@ func (d schemaCoercingDecoder) Decode(data []byte, defaults *schema.GroupVersion
for i, unknownField := range unknownFields {
applyStrictErrs[i] = fmt.Errorf(`unknown field "%s"`, unknownField)
}
// when apply indicates that preserved is true,
// we need to return a special type of strict decoding error
// (PreservedDecodingError) so that the handler knows
// that it should warn instead of error in the strict case
// in order to maintain compatibility with how
// preserve unknown fields was handled prior to server-side
// validation.
if preserved {
return obj, gvk, runtime.NewPreservedDecodingError(append(decodingStrictErrs, applyStrictErrs...))
}
return obj, gvk, runtime.NewStrictDecodingError(append(decodingStrictErrs, applyStrictErrs...))

}
Expand All @@ -1282,7 +1267,7 @@ func (v schemaCoercingConverter) Convert(in, out, context interface{}) error {
}

if u, ok := out.(*unstructured.Unstructured); ok {
if _, _, err := v.validator.apply(u); err != nil {
if _, err := v.validator.apply(u); err != nil {
return err
}
}
Expand All @@ -1297,7 +1282,7 @@ func (v schemaCoercingConverter) ConvertToVersion(in runtime.Object, gv runtime.
}

if u, ok := out.(*unstructured.Unstructured); ok {
if _, _, err := v.validator.apply(u); err != nil {
if _, err := v.validator.apply(u); err != nil {
return nil, err
}
}
Expand Down Expand Up @@ -1325,58 +1310,36 @@ type unstructuredSchemaCoercer struct {
returnUnknownFieldPaths bool
}

func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) (unknownFieldPaths []string, preserved bool, err error) {
func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) (unknownFieldPaths []string, err error) {
// save implicit meta fields that don't have to be specified in the validation spec
kind, foundKind, err := unstructured.NestedString(u.UnstructuredContent(), "kind")
if err != nil {
return nil, false, err
return nil, err
}
apiVersion, foundApiVersion, err := unstructured.NestedString(u.UnstructuredContent(), "apiVersion")
if err != nil {
return nil, false, err
return nil, err
}
objectMeta, foundObjectMeta, err := schemaobjectmeta.GetObjectMeta(u.Object, v.dropInvalidMetadata)
if err != nil {
return nil, false, err
return nil, err
}

// compare group and kind because also other object like DeleteCollection options pass through here
gv, err := schema.ParseGroupVersion(apiVersion)
if err != nil {
return nil, false, err
return nil, err
}

if gv.Group == v.structuralSchemaGK.Group && kind == v.structuralSchemaGK.Kind {
// For strict and warning validations
// (i.e. when v.returnUnknwonFIeldPaths is true)
// we must prune twice (with and without pruneOnPreserve set)
// and compare the pruned fields.
//
// If the pruned fields are different it means that at least
// one sub-object has "x-kubernetes-preserve-unknown-fields"
// set to true and we should notify the caller via the `preserved`
// field so that they can warn instead of error.
if v.returnUnknownFieldPaths {
objCopy := u.DeepCopy()
unknownFieldPaths = structuralpruning.PruneWithOptions(objCopy.Object, v.structuralSchemas[gv.Version],
structuralpruning.PruneOptions{
IsResourceRoot: false,
PruneOnPreserveUnknownFields: true,
})
sort.Strings(unknownFieldPaths)
}
if !v.preserveUnknownFields {
// TODO: switch over pruning and coercing at the root to schemaobjectmeta.Coerce too
pruned := structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version], false)
unknownFieldPaths = structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version], false)
structuraldefaulting.PruneNonNullableNullsWithoutDefaults(u.Object, v.structuralSchemas[gv.Version])
sort.Strings(pruned)
preserved = !reflect.DeepEqual(pruned, unknownFieldPaths)
} else {
preserved = true
}

if err := schemaobjectmeta.Coerce(nil, u.Object, v.structuralSchemas[gv.Version], false, v.dropInvalidMetadata); err != nil {
return nil, false, err
return nil, err
}
// fixup missing generation in very old CRs
if v.repairGeneration && objectMeta.Generation == 0 {
Expand All @@ -1393,11 +1356,11 @@ func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) (unknown
}
if foundObjectMeta {
if err := schemaobjectmeta.SetObjectMeta(u.Object, objectMeta); err != nil {
return nil, false, err
return nil, err
}
}

return unknownFieldPaths, preserved, nil
return unknownFieldPaths, nil
}

// hasServedCRDVersion returns true if the given version is in the list of CRD's versions and the Served flag is set.
Expand Down
54 changes: 31 additions & 23 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ limitations under the License.
package pruning

import (
"fmt"
"strconv"
"strings"

structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
)
Expand All @@ -29,17 +29,11 @@ type PruneOptions struct {
// IsResourceRoot indicates whether
// this is the root of the object.
IsResourceRoot bool
// PruneOnPreserveUnknownFields
// indicates whether Prune should
// prune fields that are unknown
// even if s.XPreserveUnknownFields
// is set.
PruneOnPreserveUnknownFields bool
// parentPath collects the path that the pruning
// takes as it travers the object.
// It is used to report the full path to any unknown
// fields that the pruning encounters.
parentPath string
parentPath []string
}

// Prune removes object fields in obj which are not specified in s. It skips TypeMeta and ObjectMeta fields
Expand All @@ -57,13 +51,16 @@ func PruneWithOptions(obj interface{}, s *structuralschema.Structural, opts Prun
s = &clone
}
}
if opts.parentPath == nil {
opts.parentPath = []string{}
}
pruned := prune(obj, s, opts)
return pruned
}

// Prune calls into PruneWithOptions
func Prune(obj interface{}, s *structuralschema.Structural, isResourceRoot bool) []string {
return PruneWithOptions(obj, s, PruneOptions{true, false, ""})
return PruneWithOptions(obj, s, PruneOptions{isResourceRoot, []string{}})
}

var metaFields = map[string]bool{
Expand All @@ -72,16 +69,27 @@ var metaFields = map[string]bool{
"metadata": true,
}

func copyOptionsUpdatePath(opts PruneOptions, parent string) PruneOptions {
newPath := fmt.Sprintf("%s/%s", opts.parentPath, parent)
func getSeparator(path []string) string {
if len(path) > 0 {
return "."
}
return ""
}

func copyOptionsUpdatePath(opts PruneOptions, parent string, isIndex bool) PruneOptions {
var parentPath []string
if isIndex {
parentPath = append(opts.parentPath, "[", parent, "]")
} else {
parentPath = append(opts.parentPath, getSeparator(opts.parentPath), parent)
}
return PruneOptions{
PruneOnPreserveUnknownFields: opts.PruneOnPreserveUnknownFields,
parentPath: newPath,
parentPath: parentPath,
}
}

func prune(x interface{}, s *structuralschema.Structural, opts PruneOptions) []string {
if s != nil && s.XPreserveUnknownFields && !opts.PruneOnPreserveUnknownFields {
if s != nil && s.XPreserveUnknownFields {
return skipPrune(x, s, PruneOptions{
parentPath: opts.parentPath,
})
Expand All @@ -93,7 +101,7 @@ func prune(x interface{}, s *structuralschema.Structural, opts PruneOptions) []s
if s == nil {
for k := range x {
if !metaFields[k] {
pruned = append(pruned, fmt.Sprintf("%s/%s", opts.parentPath, k))
pruned = append(pruned, strings.Join(append(opts.parentPath, getSeparator(opts.parentPath), k), ""))
}
delete(x, k)
}
Expand All @@ -105,25 +113,25 @@ func prune(x interface{}, s *structuralschema.Structural, opts PruneOptions) []s
}
prop, ok := s.Properties[k]
if ok {
pruned = append(pruned, prune(v, &prop, copyOptionsUpdatePath(opts, k))...)
pruned = append(pruned, prune(v, &prop, copyOptionsUpdatePath(opts, k, false))...)
} else if s.AdditionalProperties != nil {
pruned = append(pruned, prune(v, s.AdditionalProperties.Structural, copyOptionsUpdatePath(opts, k))...)
pruned = append(pruned, prune(v, s.AdditionalProperties.Structural, copyOptionsUpdatePath(opts, k, false))...)
} else {
if !metaFields[k] {
pruned = append(pruned, fmt.Sprintf("%s/%s", opts.parentPath, k))
pruned = append(pruned, strings.Join(append(opts.parentPath, getSeparator(opts.parentPath), k), ""))
}
delete(x, k)
}
}
case []interface{}:
if s == nil {
for i, v := range x {
pruned = append(pruned, prune(v, nil, copyOptionsUpdatePath(opts, strconv.Itoa(i)))...)
pruned = append(pruned, prune(v, nil, copyOptionsUpdatePath(opts, strconv.Itoa(i), true))...)
}
return pruned
}
for i, v := range x {
pruned = append(pruned, prune(v, s.Items, copyOptionsUpdatePath(opts, strconv.Itoa(i)))...)
pruned = append(pruned, prune(v, s.Items, copyOptionsUpdatePath(opts, strconv.Itoa(i), true))...)
}
default:
// scalars, do nothing
Expand All @@ -145,18 +153,18 @@ func skipPrune(x interface{}, s *structuralschema.Structural, opts PruneOptions)
}
if prop, ok := s.Properties[k]; ok {
pruned = append(pruned, prune(v, &prop, PruneOptions{
parentPath: fmt.Sprintf("%s/%s", opts.parentPath, k),
parentPath: append(opts.parentPath, getSeparator(opts.parentPath), k),
})...)
} else if s.AdditionalProperties != nil {
pruned = append(pruned, prune(v, s.AdditionalProperties.Structural, PruneOptions{
parentPath: fmt.Sprintf("%s/%s", opts.parentPath, k),
parentPath: append(opts.parentPath, getSeparator(opts.parentPath), k),
})...)
}
}
case []interface{}:
for i, v := range x {
pruned = append(pruned, skipPrune(v, s.Items, PruneOptions{
parentPath: fmt.Sprintf("%s/%d", opts.parentPath, i),
parentPath: append(opts.parentPath, "[", strconv.Itoa(i), "]"),
})...)
}
default:
Expand Down
Loading

0 comments on commit 43a17e1

Please sign in to comment.