Panic policy

[nightkr]

Currently, we have a bunch of helpers that can implicitly panic the program when their invariants aren't upheld externally, such as ResourceExt::name. These essentially allow the apiserver (or any other source of Resource-impling objects) to panic the whole process.

Are these a good idea to encourage? Should we at least ban their transitive usage within the core kube crates?

If we do keep them around, we should probably at the very least document a policy for which invariants should be allowed to cause kube to panic. For example, it's worth considering (in escalating order of exposure):

1. private kube invariants
2. kube invariants exposed to the application (for example: any object to be created on the apiserver must have a non-empty .metadata.name xor .metadata.generate_name)
3. apiserver invariants (for example: any object returned by the apiserver must have a non-empty .metadata.name)
4. object-level expectations (for example: Pod will pretty much always have >=1 container)

Currently we don't really have a strong policy on this (or we wouldn't need this issue), but in general we've been leaning towards 3 being the limit for when we're allowed to panic.

[nightkr]

Going into personal opinion, I think 1 is pretty much unavoidable, 2 is often undesirable but may be the least bad option, and 3+ are walking and talking DoS vulnerabilities that we ought to discourage where possible.

[clux]

Yeah, that is a good assessment, and possibly the ResourceExt::name case has probably gone a bit far for convenience internally. We probably should not use it in the general case if it can bite users who use generate_name. Are there clear cases where it's violated?

How do you propose we document this policy? There was a crate that let us ban usage of certain methods from certain paths (but can't remember the name of it), that could possibly deal with an internal limit?

There is also crates such as pre + contracts: https://docs.rs/pre/0.2.0/pre/ or https://docs.rs/contracts/0.6.2/contracts/
that allow us to put #[pre]-conditions that we have to #[assure] at the call-site. But not sure if that's super useful if it's exported.

[nightkr]

For documenting the policy itself, probably throw it in a document in the repo and maybe link to it from CONTRIBUTING?

For enforcing it, I'd probably go as far as deprecating the offending exceptions.

For guidance on what to do instead, one somewhat more ergonomic way would be to define a common helper struct with the fields you need, and a TryFrom<ObjectMeta>. That should let you centralize the error checking in one place.. There might be value in providing a derive macro to help with that process...

IIRC I've sometimes used ObjectRef<K> as that helper type, fwiw.

[clux]

For guidance on what to do instead, one somewhat more ergonomic way would be to define a common helper struct with the fields you need, and a TryFrom<ObjectMeta>. That should let you centralize the error checking in one place.. There might be value in providing a derive macro to help with that process...

IIRC I've sometimes used ObjectRef<K> as that helper type, fwiw.

For object reference part, if using ObjectReference you don't even need any error checking. I added one conversion helper for this in #663.

Related; we could potentially bite the bullet on ResourceExt::name and rename it to ResourceExt::name_unchecked, and maybe put one that just gets the Option out in its place. That at least forces users to think whether the level 2 invariant is right for them.

[nightkr]

Not sure I see how ObjectReference helps.. its fields are as Optional as ObjectMeta's as far as I can tell?

[clux]

Yeah, it doesn't really help with panic policies, but it avoids the need to do any unwraps (since the event api only takes options as input). That's probably not a thing that helps in a lot of cases though. Just thought i'd mention it.

[clux]

Have had a veeery quick go at adding something about this in kube-rs/website#21 .

This is less important now that we've dealt with the more egregious problem-child ResourceExt::name in favour of the explicit ResourcExt::name_unchecked but it's good to write down something here even if i'm not quite sure about where to put it on the website. Anyway, maybe we can discuss there.