Published swagger should not include sibling elements alongside a $ref

[Jefftree]

What happened?

Per JSON Reference documentation,

Any members other than "$ref" in a JSON Reference object SHALL be ignored.

In the first couple of lines in our swagger.json, we publish a spec that contains a schema with $ref and also an additional description field which is disallowed.

Eg:

"clientConfig": {
  "$ref": "#/definitions/io.k8s.api.admissionregistration.v1.WebhookClientConfig",
  "description": "ClientConfig defines how to communicate with the hook. Required"
},

The $ref field is only permitted to have sibling elements after JSON Schema draft 2019-09 which corresponds to OpenAPI 3.1.

The description field (or any field alongside a $ref) should not be published for OpenAPI 2.0 and 3.0 as it is an invalid field. Due to a bug in the gnostic openapiv2 library, the sibling fields are not dropped when converting our openapi to proto in v2, but are dropped in the openapiv3 library. This causes inconsistent behavior when round tripping our specs to proto and back.

I'm not exactly sure if anything (either k/k or downstream OpenAPI clients) depend on this bug and would like to have a discussion first before providing a fix.
/sig api-machinery

What did you expect to happen?

See above

How can we reproduce it (as minimally and precisely as possible)?

See above

Anything else we need to know?

No response

Kubernetes version

1.23

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[apelisse]

/assign @Jefftree
/triage accepted

[Jefftree]

Summarizing the discussion with @apelisse and @sttts:

This "bug" is a feature we rely on in kubectl explain to get information about fields that are references. Removing it will greatly reduce the amount of data we can provide in a kubectl explain and is undesirable even if it is a violation of the schema. It is important because we have a lot of properties that require an additional description in additional to a $ref. For example, these are the descriptions we would lose from a kubectl explain pod.spec:

   affinity     <Object>
     If specified, the pod's scheduling constraints

   securityContext      <Object>
     SecurityContext holds pod-level security attributes and common container
     settings. Optional: Defaults to empty. See type description for default
     values of each field.

Every field that references another struct (and isn't a map/list) would have their description stripped to be compliant with the schema. Some are self-explanatory fields but there are certain fields that benefit from the description because they describe specifically how the embed resource is used specifically embed in the selected resource rather than in general.

Solutions:

1. Strip sibling fields to $ref from OpenAPI

   • Undesirable as kubectl explain relies on these fields, see above

2. Use a workaround suggested in the OpenAPI issues (linked below) to move $ref into a single element part of allOf

   type: {
    allOf: {
      $ref: x,
     },
     description: "",
   }
   

   • This probably solves our problem in the cleanest way but makes our slightly OpenAPI messy.
   • Current selected approach for OpenAPI V3 only

3. Keep the "bug" as is for OpenAPI V2 (we haven't received any complaints) and work on solutions for OpenAPI v3.

   3.1. Fork OpenAPI v3 protobuf library and change the structs so that $ref fields are allowed

   • This is a big change since we'll be changing the proto message. There is also cost to maintaining the version and keeping it updated with upstream.

   3.2 Upgrade to OpenAPI v3.1 which allows the refs

   • There are some differences between the OpenAPI v3.0 and 3.1 spec which makes the transition non-trivial (but still easier than 2.0 to 3.0)

Ref:
OAI/OpenAPI-Specification#556
OAI/OpenAPI-Specification#1514

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[apelisse]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[Jefftree]

/close

[k8s-ci-robot]

@Jefftree: Closing this issue.

In response to this:

/close

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.

[luxas]

From what I understand, this issue was solved by kubernetes/kube-openapi#298

Opened #126731 to reactivate the integration test that was disabled because of this issue