cel: add semantic version type #123664
cel: add semantic version type #123664
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
size/L
| @@ -90,6 +90,13 @@ var baseOpts = []VersionedOptions{ | |||
| library.Quantity(), | |||
| }, | |||
| }, | |||
| { | |||
| IntroducedVersion: version.MajorMinor(1, 30), | |||
| EnvOptions: []cel.EnvOption{ | |||
There was a problem hiding this comment.
Note that, in contrast to the quantity type above, CrossTypeNumericComparisons is not supported.
When I looked at the quantity type, I was surprised that comparison of two quantity values with operators ('>', '<=', etc.) is not mentioned. Only == is mentioned.
Are other operators supported indirectly via this CrossTypeNumericComparisons thing?
Are compareTo, isGreaterThan and isLessThan really useful? I implemented them for similarity with the quantity type, but overloading the comparison operators seems like a better solution to me (not tried yet).
There was a problem hiding this comment.
@cici37: any opinion about this?
There was a problem hiding this comment.
Are other operators supported indirectly via this CrossTypeNumericComparisons thing?
I tested it, quantity("50M") < quantity("50Mi") also fails to compile.
There was a problem hiding this comment.
I will leave the actually function needed for samver reviewed by others.
The current functionality with compareTo, isGreaterThan and isLessThan is sufficient. Even just compareTo would be enough, but I think consistency with quantity is important, so let's keep isGreaterThan and isLessThan.
I am just wondering why exactly those were added for quantity. Overriding the comparison operators seems more natural to me.
There was a problem hiding this comment.
Perhaps because cost estimation is easier when not overloading operators? I see that cost.go has entries that are just based on the function name.
There was a problem hiding this comment.
Yes We have to go with compareTo, isGreaterThan and isLessThan since cel would not recognize >/< for extended type without handling and we do wanna set up cost properly for those comparison. :)
The alternative is that validation is done with a regular expression. There is even one in the spec: The error message when validation fails is going to be less useful (similar to others, like |
|
If we are not comfortable with adding this directly to the base CEL environment in 1.30, then I can add it instead to the CEL environment that gets prepared for DRA expressions. Then it's clearly alpha and we have more time to evolve it before promoting to the base CEL environment. |
|
I added one commit at the end to #123516 which:
That commit is probably easier to merge for 1.30 because it's all in alpha-quality code. |
There was a problem hiding this comment.
I will leave the actually function needed for samver reviewed by others. Wanna mention that the func cost has to be added into https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/cel/library/cost.go as well together with the tests for adding a new cel lib. Thanks!
| @@ -128,6 +128,7 @@ var baseOpts = []VersionedOptions{ | |||
| EnvOptions: []cel.EnvOption{ | |||
| library.IP(), | |||
| library.CIDR(), | |||
| library.SemVer(), | |||
There was a problem hiding this comment.
Just wanna mention that adding into base as introduced in 1.30 will not enable the power immediately as we normally would set the DefaultCompatibilityVersion one version earlier iff DRA is using the same default env version :)
There was a problem hiding this comment.
I am solving this over in #123516 in pkg/apis/resource/structured/namedresources/validation/validation.go like this:
func validateSelector(opts Options, selector string, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList
if selector == "" {
allErrs = append(allErrs, field.Required(fldPath, ""))
} else {
// TODO (https://github.com/kubernetes/kubernetes/issues/123687):
// when this API gets promoted to beta, we have to
// validate new and stored expressions differently.
// While it is alpha, new expressions are allowed to
// use everything that is currently available.
// envType := environment.NewExpressions
// if opts.StoredExpressions {
// envType = environment.StoredExpressions
// }
envType := environment.StoredExpressions
result := namedresourcescel.Compiler.CompileCELExpression(selector, envType)
Does this look right to you?
There was a problem hiding this comment.
This would depend on how to build the env in https://github.com/kubernetes/kubernetes/blob/90ed34a12ffc3d58ecedf0e0c91c4d85a67f9aab/staging/src/k8s.io/dynamic-resource-allocation/structured/namedresources/cel/compile.go#L62C19-L62C39.
I saw it's built without version control which means all added func would be available but you'll lose the benefit of version control/backward compatibility. When a new func added in the future, the cluster might running into risk of not recognizing the expression while roll back. I guess it should be fine in Alpha, but that is something better think about going forward in beta/ga. :)
There was a problem hiding this comment.
True, this needs more thought before proceeding to beta.
There was a problem hiding this comment.
commented at https://github.com/kubernetes/kubernetes/pull/123516/files#r1516500003 with one possibility for handling this for the namedresources use
| // semver("1.2.3").compareTo(semver("1.2.3")) // returns 0 | ||
| // semver("1.2.3").compareTo(semver("2.0.0")) // returns -1 | ||
| // semver("1.2.3").compareTo(semver("0.1.2")) // returns 1 | ||
|
|
There was a problem hiding this comment.
Looks like tests are still pending to add :)
There was a problem hiding this comment.
Yes, more work is needed to reach better code coverage. Embarrassingly, this copy of the code actually had type conversion problems in the implementation of the functions.
Now the code is covered as well as quantity.go is. I suppose the if !ok after type assertions cannot be tested because those are just safeguards that cannot be reached from within well-formed CEL expressions.
There was a problem hiding this comment.
We don't need type assertions anymore but we are lazy on removing them :)
k8s-ci-robot
added
size/XL
|
Did I add the necessary checks for cost and library registration correctly? The cost estimates are exactly the same as for quantity. Parsing a semver string shouldn't be more expensive. Is this perhaps okay for 1.30? If we can merge it quickly (today?) then I can drop the corresponding code from #123516 and just use this here. If we end up not merging #123516, then this PR could get reverted unless it's seen as useful on its own. |
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
|
/retest pull-kubernetes-integration pull-kubernetes-e2e-gce |
|
@pohly: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions 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/test-infra repository. |
|
LGTM label has been added. Git tree hash: a278d15a95425c23a29b5000cdf58fe05e046444
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cici37, pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign @liggitt For approval as he has some context here. |
| return v | ||
| case types.TypeType: | ||
| return SemVerType | ||
| default: |
There was a problem hiding this comment.
I don't see a semVer.toString(). Do we want it? If so, we can easily implement toString there, and call it like string(ver).
There was a problem hiding this comment.
case types.StringType:
return types.String(v.Version.String())
There was a problem hiding this comment.
I don't see the need for it.
There was a problem hiding this comment.
I am thinking of formatting an error message like "version" + string(object.version) + "is older than expected" + string(params.version) Would you want this use case?
There was a problem hiding this comment.
actually no. We almost always has the string version.
| semver.Version | ||
| } | ||
|
|
||
| func (v SemVer) ConvertToNative(typeDesc reflect.Type) (interface{}, error) { |
There was a problem hiding this comment.
Also, v has a value receiver instead of a pointer receiver? Is this intended?
There was a problem hiding this comment.
It's a small struct, so I think copying by value is fine.
|
/lgtm |
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
| return types.MaybeNoSuchOverloadErr(arg) | ||
| } | ||
|
|
||
| _, err := semver.Parse(str) |
There was a problem hiding this comment.
I started to have second thoughts about this. In core Kubernetes, we decided to do the validation with a regular expression. The semver/v4 code then is "only" used for comparison.
But here isSemVer is implemented with semver/v4. Should this be the same regular expression as for the in-tree API?
/hold
cc @liggitt
There was a problem hiding this comment.
I have the same reservations here as in the REST API field about making an implementation in an external library the definition of our API surface. Using the same regexp seems equivalent in current correctness and more guaranteed to be stable long term.
If the semver library starts accepting new stuff in the future, we wouldn't want to silently expand what we accept.
if isSemVer uses our regex, and the string_to_semver ensures the string passes the regex check before calling semver.Parse, that seems ok
k8s-ci-robot
added
the
do-not-merge/hold
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
| "semver": { | ||
| cel.Overload("string_to_semver", []*cel.Type{cel.StringType}, apiservercel.SemVerType, cel.UnaryBinding((stringToSemVer))), | ||
| }, | ||
| "isSemVer": { |
There was a problem hiding this comment.
nit: is it weird to have semver but isSemVer?
seems like we should have a matched pair:
- semver / isSemver
- semVer / isSemVer
There was a problem hiding this comment.
I tend to agree. I tripped myself up with this inconsistency a few times.
I think I was starting with "Semantic Version" -> "SemVer" and then used "semver" for consistency with "quantity".
Let's use isSemver, it's simpler.
|
|
||
| // SemVer provdes a CEL representation of a [semver.SemVer]. | ||
| type SemVer struct { | ||
| semver.Version |
There was a problem hiding this comment.
does inlining this make the semver library fields / methods public / visible to CEL? if so, that doesn't seem desirable
There was a problem hiding this comment.
No, it doesn't. To expose fields of an object we manually define them with a CEL type provider and make it index-able with a trait. This type has neither of these.
| return types.WrapErr(err) | ||
| } | ||
|
|
||
| return apiservercel.SemVer{Version: v} |
There was a problem hiding this comment.
I don't know CEL stuff well enough to know if this is exposing the underlying blang semver impl fields / methods to CEL ... I don't think we want to do that, right?
There was a problem hiding this comment.
It won't expose any fields. Only functions that are "exposed" are manually defined overloads, namely major(), minor(), and patch().
| @@ -128,6 +128,7 @@ var baseOpts = []VersionedOptions{ | |||
| EnvOptions: []cel.EnvOption{ | |||
| library.IP(), | |||
| library.CIDR(), | |||
| library.SemVer(), | |||
There was a problem hiding this comment.
commented at https://github.com/kubernetes/kubernetes/pull/123516/files#r1516500003 with one possibility for handling this for the namedresources use
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes/kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4. Kubernetes-commit: 42ee56f093133402ed860d4c5f54b049041386c9
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes/kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4. Kubernetes-commit: 42ee56f093133402ed860d4c5f54b049041386c9
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes/kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4. Kubernetes-commit: 42ee56f093133402ed860d4c5f54b049041386c9
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
This adds support for semantic version comparison to the CEL support in the "named resources" structured parameter model. For example, it can be used to check that an instance supports a certain API level. To minimize the risk, the new "semver" type is only defined in the CEL environment for DRA expressions, not in the base library. See kubernetes#123664 for a PR which adds it to the base library. Validation of semver strings is done with the regular expression from semver.org. The actual evaluation at runtime then uses semver/v4.
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions 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/test-infra repository. |
|
@jiahuif, @cici37: what is the conclusion: do you want this in the core library or shall I keep it separate as an extension in the DRA CEL environment? I'm leaning towards moving it to the code library, but I am also open for keeping it specific to DRA. If you want it, then I'll rework this PR to also remove the code in DRA. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Structured parameters for Dynamic Resource Allocation use CEL expressions to filter resource instances by their attributes. One of the possible attribute value types is a semantic version string. The corresponding filter could be "attribute value >= 2.0.0".
Which issue(s) this PR fixes:
Related-to: kubernetes/enhancements#4381
Special notes for your reviewer:
The key question from an API perspective is: can Kubernetes rely on some particular semver implementation
to validate API fields (not in this PR, but callingto implement the version comparsion (this PR)? See #123516 (comment) for @liggitt's comments in the context of a different solution for the same problem.semver.Parseis what validation of the DRA API would do) andgithub.com/blang/semver/v4 was chosen for this in this PR because it looks like a stable, high-quality implementation and also seems to be the most widely used semver implementation in Kubernetes.
Regarding the implementation: it is derived from the quantity type and supports the same operations, with one exception: comparison across types (like
<semver> == <int>?) is not supported.Does this PR introduce a user-facing change?
/cc @liggitt @klueska @cici37