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.

Sign up for GitHub

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

Jump to bottom

core: omit invalid resource version parameters when doing paged requests #1281

Merged
merged 4 commits into from
Aug 22, 2023
Merged

core: omit invalid resource version parameters when doing paged requests #1281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d73dd1
fix paging parameters when using any semantic
goenning Aug 17, 2023
ad12665
Merge branch 'kube-rs:main' into go/fix-paging-params
goenning Aug 19, 2023
086ef33
fmt
goenning Aug 20, 2023
bc1aeac
Merge branch 'main' into go/fix-paging-params
clux Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions kube-core/src/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,22 @@ impl ListParams {
}
if let Some(continue_token) = &self.continue_token {
qp.append_pair("continue", continue_token);
}

if let Some(rv) = &self.resource_version {
qp.append_pair("resourceVersion", rv.as_str());
}
match &self.version_match {
None => {}
Some(VersionMatch::NotOlderThan) => {
qp.append_pair("resourceVersionMatch", "NotOlderThan");
}
Some(VersionMatch::Exact) => {
qp.append_pair("resourceVersionMatch", "Exact");
} else {
// When there's a continue token, we don't want to set resourceVersion
if let Some(rv) = &self.resource_version {
if rv != "0" || (rv == "0" && self.limit.is_none()) {
qp.append_pair("resourceVersion", rv.as_str());
Comment on lines +107 to +110
Copy link
Member

Choose a reason for hiding this comment

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

Going over this again just as a sanity; is this possibly slightly imprecise?

If resourceVersion is "0" with no limit, we end up NOT serialising resourceVersion which puts us in "Most Recent" semantics rather than "Any" semantics. Presumably this was not intended and the resourceVersion should be serialized above the first if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need to add a test for that too, but as per the snippet above, if rv=0 and the limit is unset, the resourceVersion is serialized.


match &self.version_match {
None => {}
Some(VersionMatch::NotOlderThan) => {
qp.append_pair("resourceVersionMatch", "NotOlderThan");
}
Some(VersionMatch::Exact) => {
qp.append_pair("resourceVersionMatch", "Exact");
}
}
}
clux marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
29 changes: 22 additions & 7 deletions kube-core/src/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,16 +746,31 @@ mod test {
}

#[test]
fn list_not_older() {
fn list_paged_any_semantic() {
let url = corev1::Pod::url_path(&(), Some("ns"));
let gp = ListParams::default().limit(50).match_any();
let req = Request::new(url).list(&gp).unwrap();
assert_eq!(req.uri().query().unwrap(), "&limit=50");
}

#[test]
fn list_paged_with_continue_any_semantic() {
let url = corev1::Pod::url_path(&(), Some("ns"));
let gp = ListParams::default().limit(50).continue_token("1234").match_any();
let req = Request::new(url).list(&gp).unwrap();
assert_eq!(req.uri().query().unwrap(), "&limit=50&continue=1234");
}

#[test]
fn list_paged_with_continue_starting_at() {
let url = corev1::Pod::url_path(&(), Some("ns"));
let gp = ListParams::default()
.at("20")
.matching(VersionMatch::NotOlderThan);
.limit(50)
.continue_token("1234")
.at("9999")
.matching(VersionMatch::Exact);
let req = Request::new(url).list(&gp).unwrap();
assert_eq!(
req.uri().query().unwrap(),
"&resourceVersion=20&resourceVersionMatch=NotOlderThan"
);
assert_eq!(req.uri().query().unwrap(), "&limit=50&continue=1234");
}

#[test]
Expand Down
Loading