-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_virtual_network_gateway
: bgp setting support APIPA
#10381
Conversation
azurerm_virtual_network_gateway
: bgp setting support APIPA
azurerm_virtual_network_gateway
: bgp setting support APIPA
azurerm_virtual_network_gateway
: bgp setting support APIPA
azurerm_virtual_network_gateway
: bgp setting support APIPA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @magodo
Thanks for this PR - taking a look through here this is mostly looking good - however I believe the APIPA addresses refer only to Azure Public, rather than the other Azure Environments? So that we can validate these correctly, could you reach out to the Service Team (or an online source, but I couldn't quickly find one) to confirm the APIPA addresses for these Environments?
Thanks!
azurerm/internal/services/network/virtual_network_gateway_resource.go
Outdated
Show resolved
Hide resolved
|
||
The `bgp_settings` block supports: | ||
|
||
* `asn` - (Optional) The Autonomous System Number (ASN) to use as part of the BGP. | ||
|
||
* `peering_address` - (Optional) The BGP peer IP address of the virtual network | ||
* `peering_address` - (Optional / **Deprecated**) The BGP peer IP address of the virtual network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is Deprecated we can remove it - however the Schema field above will need to lose it's ForceNew so people can migrate over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll remove the ForceNew
option and remove this line also.
|
||
* `ip_configuration_name` - (Required) The name of the IP configuration of this Virtual Network Gateway (as you defined in the`ip_configuration`). | ||
|
||
* `apipa_addresses` - (Optional) A list of Azure custom APIPA addresses assigned to the BGP peer of the Virtual Network Gateway. The valid range for the Azure reserved APIPA address is from "169.254.21.0" to "169.254.22.255". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make this:
* `apipa_addresses` - (Optional) A list of Azure custom APIPA addresses assigned to the BGP peer of the Virtual Network Gateway. The valid range for the Azure reserved APIPA address is from "169.254.21.0" to "169.254.22.255". | |
* `apipa_addresses` - (Optional) A list of Azure custom APIPA addresses assigned to the BGP peer of the Virtual Network Gateway. | |
~> **Note:** The valid range for the reserved APIPA address in Azure Public is from `169.254.21.0` to `169.254.22.255`. |
@tombuildsstuff Thank you for the comments! I've updated accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @magodo - overall looking good, just left a couple comments inline
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, | ||
Deprecated: "Deprecated in favor of `peering_addresses`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include a little more detail here on how to move over to the new property? does this correspond to the apipa_addresses
despite it taking multiple cross multiple peering address blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is mentioned in the PR description, this is now replaced by the bgp_settings.0.peering_addresses.0.default_addresses.0
.
I'll extend the deprecation message to include this.
azurerm/internal/services/network/virtual_network_gateway_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/virtual_network_gateway_resource_test.go
Outdated
Show resolved
Hide resolved
|
||
A `peering_addresses` supports the following: | ||
|
||
* `ip_configuration_name` - (Required) The name of the IP configuration of this Virtual Network Gateway (as you defined in the`ip_configuration`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is defined in the ip_configuration
for the gateway couldn't we just pull it from that property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case there are multiple peering_addresses
(s), in which case there are also multiple ip_configuration
(s), you need to specify which ip_configuration_name
this setting correspond to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is 1 and 1 we should be able to infer this and error out of we can't? make it optional and vlaidate dthat during create/update
@katbyte I've made the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @magodo - this LGTM now
This has been released in version 2.50.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.50.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Add support for
bgp_settings.peering_addresses
, which deprecates thebgp_settings.peering_address
(in favor ofbgp_settings.peering_addresses.default_addresses
). Thebgp_settings.peering_addresses
is an extension of theip_configuration
.Although the
bgp_settings.peering_address
is settable in current schema and its replacementbgp_settings.peering_addresses.default_addresses
is read-only. Confirmed with service team that thebgp_settings.peering_address
is actually read-only in backend side. This means this change will not incur breaking changes.Fix #10262
Test Result
I guess the failed case is not introduced by this PR?