From 9568da97acee7111c86d9160699fef6767da33fd Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Fri, 8 Nov 2024 14:28:37 -0800 Subject: [PATCH] Make readyWhen variables start with their own name 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'} ``` --- examples/ack-eks-cluster/eks-cluster.yaml | 6 +++--- .../deployment-service.yaml | 2 +- internal/graph/builder.go | 19 +++++++++--------- internal/graph/builder_test.go | 20 +++++++++---------- internal/graph/variable/variable.go | 2 +- internal/runtime/runtime.go | 12 ++++------- internal/runtime/runtime_test.go | 19 +++++++----------- .../integration/suites/core/readiness_test.go | 5 +++-- 8 files changed, 38 insertions(+), 47 deletions(-) diff --git a/examples/ack-eks-cluster/eks-cluster.yaml b/examples/ack-eks-cluster/eks-cluster.yaml index 7aab749e..d69597e7 100644 --- 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 index 58635eeb..23baf1b9 100644 --- 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 index b659c156..35ba2137 100644 --- 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 index e09fc609..31a96ce5 100644 --- 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 index 5acee684..2df92cf0 100644 --- 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 index 840dd292..d0706cfb 100644 --- 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 index af6ac9ed..0ac777f4 100644 --- a/internal/runtime/runtime_test.go +++ b/internal/runtime/runtime_test.go @@ -2183,7 +2183,7 @@ 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", @@ -2191,8 +2191,7 @@ func Test_IsResourceReady(t *testing.T) { { 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,8 +2203,7 @@ 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{}{ @@ -2213,13 +2211,12 @@ func Test_IsResourceReady(t *testing.T) { }, }, 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 index fbd1a480..70fb6363 100644 --- 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",