Make readyWhen variables start with their own name

michaelhtm · michaelhtm · commit ee9843b1d753 · 2024-11-08T17:00:48.000-08:00
This change ensures that all variables referenced in readyWhen
expressions are referencing their own resource by name

This change will also allow us in the future to reference other
resource fields if needed
changing from:
```yaml
name: cluster
readyWhen:
- ${status.state == 'available'}
```

to
```yaml
name: cluster
readyWhen:
- ${cluster.status.state == 'available'}
```
diff --git a/examples/ack-eks-cluster/eks-cluster.yaml b/examples/ack-eks-cluster/eks-cluster.yaml
@@ -20,7 +20,7 @@ spec:
   resources:
   - name: clusterVPC
     readyWhen:
-    - ${status.state == "available"}
+    - ${clusterVPC.status.state == "available"}
     template:
       apiVersion: ec2.services.k8s.aws/v1alpha1
       kind: VPC
@@ -59,7 +59,7 @@ spec:
             gatewayID: ${clusterInternetGateway.status.internetGatewayID}
   - name: clusterSubnetA
     readyWhen:
-    - ${status.state == "available"}
+    - ${clusterSubnetA.status.state == "available"}
     template:
       apiVersion: ec2.services.k8s.aws/v1alpha1
       kind: Subnet
@@ -174,7 +174,7 @@ spec:
           }
   - name: cluster
     readyWhen:
-    - ${status.status == "ACTIVE"}
+    - ${cluster.status.status == "ACTIVE"}
     template:
       apiVersion: eks.services.k8s.aws/v1alpha1
       kind: Cluster
diff --git a/examples/deployment-service/deployment-service.yaml b/examples/deployment-service/deployment-service.yaml
@@ -17,7 +17,7 @@ spec:
   resources:
   - name: deployment
     readyWhen:
-      - ${spec.replicas == status.availableReplicas}
+      - ${deployment.spec.replicas == deployment.status.availableReplicas}
     template:
       apiVersion: apps/v1
       kind: Deployment
diff --git a/internal/graph/builder.go b/internal/graph/builder.go
@@ -731,27 +731,26 @@ func validateResourceCELExpressions(resources map[string]*Resource, instance *Re
 			// I would also suggest separating the dryRuns of readyWhenExpressions
 			// and the resourceExpressions.
 			for _, readyWhenExpression := range resource.readyWhenExpressions {
-				fieldNames := schema.GetResourceTopLevelFieldNames(resource.schema)
-				fieldEnv, err := scel.DefaultEnvironment(scel.WithResourceNames(fieldNames))
+				fieldEnv, err := scel.DefaultEnvironment(scel.WithResourceNames([]string{resource.id}))
 				if err != nil {
 					return fmt.Errorf("failed to create CEL environment: %w", err)
 				}
 
-				err = validateCELExpressionContext(fieldEnv, readyWhenExpression, fieldNames)
+				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{}
-				for _, n := range fieldNames {
-					context[n] = &Resource{
-						emulatedObject: &unstructured.Unstructured{
-							Object: resource.emulatedObject.Object[n].(map[string]interface{}),
-						},
-					}
+				context[resource.id] = &Resource{
+					emulatedObject: resourceEmulatedCopy,
 				}
-
 				output, err := dryRunExpression(fieldEnv, readyWhenExpression, context)
 
 				if err != nil {
diff --git a/internal/graph/builder_test.go b/internal/graph/builder_test.go
@@ -209,7 +209,7 @@ func TestGraphBuilder_Validation(t *testing.T) {
 						"enableDNSSupport":   true,
 						"enableDNSHostnames": true,
 					},
-				}, []string{"${status.state == 'available'}"}, nil),
+				}, []string{"${vpc.status.state == 'available'}"}, nil),
 				generator.WithResource("subnet1", map[string]interface{}{
 					"apiVersion": "ec2.services.k8s.aws/v1alpha1",
 					"kind":       "Subnet",
@@ -220,7 +220,7 @@ func TestGraphBuilder_Validation(t *testing.T) {
 						"cidrBlock": "10.0.1.0/24",
 						"vpcID":     "${vpc.status.vpcID}",
 					},
-				}, []string{"${status.state == 'available'}"}, []string{"${spec.enableSubnets == true}"}),
+				}, []string{"${subnet1.status.state == 'available'}"}, []string{"${spec.enableSubnets == true}"}),
 				generator.WithResource("subnet2", map[string]interface{}{
 					"apiVersion": "ec2.services.k8s.aws/v1alpha1",
 					"kind":       "Subnet",
@@ -231,7 +231,7 @@ func TestGraphBuilder_Validation(t *testing.T) {
 						"cidrBlock": "10.0.127.0/24",
 						"vpcID":     "${vpc.status.vpcID}",
 					},
-				}, []string{"${status.state == 'available'}"}, []string{"${spec.enableSubnets}"})},
+				}, []string{"${subnet2.status.state == 'available'}"}, []string{"${spec.enableSubnets}"})},
 			wantErr: false,
 		},
 		{
@@ -1001,8 +1001,8 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
 						"cidrBlocks": []interface{}{"10.0.0.0/16"},
 					},
 				}, []string{
-					"${status.state == 'available'}",
-					"${status.vpcID != ''}",
+					"${vpc.status.state == 'available'}",
+					"${vpc.status.vpcID != ''}",
 				}, nil),
 				// Resource with mix of static and dynamic expressions
 				generator.WithResource("subnet", map[string]interface{}{
@@ -1021,7 +1021,7 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
 							},
 						},
 					},
