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

feat(connectivity): Add option to set allow_non_virtual_wan_traffic in express route gateway. #914

Merged

Conversation

Slapper
Copy link
Contributor

@Slapper Slapper commented Mar 21, 2024

Overview/Summary

Add option to set allow_non_virtual_wan_traffic value to true or false in express route gateway.

This PR fixes/adds/changes/removes

  1. Give the option to set allow_non_virtual_wan_traffic to true in azurerm_express_route_gateway resource

Breaking Changes

None

Testing Evidence

On the latest azurerm versions ( v3.48.0 and higher ) azurerm_express_route_gateway resource has the option to set this value to true. At the time of writing CAF module has no option available .

We saw this issue in our Dev environment during terraform plan : the value was true so using a newer azurerm version the value goes to false, which is the default.

  # module.vWAN.module.alz_connectivity.azurerm_express_route_gateway.virtual_wan["/subscriptions/xxxxx-xxxx-xxxx-xxxxx-xxxxxxxxxx/resourceGroups/dev-connectivity/providers/Microsoft.Network/expressRouteGateways/dev-ergw-northeurope"] will be updated in-place
  ~ resource "azurerm_express_route_gateway" "virtual_wan" {
      ~ allow_non_virtual_wan_traffic = true -> false
        id                            = "/subscriptions/xxxxx-xxxx-xxxx-xxxxx-xxxxxxxxxx/resourceGroups/dev-connectivity/providers/Microsoft.Network/expressRouteGateways/dev-ergw-northeurope"
        name                          = "dev-ergw-northeurope"
        tags                          = {
            "deployedBy"  = "terraform/azure/caf-enterprise-scale"
            "deployedFor" = "Dev"
        }

More info about this option can be found here : hashicorp/terraform-provider-azurerm#20667

As part of this Pull Request I have

  • Checked for duplicate Pull Requests
  • Associated it with relevant issues, for tracking and closure.
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Performed testing and provided evidence.
  • Updated relevant and associated documentation.

@Slapper
Copy link
Contributor Author

Slapper commented Mar 21, 2024

@microsoft-github-policy-service agree

@Slapper Slapper force-pushed the add_allow_non_virtual_wan_traffic_option branch from f8906c7 to 8b46c96 Compare March 22, 2024 12:23
@matt-FFFFFF
Copy link
Member

LGTM - @luke-taylor @jaredfholgate

Please could you have a look and merge/release if you're happy?

Thanks!

@birdnathan
Copy link
Contributor

+1 - Hit by this issue today

@jaredfholgate
Copy link
Member

/azp run unit

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jaredfholgate jaredfholgate left a comment

Choose a reason for hiding this comment

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

Need to run terraform fmt -recursive to fix the tests.

@@ -80,7 +80,7 @@ resource "azurerm_express_route_gateway" "virtual_wan" {
location = each.value.template.location
virtual_hub_id = each.value.template.virtual_hub_id
scale_units = each.value.template.scale_units

allow_non_virtual_wan_traffic = each.value.template.allow_non_virtual_wan_traffic
Copy link
Member

Choose a reason for hiding this comment

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

Please run terraform fmt. Thanks!

@Slapper Slapper force-pushed the add_allow_non_virtual_wan_traffic_option branch from 8b46c96 to 34fef99 Compare April 4, 2024 06:07
@jaredfholgate
Copy link
Member

/azp run unit

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jaredfholgate
Copy link
Member

@Slapper For some reason I am still seeing formatting errors in the unit tests. Two options:

  1. Give me access to your repo and I'll fix it there.
  2. I can move your branch over to the Azure org and fix it there.

Let me know which you prefer.

Thanks

@Slapper
Copy link
Contributor Author

Slapper commented Apr 4, 2024

Hi @jaredfholgate ,

Feel free to move my branch to Azure project.

( i forgot to run the fmt with the recursive flag so maybe thats was the case )
Thanks!

@Slapper Slapper force-pushed the add_allow_non_virtual_wan_traffic_option branch from 34fef99 to 69f2d2a Compare April 4, 2024 09:34
@jaredfholgate
Copy link
Member

/azp run unit

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jaredfholgate
Copy link
Member

Hi @jaredfholgate ,

Feel free to move my branch to Azure project.

( i forgot to run the fmt with the recursive flag so maybe thats was the case ) Thanks!

That has fixed the linting issues, thanks. I will run and update the tests as nessecary now.

Copy link
Member

@jaredfholgate jaredfholgate left a comment

Choose a reason for hiding this comment

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

LGTM

@jaredfholgate jaredfholgate merged commit fc666d1 into Azure:main Apr 5, 2024
9 checks passed
@jaredfholgate
Copy link
Member

This was released in 5.2.0.

@birdnathan
Copy link
Contributor

@Slapper @jaredfholgate - something not quite right with that release
image

@birdnathan
Copy link
Contributor

birdnathan commented Apr 5, 2024

@Slapper @jaredfholgate - something not quite right with that release image

Apologies, sorted. Nothing to do with your change. #907 introduced a breaking change. Had to tweak settings.connectivity to also change threat_intelligence_allowlist = [] to threat_intelligence_allowlist = {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants