refactor: DryRunExpressions in builder.go

pkg/graph/builder.go

@@ -684,12 +684,11 @@ func extractDependencies(env *cel.Env, expression string, resourceNames []string
 // we evaluate B's CEL expressions against 2 emulated resources A and C, and so
 // on.
 func validateResourceCELExpressions(resources map[string]*Resource, instance *Resource) error {
-	resourceNames := maps.Keys(resources)
+	resourceIDs := maps.Keys(resources)
 	// We also want to allow users to refer to the instance spec in their expressions.
-	resourceNames = append(resourceNames, "schema")
-	conditionFieldNames := []string{"schema"}
+	resourceIDs = append(resourceIDs, "schema")
 
-	env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(resourceNames))
+	env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(resourceIDs))
 	if err != nil {
 		return fmt.Errorf("failed to create CEL environment: %w", err)
 	}
@@ -700,105 +699,131 @@ func validateResourceCELExpressions(resources map[string]*Resource, instance *Re
 		delete(instanceEmulatedCopy.Object, "status")
 	}
 
+	// create includeWhenContext
+	includeWhenContext := map[string]*Resource{}
+	// for now we will only support the instance context for includeWhen expressions.
+	// With this decision we will decide in creation time, and update time
+	// If we'll be creating resources or not
+	includeWhenContext["schema"] = &Resource{
+		emulatedObject: &unstructured.Unstructured{
+			Object: instanceEmulatedCopy.Object,
+		},
+	}
+
+	// create expressionsContext
+	expressionContext := map[string]*Resource{}
+	// add instance spec to the context
+	expressionContext["schema"] = &Resource{
+		emulatedObject: &unstructured.Unstructured{
+			Object: instanceEmulatedCopy.Object,
+		},
+	}
+	// include all resources, and remove individual ones
+	// during the validation
+	// this is done to avoid having to create a new context for each resource
+	for resourceName, contextResource := range resources {
+		expressionContext[resourceName] = contextResource
+	}
+
 	for _, resource := range resources {
-		for _, resourceVariable := range resource.variables {
-			for _, expression := range resourceVariable.Expressions {
-				err := validateCELExpressionContext(env, expression, resourceNames)
-				if err != nil {
-					return fmt.Errorf("failed to validate expression context: '%s' %w", expression, err)
-				}
+		// exclude resource from the context
+		delete(expressionContext, resource.id)
 
-				// create context
-				context := map[string]*Resource{}
-				for resourceName, contextResource := range resources {
-					// exclude the resource we are validating
-					if resourceName != resource.id {
-						context[resourceName] = contextResource
-					}
-				}
-				// add instance spec to the context
-				context["schema"] = &Resource{
-					emulatedObject: &unstructured.Unstructured{
-						Object: instanceEmulatedCopy.Object,
-					},
-				}
+		err := ensureResourceExpressions(env, expressionContext, resource)
+		if err != nil {
+			return fmt.Errorf("failed to ensure resource %s expressions: %w", resource.id, err)
+		}
 
-				_, err = dryRunExpression(env, expression, context)
-				if err != nil {
-					return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
-				}
-			}
+		err = ensureReadyWhenExpressions(resource)
+		if err != nil {
+			return fmt.Errorf("failed to ensure resource %s readyWhen expressions: %w", resource.id, err)
+		}
 
-			// validate readyWhen Expressions for resource
-			// Only accepting expressions accessing the status and spec for now
-			// and need to evaluate to a boolean type
-			//
-			// TODO(michaelhtm) It shares some of the logic with the loop from above..maybe
-			// we can refactor them or put it in one function.
-			// I would also suggest separating the dryRuns of readyWhenExpressions
-			// and the resourceExpressions.
-			for _, readyWhenExpression := range resource.readyWhenExpressions {
-				fieldEnv, err := krocel.DefaultEnvironment(krocel.WithResourceIDs([]string{resource.id}))
-				if err != nil {
-					return fmt.Errorf("failed to create CEL environment: %w", err)
-				}
+		err = ensureIncludeWhenExpressions(env, includeWhenContext, resource)
+		if err != nil {
+			return fmt.Errorf("failed to ensure resource %s includeWhen expressions: %w", resource.id, err)
+		}
 
-				err = validateCELExpressionContext(fieldEnv, readyWhenExpression, []string{resource.id})
-				if err != nil {
-					return fmt.Errorf("failed to validate expression context: '%s' %w", readyWhenExpression, err)
-				}
-				// create context
-				// add resource fields to the context
-				resourceEmulatedCopy := resource.emulatedObject.DeepCopy()
-				if resourceEmulatedCopy != nil && resourceEmulatedCopy.Object != nil {
-					delete(resourceEmulatedCopy.Object, "apiVersion")
-					delete(resourceEmulatedCopy.Object, "kind")
-				}
-				context := map[string]*Resource{}
-				context[resource.id] = &Resource{
-					emulatedObject: resourceEmulatedCopy,
-				}
-				output, err := dryRunExpression(fieldEnv, readyWhenExpression, context)
+		// include the resource back to the context
+		expressionContext[resource.id] = resource
+	}
 
-				if err != nil {
-					return fmt.Errorf("failed to dry-run expression %s: %w", readyWhenExpression, err)
-				}
-				if !krocel.IsBoolType(output) {
-					return fmt.Errorf("output of readyWhen expression %s can only be of type bool", readyWhenExpression)
-				}
+	return nil
+}
+
+// ensureResourceExpressions validates the CEL expressions in the resource
+// against the resources defined in the resource graph definition.
+func ensureResourceExpressions(env *cel.Env, context map[string]*Resource, resource *Resource) error {
+	// We need to validate the CEL expressions in the resource.
+	for _, resourceVariable := range resource.variables {
+		for _, expression := range resourceVariable.Expressions {
+			_, err := ensureExpression(env, expression, []string{resource.id}, context)
+			if err != nil {
+				return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
 			}
+		}
+	}
+	return nil
+}
 
-			for _, includeWhenExpression := range resource.includeWhenExpressions {
-				instanceEnv, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(resourceNames))
-				if err != nil {
-					return fmt.Errorf("failed to create CEL environment: %w", err)
-				}
+// ensureReadyWhenExpressions validates the readyWhen expressions in the resource
+// against the resources defined in the resource graph definition.
+func ensureReadyWhenExpressions(resource *Resource) error {
+	env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs([]string{resource.id}))
+	for _, expression := range resource.readyWhenExpressions {
+		if err != nil {
+			return fmt.Errorf("failed to create CEL environment: %w", err)
+		}
 
-				err = validateCELExpressionContext(instanceEnv, includeWhenExpression, conditionFieldNames)
-				if err != nil {
-					return fmt.Errorf("failed to validate expression context: '%s' %w", includeWhenExpression, err)
-				}
-				// create context
-				context := map[string]*Resource{}
-				// for now we will only support the instance context for condition expressions.
-				// With this decision we will decide in creation time, and update time
-				// If we'll be creating resources or not
-				context["schema"] = &Resource{
-					emulatedObject: &unstructured.Unstructured{
-						Object: instanceEmulatedCopy.Object,
-					},
-				}
+		resourceEmulatedCopy := resource.emulatedObject.DeepCopy()
+		if resourceEmulatedCopy != nil && resourceEmulatedCopy.Object != nil {
+			// ignore apiVersion and kind from readyWhenExpression context
+			delete(resourceEmulatedCopy.Object, "apiVersion")
+			delete(resourceEmulatedCopy.Object, "kind")
+		}
+		context := map[string]*Resource{}
+		context[resource.id] = &Resource{
+			emulatedObject: resourceEmulatedCopy,
+		}
 
-				output, err := dryRunExpression(instanceEnv, includeWhenExpression, context)
-				if err != nil {
-					return fmt.Errorf("failed to dry-run expression %s: %w", includeWhenExpression, err)
-				}
-				if !krocel.IsBoolType(output) {
-					return fmt.Errorf("output of condition expression %s can only be of type bool", includeWhenExpression)
-				}
-			}
+		output, err := ensureExpression(env, expression, []string{resource.id}, context)
+		if err != nil {
+			return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
+		}
+		if !krocel.IsBoolType(output) {
+			return fmt.Errorf("output of readyWhen expression %s can only be of type bool", expression)
 		}
 	}
