Peer AKS VNet for apiserver-ilb template #5288

nawazkh · 2024-11-19T00:52:41Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR updates Tiltfile:
- peers AKS VNets with workload cluster's VNets.
- creates Private DNS zone to route ${CLUSTER_NAME}-${APISERVER_LB_DNS_SUFFIX}.${AZURE_LOCATION}.cloudapp.azure.com to private IP (${AZURE_INTERNAL_LB_PRIVATE_IP})
- adds deletion logic to clear any VNet peering on apiserver-ilb flavor deletion.

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

Special notes for your reviewer:

TODOs:

squashed commits
includes documentation
adds unit tests
cherry-pick candidate

Release note:

Enable Tilt development for apiserver-ilb templates

k8s-ci-robot · 2024-11-19T00:52:44Z

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 · 2024-11-19T01:00:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.43%. Comparing base (2239e3b) to head (73cd188).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5288   +/-   ##
=======================================
  Coverage   52.43%   52.43%           
=======================================
  Files         272      272           
  Lines       29401    29401           
=======================================
  Hits        15417    15417           
  Misses      13178    13178           
  Partials      806      806

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

nawazkh · 2024-11-19T23:07:02Z

Tiltfile

+    if settings.get("container_args"):
+        capz_container_args = settings.get("container_args").get("capz-controller-manager")


We have often struggled in getting more logs outputted while running in tilt.
With this change, if we add the below yaml fields to our tilt-settings.yaml, we will can play around with more logging

container_args: capz-controller-manager: - "--v=2"

nawazkh · 2024-11-20T19:54:30Z

/retest

nawazkh · 2024-11-22T04:48:38Z

this PR needs to be held for feature flag for ILB can be added.
/hold

nawazkh · 2024-12-30T18:14:58Z

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

Tiltfile

mboersma · 2024-12-30T18:48:31Z

Tiltfile

+def peer_vnets():
+    # TODO: check for az cli to be installed in local
+    # wait for AKS VNet to be in the state created
+    peering_cmd = "; echo \"--------Peering VNETs--------\""


Could we maybe use multiline string literals to make this more readable?

peering_cmd = """; echo "--------Peering VNETs--------" ; az network vnet wait --resource-group ${AKS_RESOURCE_GROUP} --name ${AKS_MGMT_VNET_NAME} --created --timeout 180 ; export MGMT_VNET_ID=$(az network vnet show --resource-group ${AKS_RESOURCE_GROUP} --name ${AKS_MGMT_VNET_NAME} --query id --output tsv) ; echo " 1/8 ${AKS_MGMT_VNET_NAME} found " """

Thank you for pointing multi-line string literal. I did not use it here because multi-line string literal made it harder for me to debug/print 😅.

Plus, this code block is a result of iterative development, hence each command is broken over multiple lines.

I can put it in multi-line string literal if needed. What do you say ?

I think it would be easier to read if each group (each section starting with a comment) were one multiline string. So you'd still be doing string concatenation, but with longer, logically coherent strings without escape characters. But it's mostly a style thing, lgtm otherwise.

Updating it so :)

I tried updating it to multi-line string literal and realized that

local_resource.cmd needs sh -ec prefix else the local_resource() considers the series of commands as a filename

multi-line literals is pushing me into syntax errors.

I will try one more time to update these commands to multi-line else will open up an optimization issue regarding this update.

I didn't mean to break things, I thought it would be straightforward to substitute multiline string literals here. But if it's too fiddly, let's first make sure the commands are working and then make this a future cleanup task.

Got it working :)

