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_postgresql_virtual_network_rule #1774

Merged
merged 20 commits into from
Sep 4, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
1657d1b
Add the basics of postgresql vnet rule.
ac-astuartkregor Aug 15, 2018
5e640de
Bring across postgresql_server resource from postgresql_server_test.
ac-astuartkregor Aug 15, 2018
b0205eb
Add doc page for postgresql_virtual_network_rule.
ac-astuartkregor Aug 15, 2018
2f13ae8
Add import test for postgresql_virtual_network_rule.
ac-astuartkregor Aug 15, 2018
3db91e6
Make postgresql_virtual_network_rule example similar to postgresql_se…
ac-astuartkregor Aug 15, 2018
f356130
Fix resource ID for postgresql_virtual_network_rule import docs.
ac-astuartkregor Aug 15, 2018
0ad24c4
Add postgresql_virtual_network_rule to docs sidebar.
ac-astuartkregor Aug 15, 2018
a85e3a2
Add a client for postgresqlVirtualNetworkRules.
ac-astuartkregor Aug 15, 2018
692306f
Fix text in anchor for azurerm_postgresql_virtual_network_rule.
ac-astuartkregor Aug 15, 2018
0f333d5
Add non-zero value validation for server_name and subnet_id.
ac-astuartkregor Aug 16, 2018
be87178
(postgresql_virtual_network_rule) Adjust positioning of advice around…
ac-astuartkregor Aug 16, 2018
19d52a9
Remove currently unused import for postgresql_virtual_network_rule.
ac-astuartkregor Aug 16, 2018
2eb65c1
Fix validator function names for postgresql_virtual_network_rule.
ac-astuartkregor Aug 16, 2018
81aeb01
postgresql_virtual_network_rule Use Azure resource ID validator for s…
ac-astuartkregor Aug 20, 2018
f0ebf93
postgresql_vnet_rule Acceptance tests mostly working, ignore missing …
ac-astuartkregor Aug 21, 2018
7c91f83
postgresql vnet rule - skip failing test because of API issue.
ac-astuartkregor Aug 23, 2018
f830ec8
postgresql vnet rule remove ignore_missing_vnet_service_endpoint test.
ac-astuartkregor Aug 23, 2018
e32e5c5
Postgresql vnet rule - remove `ignore_missing_vnet_service_endpoint` …
ac-astuartkregor Aug 29, 2018
caf3cdb
postgresql vnet rule - default `IgnoreMissingVnetServiceEndpoint` to …
ac-astuartkregor Aug 30, 2018
0b0b3dd
Checking to ensure the Subnet's configured correctly so the Virtual N…
tombuildsstuff Sep 3, 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
5 changes: 5 additions & 0 deletions azurerm/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ type ArmClient struct {
postgresqlDatabasesClient postgresql.DatabasesClient
postgresqlFirewallRulesClient postgresql.FirewallRulesClient
postgresqlServersClient postgresql.ServersClient
postgresqlVirtualNetworkRulesClient postgresql.VirtualNetworkRulesClient
sqlDatabasesClient sql.DatabasesClient
sqlElasticPoolsClient sql.ElasticPoolsClient
sqlFirewallRulesClient sql.FirewallRulesClient
Expand Down Expand Up @@ -644,6 +645,10 @@ func (c *ArmClient) registerDatabases(endpoint, subscriptionId string, auth auto
c.configureClient(&postgresqlSrvClient.Client, auth)
c.postgresqlServersClient = postgresqlSrvClient

postgresqlVNRClient := postgresql.NewVirtualNetworkRulesClientWithBaseURI(endpoint, subscriptionId)
c.configureClient(&postgresqlVNRClient.Client, auth)
c.postgresqlVirtualNetworkRulesClient = postgresqlVNRClient

// SQL Azure
sqlDBClient := sql.NewDatabasesClientWithBaseURI(endpoint, subscriptionId)
setUserAgent(&sqlDBClient.Client)
Expand Down
31 changes: 31 additions & 0 deletions azurerm/import_arm_postgresql_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 TestAccAzureRMPostgreSQLVirtualNetworkRule_importBasic(t *testing.T) {
resourceName := "azurerm_postgresql_virtual_network_rule.test"

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

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMPostgreSQLVirtualNetworkRuleDestroy,
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 @@ -216,6 +216,7 @@ func Provider() terraform.ResourceProvider {
"azurerm_postgresql_database": resourceArmPostgreSQLDatabase(),
"azurerm_postgresql_firewall_rule": resourceArmPostgreSQLFirewallRule(),
"azurerm_postgresql_server": resourceArmPostgreSQLServer(),
"azurerm_postgresql_virtual_network_rule": resourceArmPostgreSQLVirtualNetworkRule(),
"azurerm_public_ip": resourceArmPublicIp(),
"azurerm_relay_namespace": resourceArmRelayNamespace(),
"azurerm_recovery_services_vault": resourceArmRecoveryServicesVault(),
Expand Down
247 changes: 247 additions & 0 deletions azurerm/resource_arm_postgresql_virtual_network_rule.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
package azurerm

import (
"context"
"fmt"
"log"
"regexp"
"time"

"github.com/Azure/azure-sdk-for-go/services/postgresql/mgmt/2017-12-01/postgresql"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add the azure import to get access to the helper validation methods?

"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"

Copy link
Contributor Author

@ac-astuartkregor ac-astuartkregor Aug 16, 2018

Choose a reason for hiding this comment

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

Would this be under the circumstances where the validation function is pulled out of this resource and made more generally available?

Edit: Sorry I misread your previous comment and saw that the validation function was azure specific.

)

func resourceArmPostgreSQLVirtualNetworkRule() *schema.Resource {
return &schema.Resource{
Create: resourceArmPostgreSQLVirtualNetworkRuleCreateUpdate,
Read: resourceArmPostgreSQLVirtualNetworkRuleRead,
Update: resourceArmPostgreSQLVirtualNetworkRuleCreateUpdate,
Delete: resourceArmPostgreSQLVirtualNetworkRuleDelete,
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},

Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validatePostgreSQLVirtualNetworkRuleName,
},

"resource_group_name": resourceGroupNameSchema(),

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

"subnet_id": {
Type: schema.TypeString,
Required: true,
ValidateFunc: azure.ValidateResourceID,
},
},
}
}

func resourceArmPostgreSQLVirtualNetworkRuleCreateUpdate(d *schema.ResourceData, meta interface{}) error {
client := meta.(*ArmClient).postgresqlVirtualNetworkRulesClient
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("subnet_id").(string)

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

_, err := client.CreateOrUpdate(ctx, resourceGroup, serverName, name, parameters)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we combine these into a single line?

if _, err := client.CreateOrUpdate(ctx, resourceGroup, serverName, name, parameters); err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand the motivation for combining these lines, could you please advise?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The err variable is scoped only to the if block that handles it, and I believe that the code is a little easier to read this way as well. Since the err variable is not needed beyond the scope of the check it makes sense to combine the lines into a single line if check.

Copy link
Contributor Author

@ac-astuartkregor ac-astuartkregor Aug 17, 2018

Choose a reason for hiding this comment

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

Thanks for clarifying. I suspect I do not feel as strongly on this issue as you do, so I'm happy to change it. I'm not a seasoned Go programmer, so I am not particularly used to its syntax or conventions and as such, the current code (split across 2 lines) is a lot easier for me to read as opposed to combining the lines. However, because of that, I don't know how go is 'supposed' to look either. I don't mind changing it, however I suspect less experienced people will find it a bit more awkward (I'm biased, I know).

In terms of scoping, that's not something I'd have thought of. Does Go give particularly strong emphasis to scope beyond the expectation of dealing with it? Are there expected patterns for scopes and so on? Sorry for all these questions but you've piqued my curiosity. Am I correct in guessing that if the if comes before the variable declaration, that the variable is then scoped to that and only that block/line?

Edit: It would also help if I fully read the code myself 😄 The err is used later on line 83 and is needed outside of the scope of the if block.

Edit: The tests are what clued me into the extra use of err.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not going to push this issue because this is the way the original resource was coded and it's not your fault. However, the use of err on line 84 is pointless because if the code gets to this point the err will always be nil. I would put the if check in a single line and remove the err variable from the log output completely.

log.Printf("[DEBUG] Waiting for PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q) to become ready.", name, serverName, resourceGroup)

Yes Go is big on scope and making the code as short and compact as possible. Go doesn't like to have variables hanging around any longer than they absolutely have too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ Good point, I totally hadn't considered that, my bad.

Ok, thanks for the input, I'll keep that in mind.

return fmt.Errorf("Error creating PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q): %+v", name, serverName, resourceGroup, err)
}

//Wait for the provisioning state to become ready
log.Printf("[DEBUG] Waiting for PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q) to become ready: %+v", name, serverName, resourceGroup, err)
stateConf := &resource.StateChangeConf{
Pending: []string{"Initializing", "InProgress", "Unknown", "ResponseNotFound"},
Target: []string{"Ready"},
Refresh: postgreSQLVirtualNetworkStateStatusCodeRefreshFunc(ctx, client, resourceGroup, serverName, name),
Timeout: 10 * time.Minute,
MinTimeout: 1 * time.Minute,
ContinuousTargetOccurence: 5,
}

if _, err := stateConf.WaitForState(); err != nil {
return fmt.Errorf("Error waiting for PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q) to be created or updated: %+v", name, serverName, resourceGroup, err)
}

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

d.SetId(*resp.ID)

return resourceArmPostgreSQLVirtualNetworkRuleRead(d, meta)
}

func resourceArmPostgreSQLVirtualNetworkRuleRead(d *schema.ResourceData, meta interface{}) error {
client := meta.(*ArmClient).postgresqlVirtualNetworkRulesClient
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 PostgreSQL Virtual Network Rule %q - removing from state", d.Id())
d.SetId("")
return nil
}

return fmt.Errorf("Error reading PostgreSQL Virtual Network Rule: %q (PostgreSQL Server: %q, Resource Group: %q): %+v", name, serverName, resourceGroup, err)
}

