Skip to content
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.

Sign up for GitHub

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

Fix issue with not being able to update connections on organizations #244

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Fix issue with not being able to update connections on organizations #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 72 additions & 74 deletions auth0/resource_auth0_organization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@ package auth0
import (
"context"
"fmt"
"log"
"net/http"

"github.com/auth0/go-auth0"
"github.com/auth0/go-auth0/management"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"

"github.com/auth0/terraform-provider-auth0/auth0/internal/hash"
)

func newOrganization() *schema.Resource {
Expand Down Expand Up @@ -39,7 +37,6 @@ func newOrganization() *schema.Resource {
Optional: true,
Computed: true,
MaxItems: 1,
MinItems: 1,
Description: "Defines how to style the login pages",
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
Expand All @@ -64,7 +61,6 @@ func newOrganization() *schema.Resource {
"connections": {
Type: schema.TypeSet,
Optional: true,
Computed: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a computed property, it's actually us within the config that decide the values of these and then it gets created on the API.

Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"connection_id": {
Expand All @@ -74,83 +70,32 @@ func newOrganization() *schema.Resource {
"assign_membership_on_login": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default is very important to correctly figure out the differences on the hashed sets.

},
},
},
Set: hash.StringKey("connection_id"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use just the connection_id for the set hashes as that will ignore anything that we update on the assign_membership_on_login field. This is mostly the core of the fix, but in order to make it work properly several other things were needed. Please follow comments.

},
},
}
}