cluster.cluster.x-k8s.io/apiserver-ilb-20737 created azurecluster.infrastructure.cluster.x-k8s.io/apiserver-ilb-20737 created kubeadmcontrolplane.controlplane.cluster.x-k8s.io/apiserver-ilb-20737-control-plane created azuremachinetemplate.infrastructure.cluster.x-k8s.io/apiserver-ilb-20737-control-plane created machinedeployment.cluster.x-k8s.io/apiserver-ilb-20737-md-0 created azuremachinetemplate.infrastructure.cluster.x-k8s.io/apiserver-ilb-20737-md-0 created kubeadmconfigtemplate.bootstrap.cluster.x-k8s.io/apiserver-ilb-20737-md-0 created azureclusteridentity.infrastructure.cluster.x-k8s.io/cluster-identity-ci unchanged Cluster apiserver-ilb-20737 created, don't forget to delete Waiting for kubeconfig to be available Kubeconfig for apiserver-ilb-20737 created and saved in the local Waiting for apiserver-ilb-20737 API Server to be accessible API Server of apiserver-ilb-20737 is accessible --------Peering VNETs-------- 1/8 aks-mgmt-vnet-21778 found 2/8 apiserver-ilb-20737-vnet found 3/8 mgmt-to-apiserver-ilb-20737 peering created in aks-mgmt-vnet-21778 4/8 apiserver-ilb-20737-to-mgmt peering created in apiserver-ilb-20737-vnet 5/8 apiserver-ilb-20737-cthekbl4yg.northeurope.cloudapp.azure.com private DNS zone created in apiserver-ilb-20737 6/8 workload cluster vnet apiserver-ilb-20737-vnet linked with private DNS zone 7/8 management cluster vnet aks-mgmt-vnet-21778 linked with private DNS zone 8/8 @ private DNS zone record created to point apiserver-ilb-20737-cthekbl4yg.northeurope.cloudapp.azure.com to 30.0.11.100 NAME: cloud-provider-azure-1735849146 LAST DEPLOYED: Thu Jan 2 12:19:08 2025 NAMESPACE: default STATUS: deployed REVISION: 1 TEST SUITE: None NAME: calico LAST DEPLOYED: Thu Jan 2 12:19:25 2025 NAMESPACE: tigera-operator STATUS: deployed REVISION: 1 TEST SUITE: None

nawazkh · 2024-12-30T21:36:11Z

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

nawazkh · 2024-12-30T22:45:46Z

/test

k8s-ci-robot · 2024-12-30T22:45:49Z

@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-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-30T22:46:01Z

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

nawazkh · 2024-12-30T22:46:59Z

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

nawazkh · 2024-12-31T01:59:32Z

I am facing issues in my local to build and test local CAPZ images.
/hold

mboersma

This lgtm, just had another minor question about the string output.

mboersma · 2024-12-31T22:56:23Z

Tiltfile

