-
Notifications
You must be signed in to change notification settings - Fork 40.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CustomResources: in OpenAPI spec allow additionalProperties without properties #62333
CustomResources: in OpenAPI spec allow additionalProperties without properties #62333
Conversation
@sttts: GitHub didn't allow me to request PR reviews from the following users: ayushpateria. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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. |
@@ -45,6 +45,7 @@ func (s *JSONSchemaPropsOrBool) UnmarshalJSON(data []byte) error { | |||
if err := json.Unmarshal(data, &sch); err != nil { | |||
return err | |||
} | |||
nw.Allows = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mistake saved us 🎉
Because for additionalProperties: {...}
we forgot to set allows: true
(and we reject Allows == false
in general), it was not possible to define a CRD with anything other than additionalProperties: true
.
ebe82de
to
a524273
Compare
a524273
to
e2e9c23
Compare
if schema.AdditionalProperties.Allows == false { | ||
allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties cannot be set to false")) | ||
if len(schema.Properties) != 0 { | ||
if schema.AdditionalProperties.Allows == false || schema.AdditionalProperties.Schema != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this extra check necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If allowed= true and schema = nil, that will cause panic. I can see that it is not possible to generate such object when unmarshaling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the implicit default of additionalProperties
is true
. So schema.AdditionalProperties.Allows == true
must be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, there is not issue with panic when allowed= true and schema = nil.
351a704
to
5cc2ce8
Compare
/lgtm |
…roperties This allows to specify map[string]Interface{} non-object types, e.g. map[string]string for selectors.
5cc2ce8
to
b9df44e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ash2k, sttts, tamalsaha 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 |
Automatic merge from submit-queue (batch tested with PRs 59027, 62333, 57661, 62086, 61584). If you want to cherry-pick this change to another branch, please follow the instructions here. |
This implements @ayushpateria's idea #59485 (comment).
With this PR it becomes possible to specify
map[string]Interface{}
non-object types, e.g.map[string]string
for selectors. On the other hand, "normal" objects useproperties
, mutually exclusively toadditionalProperties
. This way we avoid a conflict with Kubernetes API conventions that unknown objects fields are dropped.Fixes #59485