generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 255
Overhaul RGD static validation to eliminate emulation #756
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.
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
46ad38a
feat: add typed CEL environments for schema validation
a-hilaly 3b5a649
refactor: remove emulation based CEL validation
a-hilaly fa4a0c7
feat: implement CEL expression type checking validation for `spec.res…
a-hilaly 65b285e
feat: infer status schema from CEL type checking
a-hilaly 94daf91
feat: validate multi-expression string concatenation
a-hilaly f731a8e
feat: fix CEL compile-time type checking for schema references
a-hilaly db14b78
refactor: migrate type validation to CEL's native type system
a-hilaly 684a7fb
feat: validate includeWhen and readyWhen expressions
a-hilaly ee7ca06
refactor: build dependency graph before type checking
a-hilaly ba07c1d
refactor: improve CEL validation error messages
a-hilaly 92d096b
feat: Inject ObjectMeta schema into instance for moar type checking
a-hilaly a6c0484
fix: correct expression unwrapping to preserve trailing braces
a-hilaly eddbdea
refactor: temporarily move emulator to kro CLI and create separate mo…
a-hilaly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,253 @@ | ||
| // Copyright 2025 The Kubernetes Authors. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package schema | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/google/cel-go/cel" | ||
| extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| apiservercel "k8s.io/apiserver/pkg/cel" | ||
| "k8s.io/utils/ptr" | ||
| ) | ||
|
|
||
| // GenerateSchemaFromCELTypes generates a JSONSchemaProps from a map of CEL types. | ||
| // The provider is used to recursively introspect struct types and extract all fields. | ||
| func GenerateSchemaFromCELTypes(typeMap map[string]*cel.Type, provider *apiservercel.DeclTypeProvider) (*extv1.JSONSchemaProps, error) { | ||
| fieldDescriptors := make([]fieldDescriptor, 0, len(typeMap)) | ||
|
|
||
| for path, celType := range typeMap { | ||
| exprSchema, err := inferSchemaFromCELType(celType, provider) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to infer schema from type at %v: %w", path, err) | ||
| } | ||
| fieldDescriptors = append(fieldDescriptors, fieldDescriptor{ | ||
| Path: path, | ||
| Schema: exprSchema, | ||
| }) | ||
| } | ||
|
|
||
| return generateJSONSchemaFromFieldDescriptors(fieldDescriptors) | ||
| } | ||
|
|
||
| // primitiveTypeToSchema converts a CEL primitive type name to a JSONSchemaProps. | ||
| func primitiveTypeToSchema(typeName string) (*extv1.JSONSchemaProps, bool) { | ||
| switch typeName { | ||
| case "bool": | ||
| return &extv1.JSONSchemaProps{Type: "boolean"}, true | ||
| case "int", "uint": | ||
| return &extv1.JSONSchemaProps{Type: "integer"}, true | ||
| case "double": | ||
| return &extv1.JSONSchemaProps{Type: "number"}, true | ||
| case "string": | ||
| return &extv1.JSONSchemaProps{Type: "string"}, true | ||
| case "bytes": | ||
| return &extv1.JSONSchemaProps{Type: "string", Format: "byte"}, true | ||
| case "null_type": | ||
a-hilaly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return &extv1.JSONSchemaProps{ | ||
| Description: "null type - any value allowed", | ||
| XPreserveUnknownFields: ptr.To(true), | ||
| }, true | ||
| default: | ||
| return nil, false | ||
| } | ||
| } | ||
|
|
||
| // inferSchemaFromCELType converts a CEL type to an OpenAPI schema. | ||
| // For struct types, it uses the provider to recursively extract all fields. | ||
| func inferSchemaFromCELType(celType *cel.Type, provider *apiservercel.DeclTypeProvider) (*extv1.JSONSchemaProps, error) { | ||
| if celType == nil { | ||
| return nil, fmt.Errorf("type is nil") | ||
| } | ||
|
|
||
| typeName := celType.String() | ||
|
|
||
| // Handle primitive types | ||
| if schema, ok := primitiveTypeToSchema(typeName); ok { | ||
| return schema, nil | ||
| } | ||
|
|
||
| // Handle complex types based on kind | ||
| switch celType.Kind() { | ||
| case cel.ListKind: | ||
| if provider != nil { | ||
| declType, found := provider.FindDeclType(typeName) | ||
| if found { | ||
| visited := make(map[string]bool) | ||
| return extractSchemaFromDeclTypeWithCycleDetection(declType, visited) | ||
| } | ||
| } | ||
| // Fallback: try to infer from CEL type parameters | ||
| if len(celType.Parameters()) > 0 { | ||
| elemType := celType.Parameters()[0] | ||
| elemSchema, err := inferSchemaFromCELType(elemType, provider) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to infer array element type: %w", err) | ||
| } | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "array", | ||
| Items: &extv1.JSONSchemaPropsOrArray{ | ||
| Schema: elemSchema, | ||
| }, | ||
| }, nil | ||
| } | ||
| return &extv1.JSONSchemaProps{Type: "array"}, nil | ||
|
|
||
| case cel.MapKind: | ||
| if provider != nil { | ||
| declType, found := provider.FindDeclType(typeName) | ||
| if found { | ||
| visited := make(map[string]bool) | ||
| return extractSchemaFromDeclTypeWithCycleDetection(declType, visited) | ||
| } | ||
| } | ||
| // Fallback: try to infer from CEL type parameters | ||
| if len(celType.Parameters()) > 1 { | ||
| valueType := celType.Parameters()[1] | ||
| valueSchema, err := inferSchemaFromCELType(valueType, provider) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to infer map value type: %w", err) | ||
| } | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ | ||
| Schema: valueSchema, | ||
| }, | ||
| }, nil | ||
| } | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| AdditionalProperties: &extv1.JSONSchemaPropsOrBool{Allows: true}, | ||
| }, nil | ||
|
|
||
| case cel.StructKind: | ||
| if provider != nil { | ||
| declType, found := provider.FindDeclType(typeName) | ||
| if found { | ||
| visited := make(map[string]bool) | ||
| return extractSchemaFromDeclTypeWithCycleDetection(declType, visited) | ||
| } | ||
| } | ||
| // Fallback: permissive object schema | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| AdditionalProperties: &extv1.JSONSchemaPropsOrBool{Allows: true}, | ||
| }, nil | ||
|
|
||
| case cel.DynKind: | ||
| return &extv1.JSONSchemaProps{ | ||
| XPreserveUnknownFields: ptr.To(true), | ||
| }, nil | ||
|
|
||
| default: | ||
| // Unknown type - be permissive | ||
| return &extv1.JSONSchemaProps{ | ||
| Description: fmt.Sprintf("unknown CEL type: %s", typeName), | ||
| XPreserveUnknownFields: ptr.To(true), | ||
| }, nil | ||
| } | ||
| } | ||
|
|
||
| // extractSchemaFromDeclTypeWithCycleDetection recursively extracts all fields from a DeclType to build an OpenAPI schema. | ||
| // It tracks visited types to prevent infinite recursion on cyclic type definitions. | ||
| func extractSchemaFromDeclTypeWithCycleDetection(declType *apiservercel.DeclType, visited map[string]bool) (*extv1.JSONSchemaProps, error) { | ||
| if declType == nil { | ||
| return &extv1.JSONSchemaProps{XPreserveUnknownFields: ptr.To(true)}, nil | ||
| } | ||
|
|
||
| // Get type identifier for cycle detection | ||
| celType := declType.CelType() | ||
| if celType != nil { | ||
| typeName := celType.String() | ||
|
|
||
| if visited[typeName] { | ||
| return nil, fmt.Errorf("cyclic type reference detected: %s", typeName) | ||
| } | ||
|
|
||
| visited[typeName] = true | ||
| defer delete(visited, typeName) | ||
| } | ||
|
|
||
| // Handle different DeclType kinds | ||
| if declType.IsList() { | ||
| elemType := declType.ElemType | ||
| if elemType != nil { | ||
| elemSchema, err := extractSchemaFromDeclTypeWithCycleDetection(elemType, visited) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to extract array element schema: %w", err) | ||
| } | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "array", | ||
| Items: &extv1.JSONSchemaPropsOrArray{ | ||
| Schema: elemSchema, | ||
| }, | ||
| }, nil | ||
| } | ||
| return &extv1.JSONSchemaProps{Type: "array"}, nil | ||
| } | ||
|
|
||
| if declType.IsMap() { | ||
| valueType := declType.ElemType | ||
| if valueType != nil { | ||
| valueSchema, err := extractSchemaFromDeclTypeWithCycleDetection(valueType, visited) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to extract map value schema: %w", err) | ||
| } | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ | ||
| Schema: valueSchema, | ||
| }, | ||
| }, nil | ||
| } | ||
| return &extv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| AdditionalProperties: &extv1.JSONSchemaPropsOrBool{Allows: true}, | ||
| }, nil | ||
| } | ||
|
|
||
| if !declType.IsObject() { | ||
| celType := declType.CelType() | ||
| if celType != nil { | ||
| typeName := celType.String() | ||
| if schema, ok := primitiveTypeToSchema(typeName); ok { | ||
| return schema, nil | ||
| } | ||
| } | ||
| return &extv1.JSONSchemaProps{XPreserveUnknownFields: ptr.To(true)}, nil | ||
| } | ||
|
|
||
| schema := &extv1.JSONSchemaProps{ | ||
| Type: "object", | ||
| Properties: make(map[string]extv1.JSONSchemaProps), | ||
| } | ||
|
|
||
| for _, field := range declType.Fields { | ||
| if field == nil { | ||
| continue | ||
| } | ||
|
|
||
| fieldSchema, err := extractSchemaFromDeclTypeWithCycleDetection(field.Type, visited) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to extract schema for field %s: %w", field.Name, err) | ||
| } | ||
|
|
||
| if fieldSchema != nil { | ||
| schema.Properties[field.Name] = *fieldSchema | ||
| } | ||
| } | ||
|
|
||
| return schema, nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when we have a status like
correct?
Could we find/test any case where
type(expressionA)type(expressionB)is not a string and returns something valid at the CEL level?2 side notes/nit (just thinking out loud)
I wonder if we wouldn't gain readability flexibility building directly the
extva.JSONSchemaProps. Eventually, we could also declarestatusTypeMap := make(map[string][]*cel.Type)so we can handle options like ``There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 here, the CEL type of status.someField could be checked against the typed schema type for its assigneability. I think there are cases when expression results could be integers or nested objects. otherwise its a missing limitation doc I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. In status field schema generation, for "single expression fields" any type allowed:
However with "multi expression fields" we do have restrictions:
Concatenating arbitrary types (objects, bools, ints) is complex and error prone. I'm just starting restrictive, we can relax later if there are good use cases and it's technically feasible. For now, users need explicit
string()calls for multi exprssion fields.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some historical context: Initially kro didn't support multi expression fields at all. We added it because users wanted simple string templating instead of complex CEL string manipulation and concatenation expressions. For example IAM policies:
vs:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit I completely missed that we are in the else branch which only deals with concatenated expressions.
The argument right now is that status fields only accept strings, whereas in theory they can be object types or integers or others when working with the first case. for the second case, they must stay strings (as you outlined). I subscribe to that 100% and I think we should leave this as is
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about it, the less I see an instance of
${ expressionA }${ expressionB }whereexpressionAorexpressionBare not strings and the expression makes sense.edit:
Actually, yes, there is a case, when crafting a json as you shown for example
Then, generation is an int and gets into a string.
How hard would it be to force a
.String)()call?