From d294847c08094464fdc37c6cdf720e6e28cf0f8a Mon Sep 17 00:00:00 2001 From: Eirik A Date: Wed, 12 Apr 2023 14:37:28 +0100 Subject: [PATCH] Fix `WatchParams` bookmarks for `watch_metadata` (#1193) * Fix `WatchParams` bookmarks for `watch_metadata` Intended to factor out `populate_qp` from `ListParams` and do the same in `WatchParams`, but accidentally found a place where the duplication caused a split. This brings the query param serialization in line for all variants of list, and does the same for all variants of watch (where the inconsistency was wound). Signed-off-by: clux * fix a doc issue + fmt Signed-off-by: clux --------- Signed-off-by: clux --- kube-core/src/admission.rs | 8 ++-- kube-core/src/params.rs | 47 +++++++++++++++++++ kube-core/src/request.rs | 94 ++++---------------------------------- 3 files changed, 60 insertions(+), 89 deletions(-) diff --git a/kube-core/src/admission.rs b/kube-core/src/admission.rs index 94c8eb79c..f800e20ff 100644 --- a/kube-core/src/admission.rs +++ b/kube-core/src/admission.rs @@ -77,7 +77,7 @@ impl TryInto> for AdmissionReview { /// In an admission controller scenario, this is extracted from an [`AdmissionReview`] via [`TryInto`] /// /// ```no_run -/// use kube::api::{admission::{AdmissionRequest, AdmissionReview}, DynamicObject}; +/// use kube::core::{admission::{AdmissionRequest, AdmissionReview}, DynamicObject}; /// /// // The incoming AdmissionReview received by the controller. /// let body: AdmissionReview = todo!(); @@ -205,9 +205,9 @@ pub enum Operation { /// An outgoing [`AdmissionReview`] response. Constructed from the corresponding /// [`AdmissionRequest`]. /// ```no_run -/// use kube::api::{ -/// admission::{AdmissionRequest, AdmissionResponse, AdmissionReview}, -/// DynamicObject, +/// use kube::core::{ +/// admission::{AdmissionRequest, AdmissionResponse, AdmissionReview}, +/// DynamicObject, /// }; /// /// // The incoming AdmissionReview received by the controller. diff --git a/kube-core/src/params.rs b/kube-core/src/params.rs index cae1b14fa..a367c22e4 100644 --- a/kube-core/src/params.rs +++ b/kube-core/src/params.rs @@ -89,6 +89,35 @@ impl ListParams { } Ok(()) } + + // Partially populate query parameters (needs resourceVersion out of band) + pub(crate) fn populate_qp(&self, qp: &mut form_urlencoded::Serializer) { + if let Some(fields) = &self.field_selector { + qp.append_pair("fieldSelector", fields); + } + if let Some(labels) = &self.label_selector { + qp.append_pair("labelSelector", labels); + } + if let Some(limit) = &self.limit { + qp.append_pair("limit", &limit.to_string()); + } + 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"); + } + } + } } /// Builder interface to ListParams @@ -253,6 +282,24 @@ impl WatchParams { } Ok(()) } + + // Partially populate query parameters (needs resourceVersion out of band) + pub(crate) fn populate_qp(&self, qp: &mut form_urlencoded::Serializer) { + qp.append_pair("watch", "true"); + + // https://github.com/kubernetes/kubernetes/issues/6513 + qp.append_pair("timeoutSeconds", &self.timeout.unwrap_or(290).to_string()); + + if let Some(fields) = &self.field_selector { + qp.append_pair("fieldSelector", fields); + } + if let Some(labels) = &self.label_selector { + qp.append_pair("labelSelector", labels); + } + if self.bookmarks { + qp.append_pair("allowWatchBookmarks", "true"); + } + } } impl Default for WatchParams { diff --git a/kube-core/src/request.rs b/kube-core/src/request.rs index bd5c3a3b3..4195cb0e5 100644 --- a/kube-core/src/request.rs +++ b/kube-core/src/request.rs @@ -1,7 +1,7 @@ //! Request builder type for arbitrary api types use thiserror::Error; -use super::params::{DeleteParams, ListParams, Patch, PatchParams, PostParams, VersionMatch, WatchParams}; +use super::params::{DeleteParams, ListParams, Patch, PatchParams, PostParams, WatchParams}; pub(crate) const JSON_MIME: &str = "application/json"; /// Extended Accept Header @@ -58,33 +58,7 @@ impl Request { let target = format!("{}?", self.url_path); let mut qp = form_urlencoded::Serializer::new(target); lp.validate()?; - - if let Some(fields) = &lp.field_selector { - qp.append_pair("fieldSelector", fields); - } - if let Some(labels) = &lp.label_selector { - qp.append_pair("labelSelector", labels); - } - if let Some(limit) = &lp.limit { - qp.append_pair("limit", &limit.to_string()); - } - if let Some(continue_token) = &lp.continue_token { - qp.append_pair("continue", continue_token); - } - - if let Some(rv) = &lp.resource_version { - qp.append_pair("resourceVersion", rv.as_str()); - } - match &lp.version_match { - None => {} - Some(VersionMatch::NotOlderThan) => { - qp.append_pair("resourceVersionMatch", "NotOlderThan"); - } - Some(VersionMatch::Exact) => { - qp.append_pair("resourceVersionMatch", "Exact"); - } - } - + lp.populate_qp(&mut qp); let urlstr = qp.finish(); let req = http::Request::get(urlstr); req.body(vec![]).map_err(Error::BuildRequest) @@ -95,22 +69,8 @@ impl Request { let target = format!("{}?", self.url_path); let mut qp = form_urlencoded::Serializer::new(target); wp.validate()?; - - qp.append_pair("watch", "true"); + wp.populate_qp(&mut qp); qp.append_pair("resourceVersion", ver); - - // https://github.com/kubernetes/kubernetes/issues/6513 - qp.append_pair("timeoutSeconds", &wp.timeout.unwrap_or(290).to_string()); - if let Some(fields) = &wp.field_selector { - qp.append_pair("fieldSelector", fields); - } - if let Some(labels) = &wp.label_selector { - qp.append_pair("labelSelector", labels); - } - if wp.bookmarks { - qp.append_pair("allowWatchBookmarks", "true"); - } - let urlstr = qp.finish(); let req = http::Request::get(urlstr); req.body(vec![]).map_err(Error::BuildRequest) @@ -302,33 +262,7 @@ impl Request { let target = format!("{}?", self.url_path); let mut qp = form_urlencoded::Serializer::new(target); lp.validate()?; - - if let Some(fields) = &lp.field_selector { - qp.append_pair("fieldSelector", fields); - } - if let Some(labels) = &lp.label_selector { - qp.append_pair("labelSelector", labels); - } - if let Some(limit) = &lp.limit { - qp.append_pair("limit", &limit.to_string()); - } - if let Some(continue_token) = &lp.continue_token { - qp.append_pair("continue", continue_token); - } - - if let Some(rv) = &lp.resource_version { - qp.append_pair("resourceVersion", rv.as_str()); - } - match &lp.version_match { - None => {} - Some(VersionMatch::NotOlderThan) => { - qp.append_pair("resourceVersionMatch", "NotOlderThan"); - } - Some(VersionMatch::Exact) => { - qp.append_pair("resourceVersionMatch", "Exact"); - } - } - + lp.populate_qp(&mut qp); let urlstr = qp.finish(); let req = http::Request::get(urlstr) .header(http::header::ACCEPT, JSON_METADATA_LIST_MIME) @@ -342,19 +276,9 @@ impl Request { let target = format!("{}?", self.url_path); let mut qp = form_urlencoded::Serializer::new(target); wp.validate()?; - - qp.append_pair("watch", "true"); + wp.populate_qp(&mut qp); qp.append_pair("resourceVersion", ver); - qp.append_pair("timeoutSeconds", &wp.timeout.unwrap_or(290).to_string()); - - if let Some(fields) = &wp.field_selector { - qp.append_pair("fieldSelector", fields); - } - if let Some(labels) = &wp.label_selector { - qp.append_pair("labelSelector", labels); - } - let urlstr = qp.finish(); http::Request::get(urlstr) .header(http::header::ACCEPT, JSON_METADATA_MIME) @@ -548,7 +472,7 @@ mod test { let req = Request::new(url).watch(&wp, "0").unwrap(); assert_eq!( req.uri(), - "/api/v1/namespaces/ns/pods?&watch=true&resourceVersion=0&timeoutSeconds=290&allowWatchBookmarks=true" + "/api/v1/namespaces/ns/pods?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=0" ); } #[test] @@ -558,7 +482,7 @@ mod test { let req = Request::new(url).watch_metadata(&wp, "0").unwrap(); assert_eq!( req.uri(), - "/api/v1/namespaces/ns/pods?&watch=true&resourceVersion=0&timeoutSeconds=290" + "/api/v1/namespaces/ns/pods?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=0" ); assert_eq!(req.headers().get(header::CONTENT_TYPE).unwrap(), super::JSON_MIME); assert_eq!( @@ -792,10 +716,10 @@ mod test { .disable_bookmarks() .fields("metadata.name=pod=1") .labels("app=web"); - let req = Request::new(url).watch(&wp, "").unwrap(); + let req = Request::new(url).watch(&wp, "0").unwrap(); assert_eq!( req.uri().query().unwrap(), - "&watch=true&resourceVersion=&timeoutSeconds=290&fieldSelector=metadata.name%3Dpod%3D1&labelSelector=app%3Dweb" + "&watch=true&timeoutSeconds=290&fieldSelector=metadata.name%3Dpod%3D1&labelSelector=app%3Dweb&resourceVersion=0" ); }