Evaluate impact of x-kubernetes-preserve-unknown-fields: true fields introduced by 1.23 prior to 1.23.3

[ritazh]

What steps did you take and what happened:
[A clear and concise description of what the bug is.]

Does this impact Gatekeeper in anyway? seems it only impacts array types https://groups.google.com/u/0/a/kubernetes.io/g/dev/c/Xl1sm-CItaY?pli=1

Relevant code to evaluate:
https://github.com/open-policy-agent/frameworks/blob/352a1b3fc276fb2cad3629945a91cdc1ae2f18ec/constraint/pkg/schema/transform.go#L34-L44

What did you expect to happen:

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

• Gatekeeper version:
• Kubernetes version: (use kubectl version):

[julianKatz]

ABSTRACT:

We are susceptible to this bug, as there are ways of creating a JSONSchema where the user hasn't provided sufficient information for us to intuit that they mean to specify a list. In those cases, the users list information is dropped when they specify it in a constraint, causing a bug.

If we have enough information (type: array, a non-null items: schema), we fill in the other information necessary to prevent the bug, namely we add x-kubernetes-preserve-unknown-fields: true to the items schema, or add an items schema that includes that if an items schema is missing.

TESTING

To test this I created a kind cluster at version 1.23.0.

I then installed g8r release-3.7 using a prebuilt image.

Test Case 1: No specific type... even though it's supposed to be an array

validation:
  openAPIV3Schema:
    type: object
    properties:
      labels:
        # type: array
        description: >-
          A list of labels and values the object must specify.

B/c we don't know what type labels is meant to be, we just add x-kub... true to it. This doesn't add an items: schema, b/c we have no way of knowing that labels is meant to be a list.

parameters:
  properties:
    labels:
      description: A list of labels and values the object must specify.
      x-kubernetes-preserve-unknown-fields: true
  type: object
  x-kubernetes-preserve-unknown-fields: true

Thus, when we add a constraint where the labels is a list, that information is dropped as indicated in the bug.

PARAMETERS APPLIED:

...
spec:
  ...
  parameters:
    labels:
      - key: owner
        allowedRegex: "^[a-zA-Z]+.agilebank.demo$"

PERSISTED:

spec:
  ...
  parameters:
    labels:
    - {}

Test Case 2: type: array with no items: or items schema

validation:
  openAPIV3Schema:
    type: object
    properties:
      labels:
        type: array
        description: >-
          A list of labels and values the object must specify.

Our logic seems type:array and knows that there can be an unbounded items schema. It thus creates the following the in the CRD for the new constraint type:

parameters:
  properties:
    labels:
      description: A list of labels and values the object must specify.
      items:
        x-kubernetes-preserve-unknown-fields: true # This prevents the bug, so we're ok in this case
      type: array
  type: object
  x-kubernetes-preserve-unknown-fields: true

That successfully maintains the parameters section when the constraint is applied:

PERSISTED:

parameters:
  labels:
  - allowedRegex: ^[a-zA-Z]+.agilebank.demo$
    key: owner
  message: All namespaces must have an `owner` label that points to your company
    username

Test Case 3: no type: array. items:, but no schema.

template:

validation:
  openAPIV3Schema:
    type: object
    properties:
      labels:
        # type: array
        description: >-
          A list of labels and values the object must specify.
        items:

Because items: is nil, we can't tell it's there. This essentially makes this the same as Test case 1, and we get:

parameters:
  properties:
    labels:
      description: A list of labels and values the object must specify.
      x-kubernetes-preserve-unknown-fields: true
  type: object
  x-kubernetes-preserve-unknown-fields: true

in the constraint kind CRD. This creates the same bug as in test case 1.

Test Case 4: no type: array. items: with schema

template:

validation:
  openAPIV3Schema:
    type: object
    properties:
      labels:
        # type: array
        description: >-
          A list of labels and values the object must specify.
        items:
          type: object

crd:

parameters:
  properties:
    labels:
      description: A list of labels and values the object must specify.
      items:
        type: object
        x-kubernetes-preserve-unknown-fields: true # prevents the bug
      x-kubernetes-preserve-unknown-fields: true
  type: object
  x-kubernetes-preserve-unknown-fields: true

This fixes the bug.

[julianKatz]

@ritazh, given our small but real exposure to this bug, how would you suggest we move forward?

Do we have a place for putting out something like a Public Service Announcement? I think we can recommend people to use legacySchema: false in their templates, which will require them to be specific with types. That should prevent them from experiencing the bug.

[sozercan]

Should we pin this issue?

[ritazh]

@julianKatz Thank you so much for verifying!

I think we can recommend people to use legacySchema: false in their templates, which will require them to be specific with types. That should prevent them from experiencing the bug.

Can you pls provide an example of the fix with one of the test cases above?

[maxsmythe]

set spec.crd.spec.validation.legacySchema = false

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.