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

Implement client native object reference fetching #1511

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Implement client native object reference fetching #1511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
289 changes: 285 additions & 4 deletions kube-client/src/client/client_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
use crate::{Client, Error, Result};
use k8s_openapi::api::core::v1::Namespace as k8sNs;
use k8s_openapi::{
api::core::v1::{LocalObjectReference, Namespace as k8sNs, ObjectReference},
apimachinery::pkg::apis::meta::v1::OwnerReference,
};
use kube_core::{
object::ObjectList,
params::{GetParams, ListParams},
request::Request,
ClusterResourceScope, DynamicResourceScope, NamespaceResourceScope, Resource,
ApiResource, ClusterResourceScope, DynamicResourceScope, NamespaceResourceScope, Resource,
};
use serde::{de::DeserializeOwned, Serialize};
use std::fmt::Debug;
Expand Down Expand Up @@ -42,9 +45,109 @@
/// You can create this directly, or convert `From` a `String` / `&str`, or `TryFrom` an `k8s_openapi::api::core::v1::Namespace`
pub struct Namespace(String);

/// Referenced object name resolution
pub trait ObjectRef<K>: ObjectUrl<K> {
fn name(&self) -> Option<&str>;
}

/// Reference resolver for a specified namespace
pub trait NamespacedRef<K> {
/// Resolve reference in the provided namespace
fn within(&self, namespace: impl Into<Option<String>>) -> impl ObjectRef<K>;
}

impl<K> ObjectUrl<K> for ObjectReference
where
K: Resource,
{
fn url_path(&self) -> String {
url_path(
&ApiResource::from_gvk(&self.clone().into()),
self.namespace.clone(),
)
}
}
Comment on lines +59 to +69
Copy link
Member

@clux clux Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Conversion here makes sense, but wondering if will it work for other "objectreference-like things" with less information. In particular, would we be able to handle:

not saying we need to, but it's good to know the limits of the design. what would we need to do support LocalObjectReference? Would it even make sense in this design?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that both of them are quite ambiguous. I pushed a change which tries to support for both, but for LocalObjectReference there is a need of a real field which uses it, which can come only from CR. Also the definitions are open to interpretation. TypedLocalObjectReference has an api_group which is confusing, as it is different sometimes from api_version (which is actually group/version always). Forcing some definition there seems a bit questionable to me, need to think it through. Here are some examples:

I’d like to try if the exported trait can be implemented externally, and if possibly wrapping a LocalObjectReference and implementing the NamespaceRef trait for the wrapped type, providing From implementation + some kopium changes to recognize selectively LocalObjectReference can be a thing.

Copy link
Member

@clux clux Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

LocalObjectReference there is a need of a real field which uses it

i found one use-case on PodSpec::image_pull_secrets which implicitly references Secret object as per kubernetes docs. this is more awkward than I expected because it's not the same resource as the one it was found on.

TypedLocalObjectReference has an api_group which is confusing

..yeah. if that's the case then it's probably not super important to worry about it. people can write converters from one object ref to the one they want to disambiguate. that said though, i would say the first example there is wrong. apigroup should be just the group, and the usage of TypedLocalObjectReference is an indication that people should do discovery to make the version concrete.

Copy link
Member Author

Choose a reason for hiding this comment

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

So after some consideration I removed the use of TypedLocalObjectReference. This ambiguity translates to kube-rs as well. What the library needs, I think, is a rough equivalent of CR scheme.go, where a type registers itself and provides a bunch of helpers, some of them using discovery client, some of them in-memory. This would allow to look up the version for the GroupKind the TypedLocalObjectReference represents, probably even without using discovery, as a key from GroupKind<->GroupVersionKind mapping.

I’d also love to have this bits being autogenerated by the “derive” feature sometime in the future. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this is fair. The scheme registration I am not sure how it would work, but some user input feels necessary to solve the ambiguity. Happy to look at PRs or help sketch on issues for it (hard to envision it all from my POV now).


impl<K> ObjectRef<K> for ObjectReference
where
K: Resource,
{
fn name(&self) -> Option<&str> {
self.name.as_deref()
}
}

