Fix identity provider update and service account role management #171
Conversation
| @@ -123,6 +123,8 @@ func getOidcIdentityProviderFromData(data *schema.ResourceData) (*keycloak.Ident | |||
|
|
|||
| if data.HasChange("client_secret") { | |||
| rec.Config.ClientSecret = data.Get("client_secret").(string) | |||
| } else { | |||
| rec.Config.ClientSecret = "**********" | |||
There was a problem hiding this comment.
Is this going to send the string ********** to Keycloak on update? I'm wondering if it would be better to remove the if statement on line 124 and always call rec.Config.ClientSecret = data.Get("client_secret").(string) so Keycloak is always getting the correct client secret on every call.
There was a problem hiding this comment.
This is how actually browser is working
There was a problem hiding this comment.
Anyway I've updated it
| } | ||
|
|
||
| func setOpenidClientServiceAccountRoleData(data *schema.ResourceData, serviceAccountRole *keycloak.OpenidClientServiceAccountRole) { | ||
| data.SetId(serviceAccountRole.Id) | ||
| data.SetId(fmt.Sprintf("%s/%s", serviceAccountRole.ServiceAccountUserId, serviceAccountRole.Id)) |
There was a problem hiding this comment.
This might be a breaking change since the ID for this resource is being changed. Can you verify that these resources aren't being recreated because of this change?
There was a problem hiding this comment.
Just new resources will be created. In context of service account roles it'll try to apply existing changes once to add service account roles with ned IDs but POST action to service account roles endpoint always returns 204 even if role was already added
| realmId := data.Get("realm_id").(string) | ||
| serviceAccountRoleId := data.Get("service_account_user_id").(string) | ||
|
|
||
| if realmId != "" && containerId != "" { |
There was a problem hiding this comment.
realm_id and client_id are both required attributes, so I think this statement is always true.
There was a problem hiding this comment.
They can be empty if you're using realm_id = keycloak_realm.realm.id and client_id = keycloak_openid_client.client.id that do not exist yet
| return err | ||
| } | ||
|
|
||
| if serviceAccountRole == nil { |
There was a problem hiding this comment.
Can this error ever happen? I don't think it's possible for getOpenidClientServiceAccountRoleFromData to return nil without also returning an error.
| return err | ||
| } | ||
|
|
||
| if serviceAccountRole == nil { |
|
|
||
| return keycloakClient.DeleteOpenidClientServiceAccountRole(realmId, serviceAccountUserId, clientId, id) | ||
| if serviceAccountRole != nil { |
There was a problem hiding this comment.
Same comment as above, I don't think this can ever be false
There was a problem hiding this comment.
Had another problem. And tested locally this change, forgot to remove
|
@mrparkers Could you please take a look once again? |
|
|
||
| role, err := keycloakClient.GetRoleByName(realmId, containerId, roleName) | ||
| if err != nil { | ||
| if handleNotFoundError(err, data) == nil { |
There was a problem hiding this comment.
This is going to call data.SetId("") if the error is s 404, which removes this resource from state. Is this what you want?
There was a problem hiding this comment.
yes, cause if role doesn't exist yet it cannot be attached/removed to/from a client.
There was a problem hiding this comment.
There's a get request before delete action
https://github.com/mrparkers/terraform-provider-keycloak/blob/master/keycloak/openid_client_service_account_role.go#L33
There was a problem hiding this comment.
What I mean is that the keycloak_openid_client_service_account_role resource will be removed from state if the GetRoleByName call fails with a 404. That seems problematic since ultimately this function won't return an error and the creation will appear to be successful.
If the keycloak_openid_client_service_account_roleresource isn't valid without an existing role, shouldn't create just fail?
There was a problem hiding this comment.
When terraform plans resources creation it uses empty values for attributes of resource which is not yet created. Don't know how it should work, but at least have noticed such a behaviour for this provider. It'll cause errors during every plan when resources are not created yet. Without this check tests are always failing
https://circleci.com/gh/mrparkers/terraform-provider-keycloak/1152?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
|
|
||
| role, err := keycloakClient.GetRoleByName(realmId, containerId, roleName) | ||
| if err != nil { | ||
| if handleNotFoundError(err, data) == nil { |
There was a problem hiding this comment.
Okay, your logic makes sense, thanks for the explanation. I think, however, that all you really want to do is check if the error is a 404 and not necessarily remove the resource from state.
| if handleNotFoundError(err, data) == nil { | |
| if keycloak.ErrorIs404(err) { |
There was a problem hiding this comment.
@mrparkers just updated
Hi @mrparkers
Prepared a PR with fixes for