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.

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] Correctly setreplica_server_id on azurerm_postgresql_flexible_server_virtual_endpoint forazurerm_postgresql_flexible_servers that exist in separate resource groups #27509

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
41dd191
look up replica ID via API
bruceharrison1984 Sep 25, 2024
062ba9c
remove custom poller
bruceharrison1984 Sep 25, 2024
46efac4
add test case
bruceharrison1984 Oct 1, 2024
94e28c5
improve tests
bruceharrison1984 Oct 1, 2024
45a1fa3
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
7cf743e
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
5e047bd
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
f9c1d75
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
251b50a
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
2cb98cc
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
387338c
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
7b5d15a
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
2006332
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
e17cc59
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
cfcac61
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
be50e69
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
a6106be
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
360432c
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
e095372
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
3484e8a
Update internal/services/postgres/postgresql_flexible_server_virtual_…
bruceharrison1984 Oct 2, 2024
a427e1e
parse server ID before setting state
bruceharrison1984 Oct 2, 2024
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

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@ import (

"github.com/hashicorp/go-azure-helpers/lang/pointer"
"github.com/hashicorp/go-azure-helpers/lang/response"
"github.com/hashicorp/go-azure-helpers/resourcemanager/commonids"
"github.com/hashicorp/go-azure-sdk/resource-manager/postgresql/2023-06-01-preview/servers"
"github.com/hashicorp/go-azure-sdk/resource-manager/postgresql/2023-06-01-preview/virtualendpoints"
"github.com/hashicorp/go-azure-sdk/sdk/client/pollers"
"github.com/hashicorp/terraform-provider-azurerm/internal/locks"
"github.com/hashicorp/terraform-provider-azurerm/internal/sdk"
"github.com/hashicorp/terraform-provider-azurerm/internal/services/postgres/custompollers"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation"
)
Expand Down Expand Up @@ -131,6 +130,8 @@ func (r PostgresqlFlexibleServerVirtualEndpointResource) Read() sdk.ResourceFunc
Timeout: 5 * time.Minute,
Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error {
client := metadata.Client.Postgres.VirtualEndpointClient
flexibleServerClient := metadata.Client.Postgres.FlexibleServersClient

state := PostgresqlFlexibleServerVirtualEndpointModel{}

id, err := virtualendpoints.ParseVirtualEndpointID(metadata.ResourceData.Id())
Expand Down Expand Up @@ -159,9 +160,25 @@ func (r PostgresqlFlexibleServerVirtualEndpointResource) Read() sdk.ResourceFunc
return metadata.MarkAsGone(id)
}

// Model.Properties.Members should be a tuple => [source_server, replication_server]
state.SourceServerId = servers.NewFlexibleServerID(id.SubscriptionId, id.ResourceGroupName, (*resp.Model.Properties.Members)[0]).ID()
state.ReplicaServerId = servers.NewFlexibleServerID(id.SubscriptionId, id.ResourceGroupName, (*resp.Model.Properties.Members)[1]).ID()
// Model.Properties.Members is a tuple => [source_server_id, replication_server_name]
sourceServerName := (*resp.Model.Properties.Members)[0]
replicaServerName := (*resp.Model.Properties.Members)[1]

sourceServerId := servers.NewFlexibleServerID(id.SubscriptionId, id.ResourceGroupName, sourceServerName).ID()

replicaServer, err := lookupFlexibleServerByName(ctx, flexibleServerClient, id, replicaServerName, sourceServerId)
if err != nil {
return err
}

state.SourceServerId = sourceServerId

replicaId, err := servers.ParseFlexibleServerID(*replicaServer.Id)
if err != nil {
return err
}

state.ReplicaServerId = replicaId.ID()
}
}

Expand All @@ -184,8 +201,8 @@ func (r PostgresqlFlexibleServerVirtualEndpointResource) Delete() sdk.ResourceFu
locks.ByName(id.FlexibleServerName, postgresqlFlexibleServerResourceName)
defer locks.UnlockByName(id.FlexibleServerName, postgresqlFlexibleServerResourceName)

if err := DeletePostgresFlexibileServerVirtualEndpoint(ctx, client, id); err != nil {
return err
if err := client.DeleteThenPoll(ctx, *id); err != nil {
return fmt.Errorf("deleting %s: %+v", *id, err)
}

return nil
Expand Down Expand Up @@ -231,17 +248,23 @@ func (r PostgresqlFlexibleServerVirtualEndpointResource) Update() sdk.ResourceFu
}
}

// exposed so we can access from tests
func DeletePostgresFlexibileServerVirtualEndpoint(ctx context.Context, client *virtualendpoints.VirtualEndpointsClient, id *virtualendpoints.VirtualEndpointId) error {
deletePoller := custompollers.NewPostgresFlexibleServerVirtualEndpointDeletePoller(client, *id)
poller := pollers.NewPoller(deletePoller, 5*time.Second, pollers.DefaultNumberOfDroppedConnectionsToAllow)

if _, err := client.Delete(ctx, *id); err != nil {
return fmt.Errorf("deleting %s: %+v", *id, err)
// The flexible endpoint API does not store the location/rg information on replicas it only stores the name.
// This lookup is safe because replicas for a given source server are *not* allowed to have identical names
Copy link
Member

Choose a reason for hiding this comment

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

Are the server names we're looking for here guaranteed to be unique per subscription, or could we have a condition where there are multiple servers of the same name in different resource groups that could lead to an unexpected outcome here?

Copy link
Contributor Author

@bruceharrison1984 bruceharrison1984 Oct 2, 2024

Choose a reason for hiding this comment

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

The names may not be unique per subscription, however they must be unique per source server. Since we check that the replica name matches, as well as filter those results based on the source server ID we should be protected from selecting an incorrect replica. Even if the names matched, replicas must have unique names per source servers.

TLDR; For a given source server, all replicas associated with the source server must have unique names. By filtering on both conditions, we can confidently pick the correct one even if duplicate named replicas exist within the subscription.

https://github.com/hashicorp/terraform-provider-azurerm/pull/27509/files/94e28c5de14955a188f90192c06828946f9a6fcc#diff-6fceca959b7c5669ebd391456b3a69c7ff858802d5dc5e70a91c6237cd173ff0R258

I was worried about this as well, but I believe it is setup in a safe manner.

func lookupFlexibleServerByName(ctx context.Context, flexibleServerClient *servers.ServersClient, virtualEndpointId *virtualendpoints.VirtualEndpointId, replicaServerName string, sourceServerId string) (*servers.Server, error) {
postgresServers, err := flexibleServerClient.ListCompleteMatchingPredicate(ctx, commonids.NewSubscriptionID(virtualEndpointId.SubscriptionId), servers.ServerOperationPredicate{
Name: &replicaServerName,
})
if err != nil {
return nil, err
}

if err := poller.PollUntilDone(ctx); err != nil {
return err
// loop to find the replica server associated with this flexible endpoint
for i := 0; i < len(postgresServers.Items); i++ {
postgresServer := postgresServers.Items[i]
if postgresServer.Properties.SourceServerResourceId != nil && *postgresServer.Properties.SourceServerResourceId == sourceServerId {
return &postgresServer, nil
}
}
return nil

return nil, fmt.Errorf("could not locate postgres replica server with name %s", replicaServerName)
}
Loading
Loading