-				}, []string{"${status.state == 'available'}"}, nil),
+				}, []string{"${subnet.status.state == 'available'}"}, nil),
 				// Non-standalone expressions
 				generator.WithResource("cluster", map[string]interface{}{
 					"apiVersion": "eks.services.k8s.aws/v1alpha1",
@@ -1038,7 +1038,7 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
 						},
 					},
 				}, []string{
-					"${status.status == 'ACTIVE'}",
+					"${cluster.status.status == 'ACTIVE'}",
 				}, []string{
 					"${spec.createMonitoring}",
 				}),
@@ -1075,7 +1075,7 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
 						},
 					},
 				}, []string{
-					"${status.phase == 'Running'}",
+					"${monitor.status.phase == 'Running'}",
 				}, []string{
 					"${spec.createMonitoring == true}",
 				}),
@@ -1091,8 +1091,8 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
 				vpc := g.Resources["vpc"]
 				assert.Empty(t, vpc.variables)
 				assert.Equal(t, []string{
-					"status.state == 'available'",
-					"status.vpcID != ''",
+					"vpc.status.state == 'available'",
+					"vpc.status.vpcID != ''",
 				}, vpc.GetReadyWhenExpressions())
 				assert.Empty(t, vpc.GetIncludeWhenExpressions())
 
