Skip to content

Commit 46d08ba

Browse files
barney-sbarney-s
authored
Merge pull request #260 from a-hilaly/parser/complex-stuff
fix: schema parser improvements for complex CRD patterns
2 parents 64147a6 + f582124 commit 46d08ba

File tree

2 files changed

+243
-23
lines changed

2 files changed

+243
-23
lines changed

‎pkg/graph/parser/parser.go‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -68,15 +68,14 @@ func parseResource(resource interface{}, schema *spec.Schema, path string) ([]va
6868
}
6969

7070
func getExpectedTypes(schema *spec.Schema) ([]string, error) {
71+
// Handle "x-kubernetes-preserve-unknown-fields" extension first
72+
if hasStructuralSchemaMarkerEnabled(schema, xKubernetesPreserveUnknownFields) {
73+
return []string{schemaTypeAny}, nil
74+
}
75+
7176
// Handle "x-kubernetes-int-or-string" extension
72-
if ext, ok := schema.VendorExtensible.Extensions[xKubernetesIntOrString]; ok {
73-
enabled, ok := ext.(bool)
74-
if !ok {
75-
return nil, fmt.Errorf("xKubernetesIntOrString extension is not a boolean")
76-
}
77-
if enabled {
78-
return []string{"string", "integer"}, nil
79-
}
77+
if hasStructuralSchemaMarkerEnabled(schema, xKubernetesIntOrString) {
78+
return []string{"string", "integer"}, nil
8079
}
8180

8281
// Handle OneOf schemas
@@ -110,9 +109,10 @@ func getExpectedTypes(schema *spec.Schema) ([]string, error) {
110109
return types, nil
111110
}
112111

113-
if schema.Type[0] != "" {
112+
if len(schema.Type) > 0 && schema.Type[0] != "" {
114113
return schema.Type, nil
115114
}
115+
116116
if schema.AdditionalProperties != nil && schema.AdditionalProperties.Allows {
117117
// NOTE(a-hilaly): I don't like the type "any", we might want to change this to "object"
118118
// in the future; just haven't really thought about it yet.
@@ -123,14 +123,15 @@ func getExpectedTypes(schema *spec.Schema) ([]string, error) {
123123
return nil, fmt.Errorf("unknown schema type")
124124
}
125125

126-
func sliceInclude(expectedTypes []string, expectedType string) bool {
127-
return slices.Contains(expectedTypes, expectedType)
128-
}
129-
130126
func validateSchema(schema *spec.Schema, path string) error {
131127
if schema == nil {
132128
return fmt.Errorf("schema is nil for path %s", path)
133129
}
130+
131+
if hasStructuralSchemaMarkerEnabled(schema, xKubernetesPreserveUnknownFields) {
132+
return nil
133+
}
134+
134135
// Ensure the schema has at least one valid construct
135136
if len(schema.Type) == 0 && len(schema.OneOf) == 0 && len(schema.AnyOf) == 0 && schema.AdditionalProperties == nil {
136137
return fmt.Errorf("schema at path %s has no valid type, OneOf, AnyOf, or AdditionalProperties", path)
@@ -139,18 +140,14 @@ func validateSchema(schema *spec.Schema, path string) error {
139140
}
140141

141142
func parseObject(field map[string]interface{}, schema *spec.Schema, path string, expectedTypes []string) ([]variable.FieldDescriptor, error) {
142-
if !sliceInclude(expectedTypes, "object") && (schema.AdditionalProperties == nil || !schema.AdditionalProperties.Allows) {
143-
return nil, fmt.Errorf("expected object type or AdditionalProperties allowed for path %s, got %v", path, field)
144-
}
145-
146143
// Look for vendor schema extensions first
147144
if len(schema.VendorExtensible.Extensions) > 0 {
148145
// If the schema has the x-kubernetes-preserve-unknown-fields extension, we need to parse
149146
// this field using the schemaless parser. This allows us to extract CEL expressions from
150147
// fields that don't have a strict schema definition, while still preserving any unknown
151148
// fields. This is particularly important for handling custom resources and fields that
152149
// may contain arbitrary nested structures with potential CEL expressions.
153-
if enabled, ok := schema.VendorExtensible.Extensions[xKubernetesPreserveUnknownFields]; ok && enabled.(bool) {
150+
if hasStructuralSchemaMarkerEnabled(schema, xKubernetesPreserveUnknownFields) {
154151
expressions, err := parseSchemalessResource(field, path)
155152
if err != nil {
156153
return nil, err
@@ -159,6 +156,10 @@ func parseObject(field map[string]interface{}, schema *spec.Schema, path string,
159156
}
160157
}
161158

159+
if !slices.Contains(expectedTypes, "object") && (schema.AdditionalProperties == nil || !schema.AdditionalProperties.Allows) {
160+
return nil, fmt.Errorf("expected object type or AdditionalProperties allowed for path %s, got %v", path, field)
161+
}
162+
162163
var expressionsFields []variable.FieldDescriptor
163164
for fieldName, value := range field {
164165
fieldSchema, err := getFieldSchema(schema, fieldName)
@@ -176,7 +177,7 @@ func parseObject(field map[string]interface{}, schema *spec.Schema, path string,
176177
}
177178

178179
func parseArray(field []interface{}, schema *spec.Schema, path string, expectedTypes []string) ([]variable.FieldDescriptor, error) {
179-
if !sliceInclude(expectedTypes, "array") {
180+
if !slices.Contains(expectedTypes, "array") {
180181
return nil, fmt.Errorf("expected array type for path %s, got %v", path, field)
181182
}
182183

@@ -213,7 +214,7 @@ func parseString(field string, schema *spec.Schema, path string, expectedTypes [
213214
}}, nil
214215
}
215216

216-
if !sliceInclude(expectedTypes, "string") && !sliceInclude(expectedTypes, schemaTypeAny) {
217+
if !slices.Contains(expectedTypes, "string") && !slices.Contains(expectedTypes, schemaTypeAny) {
217218
return nil, fmt.Errorf("expected string type or AdditionalProperties for path %s, got %v", path, field)
218219
}
219220

@@ -234,15 +235,15 @@ func parseString(field string, schema *spec.Schema, path string, expectedTypes [
234235
func parseScalarTypes(field interface{}, _ *spec.Schema, path string, expectedTypes []string) ([]variable.FieldDescriptor, error) {
235236
// perform type checks for scalar types
236237
switch {
237-
case sliceInclude(expectedTypes, "number"):
238+
case slices.Contains(expectedTypes, "number"):
238239
if _, ok := field.(float64); !ok {
239240
return nil, fmt.Errorf("expected number type for path %s, got %T", path, field)
240241
}
241-
case sliceInclude(expectedTypes, "int"), sliceInclude(expectedTypes, "integer"):
242+
case slices.Contains(expectedTypes, "int"), slices.Contains(expectedTypes, "integer"):
242243
if !isInteger(field) {
243244
return nil, fmt.Errorf("expected integer type for path %s, got %T", path, field)
244245
}
245-
case sliceInclude(expectedTypes, "boolean"), sliceInclude(expectedTypes, "bool"):
246+
case slices.Contains(expectedTypes, "boolean"), slices.Contains(expectedTypes, "bool"):
246247
if _, ok := field.(bool); !ok {
247248
return nil, fmt.Errorf("expected boolean type for path %s, got %T", path, field)
248249
}
@@ -306,3 +307,13 @@ func joinPathAndFieldName(path, fieldName string) string {
306307
}
307308
return fmt.Sprintf("%s.%s", path, fieldName)
308309
}
310+
311+
// hasStructuralSchemaMarkerEnabled checks if a schema has a specific marker enabled.
312+
func hasStructuralSchemaMarkerEnabled(schema *spec.Schema, marker string) bool {
313+
if ext, ok := schema.VendorExtensible.Extensions[marker]; ok {
314+
if enabled, ok := ext.(bool); ok && enabled {
315+
return true
316+
}
317+
}
318+
return false
319+
}

‎pkg/graph/parser/parser_test.go‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1286,3 +1286,212 @@ func TestOneOfWithStructuralConstraints(t *testing.T) {
12861286
}
12871287
})
12881288
}
1289+
1290+
func TestPreserveUnknownFields(t *testing.T) {
1291+
testCases := []struct {
1292+
name string
1293+
schema *spec.Schema
1294+
resource map[string]interface{}
1295+
wantErr bool
1296+
expectedError string
1297+
expectedExpressions []variable.FieldDescriptor
1298+
}{
1299+
{
1300+
name: "schema with no type but x-kubernetes-preserve-unknown-fields",
1301+
schema: &spec.Schema{
1302+
VendorExtensible: spec.VendorExtensible{
1303+
Extensions: spec.Extensions{
1304+
"x-kubernetes-preserve-unknown-fields": true,
1305+
},
1306+
},
1307+
},
1308+
resource: map[string]interface{}{
1309+
"spec": map[string]interface{}{
1310+
"template": "${template.value}",
1311+
},
1312+
},
1313+
wantErr: false,
1314+
expectedExpressions: []variable.FieldDescriptor{
1315+
{
1316+
Path: "spec.template",
1317+
Expressions: []string{"template.value"},
1318+
ExpectedTypes: []string{"any"},
1319+
StandaloneExpression: true,
1320+
},
1321+
},
1322+
},
1323+
{
1324+
name: "schema with no type but x-kubernetes-preserve-unknown-fields, expression in nested object",
1325+
schema: &spec.Schema{
1326+
VendorExtensible: spec.VendorExtensible{
1327+
Extensions: spec.Extensions{
1328+
"x-kubernetes-preserve-unknown-fields": true,
1329+
},
1330+
},
1331+
},
1332+
resource: map[string]interface{}{
1333+
"spec": map[string]interface{}{
1334+
"field1": "noisy string",
1335+
"template": map[string]interface{}{
1336+
"nested": []interface{}{
1337+
map[string]interface{}{
1338+
"key": "${template.value}",
1339+
},
1340+
},
1341+
},
1342+
},
1343+
},
1344+
wantErr: false,
1345+
expectedExpressions: []variable.FieldDescriptor{
1346+
{
1347+
Path: "spec.template.nested[0].key",
1348+
Expressions: []string{"template.value"},
1349+
ExpectedTypes: []string{"any"},
1350+
StandaloneExpression: true,
1351+
},
1352+
},
1353+
},
1354+
{
1355+
name: "pulumi-style mixed schema",
1356+
schema: &spec.Schema{
1357+
SchemaProps: spec.SchemaProps{
1358+
Type: []string{"object"},
1359+
Properties: map[string]spec.Schema{
1360+
"program": {
1361+
SchemaProps: spec.SchemaProps{
1362+
Type: []string{"object"},
1363+
Properties: map[string]spec.Schema{
1364+
"resources": {
1365+
SchemaProps: spec.SchemaProps{
1366+
Type: []string{"object"},
1367+
AdditionalProperties: &spec.SchemaOrBool{
1368+
Allows: true,
1369+
Schema: &spec.Schema{
1370+
SchemaProps: spec.SchemaProps{
1371+
Type: []string{"object"},
1372+
Properties: map[string]spec.Schema{
1373+
"properties": {
1374+
VendorExtensible: spec.VendorExtensible{
1375+
Extensions: spec.Extensions{
1376+
"x-kubernetes-preserve-unknown-fields": true,
1377+
},
1378+
},
1379+
},
1380+
},
1381+
},
1382+
},
1383+
},
1384+
},
1385+
},
1386+
},
1387+
},
1388+
},
1389+
},
1390+
},
1391+
},
1392+
resource: map[string]interface{}{
1393+
"program": map[string]interface{}{
1394+
"resources": map[string]interface{}{
1395+
"app": map[string]interface{}{
1396+
"properties": map[string]interface{}{
1397+
"spec": map[string]interface{}{
1398+
"name": "${schema.spec.name}",
1399+
"region": "${schema.spec.region}",
1400+
"services": []interface{}{
1401+
map[string]interface{}{
1402+
"name": "${schema.spec.name}-service",
1403+
"instanceCount": "${schema.spec.instanceCount}",
1404+
},
1405+
},
1406+
},
1407+
},
1408+
},
1409+
},
1410+
},
1411+
},
1412+
wantErr: false,
1413+
expectedExpressions: []variable.FieldDescriptor{
1414+
{
1415+
Path: "program.resources.app.properties.spec.name",
1416+
Expressions: []string{"schema.spec.name"},
1417+
ExpectedTypes: []string{"any"},
1418+
StandaloneExpression: true,
1419+
},
1420+
{
1421+
Path: "program.resources.app.properties.spec.region",
1422+
Expressions: []string{"schema.spec.region"},
1423+
ExpectedTypes: []string{"any"},
1424+
StandaloneExpression: true,
1425+
},
1426+
{
1427+
Path: "program.resources.app.properties.spec.services[0].name",
1428+
Expressions: []string{"schema.spec.name"},
1429+
ExpectedTypes: []string{"any"},
1430+
StandaloneExpression: false,
1431+
},
1432+
{
1433+
Path: "program.resources.app.properties.spec.services[0].instanceCount",
1434+
Expressions: []string{"schema.spec.instanceCount"},
1435+
ExpectedTypes: []string{"any"},
1436+
StandaloneExpression: true,
1437+
},
1438+
},
1439+
},
1440+
}
1441+
1442+
for _, tc := range testCases {
1443+
t.Run(tc.name, func(t *testing.T) {
1444+
expressions, err := ParseResource(tc.resource, tc.schema)
1445+
if tc.wantErr {
1446+
if err == nil {
1447+
t.Error("Expected error but got none")
1448+
} else if tc.expectedError != "" && err.Error() != tc.expectedError {
1449+
t.Errorf("Expected error message %q, got %q", tc.expectedError, err.Error())
1450+
}
1451+
} else {
1452+
if err != nil {
1453+
t.Errorf("Did not expect error but got: %v", err)
1454+
return
1455+
}
1456+
1457+
if len(expressions) != len(tc.expectedExpressions) {
1458+
t.Errorf("Expected %d expressions, got %d", len(tc.expectedExpressions), len(expressions))
1459+
t.Errorf("Got expressions:")
1460+
for _, expr := range expressions {
1461+
t.Errorf(" %+v", expr)
1462+
}
1463+
return
1464+
}
1465+
1466+
// Create maps for easier comparison
1467+
actualMap := make(map[string]variable.FieldDescriptor)
1468+
expectedMap := make(map[string]variable.FieldDescriptor)
1469+
1470+
for _, expr := range expressions {
1471+
actualMap[expr.Path] = expr
1472+
}
1473+
for _, expr := range tc.expectedExpressions {
1474+
expectedMap[expr.Path] = expr
1475+
}
1476+
1477+
for path, expectedExpr := range expectedMap {
1478+
actualExpr, ok := actualMap[path]
1479+
if !ok {
1480+
t.Errorf("Missing expected expression for path %s", path)
1481+
continue
1482+
}
1483+
1484+
if !reflect.DeepEqual(actualExpr.Expressions, expectedExpr.Expressions) {
1485+
t.Errorf("Path %s: expected expressions %v, got %v", path, expectedExpr.Expressions, actualExpr.Expressions)
1486+
}
1487+
if !reflect.DeepEqual(actualExpr.ExpectedTypes, expectedExpr.ExpectedTypes) {
1488+
t.Errorf("Path %s: expected types %v, got %v", path, expectedExpr.ExpectedTypes, actualExpr.ExpectedTypes)
1489+
}
1490+
if actualExpr.StandaloneExpression != expectedExpr.StandaloneExpression {
1491+
t.Errorf("Path %s: expected StandaloneExpression %v, got %v", path, expectedExpr.StandaloneExpression, actualExpr.StandaloneExpression)
1492+
}
1493+
}
1494+
}
1495+
})
1496+
}
1497+
}

0 commit comments

Comments
 (0)