New Resource: azurerm_postgresql_virtual_network_rule

[ac-astuartkregor]

This PR allows users to create vnet rules akin to azurerm_sql_virtual_network_rule so that service endpoints can be used to access PostgreSQL instances.

The code was copied and adapted from the azurerm_sql_virtual_network_rule.

I'd be very grateful for any feedback especially if there are any styling or convention considerations.

My thanks to @lfshr and @tombuildsstuff for their advice and input.

(Fixes: #1571 )

[lfshr]

This fixes #1571 (title didn't seem to link the issue)

[ac-astuartkregor]

Good spot, although the PR title should mean the issue is closed when the PR gets merged.

[WodansSon]

Can we add some validation here for non-zero values?

ValidateFunc: validation.NoZeroValues,

[ac-astuartkregor]

@jeffreyCline Do you have any suggestions for testing that this validation has been applied to this/these attribute(s)? Is it worth testing such a thing? Sorry if this is a daft question, however I'm not sure how it could be tested given the style of the existing unit and acceptance tests.

[WodansSon]

Can we add some validation on the resource id here?

ValidateFunc: azure.ValidateResourceID,

[WodansSon]

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

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

[ac-astuartkregor]

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.

[WodansSon]

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?

[ac-astuartkregor]

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.

[WodansSon]

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.

[WodansSon]

Can we combine these into a single line?

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

[ac-astuartkregor]

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

[WodansSon]

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.

[ac-astuartkregor]

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.

[WodansSon]

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.

[ac-astuartkregor]

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

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

[WodansSon]

Can we move the Changing this forces a new resource to be created. to the end of the sentence?

• name - (Required) The name of the SQL virtual network rule. Cannot be empty and must only contain alphanumeric characters and hyphens. Cannot start with a number, and cannot start or end with a hyphen. Changing this forces a new resource to be created.

[ac-astuartkregor]

Hi @jeffreyCline, many thanks for your feedback. I'll look at addressing this today. I should point out that since this is almost a carbon copy of sql virtual network rule resource, you may wish to also apply the same refactorings to that as well.

[WodansSon]

@ac-astuartkregor this mostly LGTM, but I left a comment on a minor issue, thanks.

[WodansSon]

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.

[WodansSon]

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.

[WodansSon]

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.

[WodansSon]

@ac-astuartkregor as part of validating the new resource I ran the tests. They all failed, if you could get the tests passing that would be great.

[WodansSon]

[WodansSon]

@ac-astuartkregor Yes we need to remove the ignore_vnet_service_endpoint attribute completely. I am following up with the internal Microsoft service team to get this fixed, but I agree with Tom that there is no reason to block this from shipping because of this one issue. If you can remove that attribute from the resource and the documentation I think we will be good to go.

[ac-astuartkregor]

Cheers @jeffreyCline, working on it.

[WodansSon]

LGTM

[tombuildsstuff]

hey @ac-astuartkregor

Thanks for pushing the updates to this PR - if we can flip the default from true to false for the IgnoreMissingVnetServiceEndpoint field for the moment (and remove the field from the Read function, since it's not currently present in the schema) - this otherwise LGTM 👍

Thanks!

[tombuildsstuff]

given this defaults to false in the other resources, I think it'd be sensible to make this false by default for the moment (since otherwise this'll cause conflicts for users when we introduce this field with a default of false when it's fixed) - what do you think?

[ac-astuartkregor]

I had a brief debate about this with myself and what happens when the endpoint is missing.

I see where you're coming from and perhaps it would be best to do that. I was going to ask what you think is the best approach from a user experience perspective.

• If IgnoreMissingVnetServiceEndpoint is false and the endpoint is missing, the resource creation will timeout.
• If IgnoreMissingVnetServiceEndpoint is true and the endpoing is missing, the resource creation will succeed but the functionality will be broken.

I suspect your suggestion is probably the better choice but I'm curious as to what you think.

[tombuildsstuff]

so the other option would be to temporarily verify that the Subnet contains the Service Endpoint, e.g. something like this in the Create function:

subnetsClient := meta.(*ArmClient).subnetClient

	subnetResourceGroup := subnetParsedId.ResourceGroup
	virtualNetwork := subnetParsedId.Path["virtualNetworks"]
	subnetName := subnetParsedId.Path["subnets"]
	resp, err := subnetsClient.Get(ctx, subnetResourceGroup, virtualNetwork, subnetName, "")
	if err != nil {
		if utils.ResponseWasNotFound(resp.Response) {
			return fmt.Errorf("Subnet with ID %q was not found: %+v", subnetId, err)
		}

		return fmt.Errorf("Error obtaining Subnet %q (Virtual Network %q / Resource Group %q: %+v", subnetName, virtualNetwork, subnetResourceGroup, err)
	}
	containsEndpoint := false
	if props := resp.SubnetPropertiesFormat; props != nil {
		if endpoints := props.ServiceEndpoints; endpoints != nil {
			for _, e := range *endpoints {
				if e.Service == nil {
					continue
				}

				if strings.EqualFold(*e.Service, "Microsoft.Sql") {
					containsEndpoint = true
					break
				}
			}
		}
	}
	if !containsEndpoint {
		return fmt.Errorf("Subnet %q (Virtual Network %q / Resource Group %q) must contain a Service Endpoint for `Microsoft.Sql`", subnetName, virtualNetwork, subnetResourceGroup)
	}

This would ensure the Endpoint would work - and we should be able to revert that once the API if fixed?

[ac-astuartkregor]

Oh.

I think perhaps defaulting to false might be nicer? 🤣

[tombuildsstuff]

whilst we've removed this, to ensure this fails gracefully I've pushed a commit (I hope you don't mind!) to add the above code to the Create method, to ensure the Subnet's in a usable state prior to creating the subnet rule

[WodansSon]

@tombuildsstuff I removed the waiting-response tag since @ac-astuartkregor had committed the requested change to the PR.

[tombuildsstuff]

Tests pass:

[tombuildsstuff]

hey @ac-astuartkregor

Thanks for the work on this PR - I hope you don't mind I've pushed a commit to fix a couple of minor outstanding issues, but this now LGTM 👍

Thanks!

[ac-astuartkregor]

Hi @tombuildsstuff,
You're most welcome! Sorry that things were a bit sloppy in places and thank you for implementing the check on the subnet that you mentioned. Probably the best approach to be honest. Hopefully it won't be long before it can be removed.

Thanks for merging too :)

[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!