From b8b466230f0b87f7e7ac8c6cd31a33b848358a53 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Tue, 15 Oct 2024 20:39:02 -0700 Subject: [PATCH] Add readiness check for resources This feature ensures users are allowed to define when a resource is ready or not. For now this feature only allows to look within itself to see whether it's ready or not, so only its spec and status can be accessed using CEL fields. An example with deployement-service RG will be included in this PR --- api/v1alpha1/resource_group.go | 2 + api/v1alpha1/zz_generated.deepcopy.go | 5 ++ .../x.symphony.k8s.aws_resourcegroups.yaml | 5 ++ .../ec2-controller-instance.yaml | 2 +- .../eks-controller-instance.yaml | 4 +- examples/ack-eks-cluster/eks-cluster.yaml | 6 ++ .../deployment-service.yaml | 4 +- internal/celutil/conversions.go | 4 ++ .../instance/controller_reconcile.go | 14 ++++ internal/resourcegroup/builder.go | 69 +++++++++++++++--- internal/resourcegroup/ready_on_expression.go | 48 +++++++++++++ internal/resourcegroup/resource.go | 2 + internal/resourcegroup/runtime.go | 55 +++++++++++++++ internal/typesystem/parser/conditions.go | 45 ++++++++++++ internal/typesystem/parser/conditions_test.go | 70 +++++++++++++++++++ 15 files changed, 323 insertions(+), 12 deletions(-) create mode 100644 internal/resourcegroup/ready_on_expression.go create mode 100644 internal/typesystem/parser/conditions.go create mode 100644 internal/typesystem/parser/conditions_test.go diff --git a/api/v1alpha1/resource_group.go b/api/v1alpha1/resource_group.go index 9464b9fa..c1858472 100644 --- a/api/v1alpha1/resource_group.go +++ b/api/v1alpha1/resource_group.go @@ -72,6 +72,8 @@ type Resource struct { Name string `json:"name,omitempty"` // +kubebuilder:validation:Required Definition runtime.RawExtension `json:"definition,omitempty"` + // +kubebuilder:validation:Optional + ReadyOn []string `json:"readyOn,omitempty"` } // ResourceGroupStatus defines the observed state of ResourceGroup diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 18d0cbe9..d0762012 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -77,6 +77,11 @@ func (in *Definition) DeepCopy() *Definition { func (in *Resource) DeepCopyInto(out *Resource) { *out = *in in.Definition.DeepCopyInto(&out.Definition) + if in.ReadyOn != nil { + in, out := &in.ReadyOn, &out.ReadyOn + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Resource. diff --git a/config/crd/bases/x.symphony.k8s.aws_resourcegroups.yaml b/config/crd/bases/x.symphony.k8s.aws_resourcegroups.yaml index 2fe5b5e2..696fe0f9 100644 --- a/config/crd/bases/x.symphony.k8s.aws_resourcegroups.yaml +++ b/config/crd/bases/x.symphony.k8s.aws_resourcegroups.yaml @@ -110,6 +110,11 @@ spec: x-kubernetes-preserve-unknown-fields: true name: type: string + readyOn: + items: + type: string + x-kubernetes-preserve-unknown-fields: true + type: array required: - definition - name diff --git a/examples/ack-controller/ec2-controller/ec2-controller-instance.yaml b/examples/ack-controller/ec2-controller/ec2-controller-instance.yaml index f1b3bedc..a6649d77 100644 --- a/examples/ack-controller/ec2-controller/ec2-controller-instance.yaml +++ b/examples/ack-controller/ec2-controller/ec2-controller-instance.yaml @@ -1,5 +1,5 @@ apiVersion: x.symphony.k8s.aws/v1alpha1 -kind: EC2controller +kind: EC2Controller metadata: name: my-symphony-ec2-controller namespace: default diff --git a/examples/ack-controller/eks-controller/eks-controller-instance.yaml b/examples/ack-controller/eks-controller/eks-controller-instance.yaml index 728761b1..b082d795 100644 --- a/examples/ack-controller/eks-controller/eks-controller-instance.yaml +++ b/examples/ack-controller/eks-controller/eks-controller-instance.yaml @@ -1,5 +1,5 @@ apiVersion: x.symphony.k8s.aws/v1alpha1 -kind: EKScontroller +kind: EKSController metadata: name: my-symphony-eks-controller namespace: default @@ -12,7 +12,7 @@ spec: deployment: {} iamRole: oidcProvider: oidc.eks.us-west-2.amazonaws.com/id/50B8942190FBD3A2EF2BF6AB7D27B06B - iamRolePolicy: {} + iamPolicy: {} image: resources: requests: {} diff --git a/examples/ack-eks-cluster/eks-cluster.yaml b/examples/ack-eks-cluster/eks-cluster.yaml index 7325f53d..18549583 100644 --- a/examples/ack-eks-cluster/eks-cluster.yaml +++ b/examples/ack-eks-cluster/eks-cluster.yaml @@ -19,6 +19,8 @@ spec: # resources resources: - name: clusterVPC + readyOn: + - ${status.state == "available"} definition: apiVersion: ec2.services.k8s.aws/v1alpha1 kind: VPC @@ -56,6 +58,8 @@ spec: - destinationCIDRBlock: 0.0.0.0/0 gatewayID: ${clusterInternetGateway.status.internetGatewayID} - name: clusterSubnetA + readyOn: + - ${status.state == "available"} definition: apiVersion: ec2.services.k8s.aws/v1alpha1 kind: Subnet @@ -169,6 +173,8 @@ spec: ] } - name: cluster + readyOn: + - ${status.status == "ACTIVE"} definition: 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 fd6620c9..5db21e0b 100644 --- a/examples/deployment-service/deployment-service.yaml +++ b/examples/deployment-service/deployment-service.yaml @@ -14,13 +14,15 @@ spec: availableReplicas: ${deployment.status.availableReplicas} resources: - name: deployment + readyOn: + - ${spec.replicas == status.availableReplicas} definition: apiVersion: apps/v1 kind: Deployment metadata: name: ${spec.name} spec: - replicas: 1 + replicas: 50 selector: matchLabels: app: deployment diff --git a/internal/celutil/conversions.go b/internal/celutil/conversions.go index 93f5a9b4..0aaacd2f 100644 --- a/internal/celutil/conversions.go +++ b/internal/celutil/conversions.go @@ -48,3 +48,7 @@ func ConvertCELtoGo(v ref.Val) (interface{}, error) { return v.Value(), fmt.Errorf("unsupported type: %v", v.Type()) } } + +func IsBoolType(v ref.Val) bool { + return v.Type() == types.BoolType +} diff --git a/internal/controller/instance/controller_reconcile.go b/internal/controller/instance/controller_reconcile.go index 71958c2b..6dd610a0 100644 --- a/internal/controller/instance/controller_reconcile.go +++ b/internal/controller/instance/controller_reconcile.go @@ -213,6 +213,20 @@ func (igr *InstanceGraphReconciler) reconcileResource(ctx context.Context, resou } igr.runtime.SetLatestResource(resourceID, observed) + + log.V(1).Info("Checking if resource is Ready", "resource", resourceID) + if ready, err := igr.runtime.IsResourceReady(resourceID); err != nil { + log.V(1).Info("Resource not ready", "resource", resourceID) + resourceState.State = "WaitingForReadiness" + resourceState.Err = fmt.Errorf("resource not ready: %w", err) + return igr.delayedRequeue(resourceState.Err) + } else if !ready { + log.V(1).Info("Resource not ready", "resource", resourceID) + resourceState.State = "WaitingForReadiness" + resourceState.Err = fmt.Errorf("resource not ready") + return igr.delayedRequeue(resourceState.Err) + } + return igr.updateResource(ctx, rc, rUnstructured, observed, resourceID, resourceState) } diff --git a/internal/resourcegroup/builder.go b/internal/resourcegroup/builder.go index 581b552e..0227475e 100644 --- a/internal/resourcegroup/builder.go +++ b/internal/resourcegroup/builder.go @@ -174,17 +174,28 @@ func (b *GraphBuilder) NewResourceGroup(rg *v1alpha1.ResourceGroup) (*ResourceGr }) } } + // 6. Parse ReadyOn expressions + readyOn, err := parser.ParseConditionExpressions(rgResource.ReadyOn) + if err != nil { + return nil, fmt.Errorf("failed to parse readyOn expressions: %v", err) + } + + readyOnExpressions := []ReadyOnExpression{} + for _, e := range readyOn { + readyOnExpressions = append(readyOnExpressions, ReadyOnExpression{Resolved: false, Expression: e}) + } _, isNamespaced := namespacedResources[gvk] resources[rgResource.Name] = &Resource{ - ID: rgResource.Name, - GroupVersionKind: gvk, - Schema: resourceSchema, - EmulatedObject: emulatedResource, - OriginalObject: unstructuredResource, - Variables: resourceVariables, - Namespaced: isNamespaced, + ID: rgResource.Name, + GroupVersionKind: gvk, + Schema: resourceSchema, + EmulatedObject: emulatedResource, + OriginalObject: unstructuredResource, + Variables: resourceVariables, + ReadyOnExpressions: readyOnExpressions, + Namespaced: isNamespaced, } } @@ -335,6 +346,48 @@ func (b *GraphBuilder) validateResourceCELExpressions(resources map[string]*Reso return fmt.Errorf("failed to dry-run expression %s: %w", expression, err) } } + // validate readyOn 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 readyOnExpressions + // and the resourceExpressions. + for _, expression := range resource.ReadyOnExpressions { + fieldNames := getResourceTopLevelFieldNames(resource.Schema) + fieldEnv, err := celutil.NewEnvironement(celutil.WithResourceNames(fieldNames)) + if err != nil { + return fmt.Errorf("failed to create CEL environment: %w", err) + } + + err = b.validateCELExpressionContext(fieldEnv, expression.Expression, fieldNames) + if err != nil { + return fmt.Errorf("failed to validate expression context: '%s' %w", expression.Expression, err) + } + // create context + // add resource fields to the context + context := map[string]*Resource{} + for _, n := range fieldNames { + context[n] = &Resource{ + ID: n, + GroupVersionKind: resource.GroupVersionKind, + Schema: resource.Schema, + EmulatedObject: &unstructured.Unstructured{ + Object: resource.EmulatedObject.Object[n].(map[string]interface{}), + }, + } + } + + output, err := b.dryRunExpression(fieldEnv, expression.Expression, context) + + if err != nil { + return fmt.Errorf("failed to dry-run expression %s: %w", expression.Expression, err) + } + if !celutil.IsBoolType(output) { + return fmt.Errorf("output of readyOn expression %s can only be of type bool", expression.Expression) + } + } } } @@ -501,7 +554,7 @@ func (b *GraphBuilder) buildInstanceResource( instanceVariables := []*ResourceVariable{} for _, statusVariable := range statusVariables { // These variables needs to be injected into the status field of the instance. - path := ".status" + statusVariable.Path + path := ">status" + statusVariable.Path statusVariable.Path = path instanceVariables = append(instanceVariables, &ResourceVariable{ Kind: ResourceVariableKindDynamic, diff --git a/internal/resourcegroup/ready_on_expression.go b/internal/resourcegroup/ready_on_expression.go new file mode 100644 index 00000000..42cf3e7f --- /dev/null +++ b/internal/resourcegroup/ready_on_expression.go @@ -0,0 +1,48 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package resourcegroup + +import ( + "slices" + + "k8s.io/kube-openapi/pkg/validation/spec" +) + +type ReadyOnExpression struct { + Resolved bool + Expression string +} + +// This function goes through the schema of a resource +// and retrieves the field names fo rthe readOnly feature +// +// Currently we support all the top level fields besides +// apiVersion and kind. If we do see a case where they would +// be necessary for readyOns, we can include them here. +func getResourceTopLevelFieldNames(schema *spec.Schema) []string { + + fieldNames := []string{} + + if schema == nil || schema.Properties == nil { + return fieldNames + } + for k, _ := range schema.Properties { + if k != "apiVersion" && k != "kind" { + fieldNames = append(fieldNames, k) + } + } + + slices.Sort(fieldNames) + return fieldNames +} diff --git a/internal/resourcegroup/resource.go b/internal/resourcegroup/resource.go index edbf95d0..cd5f877f 100644 --- a/internal/resourcegroup/resource.go +++ b/internal/resourcegroup/resource.go @@ -35,6 +35,8 @@ type Resource struct { Variables []*ResourceVariable + ReadyOnExpressions []ReadyOnExpression + Dependencies []string Namespaced bool } diff --git a/internal/resourcegroup/runtime.go b/internal/resourcegroup/runtime.go index fd4622da..6f10a272 100644 --- a/internal/resourcegroup/runtime.go +++ b/internal/resourcegroup/runtime.go @@ -104,9 +104,64 @@ func (rt *RuntimeResourceGroup) CanResolveResource(resource string) bool { return false } } + return true } +func (rt *RuntimeResourceGroup) IsResourceReady(resourceID string) (bool, error) { + + expressions := rt.ResourceGroup.Resources[resourceID].ReadyOnExpressions + resource := rt.ResolvedResources[resourceID] + fieldNames := getResourceTopLevelFieldNames(rt.ResourceGroup.Resources[resourceID].Schema) + if len(expressions) == 0 { + return true, nil + } + + // fieldNames := []string{"status", "spec"} + env, err := celutil.NewEnvironement(&celutil.EnvironementOptions{ + ResourceNames: fieldNames, + }) + // we should not expect errors here since we already compiled it + // in the dryRun + if err != nil { + return false, fmt.Errorf("failed creating new Environment: %w", err) + } + context := map[string]interface{}{} + for _, n := range fieldNames { + if obj, ok := resource.Object[n]; ok { + context[n] = obj.(map[string]interface{}) + } + } + for _, e := range expressions { + if e.Resolved { + continue + } + ast, issues := env.Compile(e.Expression) + if issues != nil && issues.Err() != nil { + return false, fmt.Errorf("failed compiling expression %s: %w", e.Expression, err) + } + program, err := env.Program(ast) + if err != nil { + return false, fmt.Errorf("failed programming expression %s: %w", e.Expression, err) + } + + output, _, err := program.Eval(context) + if err != nil { + return false, fmt.Errorf("failed evaluating expression %s: %w", e.Expression, err) + } + out, err := celutil.ConvertCELtoGo(output) + if err != nil { + return false, fmt.Errorf("failed converting output %v: %w", output, err) + } + // keep checking for a false + if !out.(bool) { + return false, nil + } + e.Resolved = out.(bool) + } + return true, err +} + func (rt *RuntimeResourceGroup) SetLatestResource(name string, resource *unstructured.Unstructured) { rt.ResolvedResources[name] = resource } diff --git a/internal/typesystem/parser/conditions.go b/internal/typesystem/parser/conditions.go new file mode 100644 index 00000000..dc6e18fb --- /dev/null +++ b/internal/typesystem/parser/conditions.go @@ -0,0 +1,45 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package parser + +import ( + "fmt" + "strings" +) + +// This function parses resource condition expressions. +// These expressions need to be standalone expressions +// so, this function also does some validation. +// At the end we return the expressions with '${}' removed +// +// To be honest I wouldn't necessarily call it parse, since +// we are mostly just validating, without caring what's in +// the expression. Maybe we can rename it in the future 🤔 +func ParseConditionExpressions(conditions []string) ([]string, error) { + + expressions := make([]string, 0, len(conditions)) + + for _, e := range conditions { + ok, err := isOneShotExpression(e) + if err != nil { + return nil, err + } + if !ok { + return nil, fmt.Errorf("single expression per line allowed") + } + expressions = append(expressions, strings.Trim(e, "${}")) + } + + return expressions, nil +} diff --git a/internal/typesystem/parser/conditions_test.go b/internal/typesystem/parser/conditions_test.go new file mode 100644 index 00000000..da7a023e --- /dev/null +++ b/internal/typesystem/parser/conditions_test.go @@ -0,0 +1,70 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package parser + +import ( + "testing" +) + +func TestParseReadyOn(t *testing.T) { + testCases := []struct { + name string + expression []string + expectedError string + }{ + { + name: "Two expressions", + expression: []string{"${hello}${goodbye}"}, + expectedError: "single expression per line allowed", + }, + { + name: "With Postfix", + expression: []string{"${hello}-world"}, + expectedError: "single expression per line allowed", + }, + { + name: "With Prefix", + expression: []string{"hello-${world}"}, + expectedError: "single expression per line allowed", + }, + { + name: "Standalone expression", + expression: []string{"${hello}"}, + expectedError: "", + }, + { + name: "Complex standalone expression that works", + expression: []string{"${hello + world - someone = whoever[else]isNone dum dum {pop}out-here}"}, + expectedError: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + _, err := ParseConditionExpressions(tc.expression) + + if tc.expectedError == "" { + if err != nil { + t.Error("Expected no error, but got '%w'", err) + } + } else { + if err == nil { + t.Errorf("Expected error '%s', but got nil", tc.expectedError) + } else if err.Error() != tc.expectedError { + t.Errorf("Expected error: %s\nError we got: %s", tc.expectedError, err.Error()) + } + } + }) + } +}