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.

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

Merged
merged 6 commits into from
Feb 26, 2021

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jan 29, 2021

Add support for bgp_settings.peering_addresses, which deprecates the bgp_settings.peering_address (in favor of bgp_settings.peering_addresses.default_addresses). The bgp_settings.peering_addresses is an extension of the ip_configuration.

Although the bgp_settings.peering_address is settable in current schema and its replacement bgp_settings.peering_addresses.default_addresses is read-only. Confirmed with service team that the bgp_settings.peering_address is actually read-only in backend side. This means this change will not incur breaking changes.

Fix #10262

Test Result

terraform-provider-azurerm on  vnet_gateway_apipa via 🐹 v1.15.6 took 25m20s
💤 make testacc TEST=./azurerm/internal/services/network TESTARGS='-run TestAccVirtualNetworkGateway_'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/network -v -run TestAccVirtualNetworkGateway_ -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccVirtualNetworkGateway_basic
=== PAUSE TestAccVirtualNetworkGateway_basic
=== RUN   TestAccVirtualNetworkGateway_requiresImport
=== PAUSE TestAccVirtualNetworkGateway_requiresImport
=== RUN   TestAccVirtualNetworkGateway_lowerCaseSubnetName
=== PAUSE TestAccVirtualNetworkGateway_lowerCaseSubnetName
=== RUN   TestAccVirtualNetworkGateway_vpnGw1
=== PAUSE TestAccVirtualNetworkGateway_vpnGw1
=== RUN   TestAccVirtualNetworkGateway_activeActive
=== PAUSE TestAccVirtualNetworkGateway_activeActive
=== RUN   TestAccVirtualNetworkGateway_standard
=== PAUSE TestAccVirtualNetworkGateway_standard
=== RUN   TestAccVirtualNetworkGateway_vpnGw2
=== PAUSE TestAccVirtualNetworkGateway_vpnGw2
=== RUN   TestAccVirtualNetworkGateway_vpnGw3
=== PAUSE TestAccVirtualNetworkGateway_vpnGw3
=== RUN   TestAccVirtualNetworkGateway_generation
=== PAUSE TestAccVirtualNetworkGateway_generation
=== RUN   TestAccVirtualNetworkGateway_vpnClientConfig
=== PAUSE TestAccVirtualNetworkGateway_vpnClientConfig
=== RUN   TestAccVirtualNetworkGateway_vpnClientConfigAzureAdAuth
=== PAUSE TestAccVirtualNetworkGateway_vpnClientConfigAzureAdAuth
=== RUN   TestAccVirtualNetworkGateway_vpnClientConfigOpenVPN
=== PAUSE TestAccVirtualNetworkGateway_vpnClientConfigOpenVPN
=== RUN   TestAccVirtualNetworkGateway_enableBgp
=== PAUSE TestAccVirtualNetworkGateway_enableBgp
=== RUN   TestAccVirtualNetworkGateway_enableBgpWithAPIPA
=== PAUSE TestAccVirtualNetworkGateway_enableBgpWithAPIPA
=== RUN   TestAccVirtualNetworkGateway_activeActiveEnableBgpWithAPIPA
=== PAUSE TestAccVirtualNetworkGateway_activeActiveEnableBgpWithAPIPA
=== RUN   TestAccVirtualNetworkGateway_expressRoute
=== PAUSE TestAccVirtualNetworkGateway_expressRoute
=== RUN   TestAccVirtualNetworkGateway_privateIpAddressEnabled
=== PAUSE TestAccVirtualNetworkGateway_privateIpAddressEnabled
=== CONT  TestAccVirtualNetworkGateway_basic
=== CONT  TestAccVirtualNetworkGateway_vpnClientConfig
=== CONT  TestAccVirtualNetworkGateway_standard
=== CONT  TestAccVirtualNetworkGateway_vpnGw1
=== CONT  TestAccVirtualNetworkGateway_lowerCaseSubnetName
=== CONT  TestAccVirtualNetworkGateway_activeActiveEnableBgpWithAPIPA
=== CONT  TestAccVirtualNetworkGateway_enableBgp
=== CONT  TestAccVirtualNetworkGateway_vpnGw3
--- PASS: TestAccVirtualNetworkGateway_enableBgp (1545.25s)
=== CONT  TestAccVirtualNetworkGateway_activeActive
--- PASS: TestAccVirtualNetworkGateway_vpnGw3 (1648.47s)
=== CONT  TestAccVirtualNetworkGateway_generation
--- PASS: TestAccVirtualNetworkGateway_activeActiveEnableBgpWithAPIPA (1658.50s)
=== CONT  TestAccVirtualNetworkGateway_privateIpAddressEnabled
--- PASS: TestAccVirtualNetworkGateway_vpnClientConfig (1760.13s)
=== CONT  TestAccVirtualNetworkGateway_expressRoute
--- PASS: TestAccVirtualNetworkGateway_vpnGw1 (1772.30s)
=== CONT  TestAccVirtualNetworkGateway_enableBgpWithAPIPA
--- PASS: TestAccVirtualNetworkGateway_standard (2878.69s)
=== CONT  TestAccVirtualNetworkGateway_requiresImport
--- PASS: TestAccVirtualNetworkGateway_basic (2916.15s)
=== CONT  TestAccVirtualNetworkGateway_vpnGw2
--- PASS: TestAccVirtualNetworkGateway_expressRoute (1246.93s)
=== CONT  TestAccVirtualNetworkGateway_vpnClientConfigOpenVPN
--- PASS: TestAccVirtualNetworkGateway_activeActive (1550.36s)
=== CONT  TestAccVirtualNetworkGateway_vpnClientConfigAzureAdAuth
--- PASS: TestAccVirtualNetworkGateway_lowerCaseSubnetName (3173.35s)
--- PASS: TestAccVirtualNetworkGateway_enableBgpWithAPIPA (1860.74s)
--- PASS: TestAccVirtualNetworkGateway_generation (2560.87s)
=== CONT  TestAccVirtualNetworkGateway_vpnClientConfigAzureAdAuth
    testing.go:684: Step 0 error: Check failed: Check 2/5 error: azurerm_virtual_network_gateway.test: Attribute 'vpn_client_configuration.0.aad_tenant' expected "https://login.microsoftonline.com/vv7p8/", got "https://login.microsoftonline.com/72f988bf-86f1-41af-91ab-2d7cd011db47/"
