fix: remove refs issue

[eddycharly]

This PR fixes a removeRefs issue.

It looks like we need to reset extensions before comparing with an empty schema.

Fixes #54

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eddycharly
Once this PR has been reviewed and has the lgtm label, please assign richabanker for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alexzielenski]

Looks like in this bug's case the schema for the field that's being resolve looks like:

"strategy": {
    "allOf": [
      {
        "$ref": "#/components/schemas/io.k8s.api.apps.v1.DeploymentStrategy"
      }
    ],
    "default": {},
    "description": "The deployment strategy to use to replace existing pods with new ones.",
    "x-kubernetes-patch-strategy": "retainKeys"
},

This bug can happen if there is a field-level override of the referred schema. I am curious if other cases like this exist. The proposed solution would lose the information that the field uses retainKeys. I can imagine other field-level extensions useful for validation that would be lost here like x-kubernetes-validations or x-kuberenetes-list-type. The original code also erroneous strips the Default value, which is another bug.

This part of the code is completely a hack to satisfy the requirements of structural schema which does not like new properties inside allOf (structural schema is required to run validators). Upstream we have a loose plan to add validation that does not require the structural schema and operates instead on kube-openapi for both CRs and native types. This goal is long away, so we have this hack.

xref: kubernetes/kubernetes#106387
xref: kubernetes/kube-openapi#298

I'm thinking if we see a schema that meets the following conditions:

1. len(s.allOf) = 1
2. len(s.allOf[0].Ref) > 0

Then it should just be interpreted as a ref without any additional checks. We can be confident in this approach since k8s apiserver is the only source of $refs, and it always wraps them in a single element allOf.

We should create a copy of the referred schema, and set all non-empty properties of the field's override schema onto the referred schema.

So in the case of strategy above:

1. vCopy := removeRefs(s.allOf[0].Ref)
2. vCopy.Default = s.Default
3. vCopy.Description = s.Description
4. for k, v in range s.Extensions: vCopy.Extensions[k] = v (may need to copy vCopy.Extensions too to avoid changing the original map)
5. All other non-empty properties of the field are merged into vCopy

[eddycharly]

Thanks @alexzielenski for the detailed explanation. I will try to implement what you suggested today.

[eddycharly]

@alexzielenski i'm not very familiar with this code, I tried to follow your explanations but tests are failing, would you mind giving a look please ?

[alexzielenski]

probably want to copy all nonzero properties and not just Default, Description, Extensions. Not sure if there is a clean go-ey way to do that

[alexzielenski]

If you base on #60 I think some of the issues can go away. That would just leave the last comment from my last review to resolve

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[alexzielenski]

/close

I think all the changes from this PR were merged in #67 and #60

[k8s-ci-robot]

@alexzielenski: Closed this PR.

In response to this:

/close

I think all the changes from this PR were merged in #67 and #60

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.