Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
0dabb7d to
c06831c
Compare
| cel.CostEstimatorOptions( | ||
| checker.OverloadCostEstimate("regex_extract_string_string", estimateExtractCost()), | ||
| checker.OverloadCostEstimate("regex_extractAll_string_string", estimateExtractAllCost()), | ||
| checker.OverloadCostEstimate("regex_replace_string_string_string", estimateReplaceCost()), | ||
| checker.OverloadCostEstimate("regex_replace_string_string_string_int", estimateReplaceCost()), | ||
| ), | ||
| cel.EnvOption(optionalTypesEnabled), | ||
| } | ||
| return opts | ||
| } | ||
|
|
||
| // ProgramOptions implements the cel.Library interface method | ||
| func (r *regexLib) ProgramOptions() []cel.ProgramOption { | ||
| return []cel.ProgramOption{} | ||
| } | ||
|
|
||
| func compileRegex(regexStr string) (*regexp.Regexp, error) { | ||
| re, err := regexp.Compile(regexStr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("given regex is invalid: %w", err) | ||
| return []cel.ProgramOption{ | ||
| cel.CostTrackerOptions( | ||
| interpreter.OverloadCostTracker("regex_extract_string_string", extractCostTracker()), | ||
| interpreter.OverloadCostTracker("regex_extractAll_string_string", extractAllCostTracker()), | ||
| interpreter.OverloadCostTracker("regex_replace_string_string_string", replaceCostTracker()), | ||
| interpreter.OverloadCostTracker("regex_replace_string_string_string_int", replaceCostTracker()), | ||
| ), |
There was a problem hiding this comment.
do we know how this changes cost estimation for regexes? should we be expecting a calculated cost change in a unit test using regex cel?
do we need to adjust / improve our exposed find/findAll regex function cost estimation (https://kubernetes.io/docs/reference/using-api/cel/#kubernetes-regex-library)
@jpbetz I'm trying to remember if we have cost ratcheting in place so an update of a CRD or VAP/MAP using CEL whose cost is estimated to be more expensive is permitted if the update is a no-op
There was a problem hiding this comment.
actually, it doesn't look like we're even using the cel ext regex library, just our own, so this is non-blocking for us
question for @TristonianJones purely for the cel-go upstream, should cost estimation changes have been added in a versioned update to the regex library (e.g. with RegexVersion(1))?
There was a problem hiding this comment.
This is the first introduction of the regex libraries to core CEL, so they came with costs out of the box. Not every library had this (such as strings, we're fixing this now) and so we introduced cost changes as a version bump if the extensions didn't come with costs from the get-go
| // the base AttributeFactory implementation. | ||
| func (fac *partialAttributeFactory) MaybeAttribute(id int64, name string) Attribute { | ||
| var names []string | ||
| // When there's a single name with a dot prefix, it indicates that the 'maybe' attribute is a |
There was a problem hiding this comment.
is this surfacing a new usage pattern of .someName, or does it only kick in if there's a declared global maybe identifier (which I don't think we have)?
There was a problem hiding this comment.
As far I can tell, k8s always type-check CEL expressions, so this is not used for the moment.
There was a problem hiding this comment.
It's highly unlikely this would affect you since it's only hit on parse-only paths.
| if err = lib.subset.Validate(); err != nil { | ||
| return nil, err | ||
| } | ||
| e.variables = append(e.variables, stdlib.Types()...) |
There was a problem hiding this comment.
https://github.com/google/cel-go/releases/tag/v0.27.0
Guessing this change is for this bit in the release notes:
⚠ Breaking Changes
Remove types as variables: The logic for handling types has been relaxed to support safe rollout of feature packages which introduce new types whose names may collide with existing variables. Please review your policies if you relied on types behaving strictly as variables in previous versions. PR #1262
The related PR says
Allow user-defined variables to shadow type variables.
Does this mean any previously persisted CEL expressions using types as variables will still resolve those but via a different path (enabling shadowing), or that they will now fail compilation?
Do we want to add a unit test that demonstrates such a use, to sanity check it still behaves like it did previously?
There was a problem hiding this comment.
This is a change to better support the introduction of new CEL types in a forward compatible way.
For example, if K8s has a variable named resource and an opaque type were introduced with the same name by CEL later on, the K8s type for resource would be preserved and override the CEL type introduced in the library.
| // found. | ||
| // Note: The search is performed from innermost to outermost. | ||
| func (s *Scopes) FindIdent(name string) *decls.VariableDecl { | ||
| name = strings.TrimPrefix(name, ".") |
There was a problem hiding this comment.
I'm not sure of all the ways resolution happens, but fields whose names start with . remain addressable via ['.name'] syntax, right? Do we have test coverage for that? Same question below.
There was a problem hiding this comment.
Do spurious . prefixes (like object..field or .object) still fail properly with the trimming applied in various places?
There was a problem hiding this comment.
The leading '.' has a very specific meaning in CEL which indicates that the identifier should not be resolved within a container or alias, but should be used as-is. This would allow you to say something like the following:
env, _ := cel.NewEnv(cel.Variable('x', cel.MapType(cel.StringType, cel.StringType)))
env.Compile('x.exists(x, x == .x.y)')It's an odd statement to make, but allows for references to variables at the top scope while if they get shadowed in a comprehension.
|
looks fine overall, just a few questions about whether this changes visible behavior or surface area, and test coverage for some of those edges |
|
@ameukam I'm not sure if it interests you, I just packaged up v0.28.0 ... it has a few more features and fixes. Some will be helpful to K8s as well. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Release notes: https://github.com/google/cel-go/releases/tag/v0.27.0
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: