Container Load Balancer - Network Security Group (NSG) #8054

georgeedward2000 · 2025-01-17T10:13:41Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

For a container load balancer (CLB) with floating IP disabled, the Network Security Group (NSG) must allow traffic to dynamic pod IPs. To manage this:

Allow Rule for Pod Subnet: Configure the NSG to allow traffic from the entire pod subnet of the managed cluster, accommodating dynamic pod IPs.
Backend Pool Configuration: Ensure the NSG rules permit traffic to the pod IPs in the backend pool.
This setup ensures secure and efficient communication with the pod backend pool.
Unit Tests

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added the ability to set the type of backend pool to PodIP. 
This action allows the Load balancer attached to the service to route traffic directly to the containers, 
instead of routing to AKS nodes and then AKS node routing it to the PODs hosting the service.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla · 2025-01-17T10:13:45Z

❌ - login: @georgeedward2000 / name: George Edward Nechitoaia . The commit (2ff6f2d, 66de73a, 329464c, 182d11d, 3f0f834, 9e334c0, 8e0ad77, 056ae46, 873212a, f841903, 90d3039, 9141356) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

k8s-ci-robot · 2025-01-17T10:13:50Z

Welcome @georgeedward2000!

It looks like this is your first PR to kubernetes-sigs/cloud-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cloud-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-01-17T10:13:50Z

Hi @georgeedward2000. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

…tion

pkg/provider/config/azure.go

pkg/provider/azure_loadbalancer.go

kartickmsft · 2025-01-29T04:43:50Z

pkg/provider/azure_loadbalancer.go

+		}
+		if wantLb && !lbFound {
+			logger.Error(err, "Failed to get load balancer")
+			return nil, fmt.Errorf("unable to get lb %s", lbName)
 		}
 		var backendIPv4List, backendIPv6List []string
 		if lbFound {


For POD IP based backendpool, we are not populating the POD IPs in the NSG. Rather, the POD subnet. So, we could skip GetBackendPrivateIPs for POD IP based Backend pool.

This is part of the initial flow - node ip based backendpool

It is part of the block that is on the else branch of :
if az.IsLBBackendPoolTypePodIP() {
} else {
// HERE
}

pkg/provider/azure_loadbalancer.go

pkg/provider/loadbalancer/accesscontrol.go

k8s-ci-robot · 2025-02-03T17:05:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: georgeedward2000, kartickmsft
Once this PR has been reviewed and has the lgtm label, please assign nilo19 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

internal/testutil/fixture/azure_managedcluster.go

david-kow · 2025-02-04T09:57:42Z

pkg/provider/azure_loadbalancer.go

+					az.PodCidrIPv6 = prefix
+				}
+			}
+			az.RetrievedClusterPodCidr = true


Can podcidrs change during cluster operations (eg. adding new node pool)? If yes, then do we need to handle this differently to account for potential new cidrs?

From what I understand, the pod subnet (hence the cidr) is set during cluster's deployment and cannot be updated during its lifetime. Please confirm @kartickmsft

I think that can happen when completely new node pool is added: https://learn.microsoft.com/en-us/azure/aks/create-node-pools#add-a-node-pool-with-a-unique-subnet

