Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes crd per-version validation field path #84560

Merged
merged 1 commit into from Nov 7, 2019
Merged

Fixes crd per-version validation field path #84560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...extensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go
View file
Open in desktop
Expand Up @@ -100,7 +100,7 @@ func calculateCondition(in *apiextensions.CustomResourceDefinition) *apiextensio
allErrs = append(allErrs, schema.ValidateStructural(pth, s)...)
}

for _, v := range in.Spec.Versions {
for i, v := range in.Spec.Versions {
if v.Schema == nil || v.Schema.OpenAPIV3Schema == nil {
continue
}
Expand All @@ -112,7 +112,7 @@ func calculateCondition(in *apiextensions.CustomResourceDefinition) *apiextensio
return cond
}

pth := field.NewPath("spec", "version").Key(v.Name).Child("schema", "openAPIV3Schema")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an associative list? if so I think keyed by name is at least a somewhat acceptable way to index it. (version->versions seems correct though.)

But how to avoid fighting with a prior apiserver during an upgrade?

I'm not a fan of our current mechanism at all, it seems super brittle. Changes like this one shouldn't be so fraught with danger :/

Copy link
Member

@liggitt liggitt Oct 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

field path is intended to output the jsonpath to the field, so this should be an index

But how to avoid fighting with a prior apiserver during an upgrade?

we already protected against that, I just wanted a sanity check the mechanism was working

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

field path is intended to output the jsonpath to the field, so this should be an index

Sounds like something that could (and therefore should) be tested...

pth := field.NewPath("spec", "versions").Index(i).Child("schema", "openAPIV3Schema")

allErrs = append(allErrs, schema.ValidateStructural(pth, s)...)
}
Expand Down
12 changes: 6 additions & 6 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go
View file
Open in desktop
Expand Up @@ -1233,8 +1233,8 @@ properties:
type: string
`,
expectedViolations: []string{
"spec.version[v1beta1].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields",
"spec.version[v1beta1].schema.openAPIV3Schema.properties[b]: Required value: because it is defined in spec.version[v1beta1].schema.openAPIV3Schema.not.properties[b]",
"spec.versions[0].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields",
"spec.versions[0].schema.openAPIV3Schema.properties[b]: Required value: because it is defined in spec.versions[0].schema.openAPIV3Schema.not.properties[b]",
},
},
{
Expand All @@ -1256,10 +1256,10 @@ not:
d: {}
`,
expectedViolations: []string{
"spec.version[v1beta1].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields",
"spec.version[v1beta1].schema.openAPIV3Schema.properties[b]: Required value: because it is defined in spec.version[v1beta1].schema.openAPIV3Schema.not.properties[b]",
"spec.version[v1].schema.openAPIV3Schema.properties[c].type: Required value: must not be empty for specified object fields",
"spec.version[v1].schema.openAPIV3Schema.properties[d]: Required value: because it is defined in spec.version[v1].schema.openAPIV3Schema.not.properties[d]",
"spec.versions[0].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields",
"spec.versions[0].schema.openAPIV3Schema.properties[b]: Required value: because it is defined in spec.versions[0].schema.openAPIV3Schema.not.properties[b]",
"spec.versions[1].schema.openAPIV3Schema.properties[c].type: Required value: must not be empty for specified object fields",
"spec.versions[1].schema.openAPIV3Schema.properties[d]: Required value: because it is defined in spec.versions[1].schema.openAPIV3Schema.not.properties[d]",
},
},
{
Expand Down