🐛 Expose NodePorts on cluster network instead of 0.0.0.0/0 #2128

huxcrux · 2024-06-18T14:24:48Z

What this PR does / why we need it:

This PR changes NodePorts to only be open from cluster network.

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

Special notes for your reviewer:

I assume this is something that would be in 0.11 or 1.0? If so where do we put the changelog?

I also need to make sure what IP ranged we potentially want to use since there is multiple ways of specifying them depending on of they are operator managed or not.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

k8s-ci-robot · 2024-06-18T14:25:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cecilerobertmichon for approval. For more information see the Kubernetes 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

netlify · 2024-06-18T14:25:15Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`8230a26`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/66e00e8b3dd9ab00083552aa
😎 Deploy Preview	https://deploy-preview-2128--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mdbooth · 2024-06-19T08:53:16Z

pkg/cloud/services/networking/securitygroups.go

+	if openStackCluster.Spec.ManagedSubnets != nil && len(openStackCluster.Spec.ManagedSubnets) > 0 {
+		SubnetCIDR = openStackCluster.Spec.ManagedSubnets[0].CIDR
+	} else {
+		SubnetCIDR = "0.0.0.0/0"
+	}


Can we not use remote group for this? This won't work on deployments which aren't using managed subnets.

I just changed the behaviour to make use of status.Network.Subnet instead to support use cases where bring your own network/subnet is used.

I do not think we could use remote group due to load balancers talking over nodeports?
Load Balancers are created under the "service" project in default openstack meaning it is not added to any security groups inside the project where the cluster is running.

I also left a question in the code that may be good to bring up here.

If no IPv4 subnet is found the code currently defaults to 0.0.0.0/0 to preserve current behaviour.

We could however return an error if no IPv4 subnet is found or potentially fallback to just allowing nodes by using remote groups (this will however break octavia load balancers created using the cloud provider which feels kinda bad).

In other places we just add the rule twice with 2 remote groups, one for the control plane and one for workers. This covers machines even when they aren't using the default cluster network. Is there a reason we can't do that here?

I'd like to reduce our reliance on the cluster's status.Network over time. There are new, interesting deployments that it doesn't play nicely with, e.g. external control planes and spine/leaf.

In other places we just add the rule twice with 2 remote groups, one for the control plane and one for workers. This covers machines even when they aren't using the default cluster network. Is there a reason we can't do that here?

I'd like to reduce our reliance on the cluster's status.Network over time. There are new, interesting deployments that it doesn't play nicely with, e.g. external control planes and spine/leaf.

It would break load balancers deployed using the cloud provider (octavia) since they are not in any security group we control (know the ID of). I think it is a really common use case and support out of the box would be nice

Just wanted to follow up on this.

We could add security groups meaning all cluster nodes can contact each other and it will make the conformance test pass. However it would cause LBs created with the cloud provider to fail. I'm not at this point of any way of allowing LBs outside allowing IPs directly towards nodes.

I also think having a default where LBs "work" is preferable to have a default that works "out of the box".

…network is found

jichenjc · 2024-07-03T02:24:59Z

/test pull-cluster-api-provider-openstack-test

…roups and update comments

linux-foundation-easycla · 2024-09-10T09:17:01Z

The committers listed above are authorized under a signed CLA.

✅ login: huxcrux / name: Hugo Blom (80a63e3, 4ec41d2, f2ee085, 24259ba, cd61310, 1685c65, ba3879f, 1445bad, 89bbf42, 8b647e4)
✅ login: mikaelgron / name: Mikael Grön (8230a26)

mikaelgron · 2024-09-10T12:15:36Z

/test pull-cluster-api-provider-openstack-test

mdbooth · 2024-09-26T13:30:05Z

pkg/cloud/services/networking/securitygroups.go

@@ -169,6 +170,7 @@ func (s *Service) generateDesiredSecGroups(openStackCluster *infrav1.OpenStackCl
 	var secControlPlaneGroupID string
 	var secWorkerGroupID string
 	var secBastionGroupID string
+	var SubnetCIDR string


Suggested change

var SubnetCIDR string

var subnetCIDR string

mdbooth · 2024-09-26T13:36:41Z

pkg/cloud/services/networking/securitygroups_rules.go

+	}
+}
+
+// Allow all traffic, including from outside the cluster, to access node port services.


Suggested change

// Allow all traffic, including from outside the cluster, to access node port services.

// Allow all traffic from a specific CIDR to access node port services.

mdbooth · 2024-09-26T13:36:53Z

pkg/cloud/services/networking/securitygroups_rules.go

