Merge pull request #96 from awslabs/change/readiness-variable

a-hilaly · web-flow · commit e85271321f53 · 2024-11-08T17:06:19.000-08:00
Make readyWhen variables start with their own name
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",