impl<K> NamespacedRef<K> for ObjectReference
where
K: Resource,
K::Scope: NamespaceScope,
{
fn within(&self, namespace: impl Into<Option<String>>) -> impl ObjectRef<K> {

Check warning on line 85 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L85 

Added line #L85 was not covered by tests
Self {
namespace: namespace.into(),

Check warning on line 87 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L87 

Added line #L87 was not covered by tests
..self.clone()
}
}
}

impl<K> ObjectUrl<K> for OwnerReference
Copy link
Member

Choose a reason for hiding this comment

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

I think there are 2 more types that effectively we could implement for free as they generally have enough information:

perhaps good to do one/two of those to show the system is extensible enough.

where
K: Resource,
K::Scope: ClusterScope,
{
fn url_path(&self) -> String {
url_path(&ApiResource::from_gvk(&self.clone().into()), None)

Check warning on line 99 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L98-L99 

Added lines #L98 - L99 were not covered by tests
}
}

impl<K> ObjectRef<K> for OwnerReference
where
K: Resource,
K::Scope: ClusterScope,
{
fn name(&self) -> Option<&str> {
self.name.as_str().into()

Check warning on line 109 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L108-L109 

Added lines #L108 - L109 were not covered by tests
}
}

impl<K> NamespacedRef<K> for OwnerReference
where
K: Resource,
K::Scope: NamespaceScope,
{
fn within(&self, namespace: impl Into<Option<String>>) -> impl ObjectRef<K> {

Check warning on line 118 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L118 

Added line #L118 was not covered by tests
ObjectReference {
api_version: self.api_version.clone().into(),
namespace: namespace.into(),
name: self.name.clone().into(),
uid: self.uid.clone().into(),
kind: self.kind.clone().into(),

Check warning on line 124 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L120-L124 

Added lines #L120 - L124 were not covered by tests
..Default::default()
}
}
}

impl<K> NamespacedRef<K> for LocalObjectReference
where
K: Resource,
K::DynamicType: Default,
K::Scope: NamespaceScope,
{
fn within(&self, namespace: impl Into<Option<String>>) -> impl ObjectRef<K> {
let dt = Default::default();
ObjectReference {
api_version: K::api_version(&dt).to_string().into(),
namespace: namespace.into(),
name: self.name.clone(),
kind: K::kind(&dt).to_string().into(),
..Default::default()
}
}
}

/// Scopes for `unstable-client` [`Client#impl-Client`] extension methods
pub mod scope {
pub use super::{Cluster, Namespace};
pub use super::{Cluster, Namespace, NamespacedRef};
}

// All objects can be listed cluster-wide
Expand Down Expand Up @@ -184,6 +287,69 @@
self.request::<K>(req).await
}

/// Fetch a single instance of a `Resource` from a provided object reference.
///
/// ```no_run
/// # use k8s_openapi::api::rbac::v1::ClusterRole;
/// # use k8s_openapi::api::core::v1::Service;
/// # use k8s_openapi::api::core::v1::Secret;
/// # use k8s_openapi::api::core::v1::ObjectReference;
/// # use k8s_openapi::api::core::v1::LocalObjectReference;
/// # use k8s_openapi::api::core::v1::{Node, Pod};
/// # use kube::{Resource, ResourceExt, api::GetParams};
/// # use kube::client::scope::NamespacedRef;
/// # use kube::api::DynamicObject;
/// # async fn wrapper() -> Result<(), Box<dyn std::error::Error>> {
/// # let client: kube::Client = todo!();
/// // cluster scoped
/// let cr: ClusterRole = todo!();
/// let cr: ClusterRole = client.fetch(&cr.object_ref(&())).await?;
/// assert_eq!(cr.name_unchecked(), "cluster-admin");
/// // namespace scoped
/// let svc: Service = todo!();
/// let svc: Service = client.fetch(&svc.object_ref(&())).await?;
/// assert_eq!(svc.name_unchecked(), "kubernetes");
/// // Fetch an owner of the resource
/// let pod: Pod = todo!();
/// let owner = pod
/// .owner_references()
/// .to_vec()
/// .into_iter()
/// .find(|r| r.kind == Node::kind(&()))
/// .ok_or("Not Found")?;
/// let node: Node = client.fetch(&owner).await?;
/// // Namespace scoped objects require namespace
/// let pod: Pod = client.fetch(&owner.within("ns".to_string())).await?;
/// // Fetch dynamic object to resolve type later
/// let dynamic: DynamicObject = client.fetch(&owner.within("ns".to_string())).await?;
/// // Fetch using local object reference
/// let secret_ref = pod
/// .spec
/// .unwrap_or_default()
/// .image_pull_secrets
/// .unwrap_or_default()
/// .get(0)
/// .unwrap_or(&LocalObjectReference{name: Some("pull_secret".into())});
/// let secret: Secret = client.fetch(&secret_ref.within(pod.namespace())).await?;
/// # Ok(())
/// # }
/// ```
pub async fn fetch<K>(&self, reference: &impl ObjectRef<K>) -> Result<K>
where
K: Resource + Serialize + DeserializeOwned + Clone + Debug,
{
let mut req = Request::new(reference.url_path())
.get(
reference
.name()

Check warning on line 344 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L344 

Added line #L344 was not covered by tests
.ok_or(Error::RefResolve("Reference is empty".to_string()))?,
&GetParams::default(),
)
.map_err(Error::BuildRequest)?;
req.extensions_mut().insert("get");
self.request::<K>(req).await
}

