Skip to content

Commit 59184da

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#1019 from a-hilaly/fix/map-schema-blindspot
fix(graph): handle additionalProperties in schema lookups
2 parents 8255710 + ce492cb commit 59184da

File tree

3 files changed

+268
-28
lines changed

3 files changed

+268
-28
lines changed

‎pkg/graph/builder.go‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -879,7 +879,7 @@ func resolveSchemaAndTypeName(segments []fieldpath.Segment, rootSchema *spec.Sch
879879
for _, seg := range segments {
880880
if seg.Name != "" {
881881
typeName = typeName + "." + seg.Name
882-
currentSchema = lookupSchemaAtPath(currentSchema, seg.Name)
882+
currentSchema = lookupSchemaAtField(currentSchema, seg.Name)
883883
if currentSchema == nil {
884884
return nil, "", fmt.Errorf("field %q not found in schema", seg.Name)
885885
}
@@ -934,42 +934,30 @@ func getCelTypeFromSchema(schema *spec.Schema, typeName string) *cel.Type {
934934
return declType.CelType()
935935
}
936936

937-
// lookupSchemaAtPath traverses a schema following a field path and returns the schema at that location
938-
func lookupSchemaAtPath(schema *spec.Schema, path string) *spec.Schema {
939-
if path == "" {
937+
// lookupSchemaAtField resolves a single field name within a schema.
938+
func lookupSchemaAtField(schema *spec.Schema, field string) *spec.Schema {
939+
if schema == nil || field == "" {
940940
return schema
941941
}
942942

943-
// Split path by "." to get field names
944-
parts := strings.Split(path, ".")
945-
current := schema
943+
if prop, ok := schema.Properties[field]; ok {
944+
return &prop
945+
}
946946

947-
for _, part := range parts {
948-
if current == nil {
949-
return nil
947+
if schema.AdditionalProperties != nil {
948+
if schema.AdditionalProperties.Schema != nil {
949+
return schema.AdditionalProperties.Schema
950950
}
951-
952-
// Check if it's an object with properties
953-
if prop, ok := current.Properties[part]; ok {
954-
current = &prop
955-
continue
956-
}
957-
958-
// Check if it's an array and we need to look at items
959-
if current.Items != nil && current.Items.Schema != nil {
960-
current = current.Items.Schema
961-
// Try again with this part on the items schema
962-
if prop, ok := current.Properties[part]; ok {
963-
current = &prop
964-
continue
965-
}
951+
if schema.AdditionalProperties.Allows {
952+
return &spec.Schema{}
966953
}
954+
}
967955

968-
// Couldn't find the field
969-
return nil
956+
if schema.Items != nil && schema.Items.Schema != nil {
957+
return lookupSchemaAtField(schema.Items.Schema, field)
970958
}
971959

972-
return current
960+
return nil
973961
}
974962

975963
// validateNode validates all CEL expressions for a single node:

‎pkg/graph/builder_test.go‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,162 @@ import (
2323
memory2 "k8s.io/client-go/discovery/cached/memory"
2424
"k8s.io/client-go/rest"
2525
"k8s.io/client-go/restmapper"
26+
"k8s.io/kube-openapi/pkg/validation/spec"
2627

2728
krov1alpha1 "github.com/kubernetes-sigs/kro/api/v1alpha1"
2829
"github.com/kubernetes-sigs/kro/pkg/graph/variable"
2930
"github.com/kubernetes-sigs/kro/pkg/testutil/generator"
3031
"github.com/kubernetes-sigs/kro/pkg/testutil/k8s"
3132
)
3233

34+
func TestLookupSchemaAtField_AdditionalProperties(t *testing.T) {
35+
tests := []struct {
36+
name string
37+
schema *spec.Schema
38+
field string
39+
expectNil bool
40+
expectedType string
41+
}{
42+
{
43+
name: "direct property lookup works",
44+
schema: &spec.Schema{
45+
SchemaProps: spec.SchemaProps{
46+
Type: []string{"object"},
47+
Properties: map[string]spec.Schema{
48+
"name": {SchemaProps: spec.SchemaProps{Type: []string{"string"}}},
49+
},
50+
},
51+
},
52+
field: "name",
53+
expectNil: false,
54+
expectedType: "string",
55+
},
56+
{
57+
name: "additionalProperties lookup should return value schema",
58+
schema: &spec.Schema{
59+
SchemaProps: spec.SchemaProps{
60+
Type: []string{"object"},
61+
AdditionalProperties: &spec.SchemaOrBool{
62+
Allows: true,
63+
Schema: &spec.Schema{
64+
SchemaProps: spec.SchemaProps{Type: []string{"string"}},
65+
},
66+
},
67+
},
68+
},
69+
field: "anyDynamicKey",
70+
expectNil: false,
71+
expectedType: "string",
72+
},
73+
{
74+
name: "ConfigMap.data style schema",
75+
schema: &spec.Schema{
76+
SchemaProps: spec.SchemaProps{
77+
Type: []string{"object"},
78+
Properties: map[string]spec.Schema{
79+
"data": {
80+
SchemaProps: spec.SchemaProps{
81+
Type: []string{"object"},
82+
AdditionalProperties: &spec.SchemaOrBool{
83+
Allows: true,
84+
Schema: &spec.Schema{
85+
SchemaProps: spec.SchemaProps{Type: []string{"string"}},
86+
},
87+
},
88+
},
89+
},
90+
},
91+
},
92+
},
93+
field: "data",
94+
expectNil: false,
95+
expectedType: "object",
96+
},
97+
{
98+
name: "labels style map[string]string",
99+
schema: &spec.Schema{
100+
SchemaProps: spec.SchemaProps{
101+
Type: []string{"object"},
102+
AdditionalProperties: &spec.SchemaOrBool{
103+
Allows: true,
104+
Schema: &spec.Schema{
105+
SchemaProps: spec.SchemaProps{Type: []string{"string"}},
106+
},
107+
},
108+
},
109+
},
110+
field: "app",
111+
expectNil: false,
112+
expectedType: "string",
113+
},
114+
{
115+
name: "nested map - first level",
116+
schema: &spec.Schema{
117+
SchemaProps: spec.SchemaProps{
118+
Type: []string{"object"},
119+
AdditionalProperties: &spec.SchemaOrBool{
120+
Allows: true,
121+
Schema: &spec.Schema{
122+
SchemaProps: spec.SchemaProps{
123+
Type: []string{"object"},
124+
AdditionalProperties: &spec.SchemaOrBool{
125+
Allows: true,
126+
Schema: &spec.Schema{
127+
SchemaProps: spec.SchemaProps{Type: []string{"integer"}},
128+
},
129+
},
130+
},
131+
},
132+
},
133+
},
134+
},
135+
field: "outerKey",
136+
expectNil: false,
137+
expectedType: "object",
138+
},
139+
{
140+
name: "array items with additionalProperties",
141+
schema: &spec.Schema{
142+
SchemaProps: spec.SchemaProps{
143+
Type: []string{"array"},
144+
Items: &spec.SchemaOrArray{
145+
Schema: &spec.Schema{
146+
SchemaProps: spec.SchemaProps{
147+
Type: []string{"object"},
148+
AdditionalProperties: &spec.SchemaOrBool{
149+
Allows: true,
150+
Schema: &spec.Schema{
151+
SchemaProps: spec.SchemaProps{Type: []string{"string"}},
152+
},
153+
},
154+
},
155+
},
156+
},
157+
},
158+
},
159+
field: "dynamicKey",
160+
expectNil: false,
161+
expectedType: "string",
162+
},
163+
}
164+
165+
for _, tt := range tests {
166+
t.Run(tt.name, func(t *testing.T) {
167+
result := lookupSchemaAtField(tt.schema, tt.field)
168+
169+
if tt.expectNil {
170+
assert.Nil(t, result, "expected nil schema")
171+
return
172+
}
173+
174+
require.NotNil(t, result, "expected non-nil schema but got nil (AdditionalProperties not handled?)")
175+
if tt.expectedType != "" && len(result.Type) > 0 {
176+
assert.Equal(t, tt.expectedType, result.Type[0], "unexpected schema type")
177+
}
178+
})
179+
}
180+
}
181+
33182
func TestGraphBuilder_Validation(t *testing.T) {
34183
fakeResolver, fakeDiscovery := k8s.NewFakeResolver()
35184
restMapper := restmapper.NewDeferredDiscoveryRESTMapper(memory2.NewMemCacheClient(fakeDiscovery))
@@ -1873,6 +2022,66 @@ func TestGraphBuilder_CELTypeChecking(t *testing.T) {
18732022
},
18742023
wantErr: false,
18752024
},
2025+
{
2026+
name: "ConfigMap.data type mismatch - list where string expected",
2027+
resourceGraphDefinitionOpts: []generator.ResourceGraphDefinitionOption{
2028+
generator.WithSchema(
2029+
"Test", "v1alpha1",
2030+
map[string]interface{}{
2031+
"items": "[]string",
2032+
},
2033+
nil,
2034+
),
2035+
generator.WithResource("configmap", map[string]interface{}{
2036+
"apiVersion": "v1",
2037+
"kind": "ConfigMap",
2038+
"metadata": map[string]interface{}{
2039+
"name": "test-config",
2040+
},
2041+
"data": map[string]interface{}{
2042+
"items": "${schema.spec.items}",
2043+
},
2044+
}, nil, nil),
2045+
},
2046+
wantErr: true,
2047+
errMsg: "type mismatch",
2048+
},
2049+
{
2050+
name: "ConfigMap.data type mismatch - map() result where string expected",
2051+
resourceGraphDefinitionOpts: []generator.ResourceGraphDefinitionOption{
2052+
generator.WithSchema(
2053+
"ForEachTest", "v1alpha1",
2054+
map[string]interface{}{
2055+
"queues": "[]string",
2056+
},
2057+
nil,
2058+
),
2059+
generator.WithResourceCollection("queues", map[string]interface{}{
2060+
"apiVersion": "v1",
2061+
"kind": "Pod",
2062+
"metadata": map[string]interface{}{
2063+
"name": "${schema.metadata.name + '-' + queue}",
2064+
},
2065+
},
2066+
[]krov1alpha1.ForEachDimension{
2067+
{"queue": "${schema.spec.queues}"},
2068+
},
2069+
nil, nil),
2070+
generator.WithResource("configmap", map[string]interface{}{
2071+
"apiVersion": "v1",
2072+
"kind": "ConfigMap",
2073+
"metadata": map[string]interface{}{
2074+
"name": "${schema.metadata.name}-output",
2075+
"namespace": "${schema.metadata.namespace}",
2076+
},
2077+
"data": map[string]interface{}{
2078+
"queues": "${queues.map(q, {\"name\": q.metadata.name, \"queueARN\": q.status.phase})}",
2079+
},
2080+
}, nil, nil),
2081+
},
2082+
wantErr: true,
2083+
errMsg: "type mismatch",
2084+
},
18762085
}
18772086

18782087
for _, tt := range tests {

‎pkg/testutil/k8s/discovery.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,43 @@ func NewFakeResolver() (*FakeResolver, *fake.FakeDiscovery) {
404404
},
405405
},
406406
},
407+
// ConfigMap
408+
{Version: "v1", Kind: "ConfigMap"}: {
409+
SchemaProps: spec.SchemaProps{
410+
Type: []string{"object"},
411+
Properties: map[string]spec.Schema{
412+
"apiVersion": {SchemaProps: spec.SchemaProps{Type: []string{"string"}}},
413+
"kind": {SchemaProps: spec.SchemaProps{Type: []string{"string"}}},
414+
"metadata": metadataSchema(),
415+
"data": {
416+
SchemaProps: spec.SchemaProps{
417+
Type: []string{"object"},
418+
AdditionalProperties: &spec.SchemaOrBool{
419+
Allows: true,
420+
Schema: &spec.Schema{
421+
SchemaProps: spec.SchemaProps{
422+
Type: []string{"string"},
423+
},
424+
},
425+
},
426+
},
427+
},
428+
"binaryData": {
429+
SchemaProps: spec.SchemaProps{
430+
Type: []string{"object"},
431+
AdditionalProperties: &spec.SchemaOrBool{
432+
Allows: true,
433+
Schema: &spec.Schema{
434+
SchemaProps: spec.SchemaProps{
435+
Type: []string{"string"},
436+
},
437+
},
438+
},
439+
},
440+
},
441+
},
442+
},
443+
},
407444
// CRDs
408445
{Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"}: {
409446
SchemaProps: spec.SchemaProps{
@@ -507,6 +544,12 @@ func NewFakeResolver() (*FakeResolver, *fake.FakeDiscovery) {
507544
Kind: "Pod",
508545
Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete"},
509546
},
547+
{
548+
Name: "configmaps",
549+
Namespaced: true,
550+
Kind: "ConfigMap",
551+
Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete"},
552+
},
510553
},
511554
},
512555
// CRD

0 commit comments

Comments
 (0)