d.Set("name", resp.Name)
d.Set("resource_group_name", resourceGroup)
d.Set("server_name", serverName)

if props := resp.VirtualNetworkRuleProperties; props != nil {
d.Set("subnet_id", props.VirtualNetworkSubnetID)
d.Set("ignore_missing_vnet_service_endpoint", props.IgnoreMissingVnetServiceEndpoint)
ac-astuartkregor marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}

func resourceArmPostgreSQLVirtualNetworkRuleDelete(d *schema.ResourceData, meta interface{}) error {
client := meta.(*ArmClient).postgresqlVirtualNetworkRulesClient
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 {
if response.WasNotFound(future.Response()) {
return nil
}

return fmt.Errorf("Error deleting PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q): %+v", name, serverName, resourceGroup, err)
}

err = future.WaitForCompletionRef(ctx, client.Client)
if err != nil {
if response.WasNotFound(future.Response()) {
return nil
}

return fmt.Errorf("Error deleting PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q): %+v", name, serverName, resourceGroup, err)
}

return nil
}

/*
This function checks the format of the PostgreSQL Virtual Network Rule Name to make sure that
it does not contain any potentially invalid values.
*/
func validatePostgreSQLVirtualNetworkRuleName(v interface{}, k string) (ws []string, errors []error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did you find the definition for these constraints? Because according to the general naming rules and restrictions it states:

Name Length Case Allowable characters Suggested Pattern Example
Network Security Group Rule 1 - 80 Case insensitive Alphanumeric, hyphen, underscore, and period <descriptive context> sql-allow
  • Note: They did not have an example for this exact resource so I picked the next best one that seemed reasonable/similar.

In general, avoid having any special characters (- or _) as the first or last character in any name. These characters will cause most validation rules to fail. I am also wondering if we could abstract this out to a general validation method in the azure package and use it on other resources as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These rules are present in the resource_arm_sql_virtual_network_rule which this resource has been copied from. They overall seem to be quite sensible, although I would be happy to adjust them to be a bit more stringent, especially around length or beginning/ending characters.

In terms of abstracting out this validation, this might be worth doing although it'd be worth looking at the existing resources where the validation might apply to assess how valuable that would be.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually fine for now, In the near future I will implement the validation functions in the azure helper package based on Microsoft's Naming conventions best practices. Once I have completed that work I will update this resource to use the new validation rules.

value := v.(string)

// Cannot be empty
if len(value) == 0 {
errors = append(errors, fmt.Errorf(
"%q cannot be an empty string: %q", k, value))
}

// Cannot be more than 128 characters
if len(value) > 128 {
errors = append(errors, fmt.Errorf(
"%q cannot be longer than 128 characters: %q", k, value))
}

// Must only contain alphanumeric characters or hyphens
if !regexp.MustCompile(`^[A-Za-z0-9-]*$`).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q can only contain alphanumeric characters and hyphens: %q",
k, value))
}

