Add PrivateLinkService and PrivateEndpoint support #2733

super-harsh · 2023-02-19T12:47:49Z

Special notes for your reviewer:

With the support for PrivateLinkService and PrivateEndpoint, we don't need to export any field as DNS and Ip address to reach the endpoint/service is either provided by the user, ServiceConnection, or by the vnet.

If applicable:

this PR contains tests

…tualNetworkLinks; Add support for DNSZoneGroup; Export PLS/Alias as a configmap

…operator into feature/pls

matthchr

Bunch of comments but this overall looks pretty good to me. Some of my comments boil down to "I am not sure I understand this workflow" - which is Azure's issue... but I wanna make sure it's clear for users of ASO.

v2/api/network/customizations/private_endpoint_extensions.go

v2/api/network/v1alpha1api20201101storage/network_interface_types_gen_test.go

v2/azure-arm.yaml

matthchr · 2023-03-02T01:35:05Z

v2/internal/controllers/crd_networking_privateendpoint_test.go

+
+}
+
+func DNSZoneGroup_CRUD(tc *testcommon.KubePerTestContext, vnet *v1beta20201101.VirtualNetwork, endpoint *network.PrivateEndpoint, rg *resources.ResourceGroup) {


Ok so this test is basically testing the scenario where the user has a VNET + PrivateDNSZone already configured on that VNET, and they want the PrivateEndpoint to create an entry in that existing PrivateDNSZone?

This seems pretty OK from a user-experience for me.

What about the other scenarios? User has NO private DNS zone and just wants to associate the Private Endpoint with a VNET directly. Should we have another test for that scenario? That scenario will make use of the customDnsConfigs property of the PrivateEndpoint, right?

matthchr · 2023-03-02T01:35:42Z

v2/internal/controllers/crd_networking_privateendpoint_test.go

+	"github.com/Azure/azure-service-operator/v2/internal/testcommon"
+)
+
+func Test_Networking_PrivateEndpoint_CRUD(t *testing.T) {


Did you reference an ARM template/bicep example to produce this test? If so can you link it in a comment up here at the top?

There is an ARM bicep example identical to this, however, they use SQLServer and we're using StorageAccount in this test. Still do we add the link?

Yeah I think still worth adding the link. Just say you replaced SQLServer with StorageAccount

I don't see the comment, yet.

matthchr · 2023-03-02T01:35:55Z

v2/internal/controllers/crd_networking_privateendpoint_test.go

+				{
+					Name:                        to.StringPtr("testEndpoint"),
+					PrivateLinkServiceReference: tc.MakeReferenceFromResource(sa),
+					GroupIds:                    []string{"blob"}, // TODO: This is a bit weird that user has to figure out the group ID(s).


is this the same thing they need to do in an ARM template?

If it is the same as the ARM template story, we may want to have the GroupIds configurable from config maps so that we can automate the workflow.

the problem is, GroupIDs are not findable from a resource, it can be funky to get groupIDs for each type of resource into a configMap. For example, for Batch/BatchAccounts, it can be either batchAccount or nodeManagement(depending upon the subresource/service type), for CosmosDB/DatabaseAccounts it is Sql, for ServiceBus/namespaces it'll be namespace etc.

I am not sure I follow the configMap angle either. I just wanted to confirm that we're at parity with ARM templates or BICEP here, which I think we are. The user needs to (somehow) know what the GroupID is for their resource. I think if that's the same experience users get in the ARM template/BICEP/Terraform world then that's OK we're at parity with how it is in the rest of Azure.

v2/internal/controllers/crd_networking_privatelinkservice_test.go

matthchr · 2023-03-02T01:37:35Z

v2/internal/controllers/crd_networking_privatelinkservice_test.go

+				ConfigMaps: &network20220701.PrivateLinkServiceOperatorConfigMaps{
+					Alias: &genruntime.ConfigMapDestination{
+						Name: plsConfigMap,
+						Key:  "alias",


What exactly is alias and why do users want to consume it?

Alias is a globally unique name for your service. It helps you mask the customer data for your service and at the same time creates an easy-to-share name for your service. When you create a Private Link service, Azure generates an alias for your service that you can share with your customers. Your customers can use this alias to request a connection to your service.

The alias is composed of three parts: Prefix.GUID.Suffix

Prefix is the service name. You can pick your own prefix. After "Alias" is created, you can't change it, so select your prefix appropriately.

GUID will be provided by platform. This GUID makes the name globally unique.

Suffix is appended by Azure: region.azure.privatelinkservice

Complete alias: Prefix. {GUID}.region.azure.privatelinkservice

Reference: https://learn.microsoft.com/en-us/azure/private-link/private-link-service-overview#alias

This sounds like something that should be called out in our docs. Consider adding a doc fragment to docs/v2/azure/supported-resources so that this included.

v2/internal/controllers/crd_networking_loadbalancer_test.go

v2/internal/controllers/crd_networking_privatednszone_test.go

theunrepentantgeek · 2023-03-02T19:49:57Z

v2/internal/controllers/crd_networking_privateendpoint_test.go

+				{
+					Name:                        to.StringPtr("testEndpoint"),
+					PrivateLinkServiceReference: tc.MakeReferenceFromResource(sa),
+					GroupIds:                    []string{"blob"}, // TODO: This is a bit weird that user has to figure out the group ID(s).


If it is the same as the ARM template story, we may want to have the GroupIds configurable from config maps so that we can automate the workflow.

theunrepentantgeek · 2023-03-02T19:52:55Z

v2/internal/controllers/crd_networking_privatelinkservice_test.go

+				ConfigMaps: &network20220701.PrivateLinkServiceOperatorConfigMaps{
+					Alias: &genruntime.ConfigMapDestination{
+						Name: plsConfigMap,
+						Key:  "alias",


This sounds like something that should be called out in our docs. Consider adding a doc fragment to docs/v2/azure/supported-resources so that this included.

theunrepentantgeek

Blocking merge until we finish investigation of the version changes in the generated tests.

eskaufel · 2023-03-16T13:13:48Z

How is the progress on this? We need this as well

codecov-commenter · 2023-03-21T04:07:56Z

Codecov Report

Merging #2733 (3ce523c) into main (4c935ef) will increase coverage by 0.02%.
The diff coverage is 51.38%.

@@            Coverage Diff             @@
##             main    #2733      +/-   ##
==========================================
+ Coverage   52.45%   52.48%   +0.02%     
==========================================
  Files        1091     1135      +44     
  Lines      431744   444130   +12386     
==========================================
+ Hits       226488   233107    +6619     
- Misses     169206   173982    +4776     
- Partials    36050    37041     +991

Impacted Files	Coverage Δ
...0200601/private_dns_zones_aaaa_record_types_gen.go	`49.93% <ø> (ø)`
...20200601/private_dns_zones_txt_record_types_gen.go	`50.52% <ø> (ø)`
...200601/private_dns_zones_txt_spec_arm_types_gen.go	`33.33% <ø> (ø)`
...s_zones_virtual_network_link_spec_arm_types_gen.go	`33.33% <ø> (ø)`
...rivate_dns_zones_virtual_network_link_types_gen.go	`58.73% <ø> (ø)`
...601storage/private_dns_zones_a_record_types_gen.go	`55.55% <ø> (ø)`
...storage/private_dns_zones_aaaa_record_types_gen.go	`55.17% <ø> (ø)`
...torage/private_dns_zones_cname_record_types_gen.go	`55.17% <ø> (ø)`
...01storage/private_dns_zones_mx_record_types_gen.go	`55.17% <ø> (ø)`
...1storage/private_dns_zones_ptr_record_types_gen.go	`55.17% <ø> (ø)`
... and 43 more

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

matthchr · 2023-03-21T18:08:08Z

@eskaufel This will be part of the v2.0.0 release.

matthchr

There were a bunch of user scenarios we were tracking with this work, here's my take on what we wanted and what you've got:

Private Link Service CRUD support
Private endpoint CRUD support

The Private Endpoint connection scenarios:

PE connects to VNET with PrivateDNSZone: Supported via PrivateEndpointsPrivateDnsZoneGroup. do we also need to support via raw IP (IP exported from PE, imported into ARecord, via configmap?)
PE connects to VNET using on-prem DNS. Requires DNS forwarder stuff we don't have set up here.

Is that right?

v2/azure-arm.yaml

v2/samples/network/v1beta20220701/v1beta20220701_privateendpointsprivatednszonegroup.yaml

v2/internal/controllers/crd_networking_privatednszone_test.go

v2/internal/controllers/crd_networking_privatelinkservice_test.go

v2/api/network/customizations/private_endpoint_extensions.go

v2/azure-arm.yaml

matthchr

LGTM. We should probably merge this before my huge PR that adds the v1api version and removes v1alpha.

That way I need to rebase and fix it as opposed to you need to.

theunrepentantgeek

Looks good.

theunrepentantgeek · 2023-03-29T02:13:40Z

v2/internal/controllers/crd_networking_privateendpoint_test.go

+	"github.com/Azure/azure-service-operator/v2/internal/testcommon"
+)
+
+func Test_Networking_PrivateEndpoint_CRUD(t *testing.T) {


I don't see the comment, yet.

* Add PrivateLinkService and PrivateEndpoint support

Harshdeep Singh and others added 7 commits February 19, 2023 17:38

Add PrivateLinkService and PrivateEndpoint support

f3d6cab

update sample

203321b

Merge branch 'main' into feature/pls

75d8508

Add pre-reconcile extension for private endpoint; Add support for Vir…

d89e41d

…tualNetworkLinks; Add support for DNSZoneGroup; Export PLS/Alias as a configmap

Merge branch 'feature/pls' of https://github.com/Azure/azure-service-…

b431e66

…operator into feature/pls

Update sample; Update gen files

55ca90f

Merge branch 'main' into feature/pls

35b5aaf

super-harsh marked this pull request as ready for review February 27, 2023 09:13

super-harsh requested review from davefellows, theunrepentantgeek, matthchr and babbageclunk as code owners February 27, 2023 09:13

Harshdeep Singh added 3 commits February 27, 2023 18:32

Add samples and test recordings

2107cad

Fix log for privateEndpoint extention

8ba6af8

Update index.md

ee65319

matthchr reviewed Mar 2, 2023

View reviewed changes

theunrepentantgeek reviewed Mar 2, 2023

View reviewed changes

theunrepentantgeek requested changes Mar 2, 2023

View reviewed changes

Harshdeep Singh added 2 commits March 4, 2023 20:06

Resolve comments

6af62ce

Re-record samples test

a3e8bc2

theunrepentantgeek assigned super-harsh Mar 13, 2023

Harshdeep Singh added 4 commits March 20, 2023 14:45

Merge branch 'main' into feature/pls

a548de4

Add PrivateDnsZones Recordsets

38adc0d

Update samples names

f2f93e3

Update recordings

eb1125e

matthchr reviewed Mar 21, 2023

View reviewed changes

Update RecortSet names

b66835f

super-harsh and others added 5 commits March 22, 2023 16:15

Merge branch 'main' into feature/pls

4443b04

Make customDnzConfig readOnly; Remove PE pre-reconcile extension

a8ca5fa

Update test recording and deepcopy files

9d28c42

Merge branch 'main' into feature/pls

055575c

Update recording file

446dfab

matthchr approved these changes Mar 28, 2023

View reviewed changes

Merge branch 'main' into feature/pls

3ce523c

theunrepentantgeek approved these changes Mar 29, 2023

View reviewed changes

Add comment for bicep file

1b274cc

super-harsh enabled auto-merge (squash) March 29, 2023 02:54

super-harsh merged commit cdbcb93 into main Mar 29, 2023

super-harsh deleted the feature/pls branch March 29, 2023 03:48

emilychu9318 pushed a commit to emilychu9318/azure-service-operator that referenced this pull request Apr 11, 2023

Add PrivateLinkService and PrivateEndpoint support (Azure#2733)

9fddeb0

* Add PrivateLinkService and PrivateEndpoint support

super-harsh mentioned this pull request Apr 17, 2023

Allow support for Private Endpoints #1186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PrivateLinkService and PrivateEndpoint support #2733

Add PrivateLinkService and PrivateEndpoint support #2733

super-harsh commented Feb 19, 2023 •

edited

matthchr left a comment

matthchr Mar 2, 2023

matthchr Mar 2, 2023

super-harsh Mar 2, 2023

matthchr Mar 21, 2023

theunrepentantgeek Mar 29, 2023

super-harsh Mar 29, 2023

matthchr Mar 2, 2023

theunrepentantgeek Mar 2, 2023

super-harsh Mar 20, 2023 •

edited

matthchr Mar 21, 2023

matthchr Mar 2, 2023

super-harsh Mar 2, 2023 •

edited

theunrepentantgeek Mar 2, 2023

theunrepentantgeek Mar 2, 2023

theunrepentantgeek Mar 2, 2023

theunrepentantgeek left a comment

eskaufel commented Mar 16, 2023

codecov-commenter commented Mar 21, 2023 •

edited

matthchr commented Mar 21, 2023

matthchr left a comment •

edited

matthchr left a comment

theunrepentantgeek left a comment

theunrepentantgeek Mar 29, 2023


		}

		func DNSZoneGroup_CRUD(tc testcommon.KubePerTestContext, vnet v1beta20201101.VirtualNetwork, endpoint network.PrivateEndpoint, rg resources.ResourceGroup) {

Add PrivateLinkService and PrivateEndpoint support #2733

Add PrivateLinkService and PrivateEndpoint support #2733

Conversation

super-harsh commented Feb 19, 2023 • edited

matthchr 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

super-harsh Mar 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-harsh Mar 2, 2023 • edited

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

theunrepentantgeek left a comment

Choose a reason for hiding this comment

eskaufel commented Mar 16, 2023

codecov-commenter commented Mar 21, 2023 • edited

Codecov Report

matthchr commented Mar 21, 2023

matthchr left a comment • edited

Choose a reason for hiding this comment

matthchr left a comment

Choose a reason for hiding this comment

theunrepentantgeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-harsh commented Feb 19, 2023 •

edited

super-harsh Mar 20, 2023 •

edited

super-harsh Mar 2, 2023 •

edited

codecov-commenter commented Mar 21, 2023 •

edited

matthchr left a comment •

edited