/// List instances of a `Resource` implementing type `K` at the specified scope.
///
/// ```no_run
Expand Down Expand Up @@ -216,13 +382,32 @@
}
}

// Resource url_path resolver
fn url_path(r: &ApiResource, namespace: Option<String>) -> String {
let n = if let Some(ns) = namespace {
format!("namespaces/{ns}/")
} else {
"".into()
};
format!(
"/{group}/{api_version}/{namespaces}{plural}",
group = if r.group.is_empty() { "api" } else { "apis" },
api_version = r.api_version,
namespaces = n,
plural = r.plural
)
}

#[cfg(test)]
mod test {
use crate::client::client_ext::NamespacedRef;

use super::{
scope::{Cluster, Namespace},
Client, ListParams,
};
use kube_core::ResourceExt;
use k8s_openapi::api::core::v1::LocalObjectReference;
use kube_core::{DynamicObject, Resource, ResourceExt};

#[tokio::test]
#[ignore = "needs cluster (will list/get namespaces, pods, jobs, svcs, clusterroles)"]
Expand Down Expand Up @@ -256,4 +441,100 @@

Ok(())
}

#[tokio::test]
#[ignore = "needs cluster (will get svcs, clusterroles, pods, nodes)"]
async fn client_ext_fetch_ref_pods_svcs() -> Result<(), Box<dyn std::error::Error>> {
use k8s_openapi::api::{
core::v1::{Node, ObjectReference, Pod, Service},
rbac::v1::ClusterRole,
};

let client = Client::try_default().await?;
// namespaced fetch
let svc: Service = client
.fetch(&ObjectReference {
kind: Some(Service::kind(&()).into()),
api_version: Some(Service::api_version(&()).into()),
name: Some("kubernetes".into()),
namespace: Some("default".into()),
..Default::default()
})
.await?;
assert_eq!(svc.name_unchecked(), "kubernetes");
// global fetch
let ca: ClusterRole = client
.fetch(&ObjectReference {
kind: Some(ClusterRole::kind(&()).into()),
api_version: Some(ClusterRole::api_version(&()).into()),
name: Some("cluster-admin".into()),
..Default::default()
})
.await?;
Comment on lines +466 to +473
Copy link
Member

Choose a reason for hiding this comment

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

it would be good to see have an error test as well, to see what happens if people type inference the wrong thing (e.g. :Service with ObjectRef data for a ClusterRole say). might not be able to do something useful, but if we could reduce confusion, then that's less issues for us down the line.

assert_eq!(ca.name_unchecked(), "cluster-admin");
// namespaced fetch untyped
let svc: DynamicObject = client.fetch(&svc.object_ref(&())).await?;
assert_eq!(svc.name_unchecked(), "kubernetes");
// global fetch untyped
let ca: DynamicObject = client.fetch(&ca.object_ref(&())).await?;
assert_eq!(ca.name_unchecked(), "cluster-admin");

// Fetch using local object reference
let svc: Service = client
.fetch(
&LocalObjectReference {
name: svc.name_any().into(),
}
.within(svc.namespace()),
)
.await?;
assert_eq!(svc.name_unchecked(), "kubernetes");

let kube_system: Namespace = "kube-system".into();
for pod in client
.list::<Pod>(
&ListParams::default().labels("component=kube-apiserver"),
&kube_system,
)
.await?
{
let owner = pod

Check warning on line 501 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L501 

Added line #L501 was not covered by tests
.owner_references()
.to_vec()
.into_iter()
.find(|r| r.kind == Node::kind(&()))

Check warning on line 505 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L505 

Added line #L505 was not covered by tests
.ok_or("Not found")?;
let _: Node = client.fetch(&owner).await?;

Check warning on line 507 in kube-client/src/client/client_ext.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/client_ext.rs#L507 

Added line #L507 was not covered by tests
}

Ok(())
}

