Enable OpenAPI formats immediately at release version #132531
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| effectiveVer = DefaultBuildEffectiveVersion() | ||
| } | ||
| return effectiveVer | ||
| } |
There was a problem hiding this comment.
@siyuanfoundation @Jefftree Are you okay with introducing this function? We do this lookup quite a few places..
There was a problem hiding this comment.
An alternative I considered was to make this a receiver function of the registry, but we only have the build tag fallback for the kube component, so it didn't seem to add any value.
|
I've also opened #132532 to try to reduce the problems that can be caused by introducing new formats. |
|
I'd give this PR its own changelog entry. |
|
/triage accepted |
Added. |
| @@ -94,22 +94,23 @@ func (s basicSchemaValidator) ValidateUpdate(new, old interface{}, options ...Va | |||
|
|
|||
| // NewSchemaValidator creates an openapi schema validator for the given CRD validation using environment.DefaultCompatibilityVersion(). | |||
| func NewSchemaValidator(customResourceValidation *apiextensions.JSONSchemaProps) (SchemaValidator, *spec.Schema, error) { | |||
| return NewSchemaValidatorForVersion(customResourceValidation, environment.DefaultCompatibilityVersion()) | |||
| return NewSchemaValidatorForVersion(customResourceValidation, compatibility.KubeComponentEffectiveVersion().EmulationVersion()) | |||
There was a problem hiding this comment.
Need to change the godoc, at least
What are the implications of changing the version this uses by default? It looks like this is used when writing CRDs to decide if all the validation stuff in the CRD spec is valid to persist. Don't we have to construct that with our compatibility version to make sure we don't persist a CRD a skewed older API server would choke on?
| // If the kube component is registered in the DefaultComponentGlobalsRegistry, the | ||
| // effective version from the registry is returned. Otherwise, the DefaultBuildEffectiveVersion | ||
| // is returned. | ||
| func KubeComponentEffectiveVersion() basecompatibility.EffectiveVersion { |
There was a problem hiding this comment.
Since this is a global, it is possible to call this before the parsing / initialization of the effective version has been done, right?
We tried to add tests into the cel initialization paths to detect when things initialized based on versions or gates prematurely, but moving this into a global used by things other than cel worries me a bit. How can we protect against accidentally using this in an initialization path too early?
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The JSON Schema specification states that unrecognized formats should be allowed, but ignored, by implementations. Kubernetes honors this and allows
unrecognized formats in CRD OpenAPI schemas. The implication here is that formats like the newly introduced
k8s-long-namecan be put in CRD schemas, even in very old Kubernetes releases. The apiserver will allow the unrecognized formats to be written into the CRD and will ignore them (we don't publish them into discovery OpenAPI endpoint, however).Because of this, it is possible for us to enable newly supported formats in the same release the formats are introduced, eliminating a 1-release delay. This PR proposes making this change. That is, instead using
introducedVersion>=minCompatibilityVersion, we can useintroducedVersion>=emulationVersion.AFAIK, this change has no impact on downgrade. No matter what version we start enforcing a format, a downgrade to the previous version will cause the format to be ignored. Ratcheting ensures that unchanged fields will be treated as valid in the case of a downgrade/modify-field-to-invalid-value-according-to-format/upgrade.
There is also one reason this change might actually be SAFER: Once we document the existence of a format, it is likely to be put to use. With this change, we shorten the time window where the format is known the community but not enforced.
Does this PR introduce a user-facing change?
/sig api-machinery