migrate private endpoints service to ASO by nawazkh · Pull Request #4108 · kubernetes-sigs/cluster-api-provider-azure

nawazkh · 2023-10-09T20:03:14Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR migrates private endpoints service to use ASO framework.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3530

Special notes for your reviewer:

I tested it locally, and I was able to bring up a private-endpoint using the default template.
cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

migrate Private Endpoints Service to use ASO Framework

k8s-ci-robot · 2023-10-09T20:03:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-10-09T20:15:03Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (50cd249) 60.45% compared to head (12a2c08) 60.89%.
Report is 8 commits behind head on main.

Files	Patch %	Lines
azure/services/privateendpoints/spec.go	81.69%	10 Missing and 3 partials ⚠️
...zure/services/privateendpoints/privateendpoints.go	0.00%	5 Missing ⚠️
azure/scope/cluster.go	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4108      +/-   ##
==========================================
+ Coverage   60.45%   60.89%   +0.43%     
==========================================
  Files         191      190       -1     
  Lines       19195    18983     -212     
==========================================
- Hits        11604    11559      -45     
+ Misses       6947     6787     -160     
+ Partials      644      637       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nawazkh · 2023-10-25T21:41:12Z

Updating unit tests.
/hold

nawazkh · 2023-10-26T01:40:12Z

/unhold

nawazkh · 2023-11-29T21:42:23Z

/unhold

nawazkh · 2023-11-30T00:07:15Z

/retest

nawazkh · 2023-11-30T22:32:29Z

/retest

nawazkh · 2023-11-30T23:35:35Z

/test pull-cluster-api-provider-azure-windows-containerd-upstream-custom-builds

k8s-ci-robot · 2023-11-30T23:35:37Z

@nawazkh: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-aks
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-custom-builds
/test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow
/test pull-cluster-api-provider-azure-windows-custom-builds
/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-capi-e2e
pull-cluster-api-provider-azure-ci-entrypoint
pull-cluster-api-provider-azure-conformance
pull-cluster-api-provider-azure-conformance-custom-builds
pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
pull-cluster-api-provider-azure-conformance-with-ci-artifacts
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-aks
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify
pull-cluster-api-provider-azure-windows-custom-builds
pull-cluster-api-provider-azure-windows-with-ci-artifacts

Details

In response to this:

/test pull-cluster-api-provider-azure-windows-containerd-upstream-custom-builds

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nojnhuh · 2023-12-01T15:58:10Z

