Comments (7)
Hi @mattwelke
Can you clarify? Since the suggested workaround involves using markers defined in https://github.com/kubernetes-sigs/controller-tools, we should move this issue there?
If this remained open as a feature request, would it remain here? I'm guessing it would depend on which repo would be where new markers were added.
Yes, that should remain there. We (Kubebuilder Maintainers cannot help you out with this one)
The kubebuilder maintainers are not the same as controller-tools
The RFE that you are asking is in controller-tools and not kubebuilder
Kubebuilder use/consume controller tools
So, the discussion to see if it is or is not acceptable must be in the project where the implementations will be made
Also, see that, indeed, our documentation is generated automatically from the comments/documentation of the source code in controller-tools
So, I am closing this one in favor of: kubernetes-sigs/controller-tools#868
And re-open that there.
from kubebuilder.
The problem with this approach is the time complexity of the algorithm. It's greater than linear, and this makes it expensive.
The operation of supporting the UniqueItems
is equally expensive, it's just that it's hidden from the user in this case.
The workaround we've used so far is to use a validation rule with CEL.
This is actually probably the correct path forward rather then directly supporting this in kubebuilder. Supporting it in kubebuilder would allow users to blindly add very complex validations without any thought for the complexity of the rules in question.
When using this validation rule alone, we get errors when applying the CRDs warning us that the validation is too expensive:
- spec.validation.openAPIV3Schema.properties[spec].properties[foos].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of more than 100x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)
We find that for this CEL expression, we can only get the expensiveness down to an acceptable level by severely limiting the size of the list via MaxItems.
This is the expectation, where you have complex validations you must limit the length of your inputs, this is, in general good practice anyway.
Bear in mind that the cost calculation not only takes into account the length of the list, but also what the size of the elements within them are. So, if you have an unbounded length string within your struct, that will not be helping you here. Make sure to add maximum sizes to all structs!
Given everything I have said above, I don't think CEL or UniqueItems is what you want for your particular use case.
If I have understood correctly, your desire is to have a list of items, where there is one field in the list that is unique. This is a standard thing in kube, for example, the list of containers in a pod must all have unique names.
The following validation will achieve what you want, set the list to a map type and add the id
field as the map key
// +listType=map
// +listMapKey=id
The validations will ensure every list has a unique id
entry. This also sets you up for server side apply where different actors will be able to update their own entries in the list without affecting the whole list.
from kubebuilder.
Hi @JoelSpeed
Thank you for the hand here.
However, since those markers are controller-tools implementation, could we move this issue there?
from kubebuilder.
@JoelSpeed Thanks for the suggestions.
Bear in mind that the cost calculation not only takes into account the length of the list, but also what the size of the elements within them are. So, if you have an unbounded length string within your struct, that will not be helping you here. Make sure to add maximum sizes to all structs!
This is a great suggestion. We completely forgot about this and I think it'll make a big improvement. It should allow us to squeeze in a lot more items via MaxItems
.
// +listType=map
and// +listMapKey=id
We actually prefer using structs over maps because it means things are more strongly typed and we can add documentation for each field.
But, we found kubernetes-sigs/controller-tools#435 (comment) which talks about how you can use those markers on structs too.
type BarSpec struct {
// +listType=map
// +listMapKey=id
Foos []Foo `json:"foos,omitempty"`
}
type Foo struct {
ID string `json:"id"`
Other string `json:"other"`
}
I tested this and it worked too!
But unfortunately, it was determined by k8s to be just as expensive as the CEL approach. I actually find this surprising because as far as I know, one could determine uniqueness in an inexpensive way by doing something like this:
package main
import "fmt"
func duplicate(items []string) *string {
seen := map[string]bool{}
for _, str := range items {
if _, ok := seen[str]; ok {
// Duplicate found!
return &str
}
seen[str] = true
}
// No duplicates found.
return nil
}
func main() {
if dupStr := duplicate([]string{"a", "b", "b", "c"}); dupStr != nil {
fmt.Printf("Duplicate found: %s\n", *dupStr)
} else {
fmt.Printf("No duplicates found.\n")
}
}
Duplicate found: b
from kubebuilder.
Can you clarify? Since the suggested workaround involves using markers defined in https://github.com/kubernetes-sigs/controller-tools, we should move this issue there?
If this remained open as a feature request, would it remain here? I'm guessing it would depend on which repo would be where new markers were added.
from kubebuilder.
We actually prefer using structs over maps because it means things are more strongly typed and we can add documentation for each field.
Maps are discouraged (forbidden even) in Kube API types. The markers I'm suggesting do not mean you have to use maps. They are designed for lists, hence, listType
.
I tested this and it worked too!
Yep, what you have in that example looks like what I was expecting.
But unfortunately, it was deemed by k8s to be just as expensive as the CEL approach.
Can you provide the error message that came out? To my knowledge the listType
and listMapKey
do not have cost counting, only CEL validations have cost counting so I don't think your error will have been from this
from kubebuilder.
Maps are discouraged (forbidden even) in Kube API types. The markers I'm suggesting do not mean you have to use maps. They are designed for lists, hence, listType.
Sorry. To clarify, what I meant was that we needed to use a list regardless, and it was just a matter of what type to use with the list. So []OurStruct
vs. []map[K]V
. We prefer []OurStruct
for the reasons I mentioned.
Can you provide the error message that came out? To my knowledge the listType and listMapKey do not have cost counting, only CEL validations have cost counting so I don't think your error will have been from this
Confirmed through more troubleshooting that I was incorrect earlier. It wasn't listType
and listMapKey
that were contributing to the cost counting in that case. It was the fact that I was working from a different example (our actual code base), tweaking it, and then making code snippets to post here. So the cost counting was coming from other validation that did involve cost counting nested more deeply in our struct.
from kubebuilder.
Related Issues (20)
- Generate alpha command do not respect API namespacing HOT 2
- End to End Test should check Kubecontext before Proceeding HOT 1
- Create testdata sample for samples in the reference links HOT 4
- Support `objectSelector` in webhook's code markers HOT 2
- v2 vs v3 page is TODO HOT 3
- Fail do "kubebuilder init" in 4.0.0 HOT 2
- Updating config in PostScaffold is ignored HOT 2
- Allow Custom Marker Prefix HOT 2
- Improve Default .golangci.yml Configuration to Exclude lll Linter for kubebuilder Markers HOT 3
- Why set namespace with kustomize for cluster-scoped Mutating|ValidatingWebhookConfiguration? HOT 4
- Stacking of work queue metrics in grafana dashboard is confusing HOT 2
- kubebuilder should use 1.22.0 instead of 1.22 HOT 2
- Improved test support for 3rd party apps HOT 2
- update webhook marker docs HOT 2
- [Typo]: migration_guide_gov3_to_gov4.md doc HOT 1
- "@[" in scaffolded Makefile results in `bash: line 1: @[: command not found` HOT 4
- The CustomResourceDefinition name is too long: must have at most 262144 bytes HOT 1
- Update Cronjob and Multiversion Tutorials samples to Use Status Conditions HOT 1
- Conditionally Enable `filters.WithAuthenticationAndAuthorization` Only When `secureMetrics` is True HOT 6
- role.yaml doesn't contain all CRDs, and "make" nukes any fixes HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from kubebuilder.