Add helpers for iterating containers in a pod

[verb]

What type of PR is this?

/kind cleanup

What this PR does / why we need it: Iterating through all of the containers in a pod is common, and code is often duplicated to iterate through both Containers and InitContainers. This will get worse with additional container types such as Ephemeral Containers.

This PR adds helpers based on the existing VisitPodConfigmapNames/VisitPodSecretNames. There are two flavors: one utility supporting early exit and one supporting pathspec intended for use with validation.

In addition to adding VisitContainers(), this PR updates many of the instances where Containers and InitContainers were iterated in immediate succession. This PR explicitly avoids updating the kubelet because of the frequency with which the kubelet treats Containers and InitContainers differently. I will follow up with a separate PR to a sig-node reviewer to update the kubelet.

Which issue(s) this PR fixes:
WIP #27140

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[verb]

This PR forked from #59416

/assign @smarterclayton

[smarterclayton]

Related to my earlier comment, how would a reviewer tell this case apart from the "get error out of the closure" case? Structurally if the only difference is a single : I worry that it's not obvious.

[verb]

Good point. Maybe using something that looks like err across the boundary of a closure is an anti-pattern. Perhaps err should be reserved for local scope since that's how it's used 95% of the time.

Requiring a more verbose retErr or allErrs could signal the reviewer to the difference.

I've updated all instances of this in this PR.

[verb]

reflecting on it a bit further, an even more defensive method would be to use both:

var retErr error
podutil.VisitContainers(&pod.Spec, func(c *v1.Container) bool {
  if err := checkContainer(c); err != nil {
    retErr = err
    return false
  }
  return true
})

I don't think this is really necessary, though, so I didn't include it in my update.

[smarterclayton]

some comments

[verb]

@smarterclayton I didn't want to change the behavior of this plugin without discussing it with someone, but I noticed other plugins use utilerrors.NewAggregate(allErrs) rather than returning the first error.

[smarterclayton]

Hrm, if we do this then we have to evaluate all images on all pods, vs just the first invalid one. However, in a successful case we have to check all of them.

I'm ok with the change.

[smarterclayton]

But can you open a follow up and we can get the plugin authors to discuss?

[verb]

Opened #79378

[smarterclayton]

Unfortunate here that we miss early exit

[smarterclayton]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, verb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/OWNERS [smarterclayton]
• pkg/api/OWNERS [smarterclayton]
• plugin/pkg/admission/OWNERS [smarterclayton]
• staging/publishing/OWNERS [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tedyu]

For some use case, such as PodRequestsAndLimits() :

for _, container := range pod.Spec.InitContainers {
   maxResourceList(reqs, container.Resources.Requests)
   maxResourceList(limits, container.Resources.Limits)

The operation performed on init container is different from that performed on normal container.

@verb @smarterclayton
Do you think it is worth adding a parameter, on top of *v1.Container, which indicates the type of container: normal, init, etc.

Here is a sketch:

https://pastebin.com/6xb0PUG1

[verb]

tl;dr I'm not sure it's common enough to warrant the added complexity, but I could be convinced.

@tedyu I'm in favor of making VisitContainer more useful because it reduces repetition when one wants to iterate through all (current and future) container types, but in this case there's no repeated code to remove.

So far your example seems representative, so we'd replace:

for _, container := range pod.Spec.Containers {
	addResourceList(reqs, container.Resources.Requests)
	addResourceList(limits, container.Resources.Limits)
}
// init containers define the minimum of any resource
for _, container := range pod.Spec.InitContainers {
	maxResourceList(reqs, container.Resources.Requests)
	maxResourceList(limits, container.Resources.Limits)
}

with:

podutil.VisitContainers(&pod.Spec, func(c *Container, t ContainerType) bool {
	switch t {
	case podutil.Container:
		addResourceList(reqs, container.Resources.Requests)
		addResourceList(limits, container.Resources.Limits)
	// init containers define the minimum of any resource
	case podutil.InitContainer:
		maxResourceList(reqs, container.Resources.Requests)
		maxResourceList(limits, container.Resources.Limits)
	}
	return true
})

I usually don't like this because we've introduced a Kubernetes-specific abstraction for no real reason. Any Go programmer can read the range statements, but they'd have to know about VisitContainers() specifically, and introducing closures adds room for bugs.

That being said, it can be easily inferred what VisitContainers does here and I think it might even be more readable if you're familiar with VisitContainers. It would make adding a new container type safer because we can easily reference all locations we may need to update for the new type.

For me it really hinges on whether it would see use in places that frequently iterate the container lists to different effect, like the kubelet. @yujuhong what do you think?

---

Side thought:

I can imagine a reviewer objecting to the unnecessary iteration of the other container types (in this case, ephemeral containers). I think that's an acceptable trade off for readability until analysis shows this to be the bottleneck, but that thinking led me to omit the early return from VisitContainersWithPath().

We could solve that with a bitmask, something like:

VisitContainers(&pod.Spec, Container|InitContainer, func(c *Container, t ContainerType) bool {
    switch t {
    case InitContainer: maxResourceList()
    ...

but I think it's probably premature optimization.

[tedyu]

Thanks @verb for detailed reply.

For part 1, I agree with you - that's why I didn't send out PR.

My understanding is that, if operations for two out of 3 (maybe more in the future) container types is common, the new parameter would help reduce duplicate code.

I will continue looking for proper use case(s).

[tedyu]

For part 2, I think passing bitmask allows skipping feature gate checking for new container type(s).
That seems to be a benefit.

[verb]

It would make adding a new container type safer because we can easily reference all locations we may need to update for the new type.

Thinking about this a little more, if our goal was to consolidate container iterations for correctness, then the bitmask would be required.

[verb]

Something for the record: @msau42 better statement the argument of using VisitContainers for correctness than I did in #81838 (comment), I think.