KEYCLOAK-13634 Externalize images

[slaskawi]

JIRA ID

KEYCLOAK-13634

Additional Information

This Pull Request allows to override images using Environment Variables, which is a per-requisite for Offline Support and for productization. From now on, the image version (with SHA Digests) can be specified in the Manifest. The manifest will become a central place for managing image versions.

Also note, that this Pull Request removes patch upgrade support (previously implemented by @davidffrench). From now on, the proper way to do this kind of overrides is to specify new image SHA digest via environment variables in the manifest.

Verification Steps

Checklist:

• Verified by team member
• Comments where necessary
• Automated Tests
• Documentation changes if necessary

Additional Notes

[slaskawi]

@stianst @davidffrench @pb82 @mhajas This is an initial version of the externalizing images. I'll be explaining the long-term plan how it works tomorrow. As for the time being, please have a look if this doesn't break any of your use cases.

[coveralls]

Coverage decreased (-0.3%) to 41.019% when pulling fbbc172 on slaskawi:KEYCLOAK-13634-Externalize-images into 97045dc on keycloak:master.

[davidffrench]

@slaskawi I have scanned over the PR and the changes look good to me.

I have a broader question, how do you support a range of keycloak versions for a specific operator version with this approach? Since this allows someone to specify any image.

[slaskawi]

@davidffrench This Pull Request doesn't change our approach here. The ImageManager has a set of default images that it will use. The set-version.sh script updates it in every release.

However, with productization requirements (separate Container Image with the Operator Manifest) as well as Offline Installation mode support, I think it is no longer possible to pin the exact operand image version to the Operator. The Offline Installation mode requires you to add a relatedImages tag to the CSV (see here). The tag contains a list of images used by the Operator. At the same time, you also need to be able to change them in case someone is sealed off the network and needs to use its own registry (see here). An additional usecase for consuming modified operand images coordinates is testing unpublished builds.

The good news is that both, prod CSV (the Manifest Container Image) as well as Operatorhub CSV is something that we own. So I think nothing changes from our perspective. We'll put the proper images directly into the Operator (into the ImageManager) and we won't be changing them in the Operatorhub CSV. This capabilities will be used mainly for the productized Operator.

Does this answer your question? If not - please reach me out using our internal communication channels.

[slaskawi]

Together with @davidffrench we ensured that making Operator Manifest a central place for managing image SHA Digests is the right thing to do. We also ensured, that Operator deployments are predictable (as the OLM overrides any attempts of manual modifications of Operator Deployment object).

I'm converting this into a reviewable Pull Request.

[kfox1111]

+1. This would enable much easier offline support for those of us that need it as well.

[hmlnarik]

Is this default value necessary? Either there is a cost associated with maintaining this default, or the operator would pick this aging image.
Could it be preferably removed for RHSSO and set the image mandatorily from outside in the manifest / on startup rather than hardcoding it?
cc: @stianst

[slaskawi]

In the longer term - yes. But I'm afraid we can't do it just yet.

The RHMI Team uses this Operator with RHSSO profile. If we removed this now, we will put them in a very uncomfortable situation and force them to switch to a new RHSSO Operator immediately. If there's any bug in the initial release of the RHSSO Operator, we would all be in trouble.

So, in the long term we should remove it. In short and mid term - we should keep it.

[mhajas]

@slaskawi Thank you for the PR, great work! Just I am not 100% comfortable with having the env variable to set Keycloak/RHSSO image. I am afraid of incompatibility between Keycloak/RHSSO image and operator. If I understand correctly OLM will not permit users to change the image for a different one than we set in CSV manifest, but what if they don't use OLM but deploy operator image themselves? Do we support this? If yes do you think it could be better if we set only image tag (and sha) within environment variable and use default repository and image name from operator code?

[mhajas]

Do we plan to extend this struct in the future? I am just wondering if we need the struct there, I think we could drop struct and have something like:

const Images := map[string]string{
			KeycloakImage:         getImage(KeycloakImage, DefaultKeycloakImage),
			RHSSOImage:            getRHSSOImage(),
			RHSSOImageOpenJ9:      getImage(RHSSOImageOpenJ9, DefaultRHSSOImageOpenJ9),
			RHSSOImageOpenJDK:     getImage(RHSSOImageOpenJDK, DefaultRHSSOImageOpenJDK),
			KeycloakInitContainer: getImage(KeycloakInitContainer, DefaultKeycloakInitContainer),
			RHMIBackupContainer:   getImage(RHMIBackupContainer, DefaultRHMIBackupContainer),
			PostgresqlImage:       getImage(PostgresqlImage, DefaultPostgresqlImage),
		}

On the other hand, this would probably make it impossible to test. @slaskawi WDYT?

[slaskawi]

Having a struct has the testability advantage you mentioned. We can initialize it multiple times and prepare some environmental variables prior to creating a new instance.

My personal preference would be to leave it as is. One other thing I should implement though is to attach getImage and getRHSSOImage to the struct. Let me implement this shortly.

[mhajas]

What is the reason for having the possibility to set more profiles?

[slaskawi]

At the moment, we don't need additional profiles. However, I'm not exactly sure what the productization process will bring us, so I would prefer to have a ready-to-go implementation at hand to add more if we need them. However, if you don't like it - I can remove it. I just have a gut feeling they might be useful. Nothing more.

[slaskawi]

Just I am not 100% comfortable with having the env variable to set Keycloak/RHSSO image. I am afraid of incompatibility between Keycloak/RHSSO image and operator.

@mhajas This shouldn't be an issue as RHSSO and Keycloak images have separate environmental variables.

I do understand that at first glance it looks like users will be freely allowed to override the image and deploy some custom images. That won't be the case (please read paragraphs below, they should shed some light on it). The short answer is that OLM won't let you override anything, so we're in charge of the image that gets deployed.

