-
Notifications
You must be signed in to change notification settings - Fork 5k
azurerm_nat_gateway/azurerm_public_ip/azurerm_public_ip_prefix - support for new SKU StandardV2
#31197
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
azurerm_nat_gateway/azurerm_public_ip/azurerm_public_ip_prefix - support for new SKU StandardV2
#31197
Changes from 9 commits
5e0cac4
8669186
1ca4e10
c1dcf3c
e882770
9cd164f
f5906bd
66824ac
69e74c7
e3c8b25
d8efc68
574846b
d626e58
348e749
2de750c
79d6261
d53603e
1c0a85c
aa8b5d5
a1c8270
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,15 +74,22 @@ func resourceNatGatewaySchema() map[string]*pluginsdk.Schema { | |
| }, | ||
|
|
||
| "sku_name": { | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| Default: string(natgateways.NatGatewaySkuNameStandard), | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| string(natgateways.NatGatewaySkuNameStandard), | ||
| }, false), | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| Default: string(natgateways.NatGatewaySkuNameStandard), | ||
| ValidateFunc: validation.StringInSlice(natgateways.PossibleValuesForNatGatewaySkuName(), false), | ||
| }, | ||
|
|
||
| "zones": commonschema.ZonesMultipleOptionalForceNew(), | ||
| "zones": { | ||
| Type: schema.TypeSet, | ||
| Optional: true, | ||
| Computed: true, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making this O+C breaks the existing behaviour of replacing the resource when a user removes zones from their deployment (when sku is Additionally, O+C doesn't entirely resolve the e.g. DiffSuppress: CustomizeDiff:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense, code updated. |
||
| ForceNew: true, | ||
| Elem: &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| ValidateFunc: validation.StringIsNotEmpty, | ||
|
sreallymatt marked this conversation as resolved.
|
||
| }, | ||
| }, | ||
|
|
||
| "resource_guid": { | ||
| Type: pluginsdk.TypeString, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,21 @@ func TestAccNatGateway_update(t *testing.T) { | |
| }) | ||
| } | ||
|
|
||
| func TestAccNatGateway_standardVTwo(t *testing.T) { | ||
|
sreallymatt marked this conversation as resolved.
|
||
| data := acceptance.BuildTestData(t, "azurerm_nat_gateway", "test") | ||
| r := NatGatewayResource{} | ||
|
|
||
| data.ResourceTest(t, r, []acceptance.TestStep{ | ||
| { | ||
| Config: r.standardVTwo(data), | ||
| Check: acceptance.ComposeTestCheckFunc( | ||
| check.That(data.ResourceName).ExistsInAzure(r), | ||
| ), | ||
| }, | ||
| data.ImportStep(), | ||
| }) | ||
| } | ||
|
|
||
| func (t NatGatewayResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { | ||
| id, err := natgateways.ParseNatGatewayID(state.ID) | ||
| if err != nil { | ||
|
|
@@ -208,3 +223,24 @@ resource "azurerm_nat_gateway_public_ip_prefix_association" "test" { | |
| } | ||
| `, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, data.RandomInteger) | ||
| } | ||
|
|
||
| func (NatGatewayResource) standardVTwo(data acceptance.TestData) string { | ||
| return fmt.Sprintf(` | ||
| provider "azurerm" { | ||
| features {} | ||
| } | ||
|
|
||
| resource "azurerm_resource_group" "test" { | ||
| name = "acctestRG-network-%d" | ||
| location = "%s" | ||
| } | ||
|
|
||
| resource "azurerm_nat_gateway" "test" { | ||
| name = "acctestnatGateway-%d" | ||
| location = azurerm_resource_group.test.location | ||
| resource_group_name = azurerm_resource_group.test.name | ||
| sku_name = "StandardV2" | ||
| zones = ["1", "2", "3"] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is specifying all zones required for StandardV2? If so please add a CustomizeDiff validator for this. Refer to:
https://learn.microsoft.com/en-us/azure/nat-gateway/nat-overview
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reason1: I think we should not use a CustomizeDiff validator to force users to specify zones, because the API also allows it when users do not specify them. Reason2: About ignoring zones, according to the previous recommendations provided by HC, when the configuration of property A affects other properties—for example, property B—the affected property B should be handled by the user in the Terraform configuration. This allows users to clearly understand that when property A is modified, property B will also be impacted, rather than suppressing the issue through changes in the provider, which could cause property B to change without the user being aware of it. This can prevent users from encountering unexpected behavior. So, I add the comment for it. |
||
| } | ||
| `, data.RandomInteger, data.Locations.Primary, data.RandomInteger) | ||
| } | ||
|
sreallymatt marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -78,10 +78,12 @@ The following arguments are supported: | |||||
|
|
||||||
| * `reverse_fqdn` - (Optional) A fully qualified domain name that resolves to this public IP address. If the reverseFqdn is specified, then a PTR DNS record is created pointing from the IP address in the in-addr.arpa domain to the reverse FQDN. | ||||||
|
|
||||||
| * `sku` - (Optional) The SKU of the Public IP. Accepted values are `Basic` and `Standard`. Defaults to `Standard`. Changing this forces a new resource to be created. | ||||||
| * `sku` - (Optional) The SKU of the Public IP. Possible values are `Basic`, `Standard` and `StandardV2`. Defaults to `Standard`. Changing this forces a new resource to be created. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add the Oxford (serial) comma to the
Suggested change
NOTE: On line 3 of Reference: reference-documentation-standards
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||||||
|
|
||||||
| -> **Note:** Public IP Standard SKUs require `allocation_method` to be set to `Static`. | ||||||
|
|
||||||
| -> **Note:** `Basic` will be deprecated. [Read more](https://azure.microsoft.com/en-us/updates/upgrade-to-standard-sku-public-ip-addresses-in-azure-by-30-september-2025-basic-sku-will-be-retired/). | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we update this to be more descriptive and correct the
Suggested change
Follow up question: Do we currently have acceptance test that currently use the Reference: reference-documentation-standards
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. As the resource with Basic can still be created successfully, I didn't change the test case.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is fine that Basic will continue to work for existing resources, but we need to gate this correctly as I suggested.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will submit a follow-up PR that will prevent the creation of the basic SKU and either skip or migrate the acctests, since many other resource tests are also impacted.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @WodansSon , PR #31885 was submitted to block creation of |
||||||
|
|
||||||
| * `sku_tier` - (Optional) The SKU Tier that should be used for the Public IP. Possible values are `Regional` and `Global`. Defaults to `Regional`. Changing this forces a new resource to be created. | ||||||
|
|
||||||
| -> **Note:** When `sku_tier` is set to `Global`, `sku` must be set to `Standard`. | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.