New Resource: azurerm_sql_virtual_network_rule

[ShepardToTheStars]

Summary

New resource that adds a SQL server to a subnet. (Azure Docs, relevant Azure GO SDK client)

Issue

#955

TODO

(From the main terraform's contrib guide)

• Minimal LOC
• Resource
  • Read
  • Create
  • Update
  • Delete
• Resource Acceptance Test - SQL Endpoint Validation Enabled and Disabled
• Resource Acceptance Test - Basic Provisioning and Teardown
• Resource Acceptance Test - Multiple Subnets
• Resource Acceptance Test - Update Subnets (without forcing new resource)
• Validation Function Test - (Barely) Valid Rule Names
• Validation Function Test - Invalid Rule Names
• Import Acceptance Test
• Documentation
• Well-Formed Code - used other clients as a reference, let me know if it isn't well-formed! :)
• Requested Changes
  • Update to New Registration Syntax
  • Rule Name Validation
  • Create test case for updating subnets
  • Check for nil in resp object
  • Change virtual_network_subnet_id to subnet_id
  • Delete: Check for 404 from future objects
  • Remove Commented-Out Code
  • Fix Spacing in Test Case Configurations
  • Add Check/Waiter function for Create

[tombuildsstuff]

hey @vlouwagie

Thanks for this PR :)

I've taken a look through and this is looking pretty good; however I'm wondering is it possible to have a SQL Server connected to multiple Virtual Networks? If it's not - I'm wondering if it'd make sense to add a virtual_network block to the existing azurerm_sql_server resource and limit it to a single connection (so that it's possible to create/update the connection within that resource) - what do you think?

Questions

1. Should I include the import resource with this pull request, or as a separate feature/pull request?

If you can add it to this PR that'd be great - but I'd suggest holding off for the moment until we've worked out if this'd be better located within the azurerm_sql_server resource

Thanks!

[ShepardToTheStars]

@tombuildsstuff Yep, it is possible for a single SQL server to be connected to one or more subnets from one or more virtual networks. This resource has a lot in common with the SQL firewall rule resource, so I originally modelled it after that.

Template for multiple rules

resource "azurerm_resource_group" "test" {
    name = "acctestRG_1"
    location = "westus"
}
#Vnet 1
resource "azurerm_virtual_network" "vnet1" {
  name                = "acctestvnet1"
  address_space       = ["10.7.29.0/24"]
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
}
#Vnet 2
resource "azurerm_virtual_network" "vnet2" {
  name                = "acctestvnet2"
  address_space       = ["10.1.29.0/29"]
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
}
#Subnet 1
resource "azurerm_subnet" "vnet1subnet1" {
  name = "acctestsubnet1"
  resource_group_name = "${azurerm_resource_group.test.name}"
  virtual_network_name = "${azurerm_virtual_network.vnet1.name}"
  address_prefix = "10.7.29.0/29"
  service_endpoints = ["Microsoft.Sql"]
}
#Subnet 2 (same vnet)
resource "azurerm_subnet" "vnet1subnet2" {
  name = "acctestsubnet2"
  resource_group_name = "${azurerm_resource_group.test.name}"
  virtual_network_name = "${azurerm_virtual_network.vnet1.name}"
  address_prefix = "10.7.29.128/29"
  service_endpoints = ["Microsoft.Sql"]
}
#Subnet 3 (different vnet)
resource "azurerm_subnet" "vnet2subnet1" {
  name = "acctestsubnet3"
  resource_group_name = "${azurerm_resource_group.test.name}"
  virtual_network_name = "${azurerm_virtual_network.vnet2.name}"
  address_prefix = "10.1.29.0/29"
  service_endpoints = ["Microsoft.Sql"]
}
#Sql Server
resource "azurerm_sql_server" "test" {
    name = "acctestsqlserver1"
    resource_group_name = "${azurerm_resource_group.test.name}"
    location = "${azurerm_resource_group.test.location}"
    version = "12.0"
    administrator_login = "missadmin"
    administrator_login_password = "${md5(1)}!"
}
#Rules
resource "azurerm_sql_virtual_network_rule" "test" {
    name = "acctestsqlvnetrule1"
    resource_group_name = "${azurerm_resource_group.test.name}"
    server_name = "${azurerm_sql_server.test.name}"
    virtual_network_subnet_id = "${azurerm_subnet.vnet1subnet1.id}"
    ignore_missing_vnet_service_endpoint = true
}
resource "azurerm_sql_virtual_network_rule" "test2" {
    name = "acctestsqlvnetrule2"
    resource_group_name = "${azurerm_resource_group.test.name}"
    server_name = "${azurerm_sql_server.test.name}"
    virtual_network_subnet_id = "${azurerm_subnet.vnet1subnet2.id}"
    ignore_missing_vnet_service_endpoint = false
}
resource "azurerm_sql_virtual_network_rule" "test3" {
    name = "acctestsqlvnetrule3"
    resource_group_name = "${azurerm_resource_group.test.name}"
    server_name = "${azurerm_sql_server.test.name}"
    virtual_network_subnet_id = "${azurerm_subnet.vnet2subnet1.id}"
    ignore_missing_vnet_service_endpoint = false
}

