digitalocean / clusterlint

A best practices checker for Kubernetes clusters. 🤠
Apache License 2.0
547 stars 45 forks source link

Disable checking for each container #74

Closed valbeat closed 4 years ago

valbeat commented 4 years ago

Currently, the check is invalidated by metadata. However, this method does not allow you to disable checks on a per container basis. Is there a way to solve this?

adamwg commented 4 years ago

Hi @valbeat - thanks for the question.

This is an interesting idea - I can definitely see the use for it for checks like latest-tag where perhaps you want to use an image with the latest tag for only one container in a pod, but still want to check the others.

The Container object in the k8s API doesn't have metadata, so we can't specify any annotations there. That would be the obvious way to implement this if it were an option, since it would mirror how we currently do check suppression at the pod level.

One option would be to introduce new pod annotations that specify which containers in the pod should skip certain checks. For example, maybe something like this:

apiVersion: v1
kind: Pod
metadata:
  annotations:
    clusterlint.digitalocean.com/disabled-container-checks: "container1:latest-tag,container2:fully-qualified-image"

Does that seem reasonable?

@varshavaradarajan @timoreimann Any other ideas for how we could do this?

timoreimann commented 4 years ago

Putting a container reference in the pod annotation doesn't look super elegant, but I can't think of a better approach myself. So I'd be okay with that.

Can colons appear in today's clusterlint annotation values? Can we turn it into something reserved for this purpose?

adamwg commented 4 years ago

We don't currently use colons for anything in our annotations, just commas to separate check names. Seems like a reasonable pattern to establish for this kind of annotation, in case we want to implement other container-specific annotations in the future.

varshavaradarajan commented 4 years ago

I am not sure if we should allow this. A pod is the smallest logical unit in k8s that everyone operates with. Why should we be more granular than that. I get providing options to users to configure what they want to see, but shouldn't we deal with just API objects?

The pod checks that are applicable to a container in the pod are resource requests and limits, other image related checks. I don't think we should do allow disabling per container at all. If there is a problem, either ignore the check for the whole object or fix the configuration if possible. Especially in case of resource requirements check, if left to ignore for a specific container, can actually cause all sorts of problems later on. The use case makes sense for image related checks but those aren't severe enough in the first place to even be an error.

valbeat commented 4 years ago

Thanks for reply. I think that's true. I'll try to fix the configuration whenever possible.

But when I tune the kernel, I only want to enable privileged mode for initContainers. http://bogdan-albei.blogspot.com/2017/09/kernel-tuning-in-kubernetes.html

adamwg commented 4 years ago

In addition to the philosophical and usability concerns @varshavaradarajan raised, a practical concern I hadn't thought of is that since containers aren't a top-level resource each check that deals with them would have to implement this filtering. The annotation-based filtering we currently do happens after checks have been run, which is possible since diagnostics have a reference to the pod (or other object they apply to) in them.

Given the various concerns, I'm going to close this ticket. The use-case (especially for something like initContainers being used for kernel tuning) is reasonable, but I don't think we have a good way to implement the feature at this point. We've talked a bit in the past about implementing a way for individual checks to take their own configuration - this functionality may be a good fit for that feature if we implement it in the future.