Fix identity provider update and service account role management #171
There are no files selected for viewing
This file contains 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 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 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -40,18 +40,32 @@ func resourceKeycloakOpenidClientServiceAccountRole() *schema.Resource { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| func getOpenidClientServiceAccountRoleFromData(data *schema.ResourceData, keycloakClient *keycloak.KeycloakClient) (*keycloak.OpenidClientServiceAccountRole, error) { | ||||||
| containerId := data.Get("client_id").(string) | ||||||
| roleName := data.Get("role").(string) | ||||||
| realmId := data.Get("realm_id").(string) | ||||||
| serviceAccountRoleId := data.Get("service_account_user_id").(string) | ||||||
|
|
||||||
| 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.
Suggested change
There was a problem hiding this comment. @mrparkers just updated |
||||||
| role = &keycloak.Role{Id: ""} | ||||||
| } else { | ||||||
| return nil, err | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return &keycloak.OpenidClientServiceAccountRole{ | ||||||
| Id: role.Id, | ||||||
| ContainerId: containerId, | ||||||
| Name: roleName, | ||||||
| RealmId: realmId, | ||||||
| ServiceAccountUserId: serviceAccountRoleId, | ||||||
| }, nil | ||||||
| } | ||||||
|
|
||||||
| func setOpenidClientServiceAccountRoleData(data *schema.ResourceData, serviceAccountRole *keycloak.OpenidClientServiceAccountRole) { | ||||||
| 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 |
||||||
| data.Set("realm_id", serviceAccountRole.RealmId) | ||||||
| data.Set("client_id", serviceAccountRole.ContainerId) | ||||||
| data.Set("service_account_user_id", serviceAccountRole.ServiceAccountUserId) | ||||||
|
|
@@ -60,8 +74,12 @@ func setOpenidClientServiceAccountRoleData(data *schema.ResourceData, serviceAcc | |||||
|
|
||||||
| func resourceKeycloakOpenidClientServiceAccountRoleCreate(data *schema.ResourceData, meta interface{}) error { | ||||||
| keycloakClient := meta.(*keycloak.KeycloakClient) | ||||||
| serviceAccountRole, err := getOpenidClientServiceAccountRoleFromData(data, keycloakClient) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| err = keycloakClient.NewOpenidClientServiceAccountRole(serviceAccountRole) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
@@ -72,12 +90,12 @@ func resourceKeycloakOpenidClientServiceAccountRoleCreate(data *schema.ResourceD | |||||
| func resourceKeycloakOpenidClientServiceAccountRoleRead(data *schema.ResourceData, meta interface{}) error { | ||||||
| keycloakClient := meta.(*keycloak.KeycloakClient) | ||||||
|
|
||||||
| serviceAccountRole, err := getOpenidClientServiceAccountRoleFromData(data, keycloakClient) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| serviceAccountRole, err = keycloakClient.GetOpenidClientServiceAccountRole(serviceAccountRole.RealmId, serviceAccountRole.ServiceAccountUserId, serviceAccountRole.ContainerId, serviceAccountRole.Id) | ||||||
| if err != nil { | ||||||
| return handleNotFoundError(err, data) | ||||||
| } | ||||||
|
|
@@ -90,23 +108,27 @@ func resourceKeycloakOpenidClientServiceAccountRoleRead(data *schema.ResourceDat | |||||
| func resourceKeycloakOpenidClientServiceAccountRoleDelete(data *schema.ResourceData, meta interface{}) error { | ||||||
| keycloakClient := meta.(*keycloak.KeycloakClient) | ||||||
|
|
||||||
| serviceAccountRole, err := getOpenidClientServiceAccountRoleFromData(data, keycloakClient) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| err = keycloakClient.DeleteOpenidClientServiceAccountRole(serviceAccountRole.RealmId, serviceAccountRole.ServiceAccountUserId, serviceAccountRole.ContainerId, serviceAccountRole.Id) | ||||||
| if err != nil { | ||||||
| return handleNotFoundError(err, data) | ||||||
| } | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func resourceKeycloakOpenidClientServiceAccountRoleImport(d *schema.ResourceData, _ interface{}) ([]*schema.ResourceData, error) { | ||||||
| parts := strings.Split(d.Id(), "/") | ||||||
| if len(parts) != 3 { | ||||||
| return nil, fmt.Errorf("Invalid import. Supported import formats: {{realmId}}/{{serviceAccountUserId}}/{{clientId}}/{{roleId}}") | ||||||
| } | ||||||
| d.Set("realm_id", parts[0]) | ||||||
| d.Set("service_account_user_id", parts[1]) | ||||||
| d.Set("client_id", parts[2]) | ||||||
| d.SetId(fmt.Sprintf("%s/%s", parts[1], parts[3])) | ||||||
|
|
||||||
| return []*schema.ResourceData{d}, nil | ||||||
| } | ||||||
This file contains 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, cause if role doesn't exist yet it cannot be attached/removed to/from a client.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that the
keycloak_openid_client_service_account_roleresource will be removed from state if theGetRoleByNamecall 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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