azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

neil-yechenwei · 2024-04-30T09:40:56Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Service team confirmed that public_network_access_enabled is writeable property in the latest API version. So I submitted this PR to mark it as optional.

Below is the test scenarios for AzureRM version upgrade testing. As you can see, scenario 2 would cause breaking change 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. So users have to use ignore_changes or explicitly set it in the tf config to resolve it after AzureRM version is upgraded.

1. Create the resource with `delegated_subnet_id` and `private_dns_zone_id` without `public_network_access_enabled` using old AzureRM version -> Upgrade to new AzureRM version -> Run `tf plan` when `public_network_access_enabled` isn't set in the tf config -> No TF difference occurs.
2. Create the resource without `delegated_subnet_id`, `private_dns_zone_id` and `public_network_access_enabled` using old AzureRM version -> Upgrade to new AzureRM version -> Run `tf plan` when `public_network_access_enabled` isn't set in the tf config -> 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.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #14989
Fixes #24708
fixes #24641

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

…ed as optional

…etworkaccess01

katbyte · 2024-05-06T18:51:37Z

internal/services/postgres/postgresql_flexible_server_resource.go

 			"public_network_access_enabled": {
 				Type:     pluginsdk.TypeBool,
-				Computed: true,
+				Optional: true,
 			},


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

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.

neil-yechenwei · 2024-05-07T01:32:45Z

@katbyte , thanks for the comment. I updated PR. Please take another look. Below is the test result I just now triggered.

tombuildsstuff

hey @neil-yechenwei

Thanks for this PR - I've taken a look through and left some comments inline, but if we can fix those up then we should be able to take another look through here.

Thanks!

tombuildsstuff · 2024-05-07T13:02:22Z

internal/services/postgres/postgresql_flexible_server_resource.go

