-
Notifications
You must be signed in to change notification settings - Fork 35
chore(helm-chart): add secure keycloak admin console option #4219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
wesjdj
wants to merge
2
commits into
master
Choose a base branch
from
add-option-to-secure-keycloak-admin-console
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| --- | ||
| {{- if .Values.keycloakx.enabled }} | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: keycloak-admin-console | ||
| labels: | ||
| app: {{ template "renku.name" . }} | ||
| chart: {{ template "renku.chart" . }} | ||
| release: {{ .Release.Name }} | ||
| heritage: {{ .Release.Service }} | ||
| annotations: | ||
| helm.sh/hook: pre-install,pre-upgrade,pre-rollback | ||
| helm.sh/hook-delete-policy: before-hook-creation | ||
| data: | ||
| {{- if .Values.global.keycloak.secureAdminConsole }} | ||
| KC_HOSTNAME_ADMIN_URL: http://localhost:8080 | ||
| {{- end }} | ||
| {{- end }} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use
extraEnvdirectly? If the value changes andhelm upgradeis executed, then theextraEnvFromwill not update which means the new value will not cause a pod template re-render.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed whether we wanted to make the change as an
extraEnvin our deployment values, but it would mean having to copy all theextraEnvscurrently in the Renku chart values.yaml into our deployment values. The other option was to modify the Renku chart. The consensus was to make the change in the chart, but the value change not triggering a restart of the pod is not ideal.@aledegano a compromise could be keeping the new
configMapRefin the values.yaml, but keeping it commented out, and using that as thesecureAdminConsoletoggle instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand: if you add the definition to
extraEnvjust above, you can use helm chart rendering with the value, so I don't see why it's not just added there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that
extraEnvin the Renku chart's values.yaml is defined as a multi-line string, not as a YAML list. This means we can't simply append to it from our deployment values.When you override a string value in Helm, it completely replaces the original. So if we add our new environment variable to
extraEnvin our deployment values, we would lose all the existing environment variables defined in the chart's values.yaml.To use
extraEnvfrom our deployment values, we'd have to copy all existing environment variables from the chart plus add our new one. That's fragile because if the chart updates those values in the future, our deployment values won't pick up those changes.That's why modifying the chart directly (using
extraEnvFromwith aConfigMap) seemed like cleaner options, though I agree the pod restart issue withextraEnvFromis not ideal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure things are clear: what I am suggesting is simply to set:
in the values here. This requires no further change for deployment.