Skip to content

Commit

Permalink
Make readyWhen variables start with their own name
Browse files Browse the repository at this point in the history
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'}
```
  • Loading branch information
michaelhtm authored and a-hilaly committed Nov 9, 2024
1 parent 785b04f commit 9568da9
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 47 deletions.
6 changes: 3 additions & 3 deletions examples/ack-eks-cluster/eks-cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -174,7 +174,7 @@ spec:
}
- name: cluster
readyWhen:
- ${status.status == "ACTIVE"}
- ${cluster.status.status == "ACTIVE"}
template:
apiVersion: eks.services.k8s.aws/v1alpha1
kind: Cluster
Expand Down
2 changes: 1 addition & 1 deletion examples/deployment-service/deployment-service.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 9 additions & 10 deletions internal/graph/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
20 changes: 10 additions & 10 deletions internal/graph/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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,
},
{
Expand Down Expand Up @@ -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{}{
Expand All @@ -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",
Expand All @@ -1038,7 +1038,7 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
},
},
}, []string{
"${status.status == 'ACTIVE'}",
"${cluster.status.status == 'ACTIVE'}",
}, []string{
"${spec.createMonitoring}",
}),
Expand Down Expand Up @@ -1075,7 +1075,7 @@ func TestGraphBuilder_ExpressionParsing(t *testing.T) {
},
},
}, []string{
"${status.phase == 'Running'}",
"${monitor.status.phase == 'Running'}",
}, []string{
"${spec.createMonitoring == true}",
}),
Expand All @@ -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())

Expand Down
2 changes: 1 addition & 1 deletion internal/graph/variable/variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 4 additions & 8 deletions internal/runtime/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
19 changes: 7 additions & 12 deletions internal/runtime/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{
Expand All @@ -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,
Expand All @@ -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{}{
Expand All @@ -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{}{
Expand All @@ -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",
},
}

Expand Down
5 changes: 3 additions & 2 deletions test/integration/suites/core/readiness_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 9568da9

Please sign in to comment.