Improve the container image override mechanism

[aliok]

Problem
As seen in knative/eventing#2794, we cannot simply use the container names for overriding images for containers. Container names can be duplicated.

Current override mechanism is like this:

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
...
spec:
  registry:
    override:
      eventing-controller: docker.io/my-eventing-controller
      eventing-webhook: docker.io/my-eventing-webhook

This cannot handle the case that we have for imc-controller and broker-controller deployments the same container name, controller.

My suggestion would be instead of only providing the container name, we pass more information. Something like:

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
...
spec:
  registry:
    overrides:
    - namespace: knative-eventing
      deployment: eventing-controller
      container: eventing-controller
      image: docker.io/my-eventing-controller
    - namespace: knative-eventing
      deployment: eventing-webhook
      container: eventing-webhook
      image: docker.io/my-eventing-webhook

This would fix the case that we have for imc-controller and broker-controller deployments the same container name, controller.

Persona:
Which persona is this feature for?
System admin

Exit Criteria
Proper image overriding, regardless of container names being unique or not.

Time Estimate (optional):
1 day

Additional context (optional)

See the causing ticket: knative/eventing#2794

Related: #147

[houshengbo]

@aliok The section registry.override is able to configure the image for each deployment individually: https://github.com/houshengbo/docs/blob/operator-cr-configuration/docs/install/operator/configuring-serving-cr.md#private-repository-and-private-secrets.

Here is the example for eventing CR: https://github.com/houshengbo/docs/blob/operator-cr-configuration/docs/install/operator/configuring-eventing-cr.md#download-images-from-different-repositories-without-secrets

imc-controller and broker-controller can have their images defined in ANYTHING, in terms of format or name. Still not handle this issue?

[aliok]

@houshengbo as we talked offline, the match for the override is done using the container names, not deployment names. And, in eventing, we have container names that are not unique.

We either go with a solution that I suggested in the issue description, or we use deployment names to match. However, a deployment can have more than 1 containers, although we don't have that case yet.

[houshengbo]

@aliok Thanks.

I did not realize the difference between serving and eventing, in terms of how they define the deployment name and the container name for the deployment. In serving, both of them match, but in eventing, the container name is different from the deployment name.

To me, using deployment name is easier to find than using container name, coz everyone can easily find them by

vincent@VincentMacBookPro docs % k get deployment -n knative-eventing
NAME                  READY   UP-TO-DATE   AVAILABLE   AGE
broker-controller     1/1     1            1           15h
eventing-controller   1/1     1            1           15h
eventing-webhook      1/1     1            1           15h
imc-controller        1/1     1            1           15h
imc-dispatcher        1/1     1            1           15h

If we have to stay with container name, we have to issue other commands to check the detailed info of each deployment individually.

[aliok]

Yeah, I first tried to change the container names in eventing here: knative/eventing#2794

But I understand the concerns.

[aliok]

@jcrossley3 what do you say about this? matching with deployment names instead of container names?
This is not an ultimate solution like described in the PR, but it would solve the problem of having same container names for deployments. So, from one non-ideal solution to another.

[houshengbo]

@aliok @jcrossley3 @markusthoemmes
As we discuss in our community channel, we will take the suggestion from @markusthoemmes, by using deployment/container as the key, to form a unique name, since / is not allowed in kube resource name.

[jcrossley3]

What's the behavior when there's no / in the key: deployment, container, or either?

[aliok]

What's the behavior when there's no / in the key: deployment, container, or either?

I think the container names are optional in Kubernetes deployments.
If we were to provide this as a library to use in any Kube project, we would do some more smart things but in this case I say we simply reject or ignore that override. We don't have a webhook and I wouldn't want to introduce a webhook for this. So, we should just ignore that override.
WDYT?

[markusthoemmes]

I'd say if there's no / we default to the only container in a deployment. If the deployment has more than 1, we surface an error via the status?