Welcome @shashankram!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @shashankram. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

I wonder if we can achieve a way to improve the error messages to be more specific and helpful to end users. Presently the errors in the test output aren't fantastic to tell and end user what they did wrong

I agree, but I don't know of a way to do this. Given that these markers are optional, I believe they add value in enforcing structural schema for OneOf types. Even documentation on the types regarding the oneof constraints could help users.

I wonder if we can achieve a way to improve the error messages to be more specific and helpful to end users. Presently the errors in the test output aren't fantastic to tell and end user what they did wrong

I agree, but I don't know of a way to do this. Given that these markers are optional, I believe they add value in enforcing structural schema for OneOf types. Even documentation on the types regarding the oneof constraints could help users.

So the error messages from schema validation are coming from kube-openapi. I am checking to see if they can be improved. I don't think there's much we can do in controller-tools though.

I wonder what other maintainers (cc @alvaroaleman @sbueringer ) would think of leveraging CEL rules to implement these, instead of using the anyOf/allOf/oneOf that we have in the PR at the moment.

If we leveraged these markers to generate CEL on behalf of the end user, we could customise the messages and provide a standard format that would be more helpful, IMO, than the existing error messages

A very simple way to achieve this behaviour would be to filter based on the names of the fields, though I'm not certain how the cost estimation would handle this

+kubebuilder:validation:XValidation:=rule="self.filter(x, x in ['foo', 'bar']).size() <= 1",message="at most one of 'foo', 'bar' should be set"

I wonder what other maintainers (cc @alvaroaleman @sbueringer ) would think of leveraging CEL rules to implement these, instead of using the anyOf/allOf/oneOf that we have in the PR at the moment.

In principle that seems like a good idea, but if there aleady is a custom CEL rule at the same layer, how would we merge?

In principle that seems like a good idea, but if there aleady is a custom CEL rule at the same layer, how would we merge?

CEL rules are a list, you can have as many as you like provided the runtime cost estimates do not exceed 10,000,000 cost units. I'll have to do a little research to determine if the cost analyzer is smart enough to determine the maximum size of the object based on the number of fields within it, if it is, then the likely cost of this rule would be sub 1000 cost units in even a bad case. I tested on the CEL playground and the cost it determined for a three field validation was 46 units for example.

I wonder what other maintainers (cc @alvaroaleman @sbueringer ) would think of leveraging CEL rules to implement these, instead of using the anyOf/allOf/oneOf that we have in the PR at the moment.

If we leveraged these markers to generate CEL on behalf of the end user, we could customise the messages and provide a standard format that would be more helpful, IMO, than the existing error messages

A very simple way to achieve this behaviour would be to filter based on the names of the fields, though I'm not certain how the cost estimation would handle this

+kubebuilder:validation:XValidation:=rule="self.filter(x, x in ['foo', 'bar']).size() <= 1",message="at most one of 'foo', 'bar' should be set"

I like the CEL approach, though my concern is that a rule like this would be within the complexity budget, and may suddenly not as new fields are added. There is also a complexity budget for the entire CRD, which could become a concern with a lot of such fields. If we can make kube-openapi present more user-friendly errors, that would be a big win. I am exploring that option.

The filter suggestion I made doesn't actually work, because of the way structures are created in CEL within Kubernetes. They become a structured "selfType" and are not compatible with the filter expression.

There is an alternative though, which is easy to calculate the cost of, and definitely low cost!

+kubebuilder:validation:XValidation:=rule="[has(self.foo), has(self.bar), has(self.baz)].filter(x, x == true).size() <= 1",message="at most one of 'foo', 'bar', 'baz' should be set"

Filtering is proportional to the length of the list and we know that has a fixed size since we will control the number of elements within it.

