Adds ability to elide namespace on trigger_owners

[sjbodzo]

Update trigger_owners to enable namespace erasure on the ObjectRef constructed for the owner.

Motivation

trigger_owners assumes the child's owner is scoped to the same namespace as the child, but this assumption breaks when the owner object is global (not namespace scoped).

Solution

An additional parameter is introduced to enable namespace elision when owner_inherits_namespace is set to false, otherwise defaulting to the namespace of the child's meta ref.

I am open to more elegant approaches, but I do not see a way to avoid adding the extra parameter without client calls about the owner to infer if it is namespaced or not. Did I miss a better way?

[codecov[bot]]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c7054b5) 72.1% compared to head (c308644) 72.0%.

:exclamation: Current head c308644 differs from pull request most recent head 8a0bd55. Consider uploading reports for the commit 8a0bd55 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1383     +/-   ##
=======================================
- Coverage   72.1%   72.0%   -0.0%     
=======================================
  Files         75      75             
  Lines       6458    6436     -22     
=======================================
- Hits        4651    4633     -18     
+ Misses      1807    1803      -4     

Files	Coverage Δ
kube-runtime/src/controller/mod.rs	33.9% <0.0%> (-0.2%)	:arrow_down:

... and 3 files with indirect coverage changes

[clux]

Hey, thanks for this. This sounds like a bug if it's not working.

Naming wise this feels slightly odd. The restriction that an owner must be in the same namespace (or cluster-scoped) is a Kubernetes requirement, owner references have these docs:

An owning object must be in the same namespace as the dependent, or be cluster-scoped, so there is no namespace field.

So if this isn't working, then it's us failing to identify the scope of the resource, rather than exposing a bypass bool. I think we might be able to do this by inspectingResource impl instead. We should have the dyntype (associated type for the Resource) at hand to be able to get the scope out of it, but this might not be as exposed as I would like. I bundled up a Scope getter trait in a much-larger unmerged change. It might be worth trying to revive that part of this PR 🤔

[sjbodzo]

Hey, thanks for this. This sounds like a bug if it's not working.

Naming wise this feels slightly odd. The restriction that an owner must be in the same namespace (or cluster-scoped) is a Kubernetes requirement, owner references have these docs:

An owning object must be in the same namespace as the dependent, or be cluster-scoped, so there is no namespace field.

So if this isn't working, then it's us failing to identify the scope of the resource, rather than exposing a bypass bool. I think we might be able to do this by inspectingResource impl instead. We should have the dyntype (associated type for the Resource) at hand to be able to get the scope out of it, but this might not be as exposed as I would like. I bundled up a Scope getter trait in a much-larger unmerged change. It might be worth trying to revive that part of this PR 🤔

Thank you for the thoughtful feedback! I will carve some time out this week to review and take a stab at the requested changes.

[sjbodzo]

I spent some time reviewing the crate to try and understand. Thanks in advance for any pointers and feedback.

Is this about right?

• discovery (and perhaps kube-derive?) are able to translate objects into ApiResource and ApiCapabilities, which includes the /api data we are interested in from k8s
• the referenced PR modifies the ApiResource internals to consolidate ApiCapabilities into it, which introduces some tradeoffs and potential pitfalls (e.g. erasing typed objects from k8s_openapi and comparing them to DynamicObject references, presumably because one path generates capabilities and the other does not)
• Separate from that consolidation, the PR also introduces a trait bound on the associated type Scope with a new trait called Scope in the Resource trait, which is populated using the same mechanisms that discovery uses today..

Is it your recommendation, @clux, that we attempt to bring that Scope trait into this PR? Then we could do something like

pub fn trigger_owners<KOwner, S>(
    stream: S,
    owner_type: KOwner::DynamicType,
    child_type: <S::Ok as Resource>::DynamicType,
) -> impl Stream<Item = Result<ReconcileRequest<KOwner>, S::Error>>
where
    S: TryStream,
    S::Ok: Resource,
    <S::Ok as Resource>::DynamicType: Clone,
    KOwner: Resource,
    KOwner::DynamicType: Clone,
{
    let mapper = move |obj: S::Ok| {
        let meta = obj.meta().clone();
        let ns = match K::is_namespaced(&owner_type) {
            true => meta.namespace,
            false => None,
        };
        let owner_type = owner_type.clone();
        meta.owner_references
            .into_iter()
            .flatten()
            .filter_map(move |owner| ObjectRef::from_owner_ref(ns.as_deref(), &owner, owner_type.clone()))
    };
    trigger_others(stream, mapper, child_type)
}

It is much nicer! Any pointers on adding the Scope trait itself, or if there is a shorter path?

[clux]

Hey, yes, in essence this is what I am proposing! Although it might amount to doing similar things to the linked PR first. The difficulty of your PR would basically be transferred into getting the scope trait correct + and ensuring we can pass it easily.

The problem with using the Scope as it stands (even if we add methods on ResourceScope directly) is that it cannot be easily passed (it is an associated type and is usually used to constrain via say where K: Resource<Scope = DynamicResourceScope>, but that's not really doable when you have a DynamicType and need to support all variants (as we can't do specialisation). Either the Scope instance need to come from somewhere (multiply arguments throughout controller - horrible), or the scope must be embedded into the DynamicType => ApiResource where we can inspect it and call the .is_namespaced() (or similar) method.

There were some issues that I forget now because it was a long discussion on the PR, so will have to refamiliarise myself a bit, but I think the core thing is still necessary in some form to solve a bunch of related issues - and it also looks like it is necessary to correctly solve this.