--- PASS: TestAccVirtualNetworkGateway_vpnGw2 (1448.40s)
--- PASS: TestAccVirtualNetworkGateway_vpnClientConfigOpenVPN (1934.89s)
--- FAIL: TestAccVirtualNetworkGateway_vpnClientConfigAzureAdAuth (1923.98s)
--- PASS: TestAccVirtualNetworkGateway_requiresImport (2885.73s)
--- PASS: TestAccVirtualNetworkGateway_privateIpAddressEnabled (4722.24s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network     6380.801s
FAIL
GNUmakefile:104: recipe for target 'testacc' failed
make: *** [testacc] Error 1

I guess the failed case is not introduced by this PR?

@magodo magodo changed the title azurerm_virtual_network_gateway: bgp setting support APIPA WIP: azurerm_virtual_network_gateway: bgp setting support APIPA Jan 29, 2021
@magodo magodo changed the title WIP: azurerm_virtual_network_gateway: bgp setting support APIPA azurerm_virtual_network_gateway: bgp setting support APIPA Feb 2, 2021
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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!


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
Copy link
Contributor

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?

Copy link
Collaborator Author

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".
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make this:

Suggested change
* `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`.

@magodo
Copy link
Collaborator Author

magodo commented Feb 19, 2021

@tombuildsstuff Thank you for the comments! I've updated accordingly.
Also confirmed with service team that the APIPA works for all the clouds (also the ranges are the same).

Copy link
Collaborator

@katbyte katbyte left a 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`",
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

website/docs/r/virtual_network_gateway.html.markdown Outdated Show resolved Hide resolved
website/docs/r/virtual_network_gateway.html.markdown 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`).
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

@katbyte katbyte Feb 24, 2021

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

@magodo
Copy link
Collaborator Author

magodo commented Feb 25, 2021

@katbyte I've made the ip_configuration_name optional in the latest commit. Please take another look.

@katbyte katbyte added this to the v2.50.0 milestone Feb 26, 2021
Copy link
Collaborator

@katbyte katbyte left a 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

@katbyte katbyte merged commit 323d510 into hashicorp:master Feb 26, 2021
katbyte added a commit that referenced this pull request Feb 26, 2021
@ghost
Copy link

ghost commented Mar 5, 2021

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 ...

@ghost
Copy link

ghost commented Mar 28, 2021

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!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for APIPA bgp settings for azurerm_virtual_network_gateway
3 participants