Skip to content

Commit 876241c

Browse files
michaelhtmmichaelhtm
authored
refactor: DryRunExpressions in builder.go (#481)
* refactor: DryRunExpressions in builder.go * test: Test includeWhen failure when referencing another resource
1 parent 6840c6d commit 876241c

File tree

2 files changed

+144
-91
lines changed

2 files changed

+144
-91
lines changed

Diff for: ‎pkg/graph/builder.go

+116-91
Original file line numberDiff line numberDiff line change
@@ -684,12 +684,11 @@ func extractDependencies(env *cel.Env, expression string, resourceNames []string
684684
// we evaluate B's CEL expressions against 2 emulated resources A and C, and so
685685
// on.
686686
func validateResourceCELExpressions(resources map[string]*Resource, instance *Resource) error {
687-
resourceNames := maps.Keys(resources)
687+
resourceIDs := maps.Keys(resources)
688688
// We also want to allow users to refer to the instance spec in their expressions.
689-
resourceNames = append(resourceNames, "schema")
690-
conditionFieldNames := []string{"schema"}
689+
resourceIDs = append(resourceIDs, "schema")
691690

692-
env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(resourceNames))
691+
env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(resourceIDs))
693692
if err != nil {
694693
return fmt.Errorf("failed to create CEL environment: %w", err)
695694
}
@@ -700,105 +699,131 @@ func validateResourceCELExpressions(resources map[string]*Resource, instance *Re
700699
delete(instanceEmulatedCopy.Object, "status")
701700
}
702701

702+
// create includeWhenContext
703+
includeWhenContext := map[string]*Resource{}
704+
// for now we will only support the instance context for includeWhen expressions.
705+
// With this decision we will decide in creation time, and update time
706+
// If we'll be creating resources or not
707+
includeWhenContext["schema"] = &Resource{
708+
emulatedObject: &unstructured.Unstructured{
709+
Object: instanceEmulatedCopy.Object,
710+
},
711+
}
712+
713+
// create expressionsContext
714+
expressionContext := map[string]*Resource{}
715+
// add instance spec to the context
716+
expressionContext["schema"] = &Resource{
717+
emulatedObject: &unstructured.Unstructured{
718+
Object: instanceEmulatedCopy.Object,
719+
},
720+
}
721+
// include all resources, and remove individual ones
722+
// during the validation
723+
// this is done to avoid having to create a new context for each resource
724+
for resourceName, contextResource := range resources {
725+
expressionContext[resourceName] = contextResource
726+
}
727+
703728
for _, resource := range resources {
704-
for _, resourceVariable := range resource.variables {
705-
for _, expression := range resourceVariable.Expressions {
706-
err := validateCELExpressionContext(env, expression, resourceNames)
707-
if err != nil {
708-
return fmt.Errorf("failed to validate expression context: '%s' %w", expression, err)
709-
}
729+
// exclude resource from the context
730+
delete(expressionContext, resource.id)
710731

711-
// create context
712-
context := map[string]*Resource{}
713-
for resourceName, contextResource := range resources {
714-
// exclude the resource we are validating
715-
if resourceName != resource.id {
716-
context[resourceName] = contextResource
717-
}
718-
}
719-
// add instance spec to the context
720-
context["schema"] = &Resource{
721-
emulatedObject: &unstructured.Unstructured{
722-
Object: instanceEmulatedCopy.Object,
723-
},
724-
}
732+
err := ensureResourceExpressions(env, expressionContext, resource)
733+
if err != nil {
734+
return fmt.Errorf("failed to ensure resource %s expressions: %w", resource.id, err)
735+
}
725736

726-
_, err = dryRunExpression(env, expression, context)
727-
if err != nil {
728-
return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
729-
}
730-
}
737+
err = ensureReadyWhenExpressions(resource)
738+
if err != nil {
739+
return fmt.Errorf("failed to ensure resource %s readyWhen expressions: %w", resource.id, err)
740+
}
731741

732-
// validate readyWhen Expressions for resource
733-
// Only accepting expressions accessing the status and spec for now
734-
// and need to evaluate to a boolean type
735-
//
736-
// TODO(michaelhtm) It shares some of the logic with the loop from above..maybe
737-
// we can refactor them or put it in one function.
738-
// I would also suggest separating the dryRuns of readyWhenExpressions
739-
// and the resourceExpressions.
740-
for _, readyWhenExpression := range resource.readyWhenExpressions {
741-
fieldEnv, err := krocel.DefaultEnvironment(krocel.WithResourceIDs([]string{resource.id}))
742-
if err != nil {
743-
return fmt.Errorf("failed to create CEL environment: %w", err)
744-
}
742+
err = ensureIncludeWhenExpressions(env, includeWhenContext, resource)
743+
if err != nil {
744+
return fmt.Errorf("failed to ensure resource %s includeWhen expressions: %w", resource.id, err)
745+
}
745746

746-
err = validateCELExpressionContext(fieldEnv, readyWhenExpression, []string{resource.id})
747-
if err != nil {
748-
return fmt.Errorf("failed to validate expression context: '%s' %w", readyWhenExpression, err)
749-
}
750-
// create context
751-
// add resource fields to the context
752-
resourceEmulatedCopy := resource.emulatedObject.DeepCopy()
753-
if resourceEmulatedCopy != nil && resourceEmulatedCopy.Object != nil {
754-
delete(resourceEmulatedCopy.Object, "apiVersion")
755-
delete(resourceEmulatedCopy.Object, "kind")
756-
}
757-
context := map[string]*Resource{}
758-
context[resource.id] = &Resource{
759-
emulatedObject: resourceEmulatedCopy,
760-
}
761-
output, err := dryRunExpression(fieldEnv, readyWhenExpression, context)
747+
// include the resource back to the context
748+
expressionContext[resource.id] = resource
749+
}
762750

763-
if err != nil {
764-
return fmt.Errorf("failed to dry-run expression %s: %w", readyWhenExpression, err)
765-
}
766-
if !krocel.IsBoolType(output) {
767-
return fmt.Errorf("output of readyWhen expression %s can only be of type bool", readyWhenExpression)
768-
}
751+
return nil
752+
}
753+
754+
// ensureResourceExpressions validates the CEL expressions in the resource
755+
// against the resources defined in the resource graph definition.
756+
func ensureResourceExpressions(env *cel.Env, context map[string]*Resource, resource *Resource) error {
757+
// We need to validate the CEL expressions in the resource.
758+
for _, resourceVariable := range resource.variables {
759+
for _, expression := range resourceVariable.Expressions {
760+
_, err := ensureExpression(env, expression, []string{resource.id}, context)
761+
if err != nil {
762+
return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
769763
}
764+
}
765+
}
766+
return nil
767+
}
770768

771-
for _, includeWhenExpression := range resource.includeWhenExpressions {
772-
instanceEnv, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(resourceNames))
773-
if err != nil {
774-
return fmt.Errorf("failed to create CEL environment: %w", err)
775-
}
769+
// ensureReadyWhenExpressions validates the readyWhen expressions in the resource
770+
// against the resources defined in the resource graph definition.
771+
func ensureReadyWhenExpressions(resource *Resource) error {
772+
env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs([]string{resource.id}))
773+
for _, expression := range resource.readyWhenExpressions {
774+
if err != nil {
775+
return fmt.Errorf("failed to create CEL environment: %w", err)
776+
}
776777

777-
err = validateCELExpressionContext(instanceEnv, includeWhenExpression, conditionFieldNames)
778-
if err != nil {
779-
return fmt.Errorf("failed to validate expression context: '%s' %w", includeWhenExpression, err)
780-
}
781-
// create context
782-
context := map[string]*Resource{}
783-
// for now we will only support the instance context for condition expressions.
784-
// With this decision we will decide in creation time, and update time
785-
// If we'll be creating resources or not
786-
context["schema"] = &Resource{
787-
emulatedObject: &unstructured.Unstructured{
788-
Object: instanceEmulatedCopy.Object,
789-
},
790-
}
778+
resourceEmulatedCopy := resource.emulatedObject.DeepCopy()
779+
if resourceEmulatedCopy != nil && resourceEmulatedCopy.Object != nil {
780+
// ignore apiVersion and kind from readyWhenExpression context
781+
delete(resourceEmulatedCopy.Object, "apiVersion")
782+
delete(resourceEmulatedCopy.Object, "kind")
783+
}
784+
context := map[string]*Resource{}
785+
context[resource.id] = &Resource{
786+
emulatedObject: resourceEmulatedCopy,
787+
}
791788

792-
output, err := dryRunExpression(instanceEnv, includeWhenExpression, context)
793-
if err != nil {
794-
return fmt.Errorf("failed to dry-run expression %s: %w", includeWhenExpression, err)
795-
}
796-
if !krocel.IsBoolType(output) {
797-
return fmt.Errorf("output of condition expression %s can only be of type bool", includeWhenExpression)
798-
}
799-
}
789+
output, err := ensureExpression(env, expression, []string{resource.id}, context)
790+
if err != nil {
791+
return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
792+
}
793+
if !krocel.IsBoolType(output) {
794+
return fmt.Errorf("output of readyWhen expression %s can only be of type bool", expression)
800795
}
801796
}
797+
return nil
798+
}
802799

