New Resource: `azurerm_private_link_endpoint` New Data Source: `azurerm_private_link_endpoint_connection` and expose attibute in `azurerm_subnet` #4493

WodansSon · 2019-10-03T01:37:21Z

(fixes #4701 )

katbyte

Thanks for the PR, looking pretty good here 🙂 Left some comments inline that need to be addressed before merge.

katbyte · 2019-10-07T06:54:36Z

azurerm/data_source_private_endpoint.go

+	if location := resp.Location; location != nil {
+		d.Set("location", azure.NormalizeLocation(*location))
+	}
+	if privateEndpointProperties := resp.PrivateEndpointProperties; privateEndpointProperties != nil {


i personally would use props here as it makes the code easier to review

Suggested change

if privateEndpointProperties := resp.PrivateEndpointProperties; privateEndpointProperties != nil {

if props := resp.PrivateEndpointProperties; props != nil {

katbyte · 2019-10-07T06:55:56Z

website/docs/r/subnet.html.markdown

@@ -71,6 +71,10 @@ The following arguments are supported:

 * `delegation` - (Optional) One or more `delegation` blocks as defined below.

+* `disable_private_endpoint_network_policies` - (Optional) Enable or Disable network policies on private end point in the subnet. Default valule is `false`.
+
+-> **NOTE:** Network policies like network security groups (NSG) are not supported for private endpoints. In order to deploy a Private Endpoint on a given subnet, an explicit disable setting is required on that subnet. This setting is only applicable for the Private Endpoint. For other resources in the subnet, access is controlled based on Network Security Groups (NSG) security rules definition.


I have read this three times and i'm not entirely sure what this does 🤔, could we reword this to be easier to understand?

Fixed. I re-wrote this let me know if this makes more sense.

katbyte · 2019-10-07T06:56:13Z

website/docs/r/private_endpoint.html.markdown

+
+* `group_ids` - (Optional) The ID(s) of the group(s) obtained from the remote resource that this private endpoint should connect to.
+
+* `request_message` - (Optional) A message passed to the owner of the remote resource with this connection request. Restricted to 140 chars.


Suggested change

* `request_message` - (Optional) A message passed to the owner of the remote resource with this connection request. Restricted to 140 chars.

* `request_message` - (Optional) A message passed to the owner of the remote resource with this connection request. Restricted to `140` chars.

katbyte · 2019-10-07T06:57:10Z

website/docs/d/private_endpoint.html.markdown

+
+* `name` - The name of the resource that is unique within a resource group. This name can be used to access the resource.
+
+* `status` - Indicates whether the connection has been Approved/Rejected/Removed by the owner of the service.


Could we list the states quoted like we do in the resource?

Yep... I had already fixed that but didn't push the commit yet.

katbyte · 2019-10-07T06:57:22Z

website/docs/d/private_endpoint.html.markdown

+
+The `network_interface` block contains the following:
+
+* `id` - Resource ID.


COuld we expand on this description as in the resource?

Yep... Fixed.

katbyte · 2019-10-07T07:05:27Z

azurerm/resource_arm_private_endpoint.go

+							Type:     schema.TypeString,
+							Optional: true,
+							Default:  "Please approve my connection",
+						},


SHould we ensure no empty strings here?

Yeah, I can add that to my validation function I just wrote to check max length.

katbyte · 2019-10-07T07:05:32Z

azurerm/resource_arm_private_endpoint.go

+							Optional: true,
+							Elem: &schema.Schema{
+								Type: schema.TypeString,
+							},


Could we validate this is a actual azure resource id?

Yep... I have that code in the other PR once that is merged I can reference the helper functions for validation here.

katbyte · 2019-10-07T07:05:35Z

azurerm/resource_arm_private_endpoint.go

+						"private_link_service_id": {
+							Type:         schema.TypeString,
+							Required:     true,
+							ValidateFunc: validate.NoEmptyStrings,


Could we validate this is a actual azure resource id?

Yep... I have that code in the other PR once that is merged I can reference the helper functions for validation here.

katbyte · 2019-10-07T07:06:07Z

azurerm/resource_arm_private_endpoint.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validate.NoEmptyStrings,


Not a blocker but ideally i'd like a regex to validate the name against here (and the other name bits of this schema)

Yep... I have that code in the other PR once that is merged I can reference the helper functions for validation here.

katbyte · 2019-10-07T07:08:37Z

azurerm/data_source_private_endpoint.go

+				Computed: true,
+			},
+
+			"tags": tagsForDataSourceSchema(),


Could we use the new tags.SchemaForDataSource function?

katbyte · 2019-10-07T16:20:53Z

Also would it make more sense to call this resource azurerm_private_link_endpoint or azurerm_private_link_service_endpoint 🤔

…form-provider-azurerm into nr_private-end-point

katbyte

Thanks for the revisions @WodansSon, left some comments inline that need to be addressed before merge.

katbyte · 2019-10-10T03:08:19Z

azurerm/data_source_private_link_endpoint.go

+			"name": {
+				Type:         schema.TypeString,
+				Required:     true,
+				ValidateFunc: validate.NoEmptyStrings,


Could we do a regex validation here?

As before, could we do some better validation here?

Fixed... I was hoping that the private link service was going to be merged before this one so I can re-use the validation in that PR. However, I have added the validation in this one as well.

katbyte · 2019-10-10T03:15:00Z

azurerm/helpers/validate/strings.go

+func PrivateLinkEnpointRequestMessage(i interface{}, k string) (_ []string, errors []error) {
+	return stringMaxLength(140)(i, k)
+}
+
+func stringMaxLength(maxLength int) func(i interface{}, k string) (_ []string, errors []error) {
+	return func(i interface{}, k string) (_ []string, errors []error) {
+		v, ok := i.(string)
+		if !ok {
+			return nil, []error{fmt.Errorf("expected type of %q to be string", k)}
+		}
+
+		if len(v) > maxLength {
+			return nil, []error{fmt.Errorf("%q must not be longer than %d characters, got %d", k, maxLength, len(v))}
+		}
+
+		if strings.TrimSpace(v) == "" {
+			return nil, []error{fmt.Errorf("%q must not be empty", k)}
+		}
+
+		return
+	}
+}


We have validation.StringLenBetween that should work instead?

Same comment

stringLenBetween is a private function which is why I wrote the wrapper. I didn't want to make it public out of fear of breaking other code.

It is? looking at the code:
ValidateFunc: validation.StringLenBetween(1, 1024),

katbyte · 2019-10-10T03:15:11Z

azurerm/resource_arm_private_link_endpoint.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validate.NoEmptyStrings,


COuld we do proper validation here?

katbyte · 2019-10-10T03:15:20Z

azurerm/resource_arm_private_link_endpoint.go

+			"subnet_id": {
+				Type:         schema.TypeString,
+				Required:     true,
+				ValidateFunc: validate.NoEmptyStrings,


Could we validate this is a proper ID?

Please address this?

katbyte · 2019-10-10T03:15:31Z

azurerm/resource_arm_private_link_endpoint.go

+						"private_link_service_id": {
+							Type:         schema.TypeString,
+							Required:     true,
+							ValidateFunc: validate.NoEmptyStrings,


Could we validate this is a proper ID

Please address this?

katbyte · 2019-10-10T03:20:47Z

azurerm/resource_arm_subnet.go

@@ -195,6 +201,22 @@ func resourceArmSubnetCreateUpdate(d *schema.ResourceData, meta interface{}) err
 		properties.RouteTable = nil
 	}

+	if v, ok := d.GetOk("disable_private_endpoint_network_policies"); ok {


same comment

examples/private_endpoint/variables.tf

katbyte · 2019-10-10T03:21:40Z

website/docs/d/private_link_endpoint.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) Specifies the Name of the `Private Link Endpoint`.


We don't usually block quote the resource

Suggested change

* `name` - (Required) Specifies the Name of the `Private Link Endpoint`.

* `name` - (Required) Specifies the Name of the Private Link Endpoint.

same comment + can we use data source wording here?

katbyte · 2019-10-10T03:21:48Z

website/docs/d/private_link_endpoint.html.markdown

+
+* `name` - (Required) Specifies the Name of the `Private Link Endpoint`.
+
+* `resource_group_name` - (Required) Specifies the Name of the Resource Group within which the `Private Link Endpoint` exists.


Suggested change

* `resource_group_name` - (Required) Specifies the Name of the Resource Group within which the `Private Link Endpoint` exists.

* `resource_group_name` - (Required) Specifies the Name of the Resource Group within which the Private Link Endpoint exists.

same comment

katbyte · 2019-10-10T03:23:30Z

website/docs/r/private_link_endpoint.html.markdown

+
+* `name` - (Required) Specifies the Name of the `Private Link Endpoint`. Changing this forces a new resource to be created.
+
+* `resource_group_name` - (Required) Specifies the Name of the Resource Group within which the `Private Link Endpoint` exists.


Is this force new?

…form-provider-azurerm into nr_private-end-point

katbyte · 2019-10-15T10:32:21Z

Is this ready for re-review? it doesn't look like all comments have been addressed?

…form-provider-azurerm into nr_private-end-point

katbyte

Thanks for the revisions, i've left some more comments inline

katbyte · 2019-11-22T19:10:20Z

azurerm/resource_arm_private_link_endpoint.go

+							ValidateFunc: validate.NoEmptyStrings,
+						},
+						"group_ids": {
+							Type:     schema.TypeList,


This should be a type set?

katbyte · 2019-11-22T19:10:49Z

azurerm/resource_arm_private_link_endpoint.go

+			"network_interface_ids": {
+				Type:     schema.TypeSet,
+				Computed: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},


could we validate this is a resource ID?

katbyte · 2019-11-22T19:11:26Z

azurerm/resource_arm_private_link_endpoint.go

+		if err := d.Set("private_service_connection", flattenArmPrivateLinkEndpointServiceConnection(props.PrivateLinkServiceConnections, props.ManualPrivateLinkServiceConnections)); err != nil {
+			return fmt.Errorf("Error setting `private_service_connection`: %+v", err)
+		}
+		if props.NetworkInterfaces != nil {


This check should be in the flatten function

katbyte · 2019-11-22T19:23:56Z

website/docs/r/subnet.html.markdown

@@ -76,6 +76,10 @@ The following arguments are supported:

 * `delegation` - (Optional) One or more `delegation` blocks as defined below.

+* `disable_private_link_endpoint_network_policy_enforcement` - (Optional) Enable or Disable network policies on private end point in the subnet. Default valule is `false`.


I think we renamed this?

Yep, updated the values.

…roviders/terraform-provider-azurerm into nr_private-end-point

bentterp · 2019-11-25T10:18:30Z

Whoops guess the system automagically added me to the reviewers list, please don't think I consider myself deserving of that title just because I made a comment....

katbyte

thanks for the revisions @WodansSon, i've left some more comments inline

katbyte · 2019-11-26T16:22:39Z

azurerm/data_source_private_link_endpoint.go

+			"private_service_connection": {
+				Type:     schema.TypeList,
+				Computed: true,
+				MaxItems: 1,


We don't need this on a datasource

katbyte · 2019-11-26T16:23:04Z

azurerm/data_source_private_link_endpoint_connections.go

+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+)
+
+func dataSourceArmPrivateLinkEndpointConnections() *schema.Resource {


As there can be only one i don't think this should be plural

And would it make sense to call it: dataSourceArmPrivateLinkEndpointConnectionsStatus?

katbyte · 2019-11-26T16:29:33Z

azurerm/data_source_subnet.go

}
-

COuld we not remove all this whitespace

katbyte · 2019-11-26T16:29:38Z

azurerm/data_source_subnet.go

 		if props.NetworkSecurityGroup != nil {
 			d.Set("network_security_group_id", props.NetworkSecurityGroup.ID)
 		} else {
 			d.Set("network_security_group_id", "")
 		}
-


COuld we not remove all this whitespace

katbyte · 2019-11-26T16:29:41Z

azurerm/data_source_subnet.go

 		if props.RouteTable != nil {
 			d.Set("route_table_id", props.RouteTable.ID)
 		} else {
 			d.Set("route_table_id", "")
 		}
-


COuld we not remove all this whitespace

katbyte · 2019-11-26T17:56:41Z

azurerm/resource_arm_private_link_endpoint.go

+							Optional: true,
+							Elem: &schema.Schema{
+								Type: schema.TypeString,
+							},


same thing?

katbyte · 2019-11-26T17:58:47Z

azurerm/resource_arm_private_link_endpoint_test.go

+data "azurerm_subscription" "current" {}
+
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-privateendpoint-%d"


Could we just use private link here as it's the service name

Suggested change

name = "acctestRG-privateendpoint-%d"

name = "acctestRG-privatelink-%d"

katbyte · 2019-11-26T17:59:26Z

azurerm/resource_arm_subnet.go

@@ -140,6 +140,12 @@ func resourceArmSubnet() *schema.Resource {
 				},
 			},

+			"enforce_private_link_endpoint_network_policies": {


Should this conflict with the service policies?

Yes, you are correct... I will add the conflicts with to the policies.

katbyte · 2019-11-26T18:02:36Z

website/docs/d/private_link_endpoint.html.markdown

+
+## Attributes Reference
+
+The following attributes are exported:


I think the docs are missing the private_service_connection block here

Removed the data source as it didn't make much sense.

katbyte · 2019-11-26T18:03:20Z

website/docs/d/subnet.html.markdown

@@ -40,3 +40,4 @@ output "subnet_id" {
 * `route_table_id` - The ID of the Route Table associated with this subnet.
 * `ip_configurations` - The collection of IP Configurations with IPs within this subnet.
 * `service_endpoints` - A list of Service Endpoints within this subnet.
+* `disable_private_link_endpoint_network_policy_enforcement` - Enable or Disable network policies for a Private Link Endpoint in the subnet.


I think this description needs to be updated?

…form-provider-azurerm into nr_private-end-point

katbyte

Thanks for the revisions @WodansSon, LGTM now 👍

ghost · 2019-12-07T00:15:52Z

This has been released in version 1.38.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.38.0"
}
# ... other configuration ...

geekzter · 2019-12-07T10:47:08Z

Thanks!

There are some issue's though:

Documentation is referring to non existing attribute disable_private_link_service_network_policy_enforcement, instead of enforce_private_link_service_network_policies.
Documentation sample does not include required block private_service_connection
The resource has no output attribute for private_ip_address, hence there is no way to wire this up with a private dns record

ghost · 2020-01-02T14:50:20Z

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!

[WIP] Private Endpoint

64bb60e

WodansSon added documentation new-resource new-data-source service/network-interfaces labels Oct 3, 2019

WodansSon added this to the v1.36.0 milestone Oct 3, 2019

WodansSon self-assigned this Oct 3, 2019

ghost added the size/XXL label Oct 3, 2019

WodansSon added 2 commits October 2, 2019 18:40

[WIP] Update datasource to network_interface_ids

d445539

[WIP] Update documentation with correct value

cf252b9

WodansSon requested a review from katbyte October 3, 2019 04:05

katbyte requested changes Oct 7, 2019

View reviewed changes

katbyte added the waiting-response label Oct 7, 2019

WodansSon added 3 commits October 8, 2019 12:41

Rename resource and flatten schema

266fa1b

Merge branch 'master' of https://github.com/terraform-providers/terra…

3840307

…form-provider-azurerm into nr_private-end-point

Update for terraform-plugin-sdk

7ce24b8

WodansSon changed the title ~~[WIP]Data Source/New Resource: azurerm_private_endpoint and expose attibute in azurerm_subnet~~ Data Source/New Resource: azurerm_private_link_endpoint and expose attibute in azurerm_subnet Oct 9, 2019

WodansSon added 3 commits October 9, 2019 16:42

Gofmt and some PR comment updates

71133ca

Some documentation updates

f965560

More doc updates

87cb012

katbyte requested changes Oct 10, 2019

View reviewed changes

WodansSon added 3 commits October 11, 2019 17:15

Merge branch 'master' of https://github.com/terraform-providers/terra…

3a4b263

…form-provider-azurerm into nr_private-end-point

Update to include new terraform-plugin-sdk

fe67c8f

Fixed lint error

bd69625

WodansSon removed the waiting-response label Oct 12, 2019

tombuildsstuff modified the milestones: v1.36.0, v1.37.0 Oct 15, 2019

katbyte added the waiting-response label Oct 15, 2019

WodansSon added 4 commits November 21, 2019 17:00

Commit changes for rebase with master

e0fde44

Merge branch 'master' of https://github.com/terraform-providers/terra…

c0a3c8e

…form-provider-azurerm into nr_private-end-point

Updates per PR comments

4ba6e4e

Remove dead code

817ca0f

katbyte requested changes Nov 22, 2019

View reviewed changes

WodansSon added 2 commits November 22, 2019 12:58

Updates per PR comments

33e7fdd

Merge branch 'nr_private-end-point' of https://github.com/terraform-p…

e73e10e

…roviders/terraform-provider-azurerm into nr_private-end-point

katbyte requested changes Nov 26, 2019

View reviewed changes

Updates per PR comments

b209b87

WodansSon and others added 10 commits November 27, 2019 17:19

Remove endpoint data source from provider

bb8a136

Update service endpoint connections

ae82096

Update to documentation

7ab6a68

Fix sample code

261c704

data source update

541694f

Merge branch 'master' of https://github.com/terraform-providers/terra…

6d6a38f

…form-provider-azurerm into nr_private-end-point

Removed service connections datasource

d818bc6

Add the service connections datasource back in

017b56c

Update test case

a8e0eac

make fmt and adjust comment location

527a2f1

katbyte approved these changes Dec 3, 2019

View reviewed changes

katbyte merged commit 99ed814 into master Dec 3, 2019

katbyte deleted the nr_private-end-point branch December 3, 2019 09:30

katbyte added a commit that referenced this pull request Dec 3, 2019

Update CHANGELOG.md to include #4493

0fd80ac

geekzter mentioned this pull request Dec 9, 2019

subresource_names attribute on azurerm_private_link_endpoint.private_service_connection does not accept valid data #5110

Closed

ghost locked and limited conversation to collaborators Jan 2, 2020

	if privateEndpointProperties := resp.PrivateEndpointProperties; privateEndpointProperties != nil {
	if props := resp.PrivateEndpointProperties; props != nil {


		* `group_ids` - (Optional) The ID(s) of the group(s) obtained from the remote resource that this private endpoint should connect to.

		* `request_message` - (Optional) A message passed to the owner of the remote resource with this connection request. Restricted to 140 chars.


		* `name` - The name of the resource that is unique within a resource group. This name can be used to access the resource.

		* `status` - Indicates whether the connection has been Approved/Rejected/Removed by the owner of the service.


		The `network_interface` block contains the following:

		* `id` - Resource ID.


		The following arguments are supported:

		* `name` - (Required) Specifies the Name of the `Private Link Endpoint`.


		* `name` - (Required) Specifies the Name of the `Private Link Endpoint`.

		* `resource_group_name` - (Required) Specifies the Name of the Resource Group within which the `Private Link Endpoint` exists.

		@@ -76,6 +76,10 @@ The following arguments are supported:

		* `delegation` - (Optional) One or more `delegation` blocks as defined below.

		* `disable_private_link_endpoint_network_policy_enforcement` - (Optional) Enable or Disable network policies on private end point in the subnet. Default valule is `false`.

	name = "acctestRG-privateendpoint-%d"
	name = "acctestRG-privatelink-%d"


		## Attributes Reference

		The following attributes are exported:

New Resource: azurerm_private_link_endpoint New Data Source: azurerm_private_link_endpoint_connection and expose attibute in azurerm_subnet #4493

New Resource: azurerm_private_link_endpoint New Data Source: azurerm_private_link_endpoint_connection and expose attibute in azurerm_subnet #4493

Conversation

WodansSon commented Oct 3, 2019 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Oct 7, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Oct 15, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentterp commented Nov 25, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New Resource: `azurerm_private_link_endpoint` New Data Source: `azurerm_private_link_endpoint_connection` and expose attibute in `azurerm_subnet` #4493

New Resource: `azurerm_private_link_endpoint` New Data Source: `azurerm_private_link_endpoint_connection` and expose attibute in `azurerm_subnet` #4493

WodansSon commented Oct 3, 2019 •

edited

Loading