feat(pluginpresets): resolve CEL expressions and cross-preset references#1942
Open
k-fabryczny wants to merge 14 commits into
Open
feat(pluginpresets): resolve CEL expressions and cross-preset references#1942k-fabryczny wants to merge 14 commits into
k-fabryczny wants to merge 14 commits into
Conversation
k-fabryczny
force-pushed
the
feat/cel-expression-in-plugin-preset
branch
from
85893c2 to
cb304f4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds PluginPreset-side resolution for Plugin option values so that CEL expressions and cross-preset references are resolved before writing the resulting spec into the managed Plugin resources (plus unit/e2e coverage).
Changes:
- Introduce a PluginPreset option-value resolver that (1) evaluates
.expressionfields and (2) resolvesvalueFrom.reflookups. - Wire the resolver into PluginPreset reconciliation so Plugins are created/updated with resolved
.spec.optionValues. - Add extensive unit tests and new PluginPreset-focused e2e scenarios/test suite.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/plugin/pluginpreset_values_resolver.go | New resolver implementation for expression evaluation + reference resolution. |
| internal/controller/plugin/pluginpreset_controller.go | Calls the new resolver before writing option values into Plugin specs. |
| internal/controller/plugin/pluginpreset_controller_test.go | Adds unit tests for expression resolution and cross-preset refs. |
| e2e/pluginpreset/e2e_test.go | New PluginPreset e2e suite (build-tagged). |
| e2e/pluginpreset/scenarios/expression_evaluation.go | E2E scenario validating expression evaluation end-to-end (Plugin + HelmRelease values). |
| e2e/pluginpreset/scenarios/cross_preset_reference.go | E2E scenario validating cross-PluginPreset reference resolution. |
| e2e/pluginpreset/testdata/organization.yaml | E2E testdata organization manifest. |
Comments suppressed due to low confidence (1)
internal/controller/plugin/pluginpreset_controller.go:255
resolvePluginOptionValuesForPresetruns beforeoverridesPluginOptionValues(...), but cluster-specific overrides can also containExpression/ValueFromfields (since they arePluginOptionValue). Applying overrides after resolution can therefore reintroduce unresolved expressions/refs into the resulting Plugin. Consider applyingClusterOptionOverridesfirst (to the effective optionValues for that cluster) and then running expression/reference resolution on the merged set, or re-running resolution after overrides are applied.
resolvedValues, err := r.resolvePluginOptionValuesForPreset(ctx, preset, &cluster)
if err != nil {
return fmt.Errorf("failed to resolve option values for plugin %s: %w", plugin.Name, err)
}
plugin.Spec = preset.Spec.Plugin
plugin.Spec.OptionValues = resolvedValues
plugin.Spec.ReleaseName = releaseName
// Set the cluster name to the name of the cluster. The PluginSpec contained in the PluginPreset does not have a cluster name.
plugin.Spec.ClusterName = cluster.GetName()
// Copy over the plugin dependencies
plugin.Spec.WaitFor = preset.Spec.WaitFor
// transport plugin preset labels to plugin
plugin = (lifecycle.NewPropagator(preset, plugin).Apply()).(*greenhousev1alpha1.Plugin)
// overrides options based on preset definition
overridesPluginOptionValues(plugin, preset)
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+277
to
+316
| // evaluateCELWithObject evaluates a CEL expression against an object map. | ||
| // Supports two syntax styles: | ||
| // - ${...} syntax: ${spec.optionValues.filter(v, v.name == "foo")[0].value} | ||
| // - Plain syntax: spec.optionValues.filter(v, v.name == "foo")[0].value | ||
| func evaluateCELWithObject(expression string, object map[string]any) (any, error) { | ||
| // Strip ${...} wrapper if present | ||
| expr := strings.TrimSpace(expression) | ||
| if strings.HasPrefix(expr, "${") && strings.HasSuffix(expr, "}") { | ||
| expr = expr[2 : len(expr)-1] | ||
| } | ||
|
|
||
| env, err := celgo.NewEnv( | ||
| celgo.Variable("spec", celgo.DynType), | ||
| celgo.Variable("metadata", celgo.DynType), | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create CEL environment: %w", err) | ||
| } | ||
|
|
||
| ast, issues := env.Compile(expr) | ||
| if issues != nil && issues.Err() != nil { | ||
| return nil, fmt.Errorf("failed to compile expression: %w", issues.Err()) | ||
| } | ||
|
|
||
| prg, err := env.Program(ast) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create CEL program: %w", err) | ||
| } | ||
|
|
||
| // Pass spec and metadata directly as top-level variables | ||
| out, _, err := prg.Eval(map[string]any{ | ||
| "spec": object["spec"], | ||
| "metadata": object["metadata"], | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to evaluate expression: %w", err) | ||
| } | ||
|
|
||
| return out.Value(), nil | ||
| } |
There was a problem hiding this comment.
ExternalValueSource.Expression is already evaluated elsewhere via pkg/cel.Evaluate, which expects the root variable object (e.g. object.spec.optionValues[...]). This helper introduces a different expression shape (spec/metadata, plus optional ${...} wrapping), which will break any existing valueFrom.ref.expression written against object.* when used from a PluginPreset. Consider reusing pkg/cel.Evaluate (by passing an object-shaped map) or supporting both object and spec/metadata to avoid an API/behavior breaking change.
Comment on lines
+46
to
+69
| // resolveExpressionsForPreset evaluates all expression fields in PluginPreset option values | ||
| func (r *PluginPresetReconciler) resolveExpressionsForPreset( | ||
| ctx context.Context, | ||
| preset *greenhousev1alpha1.PluginPreset, | ||
| cluster *greenhousev1alpha1.Cluster, | ||
| ) ([]greenhousev1alpha1.PluginOptionValue, error) { | ||
|
|
||
| // Check if any expressions exist - if not, return early | ||
| hasExpressions := false | ||
| for _, ov := range preset.Spec.Plugin.OptionValues { | ||
| if ov.Expression != nil { | ||
| hasExpressions = true | ||
| break | ||
| } | ||
| } | ||
| if !hasExpressions { | ||
| return preset.Spec.Plugin.OptionValues, nil | ||
| } | ||
|
|
||
| // Build greenhouse values for CEL template data | ||
| templateData, err := r.buildTemplateData(ctx, preset, cluster) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build template data: %w", err) | ||
| } |
There was a problem hiding this comment.
Plugin option .expression evaluation is documented as feature-flagged (expressionEvaluationEnabled) and in the Plugin controller it’s explicitly gated by that flag. This resolver evaluates expressions (and also resolves valueFrom.ref) unconditionally, which can contradict the feature-flag semantics and docs. Recommend threading the feature flags into PluginPresetReconciler and skipping expression/reference resolution when the corresponding flags are disabled (treat expressions as literal values, keep refs unresolved).
2 tasks
k-fabryczny
force-pushed
the
feat/cel-expression-in-plugin-preset
branch
from
81f8002 to
ddf721b
Compare
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
…ces (#1774) Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
…ed for PluginPreset Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
k-fabryczny
force-pushed
the
feat/cel-expression-in-plugin-preset
branch
from
46ba284 to
119f19a
Compare
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Comment on lines
+270
to
+291
| results := make([]any, 0, len(presetList.Items)) | ||
| for i := range presetList.Items { | ||
| refPreset := &presetList.Items[i] | ||
|
|
||
| // Resolve expressions only if flag is enabled | ||
| var resolvedRefValues []greenhousev1alpha1.PluginOptionValue | ||
| if r.ExpressionEvaluationEnabled { | ||
| resolvedRefValues, err = r.resolveExpressionsForPreset(ctx, refPreset, cluster) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resolve expressions in referenced PluginPreset %s: %w", refPreset.Name, err) | ||
| } | ||
| } else { | ||
| resolvedRefValues = refPreset.Spec.Plugin.OptionValues | ||
| } | ||
|
|
||
| celObject := buildCELObject(refPreset.Name, refPreset.Namespace, resolvedRefValues) | ||
| value, err := evaluateCELWithObject(ref.Expression, celObject) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to evaluate reference expression for PluginPreset %s: %w", refPreset.Name, err) | ||
| } | ||
| results = appendToResults(results, value) | ||
| } |
Comment on lines
83
to
+100
| // startPluginReconciler initializes the plugin reconciler. | ||
| // Resolves expression evaluation feature flag from greenhouse-feature-flags. | ||
| func startPluginReconciler(name string, mgr ctrl.Manager) error { | ||
| return (&plugincontrollers.PluginReconciler{ | ||
| KubeRuntimeOpts: kubeClientOpts, | ||
| ExpressionEvaluationEnabled: featureFlags.IsExpressionEvaluationEnabled(), | ||
| IntegrationEnabled: featureFlags.IsIntegrationEnabled(), | ||
| OCIMirroringEnabled: featureFlags.IsOCIMirroringEnabled(), | ||
| }).SetupWithManager(name, mgr) | ||
| } | ||
|
|
||
| // Resolves feature flags for PluginPreset expression evaluation and integration. | ||
| func startPluginPresetReconciler(name string, mgr ctrl.Manager) error { | ||
| return (&plugincontrollers.PluginPresetReconciler{ | ||
| ExpressionEvaluationEnabled: featureFlags.IsExpressionEvaluationEnabled(), | ||
| IntegrationEnabled: featureFlags.IsIntegrationEnabled(), | ||
| }).SetupWithManager(name, mgr) | ||
| } |
Comment on lines
+241
to
+245
| resolvedValues, err := r.resolvePluginOptionValuesForPreset(ctx, preset, &cluster) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to resolve option values for plugin %s: %w", plugin.Name, err) | ||
| } | ||
|
|
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
feat(pluginpresets): resolve CEL expressions and cross-preset references
What type of PR is this? (check all applicable)
Related Tickets & Documents
#1774
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist