Skip to content

Commit

Permalink
Replace CELExpressionExtractor with a more robust parser implementa…
Browse files Browse the repository at this point in the history
…tion (#34)

This patch improves our CEL expression parsing system, replacing
the regex based CELExpressionsExtractor with a more robust
parser. The previous implementation relied heavily on regulars
expressions, which proved to be ineffective for handling complex nested
structures and led to limitations in accurat parsing and extracting
CEL expressions from kubernetes resource definitions (`resources[*].definition`)

The new parser uses a token aware string traversal approach. It scans
input character by character recognizing `"${"` and `"}"` tokens. It tracks
brace nesting to handle complex structures, including nestde dictionaries
and conditionals.

In a nutshell, this patch makes the following changes:

- Remove the celextractor package and its regex based functionality
- Introduce a new parser package with the token aware parsing algorithm
  - Support both schemaless and structured parsing.
- Update the `resourcegroup`/`controller` packages to integrate the new parser
- Add unit tests and reach 87.5% coverage.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • Loading branch information
a-hilaly authored Sep 29, 2024
1 parent dfd3b9c commit dab76ce
Show file tree
Hide file tree
Showing 12 changed files with 1,273 additions and 298 deletions.
2 changes: 0 additions & 2 deletions cmd/controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
"github.com/aws-controllers-k8s/symphony/internal/dynamiccontroller"
"github.com/aws-controllers-k8s/symphony/internal/kubernetes"
"github.com/aws-controllers-k8s/symphony/internal/resourcegroup"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/celextractor"
//+kubebuilder:scaffold:imports
)

Expand Down Expand Up @@ -133,7 +132,6 @@ func main() {

resourceGroupGraphBuilder, err := resourcegroup.NewResourceGroupBuilder(
kConfig,
celextractor.NewCELExpressionParser(),
)
if err != nil {
setupLog.Error(err, "unable to create resource group graph builder")
Expand Down
11 changes: 4 additions & 7 deletions internal/resourcegroup/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,14 @@ import (
"github.com/aws-controllers-k8s/symphony/internal/celutil"
"github.com/aws-controllers-k8s/symphony/internal/dag"
"github.com/aws-controllers-k8s/symphony/internal/k8smetadata"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/celextractor"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/celinspector"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/emulator"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/parser"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/simpleschema"
)

func NewResourceGroupBuilder(
clientConfig *rest.Config,
celExpressionsExtractor *celextractor.CELExpressionsExtractor,
) (*GraphBuilder, error) {
schemaResolver, err := newCombinedResolver(clientConfig)
if err != nil {
Expand All @@ -49,15 +48,13 @@ func NewResourceGroupBuilder(
rgBuilder := &GraphBuilder{
resourceEmulator: resourceEmulator,
schemaResolver: schemaResolver,
celParser: celExpressionsExtractor,
}
return rgBuilder, nil
}

type GraphBuilder struct {
schemaResolver resolver.SchemaResolver
resourceEmulator *emulator.Emulator
celParser *celextractor.CELExpressionsExtractor
}

func (b *GraphBuilder) NewResourceGroup(rg *v1alpha1.ResourceGroup) (*ResourceGroup, error) {
Expand Down Expand Up @@ -125,7 +122,7 @@ func (b *GraphBuilder) NewResourceGroup(rg *v1alpha1.ResourceGroup) (*ResourceGr
}

// 5. Extract CEL celExpressions from the schema.
celExpressions, err := b.celParser.ExtractExpressions(unstructuredResource, resourceSchema)
celExpressions, err := parser.ParseResource(unstructuredResource, resourceSchema)
if err != nil {
return nil, fmt.Errorf("failed to extract CEL expressions from schema for resource %s: %w", rgResource.Name, err)
}
Expand Down Expand Up @@ -501,7 +498,7 @@ func (b *GraphBuilder) buildInstanceSpecSchema(definition *v1alpha1.Definition)
return instanceSchema, nil
}

func (b *GraphBuilder) buildStatusSchema(definition *v1alpha1.Definition, resources map[string]*Resource) (*extv1.JSONSchemaProps, []celextractor.ExpressionField, error) {
func (b *GraphBuilder) buildStatusSchema(definition *v1alpha1.Definition, resources map[string]*Resource) (*extv1.JSONSchemaProps, []parser.ExpressionField, error) {
// The instance resource has a schema defined using the "SimpleSchema" format.
unstructuredStatus := map[string]interface{}{}
err := yaml.UnmarshalStrict(definition.Status.Raw, &unstructuredStatus)
Expand All @@ -511,7 +508,7 @@ func (b *GraphBuilder) buildStatusSchema(definition *v1alpha1.Definition, resour

// different from the instance spec, the status schema is inferred from the
// CEL expressions in the status field.
extracted, err := celextractor.NewCELExpressionParser().ExtractExpressionsUnstructured(unstructuredStatus)
extracted, err := parser.ParseSchemalessResource(unstructuredStatus)
if err != nil {
return nil, nil, fmt.Errorf("failed to extract CEL expressions from status: %w", err)
}
Expand Down
5 changes: 2 additions & 3 deletions internal/resourcegroup/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@
package resourcegroup

import (
"github.com/aws-controllers-k8s/symphony/internal/typesystem/parser"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/kube-openapi/pkg/validation/spec"

"github.com/aws-controllers-k8s/symphony/internal/typesystem/celextractor"
)

// Resource represents a resource in a resource group, it hholds
Expand Down Expand Up @@ -48,7 +47,7 @@ const (
)

type ResourceVariable struct {
celextractor.ExpressionField
parser.ExpressionField
Kind ResourceVariableKind
}

Expand Down
4 changes: 2 additions & 2 deletions internal/resourcegroup/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/aws-controllers-k8s/symphony/internal/celutil"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/celextractor"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/parser"
"github.com/aws-controllers-k8s/symphony/internal/typesystem/resolver"
)

Expand Down Expand Up @@ -236,7 +236,7 @@ func (rt *RuntimeResourceGroup) ResolveResource(resource string) error {
}

rs := resolver.NewResolver(rt.Resources[resource].Object, exprValues)
exprFields := make([]celextractor.ExpressionField, len(rt.ResourceGroup.Resources[resource].Variables))
exprFields := make([]parser.ExpressionField, len(rt.ResourceGroup.Resources[resource].Variables))
for i, v := range rt.ResourceGroup.Resources[resource].Variables {
exprFields[i] = v.ExpressionField
}
Expand Down
281 changes: 0 additions & 281 deletions internal/typesystem/celextractor/celextractor.go

This file was deleted.

Loading

0 comments on commit dab76ce

Please sign in to comment.