@@ -278,6 +278,7 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource {

 			"public_network_access_enabled": {
 				Type:     pluginsdk.TypeBool,
+				Optional: true,
 				Computed: true,


this shouldn't be Computed, but defaulted to true:

Suggested change

Computed: true,

Default: true,

I assume that we can't simply set the default value to "true" in the schema based on below reasons.

The default value of "public_network_access_enabled" is dynamic. Below is the scenarios. So we can't simply hard-code the default value in the schema.

~> **Note:** When `delegated_subnet_id` and `private_dns_zone_id` are set, the default value of `public_network_access_enabled` is `false`. ~> **Note:** When `delegated_subnet_id` and `private_dns_zone_id` aren't set, the default value of `public_network_access_enabled` is `true`.

If we set the default value of "public_network_access_enabled" to "true" in the schema, then "public_network_access_enabled" would always be set to "true" in the request payload when it isn't set. At this time, if "delegated_subnet_id" and "private_dns_zone_id" are also set in the request payload, then service API would return below error message.

Error Message:

performing Create: unexpected status 400 (400 Bad Request) with error: ConflictingPublicNetworkAccessAndVirtualNetworkConfiguration: Conflicting configuration is detected between Public Network Access and Virtual Network arguments. Public Network Access is not supported along with Virtual Network feature.

For the existing resource created with delegated_subnet_id and private_dns_zone_id, the value of public_network_access_enabled returned by service is false, then TF would cause difference since the default value is set to true in the schema.

Yes, users would need to set the value to false in their configs when they configure either delegated_subnet_id or private_dns_zone_id, that's expected.

tombuildsstuff · 2024-05-07T13:03:25Z

internal/services/postgres/postgresql_flexible_server_resource.go

+	// `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)
+	}


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.

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.

Yes, we can.

tombuildsstuff · 2024-05-07T13:04:26Z

website/docs/r/postgresql_flexible_server.html.markdown

@@ -116,6 +116,12 @@ The following arguments are supported:

 ~> **Note:** There will be a breaking change from upstream service at 15th July 2021, the `private_dns_zone_id` will be required when setting a `delegated_subnet_id`. For existing flexible servers who don't want to be recreated, you need to provide the `private_dns_zone_id` to the service team to manually migrate to the specified private DNS zone. The `azurerm_private_dns_zone` should end with suffix `.postgres.database.azure.com`.

+* `public_network_access_enabled` - (Optional) Is public network access enabled?


Suggested change

* `public_network_access_enabled` - (Optional) Is public network access enabled?

* `public_network_access_enabled` - (Optional) Specifies whether this PostgreSQL Flexible Server is publicly accessible. Defaults to `true`.

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 add "Defaults to true." here.

tombuildsstuff · 2024-05-07T13:05:23Z

website/docs/r/postgresql_flexible_server.html.markdown

+~> **Note:** When `delegated_subnet_id` and `private_dns_zone_id` are set, the default value of `public_network_access_enabled` is `false`.
+
+~> **Note:** When `delegated_subnet_id` and `private_dns_zone_id` aren't set, the default value of `public_network_access_enabled` is `true`.


By defaulting this on our side, we can make this informational:

Suggested change

~> **Note:** When `delegated_subnet_id` and `private_dns_zone_id` are set, the default value of `public_network_access_enabled` is `false`.

~> **Note:** When `delegated_subnet_id` and `private_dns_zone_id` aren't set, the default value of `public_network_access_enabled` is `true`.

-> **Note:** `public_network_access_enabled` must be set to `false` when `delegated_subnet_id` and `private_dns_zone_id` have a value.

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.

neil-yechenwei · 2024-05-08T01:35:08Z

@tombuildsstuff , thank for the comments. Please take another look.

neil-yechenwei · 2024-05-09T02:47:22Z

@tombuildsstuff , thanks for the comments. I updated PR. Please take another look. Below is the test result I just now re-triggered.

azurerm_postgresql_flexible_server - mark public_network_access_enabl…

7168d14

…ed as optional

neil-yechenwei marked this pull request as draft April 30, 2024 09:41

github-actions bot added documentation service/postgresql size/M labels Apr 30, 2024

update code

0ef5691

github-actions bot added size/S and removed size/M labels Apr 30, 2024

update code

ca141dc

github-actions bot added size/M waiting-response and removed size/S labels Apr 30, 2024

Merge remote-tracking branch 'upstream/main' into postgresqlfspublicn…

e2b2a52

…etworkaccess01

github-actions bot removed the waiting-response label Apr 30, 2024

neil-yechenwei added 2 commits May 1, 2024 06:55

update code

3ab28ee

update code

97087eb

github-actions bot added size/S and removed size/M labels Apr 30, 2024

update code

c5971c5

github-actions bot added size/XL and removed size/S labels Apr 30, 2024

neil-yechenwei added 4 commits May 1, 2024 08:16

update code

8fe8a0b

update code

ea8f0dd

update code

a20a509

update code

7555138

neil-yechenwei marked this pull request as ready for review May 6, 2024 00:09

update code

d1fea46

katbyte requested changes May 6, 2024

View reviewed changes

github-actions bot added the waiting-response label May 6, 2024

update code

66efd11

github-actions bot removed the waiting-response label May 7, 2024

neil-yechenwei requested a review from katbyte May 7, 2024 01:32

update code

a4f67cc

github-actions bot added size/M and removed size/XL labels May 7, 2024

update code

7c5f48e

github-actions bot added size/S and removed size/M labels May 7, 2024

tombuildsstuff requested changes May 7, 2024

View reviewed changes

github-actions bot added the waiting-response label May 7, 2024

neil-yechenwei requested a review from tombuildsstuff May 8, 2024 01:34

github-actions bot removed the waiting-response label May 8, 2024

update description of public_network_access_enabled in md

de337e5

github-actions bot added the waiting-response label May 8, 2024

set default value to true

a4bd274

github-actions bot added size/M and removed waiting-response size/S labels May 9, 2024

update tc

853cc27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

neil-yechenwei commented Apr 30, 2024 •

edited by katbyte

katbyte May 6, 2024

neil-yechenwei May 7, 2024

neil-yechenwei commented May 7, 2024 •

edited

tombuildsstuff left a comment

tombuildsstuff May 7, 2024

neil-yechenwei May 8, 2024 •

edited

tombuildsstuff May 8, 2024

neil-yechenwei May 9, 2024

tombuildsstuff May 7, 2024

neil-yechenwei May 8, 2024 •

edited

tombuildsstuff May 8, 2024

neil-yechenwei May 9, 2024

tombuildsstuff May 7, 2024

neil-yechenwei May 8, 2024 •

edited

neil-yechenwei May 9, 2024

tombuildsstuff May 7, 2024

neil-yechenwei May 8, 2024 •

edited

neil-yechenwei May 9, 2024

neil-yechenwei commented May 8, 2024

neil-yechenwei commented May 9, 2024 •

edited

		@@ -116,6 +116,12 @@ The following arguments are supported:

		~> Note: There will be a breaking change from upstream service at 15th July 2021, the `private_dns_zone_id` will be required when setting a `delegated_subnet_id`. For existing flexible servers who don't want to be recreated, you need to provide the `private_dns_zone_id` to the service team to manually migrate to the specified private DNS zone. The `azurerm_private_dns_zone` should end with suffix `.postgres.database.azure.com`.

		* `public_network_access_enabled` - (Optional) Is public network access enabled?

		~> Note: When `delegated_subnet_id` and `private_dns_zone_id` are set, the default value of `public_network_access_enabled` is `false`.

		~> Note: When `delegated_subnet_id` and `private_dns_zone_id` aren't set, the default value of `public_network_access_enabled` is `true`.

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

Are you sure you want to change the base?

azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812

Conversation

neil-yechenwei commented Apr 30, 2024 • edited by katbyte

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented May 7, 2024 • edited

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented May 8, 2024

neil-yechenwei commented May 9, 2024 • edited

neil-yechenwei commented Apr 30, 2024 •

edited by katbyte

neil-yechenwei commented May 7, 2024 •

edited

neil-yechenwei May 8, 2024 •

edited

neil-yechenwei May 8, 2024 •

edited

neil-yechenwei May 8, 2024 •

edited

neil-yechenwei May 8, 2024 •

edited

neil-yechenwei commented May 9, 2024 •

edited