bitnami/Keycloak] Allow manual override fo KC_HOSTNAME_URL and KC_ADMIN_HOSTNAME_URL

[djas19]

Description of the change

Allow users to provide their own value of the KC_HOSTNAME_URL and KC_ADMIN_HOSTNAME_URL parameters.

Benefits

Currently a lot of assumptions are made when constructing these environment variables. This behaviour can be wanted in simple setups but it is impossible to foresee all possible scenarios. To accommodate this we can allow users to override this themselves but fallback on autogeneration if not provided.

Possible drawbacks

The change is backwards compatible so no drawbacks are expected.

Applicable issues

• fixes [bitnami/keycloak] Cannot load AdminUI with gce based ingress #28357

Additional information

Inspired out of discussions in issues: #28357 #28121 and #28161

Closes: #28520

Checklist

• Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
• Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
• Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
• All commits signed off and in agreement of Developer Certificate of Origin (DCO)

[djas19]

I don't see how to fix the CI issue. Could somebody give me some pointers on how to setup this fork to successfully run the CI?

[EC-Sol]

I don't see how to fix the CI issue. Could somebody give me some pointers on how to setup this fork to successfully run the CI?

According to the PR checklist:

• Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm

The README.md file is automatically generated and updated if the variables are properly documented in the values.yaml. If you have directly modified the README.md file, you should remove those changes.

[carrodher]

Thanks for your contribution! The issue is that the automation trying to update the README is not able to do it due to the lack of permissions in your branch/fork. Could you please add @bitnami-bot as developer/maintainer in your fork so it can update the README?

[djas19]

Hi @carrodher, the bitnami-bot is added as a maintainer on the repo (invite pending) but the pipeline still fails.

[carrodher]

Thanks! The changelog was properly updated!
The problem now seems to be conflicts caused by recent PRs such as #28519.

Could you please rebase from main and solve the conflicts?

[singhbaljit]

Chart v22 and Keycloak v25 was released. I don't these variables are still supported anymore:

Hostname v2 options are supported by default, as the old hostname options are deprecated and will be removed in the following releases. You should migrate to them as soon as possible. New options are activated by default, so Keycloak will not recognize the old ones.

[singhbaljit]

Can't we move these env vars to the config map instead of being defined directly on the statefulset? If you want to override them, you still can override by defining them via extraEnvVars. This will lead to simpler chart parameters because the "hostname" and "hostname url" are now interchangeable in v2 hostname, see #28611.

[djas19]

@singhbaljit As this is blocking our updates/releases I wanted to get a fix out quickly hence why the implementation is presented this way. also providing these via extraEnvVars doesn't work in the current setup and caused errors for duplicate keys. Maybe that would be possible if it would be on the in the config map but I didn't look into that. Looking at #28611 it doesn't look like this would solve the issue for us and a similar approach to this or moving those variables to the config map too would be needed as it still doesn't take into account wildcards in the ingress path and/or httpRelativePath

[singhbaljit]

@djas19 the extraEnvVars won't work currently because those variables get rendered on the statefulset, and there are already these variables defined, hence the duplicate key error. However, if you revise this PR to instead just move the definition of these two variables into the config map instead of adding the conditional logic to override, it will allow you to still meet your need (which to override these variables), without the conditional logic or extra parameters to the chart. I understand the need for a quick solution; to me, the moving of the variables is less code/quicker. Introducing new parameters has long term impact, because they can't be easily removed without breaking change. You can see how the override works already with the database variables. There are values being defined in the config map, and overrides defined in the statefulset. This is an established pattern in the chart/Kubernetes in general. See the docs.

Values defined by an Env with a duplicate key will take precedence [over EnvFrom]

[singhbaljit]

see alternative, #28838

[jotamartos]

Sorry for the late reply. As #28838 is already merged, I understand this PR is not necessary anymore. Could you please confirm so we close it?

Thanks

[djas19]

Confirmed

[djas19]

closing as #28838 is merged