func createOrganization(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
organization := buildOrganization(d)
organization := expandOrganization(d)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this so we're in line with the naming conventions "expand" and "flatten".


api := m.(*management.Management)
if err := api.Organization.Create(organization); err != nil {
return diag.FromErr(err)
}

d.SetId(organization.GetID())

d.Partial(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of what caused the bug with the diffs I believe. The Partial func is a legacy func used when we had the SetPartial func as well (deprecated). This is not needed at all as on every command we run a refresh (read) even if we stop mid way here on the follow up plan or apply we'll read again and correct the state.

if err := assignOrganizationConnections(d, m); err != nil {
return diag.FromErr(fmt.Errorf("failed assigning organization connections. %w", err))
if err := updateOrganizationConnections(d, api); err != nil {
return diag.FromErr(fmt.Errorf("failed to update organization connections: %w", err))
}
d.Partial(false)

return readOrganization(ctx, d, m)
}

func assignOrganizationConnections(d *schema.ResourceData, m interface{}) (err error) {
api := m.(*management.Management)

add, rm := Diff(d, "connections")

add.Elem(func(data ResourceData) {
organizationConnection := &management.OrganizationConnection{
ConnectionID: String(data, "connection_id"),
AssignMembershipOnLogin: Bool(data, "assign_membership_on_login"),
}

log.Printf("[DEBUG] (+) auth0_organization.%s.connections.%s", d.Id(), organizationConnection.GetConnectionID())

err = api.Organization.AddConnection(d.Id(), organizationConnection)
if err != nil {
return
}
})

rm.Elem(func(data ResourceData) {
// Take connectionID before it changed (i.e. removed).
// Therefore we use GetChange() instead of the typical Get().
connectionID, _ := data.GetChange("connection_id")

log.Printf("[DEBUG] (-) auth0_organization.%s.connections.%s", d.Id(), connectionID.(string))

err = api.Organization.DeleteConnection(d.Id(), connectionID.(string))
if err != nil {
return
}
})

// Update existing connections if any mutable properties have changed.
Set(d, "connections", HasChange()).Elem(func(data ResourceData) {
connectionID := data.Get("connection_id").(string)
organizationConnection := &management.OrganizationConnection{
AssignMembershipOnLogin: Bool(data, "assign_membership_on_login"),
}

log.Printf("[DEBUG] (~) auth0_organization.%s.connections.%s", d.Id(), connectionID)

err = api.Organization.UpdateConnection(d.Id(), connectionID, organizationConnection)
if err != nil {
return
}
})

return nil
}

func readOrganization(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
api := m.(*management.Management)
organization, err := api.Organization.Read(d.Id())
Expand Down Expand Up @@ -181,17 +126,18 @@ func readOrganization(ctx context.Context, d *schema.ResourceData, m interface{}
}

func updateOrganization(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
organization := buildOrganization(d)
organization := expandOrganization(d)

api := m.(*management.Management)
if err := api.Organization.Update(d.Id(), organization); err != nil {
return diag.FromErr(err)
}

d.Partial(true)
if err := assignOrganizationConnections(d, m); err != nil {
return diag.FromErr(fmt.Errorf("failed updating organization connections. %w", err))
if d.HasChange("connections") {
if err := updateOrganizationConnections(d, api); err != nil {
return diag.FromErr(fmt.Errorf("failed to update organization connections: %w", err))
}
}
d.Partial(false)

return readOrganization(ctx, d, m)
}
Expand All @@ -210,7 +156,58 @@ func deleteOrganization(ctx context.Context, d *schema.ResourceData, m interface
return nil
}

func buildOrganization(d *schema.ResourceData) *management.Organization {
func updateOrganizationConnections(d *schema.ResourceData, api *management.Management) error {
toAdd, toRemove := Diff(d, "connections")

connectionOperations := make(map[string]string)
toRemove.Elem(func(data ResourceData) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After we updated the schema we were getting at times weird errors about the connection already being there (409s).

This was because if we change the bool flag on a connection we get the following diff:

 - connections {
    - assign_membership_on_login = false -> null
    - connection_id              = "con_E68qtlsIB68K6Yz6" -> null
  }
  + connections {
    + assign_membership_on_login = true
    + connection_id              = "con_E68qtlsIB68K6Yz6"
  }

So to correctly update the connection without adding complicated diffing logic, we delete the connection and then re-add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: We opted instead to create an internal map in the updateConnections function in order to not delete and then add even if the diff shows that in terraform. Unfortunately we couldn't suppress the diff correctly.

oldConnectionID, _ := data.GetChange("connection_id")
connectionOperations[oldConnectionID.(string)] = "deleteConnection"
})

toAdd.Elem(func(data ResourceData) {
newConnectionID := data.Get("connection_id").(string)

if _, ok := connectionOperations[newConnectionID]; ok {
delete(connectionOperations, newConnectionID)
} else {
connectionOperations[newConnectionID] = "addConnection"
}
})

for key, value := range connectionOperations {
if value == "deleteConnection" {
if err := api.Organization.DeleteConnection(d.Id(), key); err != nil {
return err
}
}
if value == "addConnection" {
organizationConnection := &management.OrganizationConnection{
ConnectionID: auth0.String(key),
}
if err := api.Organization.AddConnection(d.Id(), organizationConnection); err != nil {
return err
}
}
}

var err error
Set(d, "connections").Elem(func(data ResourceData) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was also important to remove the HasChange() from here, so that now we always loop through the final connections and send a PATCH with whatever the value is, as this operation is idempotent. This ultimately makes us correctly update the values.

connectionID := data.Get("connection_id").(string)
organizationConnection := &management.OrganizationConnection{
AssignMembershipOnLogin: Bool(data, "assign_membership_on_login"),
}

err = api.Organization.UpdateConnection(d.Id(), connectionID, organizationConnection)
if err != nil {
return
}
})

return err
}

func expandOrganization(d *schema.ResourceData) *management.Organization {
organization := &management.Organization{
Name: String(d, "name"),
DisplayName: String(d, "display_name"),
Expand All @@ -231,24 +228,25 @@ func flattenOrganizationBranding(organizationBranding *management.OrganizationBr
if organizationBranding == nil {
return nil
}

return []interface{}{
map[string]interface{}{
"logo_url": organizationBranding.LogoURL,
"logo_url": organizationBranding.GetLogoURL(),
"colors": organizationBranding.Colors,
},
}
}

func flattenOrganizationConnections(organizationConnectionList *management.OrganizationConnectionList) []interface{} {
if organizationConnectionList == nil {
func flattenOrganizationConnections(connectionList *management.OrganizationConnectionList) []interface{} {
if connectionList == nil {
return nil
}

connections := make([]interface{}, len(organizationConnectionList.OrganizationConnections))
for index, connection := range organizationConnectionList.OrganizationConnections {
connections := make([]interface{}, len(connectionList.OrganizationConnections))
for index, connection := range connectionList.OrganizationConnections {
connections[index] = map[string]interface{}{
"connection_id": connection.ConnectionID,
"assign_membership_on_login": connection.AssignMembershipOnLogin,
"connection_id": connection.GetConnectionID(),
"assign_membership_on_login": connection.GetAssignMembershipOnLogin(),
}
}

Expand Down
Loading