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

New Resource: azurerm_sql_virtual_network_rule #978

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
70d742e
Added SQL Vnet Rule
Mar 13, 2018
d02ef1a
Initial working implemtation of arm_sql_virtual_network_rule
Mar 13, 2018
35cb61e
Added initial virtual network rule test.
Mar 14, 2018
d7d4f88
Fixed SQL test cases for SQL Vnet rule.
Mar 14, 2018
8e06967
Fixed SQL Virtual Network Rule tests to run correctly.
Mar 14, 2018
0888af6
Added additional test cases and expected output for SQL Vnet rule end…
Mar 14, 2018
46aaeb2
Added Import Test for SQL Vnet Rule.
Mar 14, 2018
8a15347
Added additional test case for provisioning multiple SQL vnet rules
Mar 14, 2018
906b2f2
Fixed formatting and comments in vnet sql rule tests.
Mar 15, 2018
d4ae4fd
Updated config entry to new registration syntax
Mar 16, 2018
3c9bbc2
Updated subnet variable name to match other resources
Mar 16, 2018
d8fa04b
Added SQL vnet rule name validation and tests
Mar 16, 2018
8b7d306
Added additional error handling to the SQL vnet rule delete functions.
Mar 16, 2018
40a50bc
Added nil check for response properties to SQL vnet rule.
Mar 16, 2018
9517eb1
Reordered SQL vnet rule name validation for error message clarity
Mar 16, 2018
1aaf309
Added SQL vnet rule test for switching subnets
Mar 16, 2018
193e991
Added additional validation function acceptance test for SQL vnet rule
Mar 17, 2018
712de07
Fixed SQL Vnet Rule test case and validation logic. Added more consis…
Mar 17, 2018
0c267e9
Added initial rough draft of documentation
Mar 17, 2018
d287702
Added SQL vnet rule endpoint_state as a computed field
Mar 17, 2018
c9350bc
Revised part of the SQL vnet rule documentation.
Mar 18, 2018
299731e
Added check/wait status on create or update of a SQL virtual network …
Mar 24, 2018
b6c69da
Increased SQL Vnet Rule Create method's ContinuousTargetOccurence pro…
Mar 27, 2018
efd737a
Removing the `endpoint_state` field
tombuildsstuff Mar 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions azurerm/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ type ArmClient struct {
sqlFirewallRulesClient sql.FirewallRulesClient
sqlServersClient sql.ServersClient
sqlServerAzureADAdministratorsClient sql.ServerAzureADAdministratorsClient
sqlVirtualNetworkRulesClient sql.VirtualNetworkRulesClient

// KeyVault
keyVaultClient keyvault.VaultsClient
Expand Down Expand Up @@ -595,6 +596,13 @@ func (c *ArmClient) registerDatabases(endpoint, subscriptionId string, auth auto
sqlADClient.Sender = sender
sqlADClient.SkipResourceProviderRegistration = c.skipProviderRegistration
c.sqlServerAzureADAdministratorsClient = sqlADClient

sqlVNRClient := sql.NewVirtualNetworkRulesClientWithBaseURI(endpoint, subscriptionId)
setUserAgent(&sqlVNRClient.Client)
sqlVNRClient.Authorizer = auth
sqlVNRClient.Sender = sender
sqlVNRClient.SkipResourceProviderRegistration = c.skipProviderRegistration
c.sqlVirtualNetworkRulesClient = sqlVNRClient
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ShepardToTheStars ShepardToTheStars Mar 16, 2018

Choose a reason for hiding this comment

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

}

func (c *ArmClient) registerDNSClients(endpoint, subscriptionId string, auth autorest.Authorizer, sender autorest.Sender) {
Expand Down
31 changes: 31 additions & 0 deletions azurerm/import_arm_sql_virtual_network_rule_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package azurerm

import (
"testing"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAzureRMSqlVirtualNetworkRule_importBasic(t *testing.T) {
resourceName := "azurerm_sql_virtual_network_rule.test"

ri := acctest.RandInt()
config := testAccAzureRMSqlVirtualNetworkRule_basic(ri, testLocation())

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMSqlVirtualNetworkRuleDestroy,
Steps: []resource.TestStep{
{
Config: config,
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}
1 change: 1 addition & 0 deletions azurerm/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ func Provider() terraform.ResourceProvider {
"azurerm_sql_firewall_rule": resourceArmSqlFirewallRule(),
"azurerm_sql_active_directory_administrator": resourceArmSqlAdministrator(),
"azurerm_sql_server": resourceArmSqlServer(),
"azurerm_sql_virtual_network_rule": resourceArmSqlVirtualNetworkRule(),
"azurerm_storage_account": resourceArmStorageAccount(),
"azurerm_storage_blob": resourceArmStorageBlob(),
"azurerm_storage_container": resourceArmStorageContainer(),
Expand Down
141 changes: 141 additions & 0 deletions azurerm/resource_arm_sql_virtual_network_rule.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package azurerm

import (
"fmt"
"log"

"github.com/Azure/azure-sdk-for-go/services/sql/mgmt/2015-05-01-preview/sql"
"github.com/hashicorp/terraform/helper/schema"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
)

func resourceArmSqlVirtualNetworkRule() *schema.Resource {
return &schema.Resource{
Create: resourceArmSqlVirtualNetworkRuleCreateUpdate,
Read: resourceArmSqlVirtualNetworkRuleRead,
Update: resourceArmSqlVirtualNetworkRuleCreateUpdate,
Delete: resourceArmSqlVirtualNetworkRuleDelete,
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},

Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
//TODO: Validation (invalid if non-numeric characters)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

},

"resource_group_name": resourceGroupNameSchema(),

"server_name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},

"virtual_network_subnet_id": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ShepardToTheStars ShepardToTheStars Mar 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

},

"ignore_missing_vnet_service_endpoint": {
Type: schema.TypeBool,
Optional: true,
Default: false, //UI Default is false
},
},
}
}

func resourceArmSqlVirtualNetworkRuleCreateUpdate(d *schema.ResourceData, meta interface{}) error {
client := meta.(*ArmClient).sqlVirtualNetworkRulesClient
ctx := meta.(*ArmClient).StopContext

name := d.Get("name").(string)
serverName := d.Get("server_name").(string)
resourceGroup := d.Get("resource_group_name").(string)
virtualNetworkSubnetId := d.Get("virtual_network_subnet_id").(string)
ignoreMissingVnetServiceEndpoint := d.Get("ignore_missing_vnet_service_endpoint").(bool)

parameters := sql.VirtualNetworkRule{
VirtualNetworkRuleProperties: &sql.VirtualNetworkRuleProperties{
VirtualNetworkSubnetID: utils.String(virtualNetworkSubnetId),
IgnoreMissingVnetServiceEndpoint: utils.Bool(ignoreMissingVnetServiceEndpoint),
},
}

_, err := client.CreateOrUpdate(ctx, resourceGroup, serverName, name, parameters)
if err != nil {
return fmt.Errorf("Error creating SQL Virtual Network Rule: %+v", err)
}

resp, err := client.Get(ctx, resourceGroup, serverName, name)
if err != nil {
return fmt.Errorf("Error retrieving SQL Virtual Network Rule: %+v", err)
}

d.SetId(*resp.ID)

return resourceArmSqlVirtualNetworkRuleRead(d, meta)
}

func resourceArmSqlVirtualNetworkRuleRead(d *schema.ResourceData, meta interface{}) error {
client := meta.(*ArmClient).sqlVirtualNetworkRulesClient
ctx := meta.(*ArmClient).StopContext

id, err := parseAzureResourceID(d.Id())
if err != nil {
return err
}

resourceGroup := id.ResourceGroup
serverName := id.Path["servers"]
name := id.Path["virtualNetworkRules"]

resp, err := client.Get(ctx, resourceGroup, serverName, name)
if err != nil {
if utils.ResponseWasNotFound(resp.Response) {
log.Printf("[INFO] Error reading SQL Virtual Network Rule %q - removing from state", d.Id())
d.SetId("")
return nil
}

return fmt.Errorf("Error reading SQL Virtual Network Rule: %+v", err)
}

d.Set("name", resp.Name)
d.Set("resource_group_name", resourceGroup)
d.Set("server_name", serverName)
d.Set("virtual_network_subnet_id", resp.VirtualNetworkSubnetID)
d.Set("ignore_missing_vnet_service_endpoint", resp.IgnoreMissingVnetServiceEndpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor Author

@ShepardToTheStars ShepardToTheStars Mar 16, 2018

Choose a reason for hiding this comment

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


return nil
}

func resourceArmSqlVirtualNetworkRuleDelete(d *schema.ResourceData, meta interface{}) error {
client := meta.(*ArmClient).sqlVirtualNetworkRulesClient
ctx := meta.(*ArmClient).StopContext

id, err := parseAzureResourceID(d.Id())
if err != nil {
return err
}

resourceGroup := id.ResourceGroup
serverName := id.Path["servers"]
name := id.Path["virtualNetworkRules"]

future, err := client.Delete(ctx, resourceGroup, serverName, name)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

checking the SDK - it appears that 404 will return an error here; do we want to check for that? e.g.

if err != nil {
  if response.WasNotFound(future.Response()) {
    return nil
  }
  return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tombuildsstuff If the client.Delete function errors while returning, the future variable will still contain a valid future then, correct? (SDK Delete)

Copy link
Contributor

Choose a reason for hiding this comment

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

it'll contain the HTTP response, but not necessarily a valid future (since a 500 for example won't contain the headers needed for a future, at least from my understanding)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return fmt.Errorf("Error deleting SQL Virtual Network Rule: %+v", err)
}

err = future.WaitForCompletion(ctx, client.Client)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

checking the SDK - it appears that 404 will return an error here; do we want to check for that? e.g.

if err != nil {
  if response.WasNotFound(future.Response()) {
    return nil
  }
  return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

return nil
}
Loading