New Data Source: azurerm_virtual_network

[dcaro]

Adding a data source for vnet in Azure.

[tombuildsstuff]

Hey @dcaro

Thanks for opening this PR - I've taken a look through and left some comments in-line but this is off to a good start; the only thing missing that I can see at this point is some documentation - would it be possible to add some for this Data Source?

Thanks!

[tombuildsstuff]

can we change this to use utils.ResponseWasNotFound() to handle the connection being dropped (where this will currently crash)?

[dcaro]

Make sense, will do so.

[tombuildsstuff]

can we change this second formatting argument to %+v - which outputs the full exception, rather than just the friendly message (sometimes the actual error is hidden)

[tombuildsstuff]

in order to handle a potential crash - can we update this to:

if prefixes := input; prefixes != nil {
  for _, prefix := range *prefixes {
    # ..
  }
}

at the moment - if input isn't returned (such as, for a resource created through an older API version) - this will crash (this check should fix that)

[dcaro]

@tombuildsstuff let me know if the change that I made here is what you are expecting.

[tombuildsstuff]

(same here)

[tombuildsstuff]

(same here)

[tombuildsstuff]

can we name this azurerm_virtual_network to match the other resource?

[dcaro]

of course, done :)

[tombuildsstuff]

can we rename this to dataSourceArmVirtualNetwork to match the other resource?

[dcaro]

Including suggested changes from @tombuildsstuff and added documentation.

[dcaro]

Make sense, will do so.

[dcaro]

of course, done :)

[dcaro]

@tombuildsstuff let me know if the change that I made here is what you are expecting.

[pmarques]

Use spaces to keep indentation

[pmarques]

Use spaces to keep indentation (same for next 3 lines)

[pmarques]

There are some indentation that could be fixed but this LGTM and I need this feature.
Can we merge this?

[dcaro]

@pmarques I have updated the code to include spaces instead of tabs. @tombuildsstuff let me know if you see any issues.

[tombuildsstuff]

just noticed there isn't a sidebar entry for this Data Source - I've pushed a commit to fix this :)

[tombuildsstuff]

Hey @dcaro

Thanks for pushing the latest changes - I've taken another look through and have left some comments inline, but this mostly LGTM. I've also pushed a commit to add the Data Source to the sidebar - however upon running the tests I noticed the Peering test failed:

$ acctests azurerm TestAccDataSourceArmVirtualNetwork_
=== RUN   TestAccDataSourceArmVirtualNetwork_basic
--- PASS: TestAccDataSourceArmVirtualNetwork_basic (77.07s)
=== RUN   TestAccDataSourceArmVirtualNetwork_peering
--- FAIL: TestAccDataSourceArmVirtualNetwork_peering (95.24s)
	testing.go:459: Step 0 error: Check failed: Check 3/3 error: data.azurerm_virtual_network.test: Attribute 'vnet_peerings.%' expected "1", got "0"
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-azurerm/azurerm	172.327s

If we can fix up the broken test this PR otherwise looks good to merge :)

Thanks!

[tombuildsstuff]

ping @dcaro - have you had a chance to look into this yet? :)

[dcaro]

@tombuildsstuff I have some issues with the second test. I traced the response from the REST API and I see that the peering is correctly provisioned when running the testAccDataSourceArmVirtualNetwork_peering function. The interpretation of the response is the issue. I'm trying to narrow down the function where this fails but so far, to me, it looks like an issue in terraform. Any help on this would be welcome.

[tombuildsstuff]

@dcaro sure - used the configuration from the failing test (and added some variables) to take a look into this:

resource "azurerm_resource_group" "test" {
  name     = "tharvey-devrg"
  location = "West Europe"
}

resource "azurerm_virtual_network" "test1" {
  name                = "tharvey-nw1"
  address_space       = ["10.0.1.0/24"]
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
}

resource "azurerm_virtual_network" "test2" {
  name                = "tharvey-nw2"
  address_space       = ["10.0.2.0/24"]
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
}

resource "azurerm_virtual_network_peering" "test1" {
  name                      = "peer-1to2"
  resource_group_name       = "${azurerm_resource_group.test.name}"
  virtual_network_name      = "${azurerm_virtual_network.test1.name}"
  remote_virtual_network_id = "${azurerm_virtual_network.test2.id}"
}

data "azurerm_virtual_network" "test" {
  resource_group_name = "${azurerm_resource_group.test.name}"
  name                = "${azurerm_virtual_network.test1.name}"
}

In order to understand what's going on, we can inspect what the Terraform Graph for this Terraform Configuration using the command terraform graph | dot -Tpng > graph.png - which in this case generates:

Given the azurerm_virtual_network_peering resource isn't referenced within the azurerm_virtual_network data source (known as an Implicit Dependency) - Terraform believes it should be able to execute these in Parallel, as shown in the graph (since there's no lines between them) - where it's possible the VNet Peering may not exist.

