-
-
Notifications
You must be signed in to change notification settings - Fork 319
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.
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
Panic policy #634
Comments
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. |
Yeah, that is a good assessment, and possibly the 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 |
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 IIRC I've sometimes used |
For object reference part, if using Related; we could potentially bite the bullet on |
Not sure I see how |
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. |
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 |
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 ofResource
-impl
ing 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):kube
invariantskube
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
).metadata.name
)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.
The text was updated successfully, but these errors were encountered: