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

ObjectList now accepts null metadata like upstream k8s does #1492

Merged
merged 2 commits into from
May 13, 2024

Conversation

aviramha
Copy link
Contributor

@aviramha aviramha commented May 12, 2024

Fixes #1491

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.9%. Comparing base (3170f68) to head (4dd73a6).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1492   +/-   ##
=====================================
  Coverage   74.9%   74.9%           
=====================================
  Files         78      78           
  Lines       6806    6806           
=====================================
  Hits        5091    5091           
  Misses      1715    1715           
Files Coverage Δ
kube-core/src/object.rs 86.1% <ø> (ø)

@aviramha
Copy link
Contributor Author

Not sure if the failure is my fault?

@clux
Copy link
Member

clux commented May 13, 2024

hm, the failure seems very related:

test test::derived_resources_discoverable ... FAILED
---- test::derived_resources_discoverable stdout ----
Error: SerdeError(Error("missing field items", line: 1, column: 124))

probably the api.list call in that test in kube/src/lib.rs, but not sure why that would trigger here since you seem to be making it less strict, not more 🤔

@clux
Copy link
Member

clux commented May 13, 2024

you could run it locally with cargo test -p kube -- derived_resources_discoverable --ignored --nocapture (edit: forgot features)

@aviramha
Copy link
Contributor Author

aviramha commented May 13, 2024

Thanks, looking into it.
FWIW the command doesn't execute the tests - this seems to run it though:

cargo test --lib --workspace --exclude e2e --all-features -- derived_resources_discoverable --ignored --nocapture

@aviramha
Copy link
Contributor Author

It worked for me locally when using docker-desktop (not when using a real cluster though)
I suspect it's a flake?

@clux
Copy link
Member

clux commented May 13, 2024

hm. unlikely. same error on re-run (twice).

@clux
Copy link
Member

clux commented May 13, 2024

hm, possible hunch: this is the first PR to use k3s 1.30.0 which released a few days ago

@clux
Copy link
Member

clux commented May 13, 2024

it fails locally on k3s 1.30 with or without your change 🙃

@clux
Copy link
Member

clux commented May 13, 2024

2024-05-13T11:31:55.936900Z  WARN kube_client::client: {"kind":"ValidatingAdmissionPolicyList","apiVersion":"admissionregistration.k8s.io/v1","metadata":{"resourceVersion":"529"}}
, Error("missing field `items`", line: 1, column: 124)
2024-05-13T11:31:55.937000Z TRACE tower::buffer::worker: buffer already closed
2024-05-13T11:31:55.937022Z TRACE mio::poll: deregistering event source from poller
Error: SerdeError(Error("missing field `items`", line: 1, column: 124))
test test::derived_resources_discoverable ... FAILED

a list on admission policies returns a list with null'd out items!

...which seems to be confirmed with kg -v=9 ValidatingAdmissionPolicy -ojson..

Response Body: {"kind":"ValidatingAdmissionPolicyList","apiVersion":"admissionregistration.k8s.io/v1","metadata":{"resourceVersion":"412"}}

@aviramha
Copy link
Contributor Author

Ah, nice - not sure I'll be able to address the issue (bit problematic for me to setup k3s).

@clux
Copy link
Member

clux commented May 13, 2024

Feel free to pin CI to 1.29 in integration builds for now. This is likely an upstream bug, so gotta got hunting :(

Signed-off-by: Aviram Hassan <[email protected]>
@aviramha
Copy link
Contributor Author

just did :)

@clux clux added the changelog-fix changelog fix category for prs label May 13, 2024
@clux
Copy link
Member

clux commented May 13, 2024

Looks sensible to me, I'll merge in, but just out of interest, where did you encounter this? I have not seen this fail before.

@clux clux added this to the 0.92.0 milestone May 13, 2024
@aviramha
Copy link
Contributor Author

Our operator builds RestfulList on its own (not sure why we did tbh, maybe no existing modeling in kube-rs? or there wasn't when we needed?) and we don't populate the metadata (because it's optional)

@clux

This comment was marked as resolved.

@aviramha

This comment was marked as resolved.

@clux

This comment was marked as resolved.

@clux clux merged commit a6f4337 into kube-rs:main May 13, 2024
17 checks passed
@clux
Copy link
Member

clux commented Aug 15, 2024

For future reference the missing items in ValidatingAdmissionPolicy was indeed a bug upstream and was fixed in 1.30.4

Fixed a bug in the API server where empty collections of ValidatingAdmissionPolicies did not have an items field. (kubernetes/kubernetes#126146, @xyz-li) [SIG API Machinery]

via 1.30.4 changelog

@aviramha
Copy link
Contributor Author

For future reference the missing items in ValidatingAdmissionPolicy was indeed a bug upstream and was fixed in 1.30.4

Fixed a bug in the API server where empty collections of ValidatingAdmissionPolicies did not have an items field. (kubernetes/kubernetes#126146, @xyz-li) [SIG API Machinery]

via 1.30.4 changelog

Funny, we can open an issue to revert my PR then when 1.30.4 becomes the minimal supported version.

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.

ListMeta is required in ObjectList wrongly
2 participants