Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove invalid uniqueItems property from CRDs when Sets are used #1484

Merged
merged 4 commits into from May 2, 2024

Conversation

sbernauer
Copy link
Contributor

Motivation

This fixes the error message Forbidden: uniqueItems cannot be set to true since the runtime complexity becomes quadratic in case a HashSet (or similar) is used.

Solution

Don't write the field uniqueItems.
Ideally we could set x-kubernetes-list-type instead, but I'm not really comfortable making this change as I'm not so deep into this topic.

Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.4%. Comparing base (6d0c9b1) to head (e90ae39).

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1484     +/-   ##
=======================================
+ Coverage   74.4%   74.4%   +0.1%     
=======================================
  Files         78      78             
  Lines       6775    6778      +3     
=======================================
+ Hits        5034    5037      +3     
  Misses      1741    1741             
Files Coverage Δ
kube-core/src/schema.rs 90.0% <100.0%> (+0.3%) ⬆️
kube-derive/tests/crd_schema_test.rs 96.9% <100.0%> (+0.2%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting this. I think the fix makes sense. Unsure of whether it makes sense to feature gate it to above Kubernetes 1.30 via k8s_if_le_1_30 but that requires k8s-openapi to actually release an 1.30 one first.

kube-core/src/schema.rs Outdated Show resolved Hide resolved
@clux clux added the changelog-fix changelog fix category for prs label May 2, 2024
@clux clux added this to the 0.91.0 milestone May 2, 2024
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
@sbernauer
Copy link
Contributor Author

sbernauer commented May 2, 2024

Sorry for force-pushing, as always I forgot to sign my commits...

Maybe my "As of version 1.30 Kubernetes does not support [...]" comment was a bit misleading.
I got the error on both 1.27 and 1.29, I just added the current latest version 1.30 for future readers as the behavior might be changed in a future k8s version. Happy to remove it as it can cause confusion.

With that in mind would a feature gate still make sense here?

@sbernauer sbernauer requested a review from clux May 2, 2024 09:11
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Thanks!

@clux
Copy link
Member

clux commented May 2, 2024

A feature gate might make sense, but that would mean we wait to include this until we can gate on 1.30 (so basically bump openapi, merge this with a gate, then release next kube). 🤔

@clux
Copy link
Member

clux commented May 2, 2024

Do you have a source for the removal of uniqueItems support?

@sbernauer
Copy link
Contributor Author

I still can't wrap my head around the whole feature gate thing.

They earliest occurrence of this error message I could find is from 2017: kubernetes/kubernetes@fd09c3d
Since then (and probably forever) uniqueItems can not be set to true.

@clux
Copy link
Member

clux commented May 2, 2024

Ah, then I don't think we don't need to feature gate it imo as it's always been considered invalid (even if it hasn't been promoted to a hard error until recently). If people care about it then they can fix it properly with the override, and they will actually see the effects of this immediately with our change here (by us changing the CRD output slightly) so it should be more helpful to get this immediately than to wait for 1.30.

@clux clux changed the title Fix invalid CRD generation when Sets are used Remove invalid uniqueItems property from CRDs when Sets are used May 2, 2024
@sbernauer
Copy link
Contributor Author

even if it hasn't been promoted to a hard error until recently

Ah that was the part I was missing. Thanks for the clarification!

@clux clux merged commit 687506f into kube-rs:main May 2, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants