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

Jump to bottom

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

Merged
merged 19 commits into from
May 24, 2024
+88 −38
Merged

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7168d14
azurerm_postgresql_flexible_server - mark public_network_access_enabl…
neil-yechenwei Apr 30, 2024
0ef5691
update code
neil-yechenwei Apr 30, 2024
ca141dc
update code
neil-yechenwei Apr 30, 2024
e2b2a52
Merge remote-tracking branch 'upstream/main' into postgresqlfspublicn…
neil-yechenwei Apr 30, 2024
3ab28ee
update code
neil-yechenwei Apr 30, 2024
97087eb
update code
neil-yechenwei Apr 30, 2024
c5971c5
update code
neil-yechenwei Apr 30, 2024
8fe8a0b
update code
neil-yechenwei May 1, 2024
ea8f0dd
update code
neil-yechenwei May 1, 2024
a20a509
update code
neil-yechenwei May 1, 2024
7555138
update code
neil-yechenwei May 1, 2024
d1fea46
update code
neil-yechenwei May 6, 2024
66efd11
update code
neil-yechenwei May 7, 2024
a4f67cc
update code
neil-yechenwei May 7, 2024
7c5f48e
update code
neil-yechenwei May 7, 2024
de337e5
update description of public_network_access_enabled in md
neil-yechenwei May 8, 2024
a4bd274
set default value to true
neil-yechenwei May 9, 2024
853cc27
update tc
neil-yechenwei May 9, 2024
19ae87e
resolve conflict
neil-yechenwei May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 10 additions & 9 deletions internal/services/postgres/postgresql_flexible_server_aad_administrator_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,16 @@ resource "azurerm_resource_group" "test" {
}

resource "azurerm_postgresql_flexible_server" "test" {
name = "acctest-fs-%[1]d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
administrator_login = "adminTerraform"
administrator_password = "QAZwsx123"
storage_mb = 32768
version = "12"
sku_name = "GP_Standard_D2s_v3"
zone = "2"
name = "acctest-fs-%[1]d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
administrator_login = "adminTerraform"
administrator_password = "QAZwsx123"
storage_mb = 32768
version = "12"
sku_name = "GP_Standard_D2s_v3"
zone = "2"
public_network_access_enabled = true

authentication {
active_directory_auth_enabled = true
Expand Down
19 changes: 10 additions & 9 deletions internal/services/postgres/postgresql_flexible_server_configuration_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,15 +328,16 @@ resource "azurerm_resource_group" "test" {
}

resource "azurerm_postgresql_flexible_server" "test" {
name = "acctest-fs-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
administrator_login = "adminTerraform"
administrator_password = "QAZwsx123"
storage_mb = 32768
version = "12"
sku_name = "GP_Standard_D2s_v3"
zone = "2"
name = "acctest-fs-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
administrator_login = "adminTerraform"
administrator_password = "QAZwsx123"
storage_mb = 32768
version = "12"
sku_name = "GP_Standard_D2s_v3"
zone = "2"
public_network_access_enabled = true
}

resource "azurerm_postgresql_flexible_server_configuration" "test" {
Expand Down
13 changes: 11 additions & 2 deletions internal/services/postgres/postgresql_flexible_server_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource {

"public_network_access_enabled": {
Type: pluginsdk.TypeBool,
Computed: true,
Optional: true,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be a breaking change for existing users? as it will now set everyone to false whenthe default iirc is true?

as such we should make it optional + computed to account for existing resources + users changing the default outside terraform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. When user created the resource without delegated_subnet_id, private_dns_zone_id and public_network_access_enabled using old AzureRM version and then user upgraded to new AzureRM version, tf plan would cause difference if public_network_access_enabled isn't set in the tf config. Though there is TF difference but it's expected since service API would return the default value true for public_network_access_enabled when delegated_subnet_id and private_dns_zone_id aren't set.

OK. I will use optional + computed. Then the existing resources wouldn't be impacted anymore.


"replication_role": {
Expand Down Expand Up @@ -767,7 +767,7 @@ func resourcePostgresqlFlexibleServerUpdate(d *pluginsdk.ResourceData, meta inte
}
}

if d.HasChange("private_dns_zone_id") {
if d.HasChange("private_dns_zone_id") || d.HasChange("public_network_access_enabled") {
parameters.Properties.Network = expandArmServerNetwork(d)
}

Expand Down Expand Up @@ -964,6 +964,15 @@ func expandArmServerNetwork(d *pluginsdk.ResourceData) *servers.Network {
network.PrivateDnsZoneArmResourceId = utils.String(v.(string))
}

// `d.GetOk()` cannot identify if the bool property `public_network_access_enabled` is set or not in the tf config since d.GetOk() always returns `false` when `public_network_access_enabled` is set to `false` and `public_network_access_enabled` isn't set in the tf config
if !pluginsdk.IsExplicitlyNullInConfig(d, "public_network_access_enabled") {
publicNetworkAccessEnabled := servers.ServerPublicNetworkAccessStateDisabled
if d.Get("public_network_access_enabled").(bool) {
publicNetworkAccessEnabled = servers.ServerPublicNetworkAccessStateEnabled
}
network.PublicNetworkAccess = pointer.To(publicNetworkAccessEnabled)
}
Copy link
Member

Choose a reason for hiding this comment

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

By defaulting this in the Read we can just pull from the config:

Suggested change
// `d.GetOk()` cannot identify if the bool property `public_network_access_enabled` is set or not in the tf config since d.GetOk() always returns `false` when `public_network_access_enabled` is set to `false` and `public_network_access_enabled` isn't set in the tf config
if !pluginsdk.IsExplicitlyNullInConfig(d, "public_network_access_enabled") {
publicNetworkAccessEnabled := servers.ServerPublicNetworkAccessStateDisabled
if d.Get("public_network_access_enabled").(bool) {
publicNetworkAccessEnabled = servers.ServerPublicNetworkAccessStateEnabled
}
network.PublicNetworkAccess = pointer.To(publicNetworkAccessEnabled)
}
// `d.GetOk()` cannot identify if the bool property `public_network_access_enabled` is set or not in the tf config since d.GetOk() always returns `false` when `public_network_access_enabled` is set to `false` and `public_network_access_enabled` isn't set in the tf config
publicNetworkAccessEnabled := servers.ServerPublicNetworkAccessStateEnabled
if !d.Get("public_network_access_enabled").(bool) {
publicNetworkAccessEnabled = servers.ServerPublicNetworkAccessStateDisabled
}
network.PublicNetworkAccess = pointer.To(publicNetworkAccessEnabled)

Note: since the Azure API defaults this to true today, we'll also need to update the Read function to account for this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I above mentioned, I assume that we can't simply hard-code the default value for public_network_access_enabled. So I may not apply this suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


return &network
}

Expand Down