[tombuildsstuff]

@tombuildsstuff Yep, it is possible for a single SQL server to be connected to one or more subnets from one or more virtual networks. This resource has a lot in common with the SQL firewall rule resource, so I originally modelled it after that.

@vlouwagie cool, thanks for confirming that. Could we add a test to cover that scenario, perhaps using the configuration you've posted above?

Thanks!

[ShepardToTheStars]

Could we add a test to cover that scenario, perhaps using the configuration you've posted above?

@tombuildsstuff Sure! I will add a task for that in the pull request description.

[tombuildsstuff]

hey @vlouwagie

Thanks for this PR - I've taken a look through and left some minor comments but this mostly LGTM; if we can fix up the minor comments and add some documentation this should be good to merge :)

Thanks!

[tombuildsstuff]

minor can we switch this over to using the new registration syntax? e.g. `c.configureClient(&sqlVNRClient.Client, auth)

[ShepardToTheStars]

✅

[tombuildsstuff]

👍

[ShepardToTheStars]

✅

[tombuildsstuff]

should this be ForceNew or is it possible to migrate between Virtual Networks? Can we add a test for this if that's the case

[ShepardToTheStars]

@tombuildsstuff It does indeed look like you can change the subnets of a rule, and it does apply correctly. I will make a task to create a test case for that. 😄

On a side note, I also tested migrating from one SQL server to another without ForceNew (just in case) , and it creates a rule on the 2nd target SQL server, but does not delete it on the first server, so the ForceNew attribute is needed on the server_name.

[ShepardToTheStars]

✅

[tombuildsstuff]

for these two fields can we use the field in resp.[XXX]Properties and nil-check it (e.g. to handle bad API responses); since that's what's recommended by Azure e.g.

if props := resp.[XXX]Properties; props != nil {
  d.Set("virtual_network_subnet_id", props.VirtualNetworkSubnetID)
  d.Set("ignore_missing_vnet_service_endpoint", props.IgnoreMissingVnetServiceEndpoint)
}

[ShepardToTheStars]

✅

[tombuildsstuff]

🤔 do you think it'd be worth making this subnet_id to be more consistent with the other Terraform resources?

[ShepardToTheStars]

@tombuildsstuff Yes, that definitely makes sense.

On a similar note, what is your opinion on ignore_missing_vnet_service_endpoint? Do you think it would be better if it was shorted a bit as well to ignore_missing_service_endpoint or even ignore_missing_endpoint?

[tombuildsstuff]

given the description (in the API Docs): Create firewall rule before the virtual network has vnet service endpoint enabled. - I guess leaving it as ignore_missing_vnet_service_endpoint would probably make the most sense? Whilst I don't feel that name's particularly descriptive, it's how the Azure API's chosen to label it, so it probably makes sense to match it incase it does extra stuff in the future

[ShepardToTheStars]

✅

[tombuildsstuff]

minor could we make the spacing consistent here with 2 spaces for indents?

[ShepardToTheStars]

✅

[tombuildsstuff]

minor could we make the spacing consistent here with 2 spaces for indents?

[ShepardToTheStars]

✅

[tombuildsstuff]

minor could we make the spacing consistent here with 2 spaces for indents?

[ShepardToTheStars]

✅

[tombuildsstuff]

minor could we make the spacing consistent here with 2 spaces for indents?

[ShepardToTheStars]

✅

[tombuildsstuff]

minor could we make the spacing consistent here with 2 spaces for indents?

[ShepardToTheStars]

✅

[ShepardToTheStars]

Thanks for the feedback! I will investigate each one and get back to you. Also added the requested changes to the pull request description. 😄

[ShepardToTheStars]

@tombuildsstuff I ended up making a regex match test that checks if the subnet ID contains one subnet name but not the other in the preConfig setup, and then check for the inverse on the postConfig setup. I wasn't sure if there was a better way to check if a computed item was a different value from the previous step.

It still works, but let me know if there actually is a better way. 😄

[tombuildsstuff]

that works, the other options would be building up the ID by hand (e.g. /subscriptions/0000../foo/bar/subnets/mySubnet) or just check the suffix is mySubnet - but this LGTM :)

[ShepardToTheStars]

or just check the suffix is mySubnet

Sounds good. This is basically what I'm doing anyways, but with a little extra!

[tombuildsstuff]

to clarify this, this is the main code path - as the deletion of the SQL Network Rule's should be a long running operation (LRO); afaik :)

[ShepardToTheStars]

@tombuildsstuff Could you elaborate a bit on this comment?

[ShepardToTheStars]

@tombuildsstuff I added the endpoint state as a computed fields since that is also available from the API. I also added the documentation. I will be removing the WIP as all tasks (as of now) are complete and ready to be reviewed! 😄

[tombuildsstuff]

hey @vlouwagie

Thanks for pushing the updates - this now looks good to me. In running the tests I've noticed there appears to be an eventual consistency issue when creating this, where the API returns this as created but it doesn't exist until a few seconds later, which is causing the tests to fail intermittently:

------- Stdout: -------
=== RUN   TestAccAzureRMSqlVirtualNetworkRule_multipleSubnets
--- FAIL: TestAccAzureRMSqlVirtualNetworkRule_multipleSubnets (162.67s)
    testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
        
        * azurerm_sql_virtual_network_rule.rule3: 1 error(s) occurred:
        
        * azurerm_sql_virtual_network_rule.rule3: Error retrieving SQL Virtual Network Rule: sql.VirtualNetworkRulesClient#Get: Failure responding to request: StatusCode=404 -- Original Error: autorest/azure: Service returned an error. Status=404 Code="ResourceNotFound" Message="The requested resource of type 'Microsoft.Sql/servers/virtualNetworkRules' with name 'acctestsqlvnetrule34331446545651011683' was not found."
FAIL

We should be able to add a check/waiter function once the Network Rule's been created before retrieving it to ensure that this actually exists - something like this (modified from the IoTHub resource to add the ContinuousTargetOccurence attribute, which ensures the successful state is returned N times before marking it as successful):

func waitForIotHubToBeDeleted(ctx context.Context, client devices.IotHubResourceClient, resourceGroup, name string) error {
	stateConf := &resource.StateChangeConf{
		Pending: []string{"200"},
		Target:  []string{"404"},
		Refresh: iothubStateStatusCodeRefreshFunc(ctx, client, resourceGroup, name),
		Timeout: 40 * time.Minute,
		ContinuousTargetOccurence: 5,
	}
	if _, err := stateConf.WaitForState(); err != nil {
		return fmt.Errorf("Error waiting for IotHub (%q in Resource Group %q) to be deleted: %+v", name, resourceGroup, err)
	}

	return nil
}

If we can add this check function that should solve the eventual consistency issue and then this should be good to merge :)

Thanks!

[ShepardToTheStars]

@tombuildsstuff I've been able to fail the tests myself as well. I think I might go one step further with your example, and check the provisioning state of the create/updated rule, similar to how the resource_arm_redis_cache does it, since it's also checking state on create, rather than delete.

resource_arm_redis_cache.go (Lines 252, 463)

log.Printf("[DEBUG] Waiting for Redis Instance (%s) to become available", d.Get("name"))
stateConf := &resource.StateChangeConf{
	Pending:    []string{"Updating", "Creating"},
	Target:     []string{"Succeeded"},
	Refresh:    redisStateRefreshFunc(ctx, client, resGroup, name),
	Timeout:    60 * time.Minute,
	MinTimeout: 15 * time.Second,
}
if _, err := stateConf.WaitForState(); err != nil {
	return fmt.Errorf("Error waiting for Redis Instance (%s) to become available: %s", d.Get("name"), err)
}

func redisStateRefreshFunc(ctx context.Context, client redis.Client, resourceGroupName string, sgName string) resource.StateRefreshFunc {
	return func() (interface{}, string, error) {
		res, err := client.Get(ctx, resourceGroupName, sgName)
		if err != nil {
			return nil, "", fmt.Errorf("Error issuing read request in redisStateRefreshFunc to Azure ARM for Redis Cache Instance '%s' (RG: '%s'): %s", sgName, resourceGroupName, err)
		}

		return res, *res.ProvisioningState, nil
	}
}

What do you think of this instead?

[tombuildsstuff]

@vlouwagie yeah, sorry that's what I meant :)

The only thing I'd suggest adding to that would be a ContinuousTargetOccurence: 5 (as in my example above) to ensure that the Virtual Network Rule is returned 5 times in a row before returning; otherwise the function will return as soon as it exists a single time (which won't be the case if it's eventually consistent)

Thanks!

[ShepardToTheStars]

@tombuildsstuff I increased the ContinuousTargetOccurence attribute to 5.

Let me know if there is anything else I can do for this. 😄 @

[tombuildsstuff]

hey @vlouwagie

Thanks for pushing the latest updates - this now LGTM and the tests pass 👍

I'm going to push one commit to remove the endpoint_state field, since I don't believe it's necessary (we don't expose this on the other resources, for instance) - and then we can merge this :)

Thanks!

[tombuildsstuff]

I'm going to push a commit to remove this field, since it shouldn't be being used by end-users

[tombuildsstuff]

(done)

[ShepardToTheStars]

@tombuildsstuff I suppose that field is not necessary, even more now since we added a check/wait function for the ready status.

[tombuildsstuff]

@vlouwagie yeah - we can always expose it if there's a use-case for it, but in general Terraform should only be returning from the Create method when it's ready/replicated (as in this case 😄). Thanks for this PR - this'll go out in v1.3.1 later today :)

[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 👉 hashibot-feedback@hashicorp.com. Thanks!