Skip to content

Commit d74a9a9

Browse files
stttssttts
committed
apiextensions: disallow metadata specs other than name and generateName
1 parent b9ccdd2 commit d74a9a9

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
@@ -55,6 +55,7 @@ const (
5555
// - ... zero or more
5656
//
5757
// * every specified field or array in s is also specified outside of value validation.
58+
// * metadata at the root can only restrict the name and generateName, and not be specified at all in nested contexts.
5859
// * additionalProperties at the root is not allowed.
5960
func ValidateStructural(s *Structural, fldPath *field.Path) field.ErrorList {
6061
allErrs := field.ErrorList{}
@@ -99,7 +100,7 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path)
99100
}
100101
}
101102

102-
allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, fldPath)...)
103+
allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, lvl, fldPath)...)
103104

104105
if s.XEmbeddedResource && s.Type != "object" {
105106
if len(s.Type) == 0 {
@@ -122,6 +123,26 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path)
122123
allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), s.Type, "must be object at the root"))
123124
}
124125

126+
// restrict metadata schemas to name and generateName only
127+
if metadata, found := s.Properties["metadata"]; found && lvl == rootLevel {
128+
// metadata is a shallow copy. We can mutate it.
129+
_, foundName := metadata.Properties["name"]
130+
_, foundGenerateName := metadata.Properties["generateName"]
131+
if foundName && foundGenerateName && len(metadata.Properties) == 2 {
132+
metadata.Properties = nil
133+
} else if (foundName || foundGenerateName) && len(metadata.Properties) == 1 {
134+
metadata.Properties = nil
135+
}
136+
metadata.Type = ""
137+
if metadata.ValueValidation == nil {
138+
metadata.ValueValidation = &ValueValidation{}
139+
}
140+
if !reflect.DeepEqual(metadata, Structural{ValueValidation: &ValueValidation{}}) {
141+
// TODO: this is actually a field.Invalid error, but we cannot do JSON serialization of metadata here to get a proper message
142+
allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not specify anything other than name and generateName, but metadata is implicitly specified"))
143+
}
144+
}
145+
125146
if s.XEmbeddedResource && !s.XPreserveUnknownFields && s.Properties == nil {
126147
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"))
127148
}
@@ -164,7 +185,7 @@ func validateExtensions(x *Extensions, fldPath *field.Path) field.ErrorList {
164185
}
165186

166187
// validateValueValidation checks the value validation in a structural schema.
167-
func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, fldPath *field.Path) field.ErrorList {
188+
func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList {
168189
if v == nil {
169190
return nil
170191
}
@@ -173,7 +194,7 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf
173194

174195
if !skipAnyOf {
175196
for i := range v.AnyOf {
176-
allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, fldPath.Child("anyOf").Index(i))...)
197+
allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, lvl, fldPath.Child("anyOf").Index(i))...)
177198
}
178199
}
179200

@@ -182,31 +203,31 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf
182203
if skipFirstAllOfAnyOf && i == 0 {
183204
skipAnyOf = true
184205
}
185-
allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, fldPath.Child("allOf").Index(i))...)
206+
allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, lvl, fldPath.Child("allOf").Index(i))...)
186207
}
187208

188209
for i := range v.OneOf {
189-
allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, fldPath.Child("oneOf").Index(i))...)
210+
allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, lvl, fldPath.Child("oneOf").Index(i))...)
190211
}
191212

192-
allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, fldPath.Child("not"))...)
213+
allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, lvl, fldPath.Child("not"))...)
193214

194215
return allErrs
195216
}
196217

197218
// validateNestedValueValidation checks the nested value validation under a logic junctor in a structural schema.
198-
func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, fldPath *field.Path) field.ErrorList {
219+
func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList {
199220
if v == nil {
200221
return nil
201222
}
202223

203224
allErrs := field.ErrorList{}
204225

205-
allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, fldPath)...)
206-
allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, fldPath.Child("items"))...)
226+
allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, lvl, fldPath)...)
227+
allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, lvl, fldPath.Child("items"))...)
207228

208229
for k, fld := range v.Properties {
209-
allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fldPath.Child("properties").Key(k))...)
230+
allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fieldLevel, fldPath.Child("properties").Key(k))...)
210231
}
211232

212233
if len(v.ForbiddenGenerics.Type) > 0 {
@@ -238,5 +259,10 @@ func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllO
238259
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-int-or-string"), "must be false to be structural"))
239260
}
240261

262+
// forbid reasoning about metadata because it can lead to metadata restriction we don't want
263+
if _, found := v.Properties["metadata"]; found {
264+
allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not be specified in a nested context"))
265+
}
266+
241267
return allErrs
242268
}

‎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)