Unable to override image for broker-ingress with operator

[aliok]

Describe the bug
Unable to override images for containers that are created by the eventing controllers and not the operator.

For example, we're able to change the image of broker-controller with registry override defined in KnativeEventing CR. With that, the operator creates the broker-controller and then later, broker controller creates broker-ingress.
However, broker-ingress is always created with the official gcr.io image.

broker-controller actually supports overriding the broker-ingress image using BROKER_INGRESS_IMAGE env var. But, there's no way to pass that from the operator CR, then to broker-controller.

Expected behavior
Some mechanism to override the images created by controllers and not just the operator.

Knative release version
0.13.0

Additional context
Add any other context about the problem here such as proposed priority

[aliok]

/assign @aliok

[aliok]

Operator creates a deployment of broker-controller like this:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: broker-controller
  namespace: knative-eventing
spec:
  template:
    spec:
      serviceAccountName: eventing-controller
      containers:
      - name: broker-controller
        terminationMessagePolicy: FallbackToLogsOnError
        image: gcr.io/knative-releases/knative.dev/eventing/cmd/channel_broker@sha256:853e0d194223da4356af7c9a17c79afa0ef96b93fd956d5febb2b1f2be78c717
        env:
        - # Broker
          name: BROKER_INGRESS_IMAGE
          value: gcr.io/knative-releases/knative.dev/eventing/cmd/broker/ingress@sha256:d080653ce0792a304ba388dcb1e360dc427d8556503e0a2603da26d261ac21e4

We're able to change the image for the broker-controller itself using the eventing CR like this:

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

but not the BROKER_INGRESS_IMAGE.

I suggest that we provide a mechanism to override env vars for the containers as well, not just their images.

So, the eventing CR would look like something like:

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
...
spec:
  registry:
    override:
      broker-controller: docker.io/my-broker-controller
  envvar:
    overrides:
      - container: broker-controller
        name: BROKER_INGRESS_IMAGE
        value: docker.io/my-broker-ingress-image

This would end up in this:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: broker-controller
  namespace: knative-eventing
spec:
  template:
    spec:
      serviceAccountName: eventing-controller
      containers:
      - name: broker-controller
        terminationMessagePolicy: FallbackToLogsOnError
        image: docker.io/my-broker-controller
        env:
        - # Broker
          name: BROKER_INGRESS_IMAGE
          value: docker.io/my-broker-ingress-image

[aliok]

cc @markusthoemmes

[aliok]

what I suggest above is far from being ideal, but until we have things addressing the tickets noted at knative/serving-operator#301, I think this would be a straigtforward solution.

[aliok]

cc @k4leung4 @houshengbo

[matzew]

Looking at the suggested proposal:

...
  envvar:
    overrides:
      - container: broker-controller
        name: BROKER_INGRESS_IMAGE
        value: docker.io/my-broker-ingress-image

I think this is also interesting, since the operator can than also override other evn vars, on a given container.

[matzew]

Note there are other "env var" images in eventing.

like:

• BROKER_INGRESS_IMAGE
• BROKER_FILTER_IMAGE
• CRONJOB_RA_IMAGE
• PING_IMAGE
• APISERVER_RA_IMAGE
• DISPATCHER_IMAGE

looks like the override, was only partially working, which seems to be a problem

[jcrossley3]

Personally, I like the idea of a generic "environment variable override" feature in the Knative{Serving,Eventing} CR, e.g.

...
  env:
  - name: BROKER_INGRESS_IMAGE
    value: docker.io/my-broker-ingress-image
  - name: CRONJOB_RA_IMAGE
    value: docker.io/my-cronjob-ra-image

The transformer function in the operator would then set all the Deployment resources accordingly in the manifest.

[aliok]

@jcrossley3 I think that would make things easier. It would solve my problem here and it would be actually a nicer feature.

The only downside would be that in the future there might be cases where we're passing different values for the same env var to different containers. I don't have a strong opinion on this though. No need to overengineer and solve problems we don't have right now. So, +1!

[jcrossley3]

The only downside would be that in the future there might be cases where we're passing different values for the same env var to different containers. I don't have a strong opinion on this though. No need to overengineer and solve problems we don't have right now. So, +1!

Good point. We could provide an optional container attribute in addition to name and value. If omitted, the var would apply to all containers.

[aliok]

We had a WG meeting yesterday and this issue was discussed there.

The outcome of the discussion is that we improve the existing image override mechanism to also override images that are defined in env vars. The challenge in this case is to identify what env vars are images.

I will keep this ticket open for now.