let users configure private IP of the internal LB #5332

nawazkh · 2024-12-12T08:38:58Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR is needed for updating Tilt workflow to spin up VM based templates with custom, non-overlapping Private IPs.

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 #5264

Special notes for your reviewer:

This PR follows Use feature flag to create internal LB #5311 .
Note: This PR
- updates defaulting webhook to create and assign a default internal IP when APIServer ILB Feature flag enabled
- updates validation webhook to validate the internal IP when when APIServer ILB Feature flag enabled
- updates defaulting webhook unit tests: preserves existing functionality/unit tests and adds new tests for feature flag scenarios.
- updates validating webhook unit tests: preserves existing functionality/unit tests and adds new tests for feature flag scenarios.
- updates apiserver ilb template with custom CIDRs
- updates apiserver ilb windows template with custom CIDRs
- creates ci template for api server ILB for linux nodes
- adds e2e tests for apiserver ilb feature flag

TODOs:

squashed commits
includes documentation
adds unit tests
cherry-pick candidate

Release note:

feat APIServerILB: private IP of the internal LB can be customized

nawazkh · 2024-12-12T08:40:37Z

/test pull-cluster-api-provider-azure-apiserver-ilb

nawazkh · 2024-12-12T08:40:59Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

nawazkh · 2024-12-12T09:19:08Z

/test pull-cluster-api-provider-azure-apiserver-ilb

codecov · 2024-12-12T09:24:56Z

Codecov Report

Attention: Patch coverage is 94.95798% with 6 lines in your changes missing coverage. Please review.

Project coverage is 52.59%. Comparing base (61c6f3f) to head (ded1922).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
api/v1beta1/azurecluster_default.go	85.00%	2 Missing and 1 partial ⚠️
api/v1beta1/azurecluster_validation.go	92.30%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5332      +/-   ##
==========================================
+ Coverage   52.51%   52.59%   +0.07%     
==========================================
  Files         272      272              
  Lines       29431    29489      +58     
==========================================
+ Hits        15457    15510      +53     
- Misses      13172    13176       +4     
- Partials      802      803       +1

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

nawazkh · 2024-12-12T10:21:28Z

/test pull-cluster-api-provider-azure-apiserver-ilb

nawazkh · 2024-12-12T10:27:00Z

/test pull-cluster-api-provider-azure-apiserver-ilb

nawazkh · 2024-12-12T17:45:59Z

/test

k8s-ci-robot · 2024-12-12T17:46:01Z

@nawazkh: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-apiversion-upgrade
/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-apiserver-ilb
/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-load-test-custom-builds
/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-apiversion-upgrade
pull-cluster-api-provider-azure-build
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

In response to this:

/test

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-sigs/prow repository.

nawazkh · 2024-12-12T17:46:27Z

/test pull-cluster-api-provider-azure-apiserver-ilb

nawazkh · 2024-12-12T17:46:34Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

willie-yao

Wow great job on this Nawaz! Way to go getting everything to work here. Just a few comments from my end 🚀

api/v1beta1/azurecluster_default.go

api/v1beta1/azurecluster_validation.go

api/v1beta1/azurecluster_validation_test.go

willie-yao · 2024-12-12T19:15:58Z

test/e2e/azure_apiserver_ilb.go

+                                             v
+                                  +--------------------------------+
+                                  |        Test Complete           |
+                                  +--------------------------------+


I love this comment describing what the test does! Kudos!

test/e2e/azure_apiserver_ilb.go

willie-yao · 2024-12-12T19:23:53Z

test/e2e/azure_apiserver_ilb.go