800+
// ensureIncludeWhenExpressions validates the includeWhen expressions in the resource
801+
func ensureIncludeWhenExpressions(env *cel.Env, context map[string]*Resource, resource *Resource) error {
802+
// We need to validate the CEL expressions in the resource.
803+
for _, expression := range resource.includeWhenExpressions {
804+
output, err := ensureExpression(env, expression, []string{resource.id}, context)
805+
if err != nil {
806+
return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
807+
}
808+
if !krocel.IsBoolType(output) {
809+
return fmt.Errorf("output of includeWhen expression %s can only be of type bool", expression)
810+
}
811+
}
803812
return nil
804813
}
814+
815+
// ensureExpression validates the CEL expression in the context of the resources
816+
func ensureExpression(env *cel.Env, expression string, resources []string, context map[string]*Resource) (ref.Val, error) {
817+
err := validateCELExpressionContext(env, expression, resources)
818+
if err != nil {
819+
return nil, fmt.Errorf("failed to validate expression %s: %w", expression, err)
820+
}
821+
822+
output, err := dryRunExpression(env, expression, context)
823+
if err != nil {
824+
return nil, fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
825+
}
826+
827+
return output, nil
828+
829+
}

Diff for: ‎pkg/graph/builder_test.go

+28
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,34 @@ func TestGraphBuilder_Validation(t *testing.T) {
155155
wantErr: true,
156156
errMsg: "failed to parse includeWhen expressions",
157157
},
158+
{
159+
name: "includeWhen expression reference a different resource",
160+
resourceGraphDefinitionOpts: []generator.ResourceGraphDefinitionOption{
161+
generator.WithSchema(
162+
"Test", "v1alpha1",
163+
map[string]interface{}{
164+
"name": "string",
165+
},
166+
nil,
167+
),
168+
generator.WithResource("vpc", map[string]interface{}{
169+
"apiVersion": "ec2.services.k8s.aws/v1alpha1",
170+
"kind": "VPC",
171+
"metadata": map[string]interface{}{
172+
"name": "test-vpc",
173+
},
174+
}, nil, []string{"invalid ! syntax"}),
175+
generator.WithResource("subnet", map[string]interface{}{
176+
"apiVersion": "ec2.services.k8s.aws/v1alpha1",
177+
"kind": "VPC",
178+
"metadata": map[string]interface{}{
179+
"name": "test-vpc",
180+
},
181+
}, nil, []string{"${vpc.status.state == 'available'}"}),
182+
},
183+
wantErr: true,
184+
errMsg: "failed to parse includeWhen expressions",
185+
},
158186
{
159187
name: "missing required field",
160188
resourceGraphDefinitionOpts: []generator.ResourceGraphDefinitionOption{

0 commit comments

Comments
 (0)