We can use the depends_upon graph to specify an Explicit Dependency as shown in the configuration below:

resource "azurerm_resource_group" "test" {
  name     = "tharvey-devrg"
  location = "West Europe"
}

resource "azurerm_virtual_network" "test1" {
  name                = "tharvey-nw1"
  address_space       = ["10.0.1.0/24"]
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
}

resource "azurerm_virtual_network" "test2" {
  name                = "tharvey-nw2"
  address_space       = ["10.0.2.0/24"]
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
}

resource "azurerm_virtual_network_peering" "test1" {
  name                      = "peer-1to2"
  resource_group_name       = "${azurerm_resource_group.test.name}"
  virtual_network_name      = "${azurerm_virtual_network.test1.name}"
  remote_virtual_network_id = "${azurerm_virtual_network.test2.id}"
}

data "azurerm_virtual_network" "test" {
  resource_group_name = "${azurerm_resource_group.test.name}"
  name                = "${azurerm_virtual_network.test1.name}"
  depends_on          = ["azurerm_virtual_network_peering.test1"]
}

The Terraform Graph for this looks like so:

which means that the vnet_peerings are included in the Data Source (since they exist at this point). More information on Implicit and Explicit Dependencies is available on this page.

I've confirmed that props.VirtualNetworkPeerings has a value at this point too - so I'm hoping that should be helpful? :)

Thanks!

[dcaro]

Thanks so much @tombuildsstuff I will update my PR today !

[dcaro]

@tombuildsstuff when I introduce the dependency, I have the following error when I run the test:
API server listening at: 127.0.0.1:50704
testing.go:459: Step 0 error: After applying this step and refreshing, the plan was not empty:
DIFF:

	CREATE: data.azurerm_virtual_network.test
	  address_spaces.#:    "" => "<computed>"
	  dns_servers.#:       "" => "<computed>"
	  name:                "" => "acctestvnet-1-4667103784115642641"
	  resource_group_name: "" => "acctest4667103784115642641-rg"
	  subnets.#:           "" => "<computed>"
	  vnet_peerings.%:     "" => "<computed>"
	
	STATE:
	
	azurerm_resource_group.test:

[properties of the resource group]

            azurerm_virtual_network.test1:

[properties of the virtual network]

	Dependencies:
	    azurerm_resource_group.test

	azurerm_virtual_network.test2:

[properties of the virtual network]

           Dependencies:
	    azurerm_resource_group.test
	azurerm_virtual_network_peering.test1:

[properties of the virtual network peering]

	  Dependencies:
	    azurerm_resource_group.test
	    azurerm_virtual_network.test1
	    azurerm_virtual_network.test2

[tombuildsstuff]

LGTM - thanks for this 👍

[tombuildsstuff]

@dcaro so after digging into this, it appears that a data source with a depends_on will be re-triggered whenever a plan is attempted. Whilst it's possible to work around this by referencing the virtual network name via ${azurerm_virtual_network_peering.test1.virtual_network_name} - I instead figured it better to split this out into two separate test-steps as shown below:

func TestAccDataSourceArmVirtualNetwork_peering(t *testing.T) {
	dataSourceName := "data.azurerm_virtual_network.test"
	ri := acctest.RandInt()

	name_vnet_1 := fmt.Sprintf("acctestvnet-1-%d", ri)
	location := testLocation()

	resource.Test(t, resource.TestCase{
		PreCheck:  func() { testAccPreCheck(t) },
		Providers: testAccProviders,
		Steps: []resource.TestStep{
			{
				Config: testAccDataSourceArmVirtualNetwork_peering(ri, location),
			},
			{
				Config: testAccDataSourceArmVirtualNetwork_peeringWithDataSource(ri, location),
				Check: resource.ComposeTestCheckFunc(
					resource.TestCheckResourceAttr(dataSourceName, "name", name_vnet_1),
					resource.TestCheckResourceAttr(dataSourceName, "address_spaces.0", "10.0.1.0/24"),
					resource.TestCheckResourceAttr(dataSourceName, "vnet_peerings.%", "1"),
				),
			},
		},
	})
}

I've pushed a commit to add support for this and this Data Source now LGTM - thanks for this :)

Tests pass:

$ acctests azurerm TestAccDataSourceArmVirtualNetwork_
=== RUN   TestAccDataSourceArmVirtualNetwork_basic
--- PASS: TestAccDataSourceArmVirtualNetwork_basic (76.72s)
=== RUN   TestAccDataSourceArmVirtualNetwork_peering
--- PASS: TestAccDataSourceArmVirtualNetwork_peering (102.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	179.391s

Thanks!

[pmarques]

@tombuildsstuff do you have any details about the next release date? I would like to have this available without compiling my own.

[tombuildsstuff]

@pmarques "soon" - it'll be next week, but I'd rather not commit to a specific day/date - hope that helps :)

[ghost]

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!