// Cannot end in a hyphen
if regexp.MustCompile(`-$`).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q cannot end with a hyphen: %q", k, value))
}

// Cannot start with a number or hyphen
if regexp.MustCompile(`^[0-9-]`).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q cannot start with a number or hyphen: %q", k, value))
}

// There are multiple returns in the case that there is more than one invalid
// case applied to the name.
return
}

/*
This function refreshes and checks the state of the PostgreSQL Virtual Network Rule.

Response will contain a VirtualNetworkRuleProperties struct with a State property. The state property contain one of the following states (except ResponseNotFound).
* Deleting
* Initializing
* InProgress
* Unknown
* Ready
* ResponseNotFound (Custom state in case of 404)
*/
func postgreSQLVirtualNetworkStateStatusCodeRefreshFunc(ctx context.Context, client postgresql.VirtualNetworkRulesClient, resourceGroup string, serverName string, name string) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
resp, err := client.Get(ctx, resourceGroup, serverName, name)

if err != nil {
if utils.ResponseWasNotFound(resp.Response) {
log.Printf("[DEBUG] Retrieving PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q) returned 404.", resourceGroup, serverName, name)
return nil, "ResponseNotFound", nil
}

return nil, "", fmt.Errorf("Error polling for the state of the PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q): %+v", name, serverName, resourceGroup, err)
}

if props := resp.VirtualNetworkRuleProperties; props != nil {
log.Printf("[DEBUG] Retrieving PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q) returned Status %s", resourceGroup, serverName, name, props.State)
return resp, fmt.Sprintf("%s", props.State), nil
}

//Valid response was returned but VirtualNetworkRuleProperties was nil. Basically the rule exists, but with no properties for some reason. Assume Unknown instead of returning error.
log.Printf("[DEBUG] Retrieving PostgreSQL Virtual Network Rule %q (PostgreSQL Server: %q, Resource Group: %q) returned empty VirtualNetworkRuleProperties", resourceGroup, serverName, name)
return resp, "Unknown", nil
}
}
Loading