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

upgrade network to 2020-05-01 for vhub(conn) & vwan #7601

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jul 7, 2020

Upgrade network to 2020-05-01 for virtual hub(conn) & virtual wan resources.

Changes include:

  • Use the dedicated client for azurerm_virtualhub_connection, as the original field in azurerm_virtualhub used to create/delete vhub conn is removed from API.
  • Deprecate hub_to_vitual_network_traffic_allowed and vitual_network_to_hub_gateways_traffic_allowed from azurerm_virtualhub_connection, as they are deprecated by API.
  • Deprecate allow_vnet_to_vnet_traffic from azurerm_virtual_wan as this is not deprecated by API.

(follow-up PR for #7585 )
(fixes #7896 )

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.

left a few comments inline but this otherwise LGTM 👍


* `vitual_network_to_hub_gateways_traffic_allowed` - (Optional) Is Remote Virtual Network traffic allowed to transit the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.
* `vitual_network_to_hub_gateways_traffic_allowed` - (Optional / **Deprecated** ) Is Remote Virtual Network traffic allowed to transit the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

arguably since this is non-functional we could remove this:

Suggested change
* `vitual_network_to_hub_gateways_traffic_allowed` - (Optional / **Deprecated** ) Is Remote Virtual Network traffic allowed to transit the Hub's Virtual Network Gateway's? Changing this forces a new resource to be created.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about just keep this (as user is still allowed to assign it) but change the document so something like:

Due to a breaking behavioural change in the Azure API this property is no longer functional and will be removed in version 3.0 of the provider

@@ -58,9 +58,9 @@ The following arguments are supported:

---

* `hub_to_vitual_network_traffic_allowed` - (Optional) Is the Virtual Hub traffic allowed to transit via the Remote Virtual Network? Changing this forces a new resource to be created.
* `hub_to_vitual_network_traffic_allowed` - (Optional / **Deprecated** ) Is the Virtual Hub traffic allowed to transit via the Remote Virtual Network? Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

arguably since this is non-functional we could remove this:

Suggested change
* `hub_to_vitual_network_traffic_allowed` - (Optional / **Deprecated** ) Is the Virtual Hub traffic allowed to transit via the Remote Virtual Network? Changing this forces a new resource to be created.

@@ -40,7 +40,7 @@ The following arguments are supported:

* `allow_branch_to_branch_traffic` - (Optional) Boolean flag to specify whether branch to branch traffic is allowed. Defaults to `true`.

* `allow_vnet_to_vnet_traffic` - (Optional) Boolean flag to specify whether VNet to VNet traffic is allowed. Defaults to `false`.
* `allow_vnet_to_vnet_traffic` - (Optional / **Deprecated** ) Boolean flag to specify whether VNet to VNet traffic is allowed. Defaults to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

arguably since this is non-functional we could remove this:

Suggested change
* `allow_vnet_to_vnet_traffic` - (Optional / **Deprecated** ) Boolean flag to specify whether VNet to VNet traffic is allowed. Defaults to `false`.

@magodo
Copy link
Collaborator Author

magodo commented Jul 8, 2020

@tombuildsstuff Thank you for the review comments! I have updated accordingly. Please take a look!
I have no idea why the CI failed, as I only made some minor changes. I also tried the make depscheck locally with no problem...

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@magodo, thanks so much for this PR, LGTM! 🚀

@magodo
Copy link
Collaborator Author

magodo commented Aug 10, 2020

@WodansSon I have submit another update to ensure the routing state reaches "Provisioned" so as to avoid race condition between vhub internal routing update and hub vnet connection caused routing update.

I have tested it in West Europe:

💤 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run TestAccAzureRMVirtualHubConnection_update'

==> 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/tests -v -run TestAccAzureRMVirtualHubConnection_update -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMVirtualHubConnection_update
=== PAUSE TestAccAzureRMVirtualHubConnection_update
=== CONT  TestAccAzureRMVirtualHubConnection_update
--- PASS: TestAccAzureRMVirtualHubConnection_update (3117.31s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       3117.349s

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@magodo, thank you for pushing these changes, this LGTM 🚀

@WodansSon WodansSon modified the milestones: Blocked, v2.24.0 Aug 18, 2020
@tombuildsstuff tombuildsstuff self-assigned this Aug 19, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v2.24.0, v2.25.0 Aug 20, 2020
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 pushing those changes - I've taken a look through and left some comments inline; if we can fix those up then this should otherwise be good to merge (we'll need to add an "upgrade note" to the changelog, but we'll do that at merge time)

Thanks!

@magodo
Copy link
Collaborator Author

magodo commented Aug 26, 2020

@tombuildsstuff I have resolved the remaining comments, please take another look! Thanks!

…_security_enabled`

The deprecated attributes are always `true` on return, as they are
deprecated, we can simply suppress any diff on it.

`internet_security_enabled` seems has changed in this new version that
it is `false` even if it is omit in the request.
@tombuildsstuff tombuildsstuff modified the milestones: v2.25.0, v2.26.0 Aug 27, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v2.26.0, v2.27.0 Sep 3, 2020
@jackofallops jackofallops modified the milestones: v2.27.0, v2.28.0 Sep 10, 2020
@WodansSon WodansSon modified the milestones: v2.28.0, v2.29.0 Sep 16, 2020
@magodo
Copy link
Collaborator Author

magodo commented Sep 22, 2020

@tombuildsstuff I have resolved the remaining comments, especially this one. Would you please take another look?

@WodansSon
Copy link
Collaborator

@magodo, thanks for pushing these changes and fixing the conflict! 🚀

@WodansSon
Copy link
Collaborator

image

@Dilergore
Copy link
Contributor

Dilergore commented Sep 24, 2020

Hi,

This is very urgent. Using the old API version will clear the routes on the vWAN causing major issues. We had a production incident caused by this.

Also mentioned as recommended in the vWAN FAQ:
https://docs.microsoft.com/en-us/azure/virtual-wan/virtual-wan-faq#what-is-the-recommended-api-version-to-be-used-by-scripts-automating-various-virtual-wan-functionalities

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.

LGTM 👍

@tombuildsstuff tombuildsstuff merged commit 3515a71 into hashicorp:master Sep 24, 2020
tombuildsstuff added a commit that referenced this pull request Sep 24, 2020
@ghost
Copy link

ghost commented Sep 24, 2020

This has been released in version 2.29.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.29.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Oct 24, 2020

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 Oct 24, 2020
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.

Old Azure vWAN API issues. New vWAN Routing features are not enabled
6 participants