Repository navigation
Conversation
There was a problem hiding this comment.
This looks good. Two thoughts - it will need some kind of test, and a Boolean would be preferred to allow for null as the default.
|
Sure, I can add that in. |
5f41b9e to
4132146
Compare
There was a problem hiding this comment.
This looks great, just one small consideration.
There was a problem hiding this comment.
| public Boolean isReplacePlaceholders() { | |
| public Boolean getReplacePlaceholders() { |
Thanks for the update - sorry I was out the latter part of the week and may have missed the change. One last thing - can this method and the references to it use the get convention instead of is?
There was a problem hiding this comment.
@stustison Thank you for the PR!
I wonder what would be the full use case here. We add a supported field to the RealmImport CR but we don't officially support setting env vars on the Deployment/import Job (pod template is not considered supported). I presume the main problem this tries to solve is using Secrets for values inside the Realm representation. Would it rather make sense instead of introducing this switch to introduce a field to the RealmImport CR that would mount Secrets to env vars? We could even hide the fact it's env vars, the field could simply be called placeholders, it would take a name of the placeholder and a Secret/Key reference. Only if this field is specified, we'd internally set -Dkeycloak.migration.replace-placeholders=true.
WDYT @stustison @shawkins?
You can use either the podtemplate or the additionalOptions - if you are okay with the naming convention.
I'm not familiar enough to know, and we don't seem to have many examples - but generally are these values specific to just import, or when should they be associated with the keycloak cr? |
PodTemplate is an unsupported option. It feels weird to have a first-class citizen supported option additionalOptions is a supported option but not advocated to be used to set env vars. It's possible it won't even use env vars in the future but in any case it's an implementational detail hidden from user.
AFAIK, yes. The mappings are specific to import. Hence I was suggesting to add a dedicated mappings field to the RealmImport CR. Something like: kind: KeycloakRealmImport
spec:
keycloakCRName: example-kc
placeholders:
- name: foobar
secret:
name: mysecret
key: mykey
realm:
... |
Is this the only system property / env variables used by import? If not, it may make sense to instead keep a placeholder flag and a dedicated section for controlling the env. |
AFAIK, yes. CC @pedroigor |
|
@stustison after some more internal discussion, we'd like to move forward with the proposal above from @vmuzikar - unfortunately it adds a bit more complexity. The closest match for this logic we already have are the additionalOptions on the Keycloak CR with the ValueOrSecret class. Do you think you could adapt that for this purpose? |
|
@shawkins Yeah let me go back and take another look at what you guys are suggesting and I'll update the PR when I get to it. |
|
@stustison I am interested in the progress of this request toward the solution proposed by @vmuzikar. I may have some time in the coming weeks to assist with this. |
|
@briantward Hey! Sorry I haven't had a chance to dig into this just yet I got busy on some other things. I'll be looking into this today and tomorrow. |
|
Added a first cut of the changes, but haven't updated any of the tests yet. |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchangeCreateNewUserUsingMappersorg.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchangeorg.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testAccountLinkNoTokenStoreorg.keycloak.testsuite.client.ClientTypesTest#testUpdateClientWithClientTypeorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeWithDynamicScopesEnabledorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testClientExchangeorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testIntrospectTokenAfterImpersonationorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testPublicClientNotAllowedorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeUsingServiceAccountorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationorg.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationUsingPublicClient |
There was a problem hiding this comment.
@stustison this is looking good, thank you for taking this up again. Just a couple of comments.
34a45b2 to
ca0554b
Compare
f7f05f6 to
63f6580
Compare
There was a problem hiding this comment.
@stustison this looks great - can I have you undo the star import usage, then we should be good to merge this.
…within the realm import Closes keycloak#26470 Signed-off-by: stustison <scott.tustison@gmail.com>
|
@shawkins I removed the * imports, it should be good to go now. |
There was a problem hiding this comment.
LGTM, thank you @stustison for getting this past the finishline - it looks like others will appriciate this work.
|
I'm rerunning the failed tests - if they fail again we'll need to look into it more. There have been some intermittent flakiness with import, but the local here are failing as well which is unusual. |
…within the realm import Closes keycloak#26470 Signed-off-by: stustison <scott.tustison@gmail.com>
|
@shawkins it should be passing now. I put the test back to the way it was before. The object sent to the function is a GenericKubernetesResource, which I can't really modify in any way. It was easier to just set it back to how it was previously when it was working. |
|
Closing in favor of #31232 |
Closes #26470