Controller-runtime: Expose a union interface for metav1.Object and runtime.Object

[alvaroaleman]

Most (all?) Kubernetes API types implement both metav1.Object and runtime.Object. Code that needs both usually accepts either, then type asserts for the other.

It would be nice to have a union interface for the two and re-use that throughout our code

[DirectXMan12]

/kind feature

We've got a number of places where we error out if the object isn't a metav1.Object. When we next want to break things before 1.0.0, we should consider the implications of using this union interface, and adding helpers to assert that an object meets it (it's not a type-assert in cases where you have a generic runtime.Object).

[munnerz]

Just my 2c - not all types do implement both, only those that are actually created as CRDs.

In cert-manager we have a few extensibility mechanisms that rely on webhooks, and so we use a type that implements TypeMeta but not ObjectMeta: https://github.com/jetstack/cert-manager/blob/master/pkg/acme/webhook/apis/acme/v1alpha1/types.go#L30

This means we implement runtime.Object, but not metav1.Object. This gives us conversion etc. It’s a pattern borrowed from apiextensions-apiserver itself with the AdmissionReview and ConversionReview types.

I think there are places where this sort of union interface could be useful, but we should use it sparingly where it’s actually helpful, and consider that we limit the number of things that can use such a method to those that also have a ‘metadata’ stanza.

[DirectXMan12]

Right -- there are places where we actually want to say "we need object metadata", places where we say "we can do more stuff if we have object metadata", and places where all we need is the type.

[Adirio]

As I said in #666, unifying the interface in cases when there is a type assertion (and by assertion I mean a type cast that returns an error if it doesn't meet it) would move some errors from run time to compile time and would also allow IDEs to catch this errors. SetControllerRef seems to be one of them. Should we make a list and categorize them as @DirectXMan12 suggests (both interfaces required vs extra interface helpful)?

[DirectXMan12]

Should we make a list and categorize them as @DirectXMan12 suggests (both interfaces required vs extra interface helpful)?

:+1: from me

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen

[vincepri]

/help
/priority awaiting-more-evidence

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
/priority awaiting-more-evidence

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ruromero]

FWIW we introduced the KubernetesResource to our operator-utils library and found it useful in multiple situations.

As has been already mentioned, this interface can avoid runtime errors and avoid unnecessary type conversions and maybe create higher level abstractions for some methods.

[alvaroaleman]

Heads-up for ppl watching this issue, #898 merged so we now have such an union interface in pkg/controller/controllerutil. We don't yet use it anywhere though, hence leaving this issue open until we do use it where it makes sense.