+	controlPlaneEndpointDNSName, apiServerILBPrivateIP := "", ""
+	for _, frontendIP := range deployedAzureCluster.Spec.NetworkSpec.APIServerLB.FrontendIPs {
+		if frontendIP.PublicIP != nil && frontendIP.PublicIP.DNSName != "" {
+			fmt.Fprintf(GinkgoWriter, "Control Plane Endpoint Name: %s\n", frontendIP.PublicIP.DNSName)


nit: Might be good to use Logf instead of fmt.Fprintf here, as well as in the rest of the file.

fmt.Fprintf(GinkgoWriter, .....) helped me log the output inline with the test.
Do you know if Logf() achieves the same ?

It should achieve the same and I've always used Logf to print stuff in-line. Logf under the hood is fmt.Fprintf as seen here:

cluster-api-provider-azure/test/e2e/log.go

Line 36 in 61c6f3f

fmt.Fprintf(ginkgo.GinkgoWriter, nowStamp()+": "+level+": "+format+"\n", args...)

It seems like Logf is the preferred way to print stuff in E2E tests as it prints the log level and timestamp

Updated, please take a look.

test/e2e/azure_apiserver_ilb.go

nawazkh · 2024-12-12T22:16:04Z

/test pull-cluster-api-provider-azure-apiserver-ilb

nawazkh · 2024-12-12T22:16:16Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

Jont828

LGTM pending nits and Willie's comments

api/v1beta1/azurecluster_default.go

api/v1beta1/azurecluster_validation.go

willie-yao · 2024-12-12T22:49:48Z

/retest

nawazkh · 2024-12-12T23:35:02Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

nawazkh · 2024-12-12T23:35:09Z

/test pull-cluster-api-provider-azure-apiserver-ilb

nawazkh · 2024-12-13T00:50:40Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

nawazkh · 2024-12-13T00:52:08Z

RESPONSE 400: 400 Bad Request
	ERROR CODE: PublicIPCountLimitReached
	--------------------------------------------------------------------------------
	{
	  "error": {
	    "code": "PublicIPCountLimitReached",
	    "message": "Cannot create more than 100 public IP addresses for this subscription in this region.",
	    "details": []
	  }
	}

nawazkh · 2024-12-13T03:00:50Z

/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

nawazkh · 2024-12-13T05:12:38Z

/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

- update apiserver ilb flavor with custom CIDRs - update apiserver ilb windows flavor with custom CIDRs - create ci template for api server ILB PR tests - add e2e tests for apiserver ilb template - add e2e test for a default flavored template

nawazkh · 2024-12-13T17:07:50Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

nawazkh · 2024-12-13T17:07:56Z

/test pull-cluster-api-provider-azure-apiserver-ilb

willie-yao

/lgtm

Awesome work @nawazkh! This is a huge fix for CAPZ 🚀

k8s-ci-robot · 2024-12-13T19:10:37Z

LGTM label has been added.

Git tree hash: 01e0a1b10f1ce2a8a21ed6adc5e61e345918ecb1

Jont828 · 2024-12-13T23:32:33Z

/approve

Nice work!

mboersma

/lgtm
/approve

Ignore my comment about t.Parallel; I saw your answer to @willie-yao about the same thing. 👍

mboersma · 2024-12-13T23:34:27Z

api/v1beta1/azurecluster_default_test.go

 	}

 	for _, c := range cases {
 		tc := c
 		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()


This didn't work with t.Parallel?

k8s-ci-robot · 2024-12-13T23:44:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jont828, mboersma

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Jont828,mboersma]

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 kind/feature cncf-cla: yes labels Dec 12, 2024

k8s-ci-robot requested review from jackfrancis and willie-yao December 12, 2024 08:39

k8s-ci-robot added the size/XXL label Dec 12, 2024

nawazkh added this to the v1.18 milestone Dec 12, 2024

nawazkh mentioned this pull request Dec 12, 2024

Create APIServerILB windows tempalates for prow/ci #5334

Open

nawazkh force-pushed the make_ilb_ip_configurable branch from b086166 to e0f2b44 Compare December 12, 2024 17:45

willie-yao reviewed Dec 12, 2024

View reviewed changes

nawazkh force-pushed the make_ilb_ip_configurable branch from f875002 to 3720d4b Compare December 12, 2024 22:15

Jont828 reviewed Dec 12, 2024

View reviewed changes

api/v1beta1/azurecluster_default.go Outdated Show resolved Hide resolved

api/v1beta1/azurecluster_validation.go Show resolved Hide resolved

api/v1beta1/azurecluster_validation.go Show resolved Hide resolved

nawazkh requested a review from willie-yao December 12, 2024 22:38

nawazkh force-pushed the make_ilb_ip_configurable branch from 57e9274 to 8d20398 Compare December 12, 2024 23:33

nawazkh requested a review from Jont828 December 13, 2024 06:53

nawazkh force-pushed the make_ilb_ip_configurable branch from 8d20398 to ded1922 Compare December 13, 2024 17:06

willie-yao approved these changes Dec 13, 2024

View reviewed changes

k8s-ci-robot assigned willie-yao Dec 13, 2024

k8s-ci-robot added the lgtm label Dec 13, 2024

k8s-ci-robot added the approved label Dec 13, 2024

mboersma approved these changes Dec 13, 2024

View reviewed changes

k8s-ci-robot assigned mboersma Dec 13, 2024

k8s-ci-robot merged commit 736bfbc into kubernetes-sigs:main Dec 14, 2024
31 checks passed

nawazkh deleted the make_ilb_ip_configurable branch January 17, 2025 03:57

let users configure private IP of the internal LB #5332

let users configure private IP of the internal LB #5332

Conversation

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

k8s-ci-robot commented Dec 12, 2024

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

willie-yao left a comment

Choose a reason for hiding this comment

willie-yao Dec 12, 2024

Choose a reason for hiding this comment

willie-yao Dec 12, 2024

Choose a reason for hiding this comment

nawazkh Dec 12, 2024

Choose a reason for hiding this comment

willie-yao Dec 12, 2024

Choose a reason for hiding this comment

nawazkh Dec 13, 2024

Choose a reason for hiding this comment

nawazkh Dec 13, 2024

Choose a reason for hiding this comment

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

Jont828 left a comment

Choose a reason for hiding this comment

willie-yao commented Dec 12, 2024

nawazkh commented Dec 12, 2024

nawazkh commented Dec 12, 2024

nawazkh commented Dec 13, 2024

nawazkh commented Dec 13, 2024

nawazkh commented Dec 13, 2024

nawazkh commented Dec 13, 2024

nawazkh commented Dec 13, 2024

nawazkh commented Dec 13, 2024

willie-yao left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 13, 2024

Jont828 commented Dec 13, 2024

mboersma left a comment

Choose a reason for hiding this comment

mboersma Dec 13, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 13, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading