Skip to content

Commit fb41b7a

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#77653 from sttts/sttts-structural-schema-metadata
apiextensions: disallow metadata specs other than name and generateName
2 parents cf8e8e4 + d74a9a9 commit fb41b7a

File tree

3 files changed

+146
-12
lines changed

3 files changed

+146
-12
lines changed

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go‎

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ const (
5656
// - ... zero or more
5757
//
5858
// * every specified field or array in s is also specified outside of value validation.
59+
// * metadata at the root can only restrict the name and generateName, and not be specified at all in nested contexts.
5960
// * additionalProperties at the root is not allowed.
6061
func ValidateStructural(s *Structural, fldPath *field.Path) field.ErrorList {
6162
allErrs := field.ErrorList{}
@@ -106,7 +107,7 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path)
106107
}
107108
}
108109

109-
allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, fldPath)...)
110+
allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, lvl, fldPath)...)
110111

111112
if s.XEmbeddedResource && s.Type != "object" {
112113
if len(s.Type) == 0 {
@@ -129,6 +130,26 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path)
129130
allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), s.Type, "must be object at the root"))
130131
}
131132

133+
// restrict metadata schemas to name and generateName only
134+
if metadata, found := s.Properties["metadata"]; found && lvl == rootLevel {
135+
// metadata is a shallow copy. We can mutate it.
136+
_, foundName := metadata.Properties["name"]
137+
_, foundGenerateName := metadata.Properties["generateName"]
138+
if foundName && foundGenerateName && len(metadata.Properties) == 2 {
139+
metadata.Properties = nil
140+
} else if (foundName || foundGenerateName) && len(metadata.Properties) == 1 {
141+
metadata.Properties = nil
142+
}
143+
metadata.Type = ""
144+
if metadata.ValueValidation == nil {
145+
metadata.ValueValidation = &ValueValidation{}
146+
}
147+
if !reflect.DeepEqual(metadata, Structural{ValueValidation: &ValueValidation{}}) {
148+
// TODO: this is actually a field.Invalid error, but we cannot do JSON serialization of metadata here to get a proper message
149+
allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not specify anything other than name and generateName, but metadata is implicitly specified"))
150+
}
151+
}
152+
132153
if s.XEmbeddedResource && !s.XPreserveUnknownFields && s.Properties == nil {
133154
allErrs = append(allErrs, field.Required(fldPath.Child("properties"), "must not be empty if x-kubernetes-embedded-resource is true without x-kubernetes-preserve-unknown-fields"))
134155
}
@@ -171,7 +192,7 @@ func validateExtensions(x *Extensions, fldPath *field.Path) field.ErrorList {
171192
}
172193

173194
// validateValueValidation checks the value validation in a structural schema.
174-
func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, fldPath *field.Path) field.ErrorList {
195+
func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList {
175196
if v == nil {
176197
return nil
177198
}
@@ -180,7 +201,7 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf
180201

181202
if !skipAnyOf {
182203
for i := range v.AnyOf {
183-
allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, fldPath.Child("anyOf").Index(i))...)
204+
allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, lvl, fldPath.Child("anyOf").Index(i))...)
184205
}
185206
}
186207

@@ -189,31 +210,31 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf
189210
if skipFirstAllOfAnyOf && i == 0 {
190211
skipAnyOf = true
191212
}
192-
allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, fldPath.Child("allOf").Index(i))...)
213+
allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, lvl, fldPath.Child("allOf").Index(i))...)
193214
}
194215

195216
for i := range v.OneOf {
196-
allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, fldPath.Child("oneOf").Index(i))...)
217+
allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, lvl, fldPath.Child("oneOf").Index(i))...)
197218
}
198219

199-
allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, fldPath.Child("not"))...)
220+
allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, lvl, fldPath.Child("not"))...)
200221

201222
return allErrs
202223
}
203224

204225
// validateNestedValueValidation checks the nested value validation under a logic junctor in a structural schema.
205-
func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, fldPath *field.Path) field.ErrorList {
226+
func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList {
206227
if v == nil {
207228
return nil
208229
}
209230

210231
allErrs := field.ErrorList{}
211232

212-
allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, fldPath)...)
213-
allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, fldPath.Child("items"))...)
233+
allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, lvl, fldPath)...)
234+
allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, lvl, fldPath.Child("items"))...)
214235

215236
for k, fld := range v.Properties {
216-
allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fldPath.Child("properties").Key(k))...)
237+
allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fieldLevel, fldPath.Child("properties").Key(k))...)
217238
}
218239

219240
if len(v.ForbiddenGenerics.Type) > 0 {
@@ -245,5 +266,10 @@ func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllO
245266
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-int-or-string"), "must be false to be structural"))
246267
}
247268

