New Resource: azurerm_nat_gateway_public_ip_v6_association#31321
New Resource: azurerm_nat_gateway_public_ip_v6_association#31321neil-yechenwei wants to merge 23 commits intohashicorp:mainfrom
azurerm_nat_gateway_public_ip_v6_association#31321Conversation
…ort for new SKU StandardV2
teowa
left a comment
There was a problem hiding this comment.
Hi @neil-yechenwei, thank you for submitting the PR. I’ve added some comments inline. Also, the PR might need to be rebased onto the main branch once #31197 is merged.
| "nat_gateway_id": { | ||
| Type: pluginsdk.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| ValidateFunc: natgateways.ValidateNatGatewayID, | ||
| }, | ||
|
|
||
| "public_ip_address_id": { | ||
| Type: pluginsdk.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| ValidateFunc: commonids.ValidatePublicIPAddressID, | ||
| }, |
There was a problem hiding this comment.
| "nat_gateway_id": { | |
| Type: pluginsdk.TypeString, | |
| Required: true, | |
| ForceNew: true, | |
| ValidateFunc: natgateways.ValidateNatGatewayID, | |
| }, | |
| "public_ip_address_id": { | |
| Type: pluginsdk.TypeString, | |
| Required: true, | |
| ForceNew: true, | |
| ValidateFunc: commonids.ValidatePublicIPAddressID, | |
| }, | |
| "nat_gateway_id": commonschema.ResourceIDReferenceRequiredForceNew(&natgateways.NatGatewayId{}), | |
| "public_ip_address_id": commonschema.ResourceIDReferenceRequiredForceNew(&commonids.PublicIPAddressId{}), |
| natGateway, err := client.Get(ctx, *natGatewayId, natgateways.DefaultGetOperationOptions()) | ||
| if err != nil { | ||
| if response.WasNotFound(natGateway.HttpResponse) { | ||
| return fmt.Errorf("%s was not found", natGatewayId) |
There was a problem hiding this comment.
Pointers used in fmt.Errorf() should be dereferenced, please fix similar issues in the _resource file.
| return fmt.Errorf("%s was not found", natGatewayId) | |
| return fmt.Errorf("%s was not found", *natGatewayId) |
|
@teowa , thanks for the comments. I updated PR. Please take another review. Below is the latest test result. |
|
Thanks @neil-yechenwei , LGTM! |
| return err | ||
| } | ||
|
|
||
| locks.ByName(natGatewayId.NatGatewayName, natGatewayResourceName) |
There was a problem hiding this comment.
Do we need to lock by the nat gateway's ID? Will there be two gateways in different resource group but same name?
There was a problem hiding this comment.
Updated. API allows to create two gateways with same name in different resource groups.
| if props := model.Properties; props != nil { | ||
| publicIpAddressesV6 := make([]natgateways.SubResource, 0) | ||
|
|
||
| if publicIPAddressesV6 := props.PublicIPAddressesV6; publicIPAddressesV6 != nil { |
There was a problem hiding this comment.
publicIPAddressesV6 is assigned (using :=) to props.PublicIPAddressesV6 in this if-condition, the modifications to publicIPAddressesV6 stays in the if-condition. Consider using another variable name?
|
|
||
| type NatGatewayPublicIpV6AssociationResource struct{} | ||
|
|
||
| func TestAccNatGatewayPublicIpV6Association_basic(t *testing.T) { |
There was a problem hiding this comment.
Please add another test to show verify if multiple associations can work together.
|
Hi @ms-zhenhua , thanks for reviewing this, I have updated the code, please kindly take another look. |
wuxu92
left a comment
There was a problem hiding this comment.
Thanks for the updates on this PR! I left some comments while it looks good overall. we are good to go once you addressed these comments.
| "github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk" | ||
| ) | ||
|
|
||
| type NatGatewayPublicIpV6AssociationResource struct{} |
There was a problem hiding this comment.
we should use IPv6 or ipv6, althought the SDK generated with IPAddressV6, the standard spelling is a lowercased v6 as IPv6. we should rename all other words in this PR too.
| type NatGatewayPublicIpV6AssociationResource struct{} | |
| type NatGatewayPublicIPv6AssociationResource struct{} |
| if model := natGateway.Model; model != nil { | ||
| if props := model.Properties; props != nil { | ||
| if props.PublicIPAddressesV6 == nil { | ||
| return metadata.MarkAsGone(id) | ||
| } | ||
|
|
||
| publicIPAddressFound := false | ||
| for _, pip := range *props.PublicIPAddressesV6 { | ||
| if pip.Id == nil { | ||
| continue | ||
| } | ||
|
|
||
| if strings.EqualFold(*pip.Id, id.Second.ID()) { | ||
| publicIPAddressFound = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !publicIPAddressFound { | ||
| return metadata.MarkAsGone(id) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
we need to handle if model or props is nil as well, and we can make it clear:
| if model := natGateway.Model; model != nil { | |
| if props := model.Properties; props != nil { | |
| if props.PublicIPAddressesV6 == nil { | |
| return metadata.MarkAsGone(id) | |
| } | |
| publicIPAddressFound := false | |
| for _, pip := range *props.PublicIPAddressesV6 { | |
| if pip.Id == nil { | |
| continue | |
| } | |
| if strings.EqualFold(*pip.Id, id.Second.ID()) { | |
| publicIPAddressFound = true | |
| break | |
| } | |
| } | |
| if !publicIPAddressFound { | |
| return metadata.MarkAsGone(id) | |
| } | |
| } | |
| } | |
| if natGateway.Model == nil || natGateway.Model.Properties == nil { | |
| return fmt.Errorf("retrieving %s: `model` or `properties` was nil", id) | |
| } | |
| publicIPAddressFound := false | |
| for _, pip := range pointer.From(model.Properties.PublicIPAddressesV6) { | |
| if strings.EqualFold(pointer.From(pip.Id), id.Second.ID()) { | |
| publicIPAddressFound = true | |
| break | |
| } | |
| } | |
| if !publicIPAddressFound { | |
| return metadata.MarkAsGone(id) | |
| } |
| return map[string]*pluginsdk.Schema{ | ||
| "nat_gateway_id": commonschema.ResourceIDReferenceRequiredForceNew(&natgateways.NatGatewayId{}), | ||
|
|
||
| "public_ip_address_id": commonschema.ResourceIDReferenceRequiredForceNew(&commonids.PublicIPAddressId{}), |
There was a problem hiding this comment.
Can we add a validator in CustomizeDiff to ensure the ID is an IPv6 address, assuming the Get API is a lightweight call? That way we’ll catch the error during the plan stage if an IPv4 ID is supplied.
| locks.ByID(natGatewayId.ID()) | ||
| defer locks.UnlockByID(natGatewayId.ID()) |
There was a problem hiding this comment.
we need to update the locks of azurerm_nat_gateway_public_ip_prefix_association and azurerm_nat_gateway_public_ip_association to use ID instead locks.ByName if we use ID here.
| } | ||
|
|
||
| func (r NatGatewayPublicIpV6AssociationResource) ResourceType() string { | ||
| return "azurerm_nat_gateway_public_ip_v6_association" |
There was a problem hiding this comment.
should it be no breaks in ipv6?
| return "azurerm_nat_gateway_public_ip_v6_association" | |
| return "azurerm_nat_gateway_public_ipv6_association" |
| publicIpAddressesV6 := make([]natgateways.SubResource, 0) | ||
|
|
||
| if v := natGateway.Model.Properties.PublicIPAddressesV6; v != nil { | ||
| for _, existingPublicIPAddress := range *v { | ||
| if existingPublicIPAddress.Id == nil { | ||
| continue | ||
| } | ||
|
|
||
| if strings.EqualFold(*existingPublicIPAddress.Id, publicIpAddressId.ID()) { | ||
| return metadata.ResourceRequiresImport(r.ResourceType(), id) | ||
| } | ||
|
|
||
| publicIpAddressesV6 = append(publicIpAddressesV6, existingPublicIPAddress) | ||
| } | ||
| } | ||
|
|
||
| publicIpAddressesV6 = append(publicIpAddressesV6, natgateways.SubResource{ | ||
| Id: pointer.To(state.PublicIpAddressId), | ||
| }) | ||
| natGateway.Model.Properties.PublicIPAddressesV6 = pointer.To(publicIpAddressesV6) |
There was a problem hiding this comment.
it seems we don't need to copy the list to publicIpAddressesV6?
| publicIpAddressesV6 := make([]natgateways.SubResource, 0) | |
| if v := natGateway.Model.Properties.PublicIPAddressesV6; v != nil { | |
| for _, existingPublicIPAddress := range *v { | |
| if existingPublicIPAddress.Id == nil { | |
| continue | |
| } | |
| if strings.EqualFold(*existingPublicIPAddress.Id, publicIpAddressId.ID()) { | |
| return metadata.ResourceRequiresImport(r.ResourceType(), id) | |
| } | |
| publicIpAddressesV6 = append(publicIpAddressesV6, existingPublicIPAddress) | |
| } | |
| } | |
| publicIpAddressesV6 = append(publicIpAddressesV6, natgateways.SubResource{ | |
| Id: pointer.To(state.PublicIpAddressId), | |
| }) | |
| natGateway.Model.Properties.PublicIPAddressesV6 = pointer.To(publicIpAddressesV6) | |
| existingIPs := pointer.From(natGateway.Model.Properties.PublicIPAddressesV6) | |
| for _, existingPublicIPAddress := range existingIPs { | |
| if strings.EqualFold(pointer.From(existingPublicIPAddress.Id), publicIpAddressId.ID()) { | |
| return metadata.ResourceRequiresImport(r.ResourceType(), id) | |
| } | |
| } | |
| publicIPAddressesV6 := append(existingIPs, natgateways.SubResource{ | |
| Id: pointer.To(state.PublicIpAddressId), | |
| }) | |
| natGateway.Model.Properties.PublicIPAddressesV6 = pointer.To(publicIPAddressesV6) |
| if v := natGateway.Model.Properties.PublicIPAddressesV6; v != nil { | ||
| for _, publicIPAddress := range *v { | ||
| if publicIPAddress.Id == nil { | ||
| continue | ||
| } | ||
|
|
||
| if !strings.EqualFold(*publicIPAddress.Id, id.Second.ID()) { | ||
| publicIpAddressesV6 = append(publicIpAddressesV6, publicIPAddress) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if v := natGateway.Model.Properties.PublicIPAddressesV6; v != nil { | |
| for _, publicIPAddress := range *v { | |
| if publicIPAddress.Id == nil { | |
| continue | |
| } | |
| if !strings.EqualFold(*publicIPAddress.Id, id.Second.ID()) { | |
| publicIpAddressesV6 = append(publicIpAddressesV6, publicIPAddress) | |
| } | |
| } | |
| } | |
| need2Delete := false | |
| for _, publicIPAddress := range pointer.From(natGateway.Model.Properties.PublicIPAddressesV6) { | |
| if !strings.EqualFold(pointer.From(publicIPAddress.Id), id.Second.ID()) { | |
| publicIpAddressesV6 = append(publicIpAddressesV6, publicIPAddress) | |
| } else { | |
| need2Delete = true | |
| } | |
| } | |
| // if addresss already not exists in gateway, then we don't need to call the update | |
| if !need2Delete { | |
| // add optional logging here | |
| return nil | |
| } |
|
Hi @wuxu92 , thanks for reviewing this! I've updated the code, please take another look. |
There was a problem hiding this comment.
Thanks @teowa for the updates. This is good enough to me. I just noticed that the public_ip_address_id usually references to a public ip resource's id field, which means it is unknown in plan stage but still need to the apply stage to known to value. but the CustomizeDiff still has value if the public ip resource is under provision already. I approved this PR and wait HC's review on this.
also please update the PR title to the new resource name.
| type NatGatewayPublicIPv6AssociationResource struct{} | ||
|
|
||
| var ( | ||
| _ sdk.Resource = NatGatewayPublicIPv6AssociationResource{} |
There was a problem hiding this comment.
| _ sdk.Resource = NatGatewayPublicIPv6AssociationResource{} |
|
Is being discussed internally with @wuxu92 |

Community Note
Description
This PR is to support new association resource
azurerm_nat_gateway_public_ip_v6_associationfor new featurepublicIpAddressesV6. This PR depends on #31197. Once PR 31197 is merged, please don't merge this PR since I will rebase the code.PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_nat_gateway_public_ip_v6_associationThis is a (please select all that apply):
Related Issue(s)
Fixes # 0000
AI Assistance Disclosure
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the provider.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.