Set default Authorino image via env var

[guicassolato]

We may need to roll back to setting the default Authorino image used by the Operator based on an env var, instead of the current approach of branding this value inside the Operator's binary, at compilation time.

This PR removes the controllers.DefaultAuthorinoImage ldflag from the Go binary, replacing it with a new env var DEFAULT_AUTHORINO_IMAGE.

Why to we need this change?

TLDR: productisation.

It turns out that Red Hat productisation machinery for building Kubernetes operators is more prepared to tweak with the image build settings when using env vars for configuring the related operand image, than by following our current approach.

Some automatic pinning of sha256 image digests from floating tags and conversion between registry references can only be leveraged at certain steps of the productisation pipeline. These exclude the building of the operator image following the lastest build of the operand, as part of the same pipeline.

Why would we NOT want this change?

This change does not come without a cost. By ditching the ldflag set at compilation time, we:

1. lose some traceability of the Operator's binary and container image, whose checksums will no longer reflect the paring to a specific default Authorino image;
2. introduce the risk of misuse of this configuration potentially breaking all deployments of Authorino in a cluster.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 61.78%. Comparing base (99e413d) to head (76836e1).

Files	Patch %	Lines
controllers/authorino_controller.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #174   +/-   ##
=======================================
  Coverage   61.78%   61.78%           
=======================================
  Files           2        2           
  Lines         785      785           
=======================================
  Hits          485      485           
  Misses        249      249           
  Partials       51       51           

Flag	Coverage Δ
unit	61.78% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[eguzki]

RELATED_IMAGE_AUTHORINO

In the operator bundle building pipeline, there is some logic to catch RELATED_IMAGE_* env vars to get dependency images and build dependency tree used for some verification steps. I can try to find the docs out there.

[eguzki]

I think it is about automatic rebuild of the bundle when there is a rebuild of one of the dependencies to catch the latest builds.

[guicassolato]

In the specific case of Authorino, I worry that RELATED_IMAGE_ could be bit misleading, because of the override in the Authorino CR.

Not that changing would be a big deal for me. I just didn't want to convey that it means the same thing as it does in Limitador.

[eguzki]

Sorry, I do not follow. The usual order of preference is:

1. Authorino CR -> image name
2. CSV -> env var RELATED_IMAGE_AUTHORINO of the container (also useful for dev/testing)
3. Default image hardcoded in some const default variable (usually latest)

But, your call!

[guicassolato]

1. Authorino CR -> image name
2. CSV -> env var RELATED_IMAGE_AUTHORINO of the container (also useful for dev/testing)

Exactly how you described, but notice that Limitador is not the same, right? (Or maybe it is and I missed that in the Limitador CR.)

Last time I checked, in Authorino Operator, the related Authorino image is a default value that can be overridden in the Authorino CR. Whereas in Limitador Operator, the related Limitador image is NOT a default value that can be overridden anywhere; rather, if set, the value of the env var is final.

IOW, Limitador Operator has a "related operand image"; Authorino Operator has a "default related operand image", if that makes sense.

3. Default image hardcoded in some const default variable (usually latest)

Another difference between Limitador and Authorino operators here.

Notice that, for Authorino Operator, we did NOT hard code a default image value for the case where the env var is missing. I'd argue that breaking the deployments if the env var is missing is better than hiding a silent potential problem that could happen by bumping all operands to latest.

[eguzki]

Exactly how you described, but notice that Limitador is not the same, right? (Or maybe it is and I missed that in the Limitador CR.)

In the limitador operator the order of preference is as I described.

1. If the image is set in the CR, then use that here

image := GetLimitadorImageVersion()
if limitador.Spec.Version != nil {
        image = fmt.Sprintf("%s:%s", LimitadorRepository, *limitador.Spec.Version)
}

2. CSV -> env var RELATED_IMAGE_LIMITADOR here

func GetLimitadorImageVersion() string {
	return env.GetString("RELATED_IMAGE_LIMITADOR", defaultImageVersion)
}

3.- Default image hardcoded in some const default variable (usually latest)

var (
	defaultImageVersion = fmt.Sprintf("%s:%s", LimitadorRepository, "latest")
)

Notice that, for Authorino Operator, we did NOT hard code a default image value for the case where the env var is missing. I'd argue that breaking the deployments if the env var is missing is better than hiding a silent potential problem that could happen by bumping all operands to latest.

Not a bad idea. But in practice, the CSV will always have a related image (many pipeline verification steps ensure that).
Having a default hardcoded image in case it is not set in the CR nor env var is handy for dev/testing. Anyway, I also like your idea to force some env var but not a big deal and never had issues with that.

[eguzki]

There is one caveat in the limitador CR that I was not approved to implement: from the CR you can only especify the docker tag and the registry is hardcoded. I need to write RFC to fix that and blah blah blah... and in the end, the image set in the limitador CR is not a supported thing, just useful for dev/testing and a way out in case the user wants to use their own image.

[guicassolato]

TIL: one can override the env var in the Limitador CR. Well, at least the image tag part. Thanks, @eguzki!

[eguzki]

what is the rationale to get the image url from the config map? why not a plain env var? The CSV will be the source of truth.

new authorino image -> requires new operator bundle -> new CSV.

[guicassolato]

This is so we can rely on kustomize tooling to set the variable value dynamically when we run make manifests (also triggered on make bundle.) There's a configMapGenerator that picks up the dynamic value.

Alternative approaches we employ for other customisations are:

1. using envsubst, or
2. directly setting the value in the config/manager/manager.yaml with yq.

Happy to change if you prefer any of these other options.

[eguzki]

The config map is yet another resource of the bundle. When there is a new image, even though the CSV does not change (not entirely true, check my next comment), a new bundle is needed anyway because the config map changes.

Your call. Just keep in mind that the image of authorino URL is also referenced in the CSV in relatedImages section, so the CSV needs to change anyway.

[guicassolato]

I don't like adding the ConfigMap either. I'll refactor this.

[guicassolato]

Update: For the time being, we found a "hack" in the productisation system that makes it work with the current approach, based on branding the default image inside the binary at compilation time. This makes this PR less urgent than before.

Let's keep it open for a while 'cause I anticipate some further discussion on how we want to approach this – if we want to standardise for all operators, if overriding the image in the operand's CR would still make sense, etc.

Thanks, @eguzki, for the feedback!