Merge spec.container with the default one not overwrite it

[MaciekLeks]

Expected Behavior

I need to modify some aspects of the spec.container section, for example, I need to add initialDelaySeconds in startupProbe (I use Istio and the proxy container restarts before its first successful launch due to Istio). However, I can't do this without rewriting the entire container section, which I believe isn't not only complex, but also not appropriate.

Actual Behavior

I am forced to overwrite the entire container section.

Steps to Reproduce the Problem

n/a

Specifications

• Version: v1.4.1
• Platform: GKE

[hessjcg]

This is an interesting use case. I imagine you are not the only user with Istio who wants to set initialDelaySeconds to get their system to work.

My preferred solution would be to add the field AuthProxyContainerSpec.InitialDelaySeconds. This is most in line with existing functionality for container spec fields like AuthProxyContainerSpec.Resources and AuthProxyContainerSpec.Image

Alternatively, we could create a general-purpose way to patch the proxy container. However, I don't recommend this approach, because it will create conflicts with other fields like AuthProxyContainerSpec.Resources and AuthProxyContainerSpec.Image. Also, this approach gives a lot of freedom to make mistakes. This could harm the stability of the proxy in production and make the operator more complex to use and maintain.

If we do proceed with a way to patch the container, I suggest that we create a new field AuthProxyContainerSpec.containerPatch which would hold a *corev1.Container. Values set in containerPatch would override the values in the proxy container created by the operator.

We should not change the purpose of the existing field AuthProxyContainerSpec.container. This field is intended as a full replacement of the proxy container spec. It is used for testing and debugging.

[MaciekLeks]

@hessjcg, what if I need to change the failureThreshold default value some day? Would you add a new field to the AuthProxyContainerSpec then? While I understand your point, I'm certain that adding one field will open the floodgates for more fields. Eventually, we might fall into the Law of Leaky Abstractions with the standard container metadata abstraction.

[enocom]

@MaciekLeks It sounds like the bigger ask here is to make the Operator work well with Istio. Is that correct?

[MaciekLeks]

@enocom, if we analyze this in a straightforward manner, then yes, that's the root cause of my issue.