Genericize PartialObjectMeta over the underlying kind

[clux]

Have been trying to write a dynamic controller today using PartialObjectMeta as the root kind and it suffers from a problem that makes it hard to integrate into larger setups:

Multiple watches with PartialObjectMeta uses the same type

Meaning if users have generic code over K before, that suddenly won't work when including more than one metadata_watcher because you now need another method to signal out-of-band what type it is (and this has to be done with string kinds through ApiResource).

The string signalling is also sometimes counter-intuitive because the typemeta found on a PartialObjectMeta is not the same as the typemeta found on the equivalent DynamicObject. E.g. typemeta from a metadata watch from a pod shows the following typemeta: TypeMeta { api_version: "meta.k8s.io/v1", kind: "PartialObjectMetadata" } (i.e. it is actually impossible to figure out what kind a metadata watch event came from).

Proposed Solution

I think this can be improved by changing PartialObjectMeta struct to include a PhantomType<K: Resource> so that the struct becomes generic over K like everything else.

This means users have to specify PartialObjectMeta<Deployment> even though technically nothing from the underlying Deployment type is used in the object. But on the flip side, users can now continue writing generic code that is generic over K: Resource (provided we impl<K> Resource for PartialObjectMeta<K> where K: Resource).

I don't think this will massively impact the setup in metadata_watcher other than slightly change the signature.

[clux]

Example code that is hard to integrate: a polymorphic reconciler for a labeller style controller that acts on many kinds:

pub async fn reconcile<K>(obj: Arc<K>, ctx: Arc<Context>) -> Result<Action>
where
    K: Resource<Scope = NamespaceResourceScope, DynamicType = ()>
        + Clone
        + DeserializeOwned
        + Debug,
    <K as Resource>::DynamicType: Default,
{
    let kind = K::kind(&()).to_string();

created by multiple invocations of Controller::new() on various object types.

details

async fn run_controller<K>(ctx: Context) -> Result<()>
where
    K: Resource<Scope = NamespaceResourceScope, DynamicType = ()>
        + Clone
        + DeserializeOwned
        + Debug
        + Sync
        + Send
        + 'static,
{
    let kind = K::kind(&()).to_string();
    tracing::info!("Starting controller for {kind}");

    let api = Api::<K>::all(ctx.client.clone());
    let context = Arc::new(ctx);

    Controller::new(api, ListParams::default())
        .run(reconcile, error_policy, context)
        .for_each(|_| futures::future::ready(()))
        .await;

[nightkr]

I'm curious about the use case where you need the type to be unique, although I'm not entirely opposed to doing this. Is this about using it in a TypeMap or something similar that relies on TypeId?

[clux]

The type does not need to be unique, it's just a very convenient way to carry type information. One that comes with many safety benefits, and it's a method we already use to carry such information.

To illustrate, it is possible to change the above reconciler into a purely dynamic one that does not use generics at all:

pub async fn reconcile(
    obj: Arc<PartialObjectMeta>,
    ctx: Arc<Context>,
    ar: Arc<ApiResource>,
) -> Result<Action> {
    let kind = ar.kind.clone();

but you do need an extra argument here because you can't backtrack from a PartialObjectMeta to the concrete type (there's no identifying information inside of it).

I have a workaround by invoking this 3-argument reconciler through a lambda from Controller::run by passing an arc'd ApiResource around:

    Controller::new_with(api, ListParams::default(), (*ar).clone())
        .run(
            |obj, ctx| reconcile(obj, ctx, ar.clone()),
            |obj, err, ctx| error_policy(obj, err, ctx, ar.clone()),
            context
        )

but this is obviously awkward

• type information requires extra discovery steps or hacky creations of ApiResources before starting a controller
• we cannot constrain on the scope of resource anymore (because dynamic types are much weaker that way atm)

and on top of this ApiResource needs to be unpacked from an arc all the time in this setting (Api::all_with needs an un-arcd one).

[clux]

As a comparison with DynamicObject (which tbh also could benefit of having a way to grab static type info from a root type - but maybe an optional one), there is something more pervasive about PartialObjectMeta;

It's the first type that does not have any way of gathering any runtime type information. With DynamicObject you can at the very least look up TypeMeta.

[mateiidavid]

Bit of an oversight on my part, hadn't even thought about this. I had some questions about this that were answered on Discord. IIUC having type safety here and ensuring we can easily get the type info at runtime is desirable, regardless of whether a concrete use case requires it.

I personally think it makes sense. We check types at runtime in our admission controller, and we do it through a generic function, something like:

is_kind<T>(obj: &AdmissionRequest) -> bool {
T: Resource + (...)
}

if is_kind::<Pod>(&obj) {
  // do something
}

I extended this to @clux's example:

// In certain configurations, namespaces should be considered the owners of a resource and will likely
// represent the root of configuration. This is a trivial example but we could propagate the labels from
// namespace to all pods within, or whatever else would be necessary in a more generic reconciler.
async fn reconcile<K>(obj: Arc<K>, ctx: Arc<Data>) -> Result<Action, Error>
where
    K: Resource<DynamicType = ()> + Clone + DeserializeOwned + std::fmt::Debug + Sync + Send + 'static,
{
    let client = &ctx.client;
    if is_kind::<Namespace, _>(&*obj) {
        let _api = Api::<Namespace>::all(client.clone());
        // api.patch_metadata(name, pp, patch)
    }

    if is_kind::<Pod, _>(&*obj) {
        let _api = Api::<Pod>::default_namespaced(client.clone());
        // api.patch_metadata(...)
    }


    Ok(Action::requeue(Duration::from_secs(300)))
}

fn is_kind<T, K>(obj: &K) -> bool
where
    T: Resource,
    T::DynamicType: Default,
    K: Resource<DynamicType = ()> + Clone + DeserializeOwned + std::fmt::Debug + Sync + Send + 'static,
{
    let dt = Default::default();
    K::group(&()).to_string().eq_ignore_ascii_case(&*T::group(&dt))
        && K::kind(&()).to_string().eq_ignore_ascii_case(&*T::kind(&dt))
}

// Run controller on both pod and namespace types

Maybe I'm going on a tangent here and don't fully understand the breadth of the change, but imo it makes sense to ensure that even when we operate on PartialObjectMetadata, we have an easy and robust way of checking the type info.

The alternative would be to either support a reconciler for every new type we add, or to do something similar to what was recommended above which is going to end up complicating the code even further. Don't think this is super maintainable down the line.

I can't think of a better example, it's true we could always just use concrete types and not do any metadata watches here, but in hindsight it does feel like an incomplete solution if it can't be used to build similar abstractions 🤷🏻 . Happy to make the change but I'll default to whatever the consensus is amongst the maintaining team, you know best. Also let me know if I got anything wrong here.

[clux]

I ended up playing with this one, because have an actual controller that struggles with this exact problem in my day-job. See #1152