POD subnet can be newly configured for a new nodepool for Azure CNI Dynamic IP allocation and Enhanced subnet option (https://learn.microsoft.com/en-us/azure/aks/configure-azure-cni-dynamic-ip-allocation#adding-node-pool).
So, yeah, we need to handle it differently. As per my understanding, cloud-provider doesn't get any notification about nodepool addition/deletion. If this understanding is correct, maybe reading PODCIDRs each time maybe the only option.

yes, it may change. We'd need to figure out a way to get notified on such changes

If this understanding is correct, maybe reading PODCIDRs each time maybe the only option.

This may be not enough as there may be down times when new IPs are picked while they are blocked by NSG.

david-kow · 2025-02-04T10:15:02Z

internal/testutil/fixture/azure_loadbalancer.go

@@ -47,6 +47,18 @@ func (f *AzureLoadBalancerFixture) IPv4Addresses() []string {
 	}


Can you add some details about the change in the PR description?

updated the PR description

feiskyer · 2025-02-18T05:26:33Z

@georgeedward2000 Thanks for the contribution. Could you sign CLA?

feiskyer · 2025-02-18T05:29:36Z

And could you use the template (the template would show when you open the PR) and fill the template contents (including release notes information together with what this PR does)?

/kind feature

feiskyer

As discussed, there is a security limitation of current design, please also add it as comments and TODOs.

feiskyer · 2025-02-18T05:37:00Z

pkg/provider/azure_loadbalancer.go

-		{
+
+	if az.IsLBBackendPoolTypePodIP() {
+		if !az.RetrievedClusterPodCidr {


Generally, this would block the security boundary of the Kubernetes cluster. The cluster identity should not have the permission to manage itself.

let's use new cloud config options and ask the provisioning service (e.g. AKS or capz) to setup the CIDRs on changing.

feiskyer · 2025-02-18T05:39:04Z

pkg/provider/config/azure.go

+	// If the pod subnet prefix is not set, the value is false.
+	// If the pod subnet prefix is set, the value is true.
+	// Note: Multiple rules per subnet with different protocols and destination ports can exist.
+	RetrievedClusterPodCidr bool `json:"retrievedClusterPodCidr" yaml:"retrievedClusterPodCidr"`


As commented, let's remove this option and ask provisioning service to set the PodCIDRs

feiskyer · 2025-02-18T05:41:14Z

pkg/provider/config/azure.go

+	// PodCidrIPv6 is the IPv6 pod subnet prefix for the cluster.
+	// The pod subnet prefix is used to configure the NSG for the pod subnet.
+	PodCidrIPv4 netip.Prefix `json:"podCidrIPv4" yaml:"podCidrIPv4"`
+	PodCidrIPv6 netip.Prefix `json:"podCidrIPv6" yaml:"podCidrIPv6"`


For extensibility, let's use LIST here?

feiskyer · 2025-02-18T05:43:43Z

pkg/provider/config/azure.go

+
+	// PodCidrIPv4 is the IPv4 pod subnet prefix for the cluster.
+	// PodCidrIPv6 is the IPv6 pod subnet prefix for the cluster.
+	// The pod subnet prefix is used to configure the NSG for the pod subnet.


Could you also add a message to say all Pod CIDR would be opened to internet by default?

and add a TODO comment to improve the security later?

feiskyer · 2025-02-18T05:45:05Z

Let's also bake some tests.

/ok-to-test

…tion

Co-authored-by: David Kowalski <[email protected]>

…-azure into clb-nsg

k8s-ci-robot · 2025-02-19T12:20:16Z

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

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.

nilo19 · 2025-02-20T03:25:42Z

/label tide/merge-method-squash

…ning

Update NSG - CLB Load Balancer

66de73a

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jan 17, 2025

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-kind labels Jan 17, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 17, 2025

georgeedward2000 changed the title ~~Update NSG - CLB Load Balancer~~ Update NSG - Container Load Balancer Jan 17, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 17, 2025

k8s-ci-robot requested review from MartinForReal and nilo19 January 17, 2025 10:13

Add unit tests for a part of CLB NSG flow + update CLB NSG implementa…

3f0f834

…tion

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2025

update nsg logic for clb + add/refactor tests

2ff6f2d

kartickmsft reviewed Jan 28, 2025

View reviewed changes

pkg/provider/config/azure.go Show resolved Hide resolved

kartickmsft reviewed Jan 28, 2025

View reviewed changes

pkg/provider/azure_loadbalancer.go Outdated Show resolved Hide resolved

kartickmsft reviewed Jan 28, 2025

View reviewed changes

pkg/provider/azure_loadbalancer.go Outdated Show resolved Hide resolved

kartickmsft reviewed Jan 29, 2025

View reviewed changes

pkg/provider/azure_loadbalancer.go Outdated Show resolved Hide resolved

kartickmsft reviewed Jan 29, 2025

View reviewed changes

pkg/provider/loadbalancer/accesscontrol.go Outdated Show resolved Hide resolved

update CLB NSG to support dual-stack rules

182d11d

kartickmsft approved these changes Feb 3, 2025

View reviewed changes

david-kow reviewed Feb 4, 2025

View reviewed changes

georgeedward2000 changed the title ~~Update NSG - Container Load Balancer~~ Container Load Balancer - Network Security Group (NSG) Feb 4, 2025

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 18, 2025

k8s-ci-robot removed the do-not-merge/needs-kind label Feb 18, 2025

feiskyer requested changes Feb 18, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 18, 2025

georgeedward2000 and others added 5 commits February 18, 2025 12:54

Update NSG - CLB Load Balancer

056ae46

Add unit tests for a part of CLB NSG flow + update CLB NSG implementa…

8e0ad77

…tion

update nsg logic for clb + add/refactor tests

329464c

update CLB NSG to support dual-stack rules

9e334c0

Update internal/testutil/fixture/azure_managedcluster.go

9141356

Co-authored-by: David Kowalski <[email protected]>

georgeedward2000 force-pushed the clb-nsg branch from 8bf63d9 to 9141356 Compare February 18, 2025 10:54

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 18, 2025

Merge branch 'clb-nsg' of https://github.com/david-kow/cloud-provider…

f841903

…-azure into clb-nsg

k8s-ci-robot added the do-not-merge/contains-merge-commits label Feb 19, 2025

Typo

873212a

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 20, 2025

georgeedward2000 mentioned this pull request Feb 21, 2025

Add support for backendpool of type PodIP #8395

Open

update code + tests for new context: pod cidrs setup through provisio…

90d3039

…ning

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Load Balancer - Network Security Group (NSG) #8054

Container Load Balancer - Network Security Group (NSG) #8054

georgeedward2000 commented Jan 17, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 17, 2025 •

edited

Loading

k8s-ci-robot commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

kartickmsft Jan 29, 2025

georgeedward2000 Jan 29, 2025 •

edited

Loading

k8s-ci-robot commented Feb 3, 2025

david-kow Feb 4, 2025

georgeedward2000 Feb 4, 2025

david-kow Feb 5, 2025

kartickmsft Feb 5, 2025

feiskyer Feb 18, 2025

feiskyer Feb 18, 2025 •

edited

Loading

david-kow Feb 4, 2025

georgeedward2000 Feb 4, 2025

feiskyer commented Feb 18, 2025

feiskyer commented Feb 18, 2025

feiskyer left a comment

feiskyer Feb 18, 2025

feiskyer Feb 18, 2025

feiskyer Feb 18, 2025

feiskyer Feb 18, 2025

feiskyer Feb 18, 2025

feiskyer commented Feb 18, 2025

k8s-ci-robot commented Feb 19, 2025

nilo19 commented Feb 20, 2025

		@@ -47,6 +47,18 @@ func (f *AzureLoadBalancerFixture) IPv4Addresses() []string {
		}

Container Load Balancer - Network Security Group (NSG) #8054

Are you sure you want to change the base?

Container Load Balancer - Network Security Group (NSG) #8054

Conversation

georgeedward2000 commented Jan 17, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Jan 17, 2025 • edited Loading

k8s-ci-robot commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

Choose a reason for hiding this comment

georgeedward2000 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 3, 2025

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

feiskyer Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Feb 18, 2025

feiskyer commented Feb 18, 2025

feiskyer 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

feiskyer commented Feb 18, 2025

k8s-ci-robot commented Feb 19, 2025

nilo19 commented Feb 20, 2025

georgeedward2000 commented Jan 17, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 17, 2025 •

edited

Loading

georgeedward2000 Jan 29, 2025 •

edited

Loading

feiskyer Feb 18, 2025 •

edited

Loading