+    # TODO: check for az cli to be installed in local
+    # wait for AKS VNet to be in the state created
+    peering_cmd = '''
+    ; echo \"--------Peering VNETs--------\""


Are we trying to print the quotation marks? If so, this is correct, but I wasn't sure why they would be included. Quotes don't need to be escaped generally in multiline string literals, or included for echo. I thought the output

echo --------Peering VNETs--------

was sufficient, but I may be confused.

Encapsulating the string that needs to be echoed does not seem to be printing the quotes in the final output.
Example:

. . . --------Peering VNETs-------- 1/8 aks-mgmt-vnet-21778 found 2/8 apiserver-ilb-20737-vnet found 3/8 mgmt-to-apiserver-ilb-20737 peering created in aks-mgmt-vnet-21778 4/8 apiserver-ilb-20737-to-mgmt peering created in apiserver-ilb-20737-vnet 5/8 apiserver-ilb-20737-cthekbl4yg.northeurope.cloudapp.azure.com private DNS zone created in apiserver-ilb-20737 6/8 workload cluster vnet apiserver-ilb-20737-vnet linked with private DNS zone 7/8 management cluster vnet aks-mgmt-vnet-21778 linked with private DNS zone 8/8 @ private DNS zone record created to point apiserver-ilb-20737-cthekbl4yg.northeurope.cloudapp.azure.com to 30.0.11.100 . . .

So I think we can leave the " in there

nawazkh · 2025-01-02T20:36:57Z

/unhold

- update aks-as-mgmt scripts with VNet creation and all clusters deletion - internal LB IP can be set using a env variable

nawazkh · 2025-01-02T20:59:18Z

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

nawazkh · 2025-01-02T20:59:37Z

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

mboersma

/lgtm

k8s-ci-robot · 2025-01-02T22:33:18Z

LGTM label has been added.

Git tree hash: a6fee83bd86db191d5cfbea66b0189749fe8614b

nawazkh · 2025-01-02T23:29:39Z

/approve

k8s-ci-robot · 2025-01-02T23:29:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nawazkh

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~~ [nawazkh]

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 requested review from jackfrancis and marosset November 19, 2024 00:52

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 19, 2024

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch from d24a789 to dd27c47 Compare November 19, 2024 00:56

nawazkh marked this pull request as ready for review November 19, 2024 00:56

k8s-ci-robot requested a review from nojnhuh November 19, 2024 00:56

nawazkh mentioned this pull request Nov 19, 2024

Update Makefile to install az cli if unavailable #5291

Open

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch 2 times, most recently from cbba70f to 3025950 Compare November 19, 2024 22:53

nawazkh commented Nov 19, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 22, 2024

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch from 3025950 to b3a6424 Compare December 14, 2024 07:47

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 14, 2024

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch from cca6f89 to f0b219e Compare December 14, 2024 08:34

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch 2 times, most recently from 9ba18dd to 3d125d8 Compare December 30, 2024 11:52

nawazkh changed the title ~~[WIP] Peer AKS VNet (mgmt) with VM based default templates~~ Peer AKS VNet for apiserver-ilb template Dec 30, 2024

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 Dec 30, 2024

mboersma reviewed Dec 30, 2024

View reviewed changes

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch from cfecb11 to 92c8a72 Compare December 30, 2024 20:16

nawazkh requested a review from mboersma December 30, 2024 20:16

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

mboersma reviewed Dec 31, 2024

View reviewed changes

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch from f3c595d to 96bfced Compare January 2, 2025 20:30

nawazkh requested a review from mboersma January 2, 2025 20:36

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

Update Tiltfile with AKS VNet peering and deletion logic

73cd188

- update aks-as-mgmt scripts with VNet creation and all clusters deletion - internal LB IP can be set using a env variable

nawazkh force-pushed the peer-aks-vnet-with-in-tiltfile branch from 96bfced to 73cd188 Compare January 2, 2025 20:56

mboersma approved these changes Jan 2, 2025

View reviewed changes

k8s-ci-robot assigned mboersma Jan 2, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2025

k8s-ci-robot merged commit 6bce7f8 into kubernetes-sigs:main Jan 2, 2025
28 of 31 checks passed

k8s-ci-robot added this to the v1.18 milestone Jan 2, 2025

nawazkh deleted the peer-aks-vnet-with-in-tiltfile branch January 3, 2025 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Peer AKS VNet for apiserver-ilb template #5288

Peer AKS VNet for apiserver-ilb template #5288

nawazkh commented Nov 19, 2024 •

edited

Loading

k8s-ci-robot commented Nov 19, 2024

codecov bot commented Nov 19, 2024 •

edited

Loading

nawazkh Nov 19, 2024

This comment was marked as outdated.

nawazkh commented Nov 20, 2024

nawazkh commented Nov 22, 2024

nawazkh commented Dec 30, 2024

mboersma Dec 30, 2024

nawazkh Dec 30, 2024

mboersma Dec 30, 2024

nawazkh Dec 31, 2024

nawazkh Jan 2, 2025 •

edited

Loading

mboersma Jan 2, 2025

nawazkh Jan 2, 2025

nawazkh commented Dec 30, 2024

nawazkh commented Dec 30, 2024

k8s-ci-robot commented Dec 30, 2024

nawazkh commented Dec 30, 2024

nawazkh commented Dec 30, 2024

nawazkh commented Dec 31, 2024

mboersma left a comment

mboersma Dec 31, 2024 •

edited

Loading

nawazkh Jan 2, 2025

nawazkh commented Jan 2, 2025

nawazkh commented Jan 2, 2025

nawazkh commented Jan 2, 2025

mboersma left a comment

k8s-ci-robot commented Jan 2, 2025

nawazkh commented Jan 2, 2025

k8s-ci-robot commented Jan 2, 2025

		if settings.get("container_args"):
		capz_container_args = settings.get("container_args").get("capz-controller-manager")

Peer AKS VNet for apiserver-ilb template #5288

Peer AKS VNet for apiserver-ilb template #5288

Conversation

nawazkh commented Nov 19, 2024 • edited Loading

k8s-ci-robot commented Nov 19, 2024

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

This comment was marked as outdated.

nawazkh commented Nov 20, 2024

nawazkh commented Nov 22, 2024

nawazkh commented Dec 30, 2024

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

nawazkh Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nawazkh commented Dec 30, 2024

nawazkh commented Dec 30, 2024

k8s-ci-robot commented Dec 30, 2024

nawazkh commented Dec 30, 2024

nawazkh commented Dec 30, 2024

nawazkh commented Dec 31, 2024

mboersma left a comment

Choose a reason for hiding this comment

mboersma Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nawazkh commented Jan 2, 2025

nawazkh commented Jan 2, 2025

nawazkh commented Jan 2, 2025

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 2, 2025

nawazkh commented Jan 2, 2025

k8s-ci-robot commented Jan 2, 2025

nawazkh commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

nawazkh Jan 2, 2025 •

edited

Loading

mboersma Dec 31, 2024 •

edited

Loading