-
Notifications
You must be signed in to change notification settings - Fork 18
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
Import Resources by Identifiers (Name, etc.) #206
base: main
Are you sure you want to change the base?
Import Resources by Identifiers (Name, etc.) #206
Conversation
i like the idea. Importing by Changing this might break this function: https://gitlab.com/corewire/images/crossplane/function-keycloak-builtin-objects but we have to test it |
2f8e68d
to
b300910
Compare
@Breee I stumbled upon an case where the change lead to probably unwanted/unexpected behavior: When one of the identifying properties are changed i.e. Examples:
What if we still maintain the UUID´s in external-name, but if UUID is empty or resource with that UUID is not existing, then utilize the identifying properties to retrieve the UUID? We could apply the following logic:
|
yes
Yes, good catch. If people mess with the objects in keycloak that's probably in more cases a problem (it's a shame that they can not be made immutable on the UI) I like the logic you proposed. We then also have to test, how this affects existing deployments |
Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
Fixed that renewing tf-state creates new resource instead of updating existing when renaming one of the identifying properties Signed-off-by: Dennis Kniep <[email protected]>
@Breee seems to be that
Any ideas how to solve that? Else I will switch back to my previous approach:
...then we won´t get any compiler errors once those used attribute names are changed.
|
Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
27d3742
to
a1ec3ff
Compare
Signed-off-by: Dennis Kniep <[email protected]>
+ Fixed bug keycloak_saml_client_default_scopes.client_id point to keycloak_saml_client + Added Ref fields for clientpolicy, grouppolicy, rolepolicy, userpolicy Signed-off-by: Dennis Kniep <[email protected]>
@Breee Can you already have a look at it? How should we proceed? Does it make sense to release a new major version along with that feature to highlight the change? What do you think? |
Yeah i think a RC would be a good idea. |
Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
@Breee okay cool, I already added handling for the missing ldap resources and added support for end to end tests. |
Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
cluster/test/setup.sh
Outdated
echo "Waiting for all pods to come online..." | ||
${KUBECTL} -n upbound-system wait --for=condition=Available deployment --all --timeout=5m | ||
#echo "Waiting for all pods to come online..." | ||
#${KUBECTL} -n crossplane-system wait --for=condition=Available deployment --all --timeout=5m |
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.
these can be removed now
// ExecutionIdentifierFromIdentifyingProperties is used to find the existing resource by it´s identifying properties | ||
var ExecutionIdentifierFromIdentifyingProperties = lookup.BuildIdentifyingPropertiesLookup(executionIdentifyingPropertiesLookup) | ||
|
||
func getExecutionIDByExternalName(ctx context.Context, id string, parameters map[string]any, kcClient *keycloak.KeycloakClient) (string, error) { |
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.
Does using a struct for the parameters make sense here? This would enforce the correct types.
upgraded keycloak to 26.1.2 Signed-off-by: Dennis Kniep <[email protected]>
Signed-off-by: Dennis Kniep <[email protected]>
Description of your changes
There is a feature request around how to import objects via <realm/name> instead of < uuid >
see: #126
There are already some workarounds in place:
import
property (only a few resources supports that!)This is a proposal how to import resources by name.
We could use a custom implementation of
ExternalName
config (see here and here)GetIDFn
: Use defined props to lookup the Id used in the specific keycloak instance for that resource (i.e. by realmId, clientId & roleId). This could be done by using the terraform projects´skeycloakClient
(i.e.kcClient.GetRoleByName(ctx, realmId.(string), clientId.(string), name.(string))
)GetExternalNameFn
: Builds the ExternalName based on deterministic props (i.e.<realmId>/<clientId>/<roleId>
)What do you think about this approach?
Topics to clarify:
OmittedFields
&SetIdentifierArgumentFn
. Issue with that is, that we can´t specify ClientIDRef which is a big downside!Fixes #126