+	return nil
+}
 
+// ensureIncludeWhenExpressions validates the includeWhen expressions in the resource
+func ensureIncludeWhenExpressions(env *cel.Env, context map[string]*Resource, resource *Resource) error {
+	// We need to validate the CEL expressions in the resource.
+	for _, expression := range resource.includeWhenExpressions {
+		output, err := ensureExpression(env, expression, []string{resource.id}, context)
+		if err != nil {
+			return fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
+		}
+		if !krocel.IsBoolType(output) {
+			return fmt.Errorf("output of includeWhen expression %s can only be of type bool", expression)
+		}
+	}
 	return nil
 }
+
+// ensureExpression validates the CEL expression in the context of the resources
+func ensureExpression(env *cel.Env, expression string, resources []string, context map[string]*Resource) (ref.Val, error) {
+	err := validateCELExpressionContext(env, expression, resources)
+	if err != nil {
+		return nil, fmt.Errorf("failed to validate expression %s: %w", expression, err)
+	}
+
+	output, err := dryRunExpression(env, expression, context)
+	if err != nil {
+		return nil, fmt.Errorf("failed to dry-run expression %s: %w", expression, err)
+	}
+
+	return output, nil
+
+}

pkg/graph/builder_test.go

@@ -155,6 +155,34 @@ func TestGraphBuilder_Validation(t *testing.T) {
 			wantErr: true,
 			errMsg:  "failed to parse includeWhen expressions",
 		},
+		{
+			name: "includeWhen expression reference a different resource",
+			resourceGraphDefinitionOpts: []generator.ResourceGraphDefinitionOption{
+				generator.WithSchema(
+					"Test", "v1alpha1",
+					map[string]interface{}{
+						"name": "string",
+					},
+					nil,
+				),
+				generator.WithResource("vpc", map[string]interface{}{
+					"apiVersion": "ec2.services.k8s.aws/v1alpha1",
+					"kind":       "VPC",
+					"metadata": map[string]interface{}{
+						"name": "test-vpc",
+					},
+				}, nil, []string{"invalid ! syntax"}),
+				generator.WithResource("subnet", map[string]interface{}{
+					"apiVersion": "ec2.services.k8s.aws/v1alpha1",
+					"kind":       "VPC",
+					"metadata": map[string]interface{}{
+						"name": "test-vpc",
+					},
+				}, nil, []string{"${vpc.status.state == 'available'}"}),
+			},
+			wantErr: true,
+			errMsg:  "failed to parse includeWhen expressions",
+		},
 		{
 			name: "missing required field",
 			resourceGraphDefinitionOpts: []generator.ResourceGraphDefinitionOption{