diff --git a/internal/graph/variable/variable.go b/internal/graph/variable/variable.go
@@ -112,7 +112,7 @@ const (
 	// For example:
 	//   name: cluster
 	//   readyWhen:
-	//   - ${status.status == "Active"}
+	//   - ${cluster.status.status == "Active"}
 	ResourceVariableKindReadyWhen ResourceVariableKind = "readyWhen"
 	// ResourceVariableKindIncludeWhen represents an includeWhen variable.
 	// IncludeWhen variables are resolved at the beginning of the execution and
diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go
@@ -472,20 +472,16 @@ func (rt *ResourceGroupRuntime) IsResourceReady(resourceID string) (bool, string
 		return true, "", nil
 	}
 
-	topLevelFields := rt.resources[resourceID].GetTopLevelFields()
-
 	// we should not expect errors here since we already compiled it
 	// in the dryRun
-	env, err := scel.DefaultEnvironment(scel.WithResourceNames(topLevelFields))
+	env, err := scel.DefaultEnvironment(scel.WithResourceNames([]string{resourceID}))
 	if err != nil {
 		return false, "", fmt.Errorf("failed creating new Environment: %w", err)
 	}
-	context := map[string]interface{}{}
-	for _, n := range topLevelFields {
-		if obj, ok := observed.Object[n]; ok {
-			context[n] = obj.(map[string]interface{})
-		}
+	context := map[string]interface{}{
+		resourceID: observed.Object,
 	}
+
 	for _, expression := range expressions {
 		out, err := evaluateExpression(env, context, expression)
 		if err != nil {
diff --git a/internal/runtime/runtime_test.go b/internal/runtime/runtime_test.go
@@ -2183,16 +2183,15 @@ func Test_IsResourceReady(t *testing.T) {
 		{
 			name: "resource not resolved",
 			resource: newTestResource(
-				withReadyExpressions([]string{"status.ready"}),
+				withReadyExpressions([]string{"test.status.ready"}),
 			),
 			want:       false,
 			wantReason: "resource test is not resolved",
 		},
 		{
 			name: "ready expression true",
 			resource: newTestResource(
-				withReadyExpressions([]string{"status.ready"}),
-				withTopLevelFields([]string{"status"}),
+				withReadyExpressions([]string{"test.status.ready"}),
 			),
 			resolvedObject: map[string]interface{}{
 				"status": map[string]interface{}{
@@ -2204,22 +2203,20 @@ func Test_IsResourceReady(t *testing.T) {
 		{
 			name: "ready expression false",
 			resource: newTestResource(
-				withReadyExpressions([]string{"status.ready"}),
-				withTopLevelFields([]string{"status"}),
+				withReadyExpressions([]string{"test.status.ready"}),
 			),
 			resolvedObject: map[string]interface{}{
 				"status": map[string]interface{}{
 					"ready": false,
 				},
 			},
 			want:       false,
-			wantReason: "expression status.ready evaluated to false",
+			wantReason: "expression test.status.ready evaluated to false",
 		},
 		{
 			name: "invalid expression",
 			resource: newTestResource(
 				withReadyExpressions([]string{"invalid )"}),
-				withTopLevelFields([]string{"status"}),
 			),
 			resolvedObject: map[string]interface{}{},
 			want:           false,
@@ -2228,8 +2225,7 @@ func Test_IsResourceReady(t *testing.T) {
 		{
 			name: "multiple expressions all true",
 			resource: newTestResource(
-				withReadyExpressions([]string{"status.ready", "status.healthy && status.count > 10", "status.count > 5"}),
-				withTopLevelFields([]string{"status"}),
+				withReadyExpressions([]string{"test.status.ready", "test.status.healthy && test.status.count > 10", "test.status.count > 5"}),
 			),
 			resolvedObject: map[string]interface{}{
 				"status": map[string]interface{}{
@@ -2243,8 +2239,7 @@ func Test_IsResourceReady(t *testing.T) {
 		{
 			name: "multiple expressions one false",
 			resource: newTestResource(
-				withReadyExpressions([]string{"status.ready", "status.healthy"}),
-				withTopLevelFields([]string{"status"}),
+				withReadyExpressions([]string{"test.status.ready", "test.status.healthy"}),
 			),
 			resolvedObject: map[string]interface{}{
 				"status": map[string]interface{}{
@@ -2253,7 +2248,7 @@ func Test_IsResourceReady(t *testing.T) {
 				},
 			},
 			want:       false,
-			wantReason: "expression status.healthy evaluated to false",
+			wantReason: "expression test.status.healthy evaluated to false",
 		},
 	}
 
diff --git a/test/integration/suites/core/readiness_test.go b/test/integration/suites/core/readiness_test.go
@@ -51,7 +51,8 @@ var _ = Describe("Readiness", func() {
 		Expect(env.Client.Create(ctx, ns)).To(Succeed())
 	})
 
-	It("should wait for deployment to have spec.replicas == status.availableReplicas before creating service", func() {
+	It(`should wait for deployment to have deployment.spec.replicas 
+		== deployment.status.availableReplicas before creating service`, func() {
 		rg := generator.NewResourceGroup("test-readiness",
 			generator.WithNamespace(namespace),
 			generator.WithSchema(
@@ -97,7 +98,7 @@ var _ = Describe("Readiness", func() {
 						},
 					},
 				},
-			}, []string{"${spec.replicas == status.availableReplicas}"}, nil),
+			}, []string{"${deployment.spec.replicas == deployment.status.availableReplicas}"}, nil),
 			// ServiceB - depends on deploymentA and deploymentB
 			generator.WithResource("service", map[string]interface{}{
 				"apiVersion": "v1",
