Switches the merging logic to using the override as the basis - #21415
Conversation
vmuzikar
left a comment
There was a problem hiding this comment.
@shawkins Sorry, didn't have time today for in-depth review today. But in general I'm happy with this approach, it's much cleaner and even existing tests pass!
The only thing that is somewhat different with what came before is that the template metadata labels from the operator are forcibly set over whatever is on the unsupported template.
I think that's ok. It's in any case in the "unsupported" (a.k.a. best effort only) territory.
|
Made some minor refinements. The only thing of note is to always set the image pull secrets from the cr - which seems consistent with the existing warning. |
There was a problem hiding this comment.
@shawkins Thanks for the update. LGTM in general but I noticed we changed the order of how env vars are added to the final list. Before, we first added ours env vars (generated by the Operator) and then the user defined env vars from the podTemplate. Now (if I'm not mistaken) it's the other way around. AFAIK, if there are multiple occurrences of an env var with the same key, the last takes precedence. Therefore the user defined env vars need to come last to allow users to overwrite our env vars if needed. Could you also please add a test of it if possible?
The order in this pr is template first, then KeycloakDistConfigurator (which won't overwrite values from the template, but does produce a warning), then KeycloakDeployment.getEnvVars - which is simply appending. To confirm, via the PodTemplate, you want users to be able to override KeycloakDeployment.getEnvVars:
Edit: based upon the warning I would expect that the template env variables would be of lowest priority. The user should use the first class option first, then additionalOptions, finally the template.
That's true for a post, but it's unfortunately a little more complicated than that with SSA. If there are multiple env in the patch with the same name, it will fail to validate. So yes the handling will need to be corrected to account for that. That would also be a bug in 22 / main - I can separate off a fix for that if need be. Let's confirm the expected order, then I can get things finalized. |
The warning doesn't mean it was correct in the first place. :) However, it's more about preventing user from using the generic additional options field over first class citizen. Not directly about env vars. The pod template field is IMHO something a bit different, though. It serves as sort of back door for workaround. Let's say the Operator is setting some env vars that are based on either first class fields, additional options, or even some derived defaults that user didn't specify directly. What if the user needs to override some of the derived values for some reason? I know it's unsupported, undefined behaviour, they should eventually file an RFE but they should have the option to override what they need. So from my perspective the order should be: pod template > first class fields > additional options.
Good point, we can fix that in 22.0.1. |
1 flaky tests on run #8029 ↗︎Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Authentication test > should add a condition |
Output
Screenshots
|
|
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
The pr has been updated to enforce this and ensure there won't ever be duplicate envvars.
Would that be by pulling in this commit, or do you want a more targetted fix? |
Ensures additionalProperties are the lowest precedence Also removes support for setting the image pull secrets via the unsupported podtemplate Closes keycloak#10503
We can keep the fix in this PR. 22.0.0 has been branched already, so we'll need to backport this PR later (incl. the changes to podtemplate logic). |
vmuzikar
left a comment
There was a problem hiding this comment.
@shawkins LGTM, thanks.
The logic for merging env vars from various different sources is a bit complex and hard to read (especially for someone who might not be too familiar with the code base). But it was similar case even before this PR and we can refactor later.
I believe this also solve a bug around how we treat env vars, so from my opinion this should be backported to 20.0.1. WDYT?
Yes unfortunately list merging is currently a little painful and a lot of logic is being triggered by the KeycloakDeployment constructor. Moving forward we'll be moving the logic out of the constructor and will probably want to redo the dist config logic to return a map of env vars rather than working on the container directly.
Yes it should target 22.0.1 as well. |
Can you please prepare the backport (similarly to e.g. #21607)? |
An alternative to the merging logic that uses the unsupported template as the basis for the template builder. This cuts down a bit on the code and allows for anything new added to the pod template to be picked up automatically. It does not address the concerns about how to merge the other lists, like ports, evn vars, volume mounts, etc. Obviously it makes the base case of simply constructing the statefulset without the unsupported template a little harder to follow, but some minor reorganization helped with readability a bit. It should be somewhat clear what prpoerties defer to the unsupported template.
The insertion ordering for the volume mounts was just done to line the logic up with an existing test.
The only thing that is somewhat different with what came before is that the template metadata labels from the operator are forcibly set over whatever is on the unsupported template.
@vmuzikar please let me know if this is along the lines of what you are looking for and I'll add whatever additional tests seem relevant.
Of course the naming convention baseDeployment could change as well.
Closes #10503