If I understand correctly OLM will not permit users to change the image for a different one than we set in CSV manifest, but what if they don't use OLM but deploy operator image themselves?

@mhajas There are probably 3 common ways of deploying the Operator:

• By deploying deploy/operator.yaml. I call it "the manual mode".
• By using the "Community Operators", from the OLM Operator Catalog (e.g. with or without the OLM web console from OpenShift).
• By deploying the Operator manually to the OLM (which means pushing it to Quay and creating a new CatalogSource). This way however, your Operator will appear in "Custom Operators" category instead of "Community" or "Red Hat" Operators.

With this proposal, nothing changes in our deployment model. We will be releasing the Keycloak Operator through the Operatorhub and RHSSO Operator through the Red Hat Supported Operators channels. In both cases we control the Deployment objects for the Operator. If you manually try to override it, the OLM will reconciliate it and wipe your changes out. This simply means, that users won't be able to override it.

Do we support this? If yes do you think it could be better if we set only image tag (and sha) within environment variable and use default repository and image name from operator code?

@mhajas @stianst I think the maintenance will be much easier if we proceed with community deployments as they are now. That means, we will be releasing a new Keycloak Operator at every Keycloak release and pin Keycloak Container tag to the Operator (this Pull Request already modifies all required bits in set-version.sh script). This way, @stianst can hire his automation scripts to release the Operator automatically in the future (as it will be released exactly the same way as our other images).

As for the RHSSO Operator - the manifest becomes a central place for managing image versions (as we discussed using other communication channels).

In the future we might be forced to manage Keycloak versions in the manifest as well. However, we'll need some sort of migration for all the Operators deployed in the Operatorhub. As far as I know, there are no such plans at the moment.

[kfox1111]

@slaskawi Thank you for the PR, great work! Just I am not 100% comfortable with having the env variable to set Keycloak/RHSSO image. I am afraid of incompatibility between Keycloak/RHSSO image and operator. If I understand correctly OLM will not permit users to change the image for a different one than we set in CSV manifest, but what if they don't use OLM but deploy operator image themselves? Do we support this? If yes do you think it could be better if we set only image tag (and sha) within environment variable and use default repository and image name from operator code?

Why are you so afraid of that? Just say "If you don't use the images we support, your on your own". Since its open source, they could just patch in their own shasums/image names / whatever else and build their own binary. It is impossible to ensure a user will never do something you don't want them to. You can only put in some reasonable protections in place to prevent them from accidentally doing something dumb and let it be on them when they void the warranty. I think the protections in the PR already prevent a user from doing something dumb by mistake. They would have to try hard to work around the protections. I think that's good enough.

[slaskawi]

Why are you so afraid of that? Just say "If you don't use the images we support, your on your own". Since its open source, they could just patch in their own shasums/image names / whatever else and build their own binary. It is impossible to ensure a user will never do something you don't want them to. You can only put in some reasonable protections in place to prevent them from accidentally doing something dumb and let it be on them when they void the warranty. I think the protections in the PR already prevent a user from doing something dumb by mistake. They would have to try hard to work around the protections. I think that's good enough.

@kfox1111 Thanks for the comment! This goes down to two things - the long term architectural vision of the Keycloak Operator (I hope it will be ready for community review soon, but a work-in-progress version of it might be found here) and driving the Operator development by use-cases.

One of the most important things we decided when working on the Keycloak Operator vision is that it needs to be an opinionated way of installing and managing Keycloak. This limits the number of things we need to support. Giving you a very specific example - supporting multiple ways of installing the Operator might be something what the community would like to have, but testing all of them on every release, documenting and keeping the documentation up to date will add a lot of maintenance burden.

At the same time, we would like to develop new features in a responsible way. This is why we are trying to focus on use cases from our community. Again, giving you a very specific example - we were asked to allow custom images because of template support. However, custom themes can be deployed as extensions using extensions functionality that we already have.

To sum it up - we believe focusing on use cases rather than small toggles/switches and patches will work better in the longer term. We hope, this way the project will evolve faster and the code won't become a maintenance headache in the mid-to-long term. We try to avoid code that opens up the "Pandora's box".

I hope this shed some light on our approach to the project maintenance aspect. We really appreciate your comment and we find it very valuable. We know our approach is not perfect but hopefully it will turn out to be the right direction in the long term.

[slaskawi]

@mhajas Updated and rebased.

[mhajas]

Thanks for the fixes @slaskawi! However, it seems like during the rebase you forgot to update RHSSO image to 7.4.

[slaskawi]

@mhajas You're right... It seems I didn't resolve the conflict correctly. Fixed now.

[mhajas]

@slaskawi Thank you very much for all fixes and explanations! Now it looks good to me.

[vinay2107]

@slaskawi Does this PR enable providing image names (or only versions from public reg ?) by the manifest (keycloak CR) ? By title, it seems straightforward but its not that easy to assume from the subsequent conversations.

[slaskawi]

@vinay2107 Yes, you can provide any arbitrary image there. Although, when deploying through OLM, you won't be able to override it. The only way is to do a manual deployment.

[slaskawi]

Travis build is fine for this one: https://travis-ci.org/github/keycloak/keycloak-operator/builds/679983391

I'm not sure why it wasn't updated. @stianst @abstractj May I ask you to check why Travis didn't publish the status?

[hmlnarik]

Approving based on @mhajas' review.

[slaskawi]

Putting Hold label as you may need to change the variable names (to RELATED_IMAGE_*).

[slaskawi]

As per https://github.com/containerbuildsystem/operator-manifest#pull-specifications, renamed variables to RELATED_IMAGE_*.

@mhajas Ready for re-review.

[mhajas]

Approving.

[hmlnarik]

Approving based on @mhajas 's approval