Setting PreserverUnknownFields marker on both type and field generates an invalid schema

[eddycharly]

When setting the kubebuilder:pruning:PreserveUnknownFields on a struct and a field using that struct, the generated schema is not valid.

For example:

// +kubebuilder:pruning:PreserveUnknownFields
type Preserved struct {
	ConcreteField string                 `json:"concreteField"`
	Rest          map[string]interface{} `json:"-"`
}

type Spec struct {
	// +kubebuilder:pruning:PreserveUnknownFields
	Preserved Preserved `json:"preserved"`
}

// +kubebuilder:object:root=true
type Test struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec Spec `json:"spec,omitempty"`
}

Will generate the following schema:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: (devel)
  creationTimestamp: null
  name: tests.testdata.kubebuilder.io
spec:
  group: testdata.kubebuilder.io
  names:
    kind: Test
    listKind: TestList
    plural: tests
    singular: test
  scope: Namespaced
  versions:
  - name: v1
    schema:
      openAPIV3Schema:
        properties:
          apiVersion:
            description: 'APIVersion defines the versioned schema of this representation
              of an object. Servers should convert recognized schemas to the latest
              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
            type: string
          kind:
            description: 'Kind is a string value representing the REST resource this
              object represents. Servers may infer this from the endpoint the client
              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
            type: string
          metadata:
            type: object
          spec:
            properties:
              preserved:
                allOf:
                - x-kubernetes-preserve-unknown-fields: true
                - x-kubernetes-preserve-unknown-fields: true
                properties:
                  concreteField:
                    type: string
                required:
                - concreteField
                type: object
            required:
            - preserved
            type: object
        type: object
    served: true
    storage: true

The problematic part being:

             preserved:
                allOf:
                - x-kubernetes-preserve-unknown-fields: true
                - x-kubernetes-preserve-unknown-fields: true

From what I observed, this happens when flattening the schema.
I managed to fix the bug by adding a case for XPreserveUnknownFields here to prevent hoisting:

controller-tools/pkg/crd/flatten.go

Lines 105 to 156 in f89071a

	switch fieldName {
	case "Properties":
	// merge if possible, use all of otherwise
	srcMap := srcInt.(map[string]apiext.JSONSchemaProps)
	dstMap := dstInt.(map[string]apiext.JSONSchemaProps)
	for k, v := range srcMap {
	dstProp, exists := dstMap[k]
	if !exists {
	dstMap[k] = v
	continue
	}
	flattenAllOfInto(&dstProp, v, errRec)
	dstMap[k] = dstProp
	}
	case "Required":
	// merge
	dstField.Set(reflect.AppendSlice(dstField, srcField))
	case "Type":
	if srcInt != dstInt {
	// TODO(directxman12): figure out how to attach this back to a useful point in the Go source or in the schema
	errRec.AddError(fmt.Errorf("conflicting types in allOf branches in schema: %s vs %s", dstInt, srcInt))
	}
	// keep the destination value, for now
	// TODO(directxman12): Default -- use field?
	// TODO(directxman12):
	// - Dependencies: if field x is present, then either schema validates or all props are present
	// - AdditionalItems: like AdditionalProperties
	// - Definitions: common named validation sets that can be references (merge, bail if duplicate)
	case "AdditionalProperties":
	// as of the time of writing, `allows: false` is not allowed, so we don't have to handle it
	srcProps := srcInt.(*apiext.JSONSchemaPropsOrBool)
	if srcProps.Schema == nil {
	// nothing to merge
	continue
	}
	dstProps := dstInt.(*apiext.JSONSchemaPropsOrBool)
	if dstProps.Schema == nil {
	dstProps.Schema = &apiext.JSONSchemaProps{}
	}
	flattenAllOfInto(dstProps.Schema, *srcProps.Schema, errRec)
	// NB(directxman12): no need to explicitly handle nullable -- false is considered to be the zero value
	// TODO(directxman12): src isn't necessarily the field value -- it's just the most recent allOf entry
	default:
	// hoist into allOf...
	hoisted = true
	srcRemVal.Field(i).Set(srcField)
	dstRemVal.Field(i).Set(dstField)
	// ...and clear the original
	dstField.Set(zeroVal)
	}

While this fixed the bug, I'm not too sure about the fix itself.

I can open a PR but would like to confirm it's a bug first and eventually if the fix I tried makes sense.

cc @akutz @sbueringer (per Alvaro's recommendation).

[sbueringer]

Looks definitely weird, but I don't know what the expected behavior of controller-gen is if markers are set twice.

In ClusterAPI we use +kubebuilder:pruning:PreserveUnknownFields only on the field which is why we didn't had this issue.

It seems logical to deduplicate when the field and struct markers are identical. I assume in general the behavior is to just use both the markers from the struct and from the field when generating the schema? (so currently it works in general if there are no duplicate markers?)

[eddycharly]

I'm not very familiar with the code but it looks weird to me too.

From what I understand, every struct is first processed separately, then root types are flattened to produce the CRD schema by walking the struct definitions.
The flattening process has some logic to merge markers coming from the source schema definition into the target schema (this is the code I linked above). In this logic there's nothing in the code to take care of +kubebuilder:pruning:PreserveUnknownFields and this ends up in the general case where things get injected in the allOf part of the schema.

TBH I don't see a good reason to not merge +kubebuilder:pruning:PreserveUnknownFields.
Shall I open a PR with the fix I tried locally ?

[sbueringer]

Let's see what @alvaroaleman and/or @akutz think. I'm just more like an occasional contributor and user :).

[eddycharly]

Until then I created a WIP PR with the changes I did locally #689 (hopefully it will help the discussion around the issue and the eventual fix)