Breaking down the cost elements of this (I've stepped through this with a debugger):

• _ <= _ is the total of the cost of the lhs + rhs + 1. The rhs has zero cost (it's a constant), so we are at 1 + lhs.
• .size() has a fixed cost of 1.
• filter is the cost of the iterator range (the list input), the allocation of the variables, and then the comparison multiplied by the number of items in the input
• Checking the field presence has(...) has a fixed cost, but there's a cost of 10 units to allocate the list, so [has(self.foo), has(self.bar), has(self.baz)] costs 12 units, and adding new elements would add 1 extra cost unit
• The variable allocations also allocate a list, so that's another 10 units, this is constant cost
• The loop cost is 0, this is constant
• The step cost is 14, this is constant

So for a N item list, this becomes

1 + 1 + (10 + N) +  10 + (14*N) = 22 + 15N

So for a 2 item list, 52, 3 items 67, 10 items 172.

The per rule limit is 10,000,000 and the per CRD limit is 100,000,000.

I think the cost growth of this rule is small enough that we will be safe without worry.

@JoelSpeed I updated the impl to use CEL and added a test to verify the errors when validation fails

/ok-to-test

/lgtm

@alvaroaleman @sbueringer What do you think of this one? It's not exposing anything a user couldn't do by themselves today, but is a useful syntactic sugar to help make it easier (and means folks don't have to think about what the correct CEL rule would be)

LGTM label has been added.

LGTM label has been added.

/approve

Idea seems fine, but looks like there are still some test failures

My bad, forgot to include the new test directory.
@alvaroaleman would you mind approving again? Thanks

LGTM label has been added.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, shashankram

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@JoelSpeed While the CEL based oneofs work fine, they do suffer from the CEL limitation where Arrays are deemed to exceed the complexity budget even with MaxItems constraint, so defining a Oneoff for a type within an array may be infeasible sometimes.

Should we enable an OpenAPI based toggle for these OneOfs that don't suffer from general CEL limitations?

Can you provide an example that breaks the current implementation?

The CEL rule was deliberately crafted to not have a ballooning cost.

The only array involved in the CEL rule is the one we construct as part of the rule. It doesn't actually perform any array type operations on arrays from within the API.

See #1212 (comment) where I've broken down how the cost of these rules are calculated.

Even with 1000 items in the one of, the rule cost will still be manageable and well, well within the limit. Approx 15000 cost units.

Can you provide an example that breaks the current implementation?

I am not referring to the CEL rule for the constraint itself, but if the rule is applied on a type that is within an array, then CEL cost estimation could reject this.

I am not referring to the CEL rule for the constraint itself, but if the rule is applied on a type that is within an array, then CEL cost estimation could reject this.

Ahh, so in that case yes, you're right, this could violate the cost estimation as we would estimate the cost of the rule multiplied by the worst case number of elements in the array. Hmm. On the one hand, preventing these complex validations in unbounded lists has a benefit, the API server still has to do the same complex validations even with the anyOf approach.

But I could see how that could be frustrating.

Perhaps we could add an optional parameter to the marker that uses the anyOf style behaviour instead of CEL, but default to CEL so that our defaults recommend something that is using cost estimation and fair sharing style semantics within the API server.

where Arrays are deemed to exceed the complexity budget even with MaxItems constraint

Thinking this through, the rule cost is the same as in my previous comment, but then the cardinality is set by the max length of the array.

So taking a 5 member AtMostOneOf, this would have a cost of 97. The maximum length of the array then would be 10,000,000 / 97 =~ 103092. Is this really likely to be an issue for people?

We could document that where these markers are used, parent arrays must have sensible maximum lengths. I'd be surprised if anyone was creating an API where they could enter 100000 items into an array, no?

I am not referring to the CEL rule for the constraint itself, but if the rule is applied on a type that is within an array, then CEL cost estimation could reject this.

Ahh, so in that case yes, you're right, this could violate the cost estimation as we would estimate the cost of the rule multiplied by the worst case number of elements in the array. Hmm. On the one hand, preventing these complex validations in unbounded lists has a benefit, the API server still has to do the same complex validations even with the anyOf approach.

But I could see how that could be frustrating.

Perhaps we could add an optional parameter to the marker that uses the anyOf style behaviour instead of CEL, but default to CEL so that our defaults recommend something that is using cost estimation and fair sharing style semantics within the API server.

I think this would be a reasonable alternative to enforce structural schema without needing to drop it due to CEL limitations. E.g., there are bugs in older k8s versions where CEL misbehaves and we are unable to use these oneofs there, or there may be APIs where adding MaxItems is not backward compatible and can thus exceed the complexity budget.

So the OpenAPI based OneOf/AnyOf constraints would be a good alternative despite the unfriendly error message.

where Arrays are deemed to exceed the complexity budget even with MaxItems constraint

Thinking this through, the rule cost is the same as in my previous comment, but then the cardinality is set by the max length of the array.

So taking a 5 member AtMostOneOf, this would have a cost of 97. The maximum length of the array then would be 10,000,000 / 97 =~ 103092. Is this really likely to be an issue for people?

We could document that where these markers are used, parent arrays must have sensible maximum lengths. I'd be surprised if anyone was creating an API where they could enter 100000 items into an array, no?

With deeply nested types containing arrays, I have seen CEL break down. In most cases with newer k8s versions, MaxItems should suffice.

there are bugs in older k8s versions where CEL misbehaves

That sounds interesting, any links?

So the OpenAPI based OneOf/AnyOf constraints would be a good alternative despite the unfriendly error message.

Yeah so perhaps we take that route of a field on the marker useOpenAPIValidation=true or something like that. @alvaroaleman any opinions?

With deeply nested types containing arrays, I have seen CEL break down. In most cases with newer k8s versions, MaxItems should suffice

Yeah I guess when adding a compatible to change to an existing API, the argument for bounding all parents can be more difficult