#[tokio::test]
#[ignore = "needs cluster (will get svcs, clusterroles, pods, nodes)"]
async fn fetch_fails() -> Result<(), Box<dyn std::error::Error>> {
use crate::error::Error;
use k8s_openapi::api::core::v1::{ObjectReference, Pod, Service};

let client = Client::try_default().await?;
// namespaced fetch
let svc: Service = client
.fetch(&ObjectReference {
kind: Some(Service::kind(&()).into()),
api_version: Some(Service::api_version(&()).into()),
name: Some("kubernetes".into()),
namespace: Some("default".into()),
..Default::default()
})
.await?;
let err = client.fetch::<Pod>(&svc.object_ref(&())).await.unwrap_err();
assert!(matches!(err, Error::SerdeError(_)));
assert_eq!(err.to_string(), "Error deserializing response: invalid value: string \"Service\", expected Pod at line 1 column 17".to_string());

let obj: DynamicObject = client.fetch(&svc.object_ref(&())).await?;
let err = obj.try_parse::<Pod>().unwrap_err();
assert_eq!(err.to_string(), "failed to parse this DynamicObject into a Resource: invalid value: string \"Service\", expected Pod".to_string());

Ok(())
}
}
6 changes: 6 additions & 0 deletions kube-client/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ pub enum Error {
#[cfg_attr(docsrs, doc(cfg(feature = "client")))]
#[error("auth error: {0}")]
Auth(#[source] crate::client::AuthError),

/// Error resolving resource reference
#[cfg(feature = "unstable-client")]
#[cfg_attr(docsrs, doc(cfg(feature = "unstable-client")))]
#[error("Reference resolve error: {0}")]
RefResolve(String),
}

#[derive(Error, Debug)]
Expand Down
30 changes: 30 additions & 0 deletions kube-core/src/gvk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
use std::str::FromStr;

use crate::TypeMeta;
use k8s_openapi::{api::core::v1::ObjectReference, apimachinery::pkg::apis::meta::v1::OwnerReference};
use serde::{Deserialize, Serialize};
use thiserror::Error;

Expand Down Expand Up @@ -47,6 +48,35 @@
}
}

impl From<OwnerReference> for GroupVersionKind {
fn from(value: OwnerReference) -> Self {
let (group, version) = match value.api_version.split_once("/") {
Some((group, version)) => (group, version),
None => ("", value.api_version.as_str()),

Check warning on line 55 in kube-core/src/gvk.rs

View check run for this annotation

Codecov / codecov/patch

kube-core/src/gvk.rs#L52-L55 

Added lines #L52 - L55 were not covered by tests
};
Self {
group: group.into(),
version: version.into(),
kind: value.kind,

Check warning on line 60 in kube-core/src/gvk.rs

View check run for this annotation

Codecov / codecov/patch

kube-core/src/gvk.rs#L58-L60 

Added lines #L58 - L60 were not covered by tests
}
}
}

impl From<ObjectReference> for GroupVersionKind {
fn from(value: ObjectReference) -> Self {
let api_version = value.api_version.unwrap_or_default();
let (group, version) = match api_version.split_once("/") {
Some((group, version)) => (group, version),
None => ("", api_version.as_str()),
};
Self {
group: group.into(),
version: version.into(),
kind: value.kind.unwrap_or_default(),
}
}
}

/// Core information about a family of API Resources
#[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq, Hash)]
pub struct GroupVersion {
Expand Down
Loading