+}
+
+// Allow all traffic, including from outside the cluster, to access node port services.
+func getSGWorkerNodePortCidr(cidr string) []resolvedSecurityGroupRuleSpec {


Suggested change

func getSGWorkerNodePortCidr(cidr string) []resolvedSecurityGroupRuleSpec {

func getSGWorkerNodePortCIDR(cidr string) []resolvedSecurityGroupRuleSpec {

mdbooth · 2024-09-26T13:38:00Z

pkg/cloud/services/networking/securitygroups.go

-	workerRules = append(workerRules, getSGWorkerNodePort()...)
+
+	// Fetch subnet to use for worker node port rules
+	// In the future IPv6 support need to be added here


IPv4-only is icky, but everything else is still IPv4 only and it's not worth fixing it piecemeal so this is fine.

mdbooth · 2024-09-26T13:39:37Z

pkg/cloud/services/networking/securitygroups.go

+
+	// Fetch subnet to use for worker node port rules
+	// In the future IPv6 support need to be added here
+	if openStackCluster.Status.Network != nil && len(openStackCluster.Status.Network.Subnets) > 0 {


Iterating over a nil or empty list is safe and idiomatic. The loop below performs this check implicitly anyway.

Suggested change

if openStackCluster.Status.Network != nil && len(openStackCluster.Status.Network.Subnets) > 0 {

if openStackCluster.Status.Network != nil {

mdbooth · 2024-09-26T13:39:59Z

pkg/cloud/services/networking/securitygroups.go

+	// In the future IPv6 support need to be added here
+	if openStackCluster.Status.Network != nil && len(openStackCluster.Status.Network.Subnets) > 0 {
+		for _, subnet := range openStackCluster.Status.Network.Subnets {
+			// Check uf subnet.CIDR is ipv4 subnet


Suggested change

// Check uf subnet.CIDR is ipv4 subnet

// Check if subnet.CIDR is ipv4 subnet

mdbooth · 2024-09-26T13:45:30Z

pkg/cloud/services/networking/securitygroups.go

+			_, ipnet, err := net.ParseCIDR(subnet.CIDR)
+			if err != nil {
+				return nil, fmt.Errorf("invalid subnet found during security groups reconcile: %v", err)
+			}
+			if ipnet.IP.To4() != nil {
+				SubnetCIDR = subnet.CIDR
+				break
+			}


This whole block can be replaced with netutils.IsIPv4CIDRString(subnet.CIDR)

mdbooth · 2024-09-26T13:47:09Z

pkg/cloud/services/networking/securitygroups.go

+	}
+	if SubnetCIDR != "" {
+		// If SubnetCIDR is found we allow tcp and udp traffic from said SubnetCIDR, this in order to allow octavia Loadbalancers created by CCM to function properly
+		workerRules = append(workerRules, getSGWorkerNodePortCidr(SubnetCIDR)...)


You could move this inside the block above and also remove the break so you simply add any listed IPv4 subnet. Any other restrictions we have on subnets aren't relevant here, so might as well make it handle whatever's there.

mdbooth · 2024-09-26T13:51:46Z

Please can you also add a code comment in there about why we're explicitly adding this subnet? Namely because this is the default subnet used by CPO as the origin of traffic from Octavia load balancers to load balancer members? Otherwise it risks being deleted by somebody because it looks redundant, even me in 6 months when I've forgotten this discussion again 🙄

Only allow nodeports from CIDR in first managed subnet

1685c65

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

k8s-ci-robot requested review from jichenjc and mdbooth June 18, 2024 14:25

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 18, 2024

Fix formating

cd61310

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 19, 2024

Fallback to 0.0.0.0/0 if no subnet was found

4ec41d2

mdbooth reviewed Jun 19, 2024

View reviewed changes

huxcrux and others added 7 commits June 19, 2024 10:55

Make use of status instead since subnets have already been verified

80a63e3

Make sure that all comments have a space between the slashes and text

24259ba

Always allow nodeports from cluster nodes and fail closed if no IPv4 …

ba3879f

…network is found

Formating fix

1445bad

Fix more formatting

f2ee085

even more lint fixes

89bbf42

Update tests to contain the new number of secrules

8b647e4

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2024

Change the number of expected rules in TestService_ReconcileSecurityG…

8230a26

…roups and update comments

mdbooth reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Expose NodePorts on cluster network instead of 0.0.0.0/0 #2128

🐛 Expose NodePorts on cluster network instead of 0.0.0.0/0 #2128

huxcrux commented Jun 18, 2024

k8s-ci-robot commented Jun 18, 2024

netlify bot commented Jun 18, 2024 •

edited

Loading

mdbooth Jun 19, 2024

huxcrux Jun 19, 2024

huxcrux Jun 19, 2024

mdbooth Jun 19, 2024

huxcrux Jun 19, 2024

huxcrux Jun 26, 2024

jichenjc commented Jul 3, 2024

linux-foundation-easycla bot commented Sep 10, 2024 •

edited

Loading

mikaelgron commented Sep 10, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth Sep 26, 2024

mdbooth commented Sep 26, 2024

	// Allow all traffic, including from outside the cluster, to access node port services.
	// Allow all traffic from a specific CIDR to access node port services.

	func getSGWorkerNodePortCidr(cidr string) []resolvedSecurityGroupRuleSpec {
	func getSGWorkerNodePortCIDR(cidr string) []resolvedSecurityGroupRuleSpec {

	if openStackCluster.Status.Network != nil && len(openStackCluster.Status.Network.Subnets) > 0 {
	if openStackCluster.Status.Network != nil {

	// Check uf subnet.CIDR is ipv4 subnet
	// Check if subnet.CIDR is ipv4 subnet

🐛 Expose NodePorts on cluster network instead of 0.0.0.0/0 #2128

Are you sure you want to change the base?

🐛 Expose NodePorts on cluster network instead of 0.0.0.0/0 #2128

Conversation

huxcrux commented Jun 18, 2024

k8s-ci-robot commented Jun 18, 2024

netlify bot commented Jun 18, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

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

jichenjc commented Jul 3, 2024

linux-foundation-easycla bot commented Sep 10, 2024 • edited Loading

mikaelgron commented Sep 10, 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

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

mdbooth commented Sep 26, 2024

netlify bot commented Jun 18, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 10, 2024 •

edited

Loading