+	if len(s.ApplicationSecurityGroups) > 0 {
+		applicationSecurityGroups := make([]asonetworkv1.ApplicationSecurityGroupSpec_PrivateEndpoint_SubResourceEmbedded, 0, len(s.ApplicationSecurityGroups))


This length check (along with the other similar ones) could be made redundant if we initialize the slices as nil:

Suggested change

if len(s.ApplicationSecurityGroups) > 0 {

applicationSecurityGroups := make([]asonetworkv1.ApplicationSecurityGroupSpec_PrivateEndpoint_SubResourceEmbedded, 0, len(s.ApplicationSecurityGroups))

var applicationSecurityGroups []asonetworkv1.ApplicationSecurityGroupSpec_PrivateEndpoint_SubResourceEmbedded

Then the linter will complain that we're not pre-allocating the slice, but I'd be +1 to ignoring that since I can't imagine that makes any difference in performance unless users are defining 1000s of these in a single resource YAML.

I agree. But if this minor over-engineering guardrails us from unforeseen scenarios and adds a bit of performance, I am happy to take it. 😃

nojnhuh · 2023-12-01T16:17:08Z

-				PrivateEndpointNetworkPolicies:    ptr.To(armnetwork.VirtualNetworkPrivateEndpointNetworkPoliciesDisabled),
-				PrivateLinkServiceNetworkPolicies: ptr.To(armnetwork.VirtualNetworkPrivateLinkServiceNetworkPoliciesEnabled),


I don't see where ASO lets us set these fields. Is that because they don't actually have any effect? Should we be setting these on the real subnet resource?

Good question @nojnhuh , Let me put a question on the ASO channel.

My theory from probing the Azure SDK request body params (xref: https://learn.microsoft.com/en-us/rest/api/virtualnetwork/private-endpoints/create-or-update?view=rest-virtualnetwork-2023-05-01&tabs=HTTP#request-body) is that PrivateEndpointNetworkPolicy: Disabled and PrivateLinkServiceNetworkPolicy: Enabled appear to be the default values for the fields being explicitly specified.

~~From the examples of creating PrivateEndpoints using Azure SDK, I see that privateEndpoints request payload only requires the SubnetIDs, I am guessing ASO is also following the same~~

But, let me confirm.

Update: scratch the theory.

nojnhuh · 2023-12-01T16:52:41Z

@nawazkh If there isn't one already, could you please open an issue to track adding e2e coverage for private endpoints?

willie-yao

Looks good pending one minor comment!

nawazkh · 2023-12-05T00:44:50Z

@nawazkh If there isn't one already, could you please open an issue to track adding e2e coverage for private endpoints?

Created #4336

CAPZ sets the following ASO fields if found: - ApplicationSecurityGroups - AzureName - CustomNetworkInterfaceName - IpConfigurations - Location - ManualPrivateLinkServiceConnections - Owner - PrivateLinkServiceConnections - Subnet - Tags ASO fields not managed by CAPZ - ExtendedLocation

k8s-ci-robot · 2023-12-05T02:02:13Z

@nawazkh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-windows-containerd-upstream-custom-builds	`7c8f0d3`	link	false	`/test pull-cluster-api-provider-azure-windows-containerd-upstream-custom-builds`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

nojnhuh · 2023-12-08T17:52:39Z

/retest

nojnhuh

/lgtm

k8s-ci-robot · 2023-12-08T18:01:48Z

LGTM label has been added.

Details

Git tree hash: e4ff7a484cb390fd12a7e365e1b45dcd4d63281a

willie-yao · 2023-12-08T19:16:16Z

/lgtm

nojnhuh · 2023-12-08T19:23:14Z

/approve

k8s-ci-robot · 2023-12-08T19:23:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nojnhuh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 9, 2023

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 9, 2023

k8s-ci-robot requested review from jsturtevant and willie-yao October 9, 2023 20:03

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2023

nawazkh force-pushed the private_endpoints_to_aso branch 2 times, most recently from e39646d to e2ea835 Compare October 17, 2023 23:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2023

nawazkh force-pushed the private_endpoints_to_aso branch from cc54078 to cd87db8 Compare October 24, 2023 20:37

nawazkh commented Oct 24, 2023

View reviewed changes

Comment thread controllers/azuremanagedcontrolplane_controller.go

nawazkh force-pushed the private_endpoints_to_aso branch from 829cad7 to 61ef909 Compare October 25, 2023 01:37

nawazkh commented Oct 25, 2023

View reviewed changes

Comment thread azure/services/privateendpoints/spec.go

nawazkh force-pushed the private_endpoints_to_aso branch from 61ef909 to 7c8f0d3 Compare October 25, 2023 02:11

nawazkh marked this pull request as ready for review October 25, 2023 02:11

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2023

k8s-ci-robot requested review from CecileRobertMichon and mboersma October 25, 2023 02:11

nawazkh requested a review from nojnhuh October 25, 2023 02:12

nawazkh commented Oct 25, 2023

View reviewed changes

Comment thread azure/scope/cluster_test.go

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2023

nawazkh force-pushed the private_endpoints_to_aso branch from 7c8f0d3 to 3d96af5 Compare October 26, 2023 01:39

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2023

CecileRobertMichon assigned nojnhuh Oct 26, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023

nawazkh force-pushed the private_endpoints_to_aso branch from 1ad6e60 to 948c69a Compare November 30, 2023 17:57

nawazkh force-pushed the private_endpoints_to_aso branch from 948c69a to 1b4f345 Compare December 1, 2023 16:45

nojnhuh reviewed Dec 1, 2023

View reviewed changes

willie-yao reviewed Dec 1, 2023

View reviewed changes

Comment thread azure/services/privateendpoints/spec.go

nawazkh force-pushed the private_endpoints_to_aso branch 2 times, most recently from a106c3b to 8effa61 Compare December 5, 2023 00:38

nawazkh mentioned this pull request Dec 5, 2023

Add e2e tests for private endpoints #4336

Closed

nawazkh force-pushed the private_endpoints_to_aso branch from 8effa61 to 12a2c08 Compare December 5, 2023 01:02

nojnhuh reviewed Dec 8, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2023

k8s-ci-robot merged commit 7d7e883 into kubernetes-sigs:main Dec 8, 2023

k8s-ci-robot added this to the v1.13 milestone Dec 8, 2023

nawazkh deleted the private_endpoints_to_aso branch December 11, 2023 17:32

		if len(s.ApplicationSecurityGroups) > 0 {
		applicationSecurityGroups := make([]asonetworkv1.ApplicationSecurityGroupSpec_PrivateEndpoint_SubResourceEmbedded, 0, len(s.ApplicationSecurityGroups))

	if len(s.ApplicationSecurityGroups) > 0 {
	applicationSecurityGroups := make([]asonetworkv1.ApplicationSecurityGroupSpec_PrivateEndpoint_SubResourceEmbedded, 0, len(s.ApplicationSecurityGroups))
	var applicationSecurityGroups []asonetworkv1.ApplicationSecurityGroupSpec_PrivateEndpoint_SubResourceEmbedded

		PrivateEndpointNetworkPolicies: ptr.To(armnetwork.VirtualNetworkPrivateEndpointNetworkPoliciesDisabled),
		PrivateLinkServiceNetworkPolicies: ptr.To(armnetwork.VirtualNetworkPrivateLinkServiceNetworkPoliciesEnabled),

Conversation

nawazkh commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 9, 2023

Uh oh!

codecov Bot commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nawazkh commented Oct 25, 2023

Uh oh!

nawazkh commented Oct 26, 2023

Uh oh!

nawazkh commented Nov 29, 2023

Uh oh!

nawazkh commented Nov 30, 2023

Uh oh!

nawazkh commented Nov 30, 2023

Uh oh!

nawazkh commented Nov 30, 2023

Uh oh!

k8s-ci-robot commented Nov 30, 2023

Uh oh!

Uh oh!

Uh oh!

nojnhuh Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

nawazkh Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nojnhuh Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

nawazkh Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nojnhuh commented Dec 1, 2023

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nawazkh commented Dec 5, 2023

Uh oh!

k8s-ci-robot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojnhuh commented Dec 8, 2023

Uh oh!

nojnhuh left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 8, 2023

Uh oh!

willie-yao commented Dec 8, 2023

Uh oh!

nojnhuh commented Dec 8, 2023

Uh oh!

k8s-ci-robot commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nawazkh commented Oct 9, 2023 •

edited

Loading

codecov Bot commented Oct 9, 2023 •

edited

Loading

nawazkh Dec 5, 2023 •

edited

Loading

k8s-ci-robot commented Dec 5, 2023 •

edited

Loading