269+
// forbid reasoning about metadata because it can lead to metadata restriction we don't want
270+
if _, found := v.Properties["metadata"]; found {
271+
allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not be specified in a nested context"))
272+
}
273+
248274
return allErrs
249275
}

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ func TestValidateNestedValueValidationComplete(t *testing.T) {
4949
i := rand.Intn(x.NumField())
5050
fuzzer.Fuzz(x.Field(i).Addr().Interface())
5151

52-
errs := validateNestedValueValidation(vv, false, false, nil)
52+
errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil)
5353
if len(errs) == 0 && !reflect.DeepEqual(vv.ForbiddenGenerics, Generic{}) {
5454
t.Errorf("expected ForbiddenGenerics validation errors for: %#v", vv)
5555
}
@@ -63,7 +63,7 @@ func TestValidateNestedValueValidationComplete(t *testing.T) {
6363
i := rand.Intn(x.NumField())
6464
fuzzer.Fuzz(x.Field(i).Addr().Interface())
6565

66-
errs := validateNestedValueValidation(vv, false, false, nil)
66+
errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil)
6767
if len(errs) == 0 && !reflect.DeepEqual(vv.ForbiddenExtensions, Extensions{}) {
6868
t.Errorf("expected ForbiddenExtensions validation errors for: %#v", vv)
6969
}

‎staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1222,6 +1222,114 @@ not:
12221222
"spec.version[v1].schema.openAPIV3Schema.properties[d]: Required value: because it is defined in spec.version[v1].schema.openAPIV3Schema.not.properties[d]",
12231223
},
12241224
},
1225+
{
1226+
desc: "metadata with non-properties",
1227+
globalSchema: `
1228+
type: object
1229+
properties:
1230+
metadata:
1231+
minimum: 42.0
1232+
`,
1233+
expectedViolations: []string{
1234+
"spec.validation.openAPIV3Schema.properties[metadata]: Forbidden: must not specify anything other than name and generateName, but metadata is implicitly specified",
1235+
"spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields",
1236+
},
1237+
},
1238+
{
1239+
desc: "metadata with other properties",
1240+
globalSchema: `
1241+
type: object
1242+
properties:
1243+
metadata:
1244+
properties:
1245+
name:
1246+
pattern: "^[a-z]+$"
1247+
labels:
1248+
type: object
1249+
maxLength: 4
1250+
`,
1251+
expectedViolations: []string{
1252+
"spec.validation.openAPIV3Schema.properties[metadata]: Forbidden: must not specify anything other than name and generateName, but metadata is implicitly specified",
1253+
"spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields",
1254+
"spec.validation.openAPIV3Schema.properties[metadata].properties[name].type: Required value: must not be empty for specified object fields",
1255+
},
1256+
},
1257+
{
1258+
desc: "metadata with name property",
1259+
globalSchema: `
1260+
type: object
1261+
properties:
1262+
metadata:
1263+
type: object
1264+
properties:
1265+
name:
1266+
type: string
1267+
pattern: "^[a-z]+$"
1268+
`,
1269+
expectedViolations: []string{},
1270+
},
1271+
{
1272+
desc: "metadata with generateName property",
1273+
globalSchema: `
1274+
type: object
1275+
properties:
1276+
metadata:
1277+
type: object
1278+
properties:
1279+
generateName:
1280+
type: string
1281+
pattern: "^[a-z]+$"
1282+
`,
1283+
expectedViolations: []string{},
1284+
},
1285+
{
1286+
desc: "metadata with name and generateName property",
1287+
globalSchema: `
1288+
type: object
1289+
properties:
1290+
metadata:
1291+
type: object
1292+
properties:
1293+
name:
1294+
type: string
1295+
pattern: "^[a-z]+$"
1296+
generateName:
1297+
type: string
1298+
pattern: "^[a-z]+$"
1299+
`,
1300+
expectedViolations: []string{},
1301+
},
1302+
{
1303+
desc: "metadata under junctors",
1304+
globalSchema: `
1305+
type: object
1306+
properties:
1307+
metadata:
1308+
type: object
1309+
properties:
1310+
name:
1311+
type: string
1312+
pattern: "^[a-z]+$"
1313+
allOf:
1314+
- properties:
1315+
metadata: {}
1316+
anyOf:
1317+
- properties:
1318+
metadata: {}
1319+
oneOf:
1320+
- properties:
1321+
metadata: {}
1322+
not:
1323+
properties:
1324+
metadata: {}
1325+
`,
1326+
expectedViolations: []string{
1327+
"spec.validation.openAPIV3Schema.anyOf[0].properties[metadata]: Forbidden: must not be specified in a nested context",
1328+
"spec.validation.openAPIV3Schema.allOf[0].properties[metadata]: Forbidden: must not be specified in a nested context",
1329+
"spec.validation.openAPIV3Schema.oneOf[0].properties[metadata]: Forbidden: must not be specified in a nested context",
1330+
"spec.validation.openAPIV3Schema.not.properties[metadata]: Forbidden: must not be specified in a nested context",
1331+
},
1332+
},
12251333
}
12261334

12271335
for i := range tests {

0 commit comments

Comments
 (0)