✨ Enable Worker Nodes to Associate with Floating IPs #1725

mikaelgron · 2023-10-12T12:31:24Z

What this PR does / why we need it:

This PR aims to enable worker nodes to associate with pre-allocated Floating IPs in OpenStack. The use-case is primarily for clients who want to whitelist their worker nodes in external services. By specifying a set of Floating IPs during the cluster setup, worker nodes can be configured to reuse these IPs, thus avoiding the need to track new floating IPs.

Note: This feature is currently in an experimental phase, and we are looking for feedback. It's not targeted to join alpha8 yet.

Future Steps

We're already contemplating further enhancements to add dynamicity to this feature. One idea is to allow the cluster CRD to dynamically create Floating IPs based on the number of machine deployments + X or something like that. While this isn't part of the current PR, it is a direction we're interested in exploring.

Feedback Requested
We are actively looking for feedback on this feature to understand its viability and possible improvements. Feel free to comment with your suggestions, questions, or concerns.

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

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

/hold

linux-foundation-easycla · 2023-10-12T12:31:31Z

✅ login: huxcrux / name: Hugo Blom (218c3db, 0875e8a)
❌ - login: @mikaelgron / name: Mikael Grön . The commit (bf8a23c, de9a1b2, 8da55ac) 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 · 2023-10-12T12:31:33Z

Welcome @mikaelgron!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-openstack 🎉. 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/cluster-api-provider-openstack 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 · 2023-10-12T12:31:34Z

Hi @mikaelgron. 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/test-infra repository.

k8s-ci-robot · 2023-10-12T12:31:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mikaelgron
Once this PR has been reviewed and has the lgtm label, please assign neolit123 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 · 2023-10-12T12:31:49Z

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

Name	Link
🔨 Latest commit	`8da55ac`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/652d378c5c3a240008596206
😎 Deploy Preview	https://deploy-preview-1725--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.

dulek · 2023-10-12T12:51:22Z

/ok-to-test

dulek · 2023-10-12T12:57:17Z

controllers/openstackmachine_controller.go

+			}
+
+			if fp.PortID != "" {
+				scope.Logger().Info("Floating IP already associated to a port", "id", fp.ID, "fixedIP", fp.FixedIP, "portID", port.ID)


This feels like something happening all the time, it should have lower log level.

Agreed. It should perhaps be reduced on the APIServerLoadBalancer flow on line 426 as well.

dulek · 2023-10-12T12:58:08Z

controllers/openstackmachine_controller.go

+					return ctrl.Result{}, fmt.Errorf("associate floating IP %q to worker node with port %q: %w", fp.FloatingIP, port.ID, err)
+				}
+			}
+		}


We should log a situation when node haven't got any FIP because pool got exhausted.

dulek · 2023-10-12T12:58:35Z

controllers/openstackmachine_controller.go

+	if !util.IsControlPlaneMachine(machine) && openStackCluster.Spec.WorkerFloatingIPConfig.Enabled {
+		scope.Logger().Info("Processing worker floating IPs")
+		for _, floatingIP := range openStackCluster.Spec.WorkerFloatingIPConfig.IPAddresses {
+			fp, err := networkingService.GetOrCreateFloatingIP(openStackMachine, openStackCluster, clusterName, floatingIP)


If FIP actually got created, where do we remove it on cluster deinstallation?

For the use case where you have to create IPs first to get the actual IP address strings to put in the spec, they would need to be removed manually anyway. It would be better to use a getter instead of a get-or-create in this case.

dulek · 2023-10-12T12:59:11Z

api/v1alpha7/types.go

@@ -29,6 +29,11 @@ type ExternalRouterIPParam struct {
 	Subnet SubnetFilter `json:"subnet"`
 }

+type WorkerFloatingIPConfig struct {
+	Enabled     bool     `json:"enabled,omitempty"`


Do we even need this flag? Could a nil or empty list work as a disabled state?

mdbooth

Let me start with a negative: I'm not a fan of the design here, particularly that this would apply to all workers without any obvious mechanism for restricting it. It also touches floating IPs in the machine controller, which is already an almighty mess. I actually opened #1674 to track removing floatingIP from the machine spec because it can't be used correctly.

However, the use case you describe sounds interesting. I'd like to help you implement it.

This sounds like it could be an IPAM controller? I believe this was first implemented by metal3, so @lentzi90 may know more about it. How about we created an OpenStackFloatingIPPool. It could contain your pre-populated list of FIPs, or create them on demand. We could add floatingIP to PortOpts, except that instead of being an IP it would be a reference to an IPAM provider (in this case our OpenStackFloatingIPPool). The machine controller would create an IPClaim for the port and wait for an IPAddress to be allocated, at which point it could create the port.

The metal3 docs are here: https://book.metal3.io/ipam/introduction.html. There was talk of CAPI adopting it, and I understand CAPV are also using it.

controllers/openstackmachine_controller.go

jichenjc · 2023-10-17T07:58:26Z

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

k8s-ci-robot · 2023-10-17T08:08:09Z

@mikaelgron: 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-openstack-test	`8da55ac`	link	true	`/test pull-cluster-api-provider-openstack-test`

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.

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.

k8s-triage-robot · 2024-01-22T12:29:51Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dulek · 2024-01-23T16:39:00Z

Looks like something being implemented by #1763?

mdbooth · 2024-01-30T12:41:12Z

Closed in favour of #1762

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 12, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 12, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 12, 2023

k8s-ci-robot requested a review from jichenjc October 12, 2023 12:31

k8s-ci-robot requested a review from seanschneeweiss October 12, 2023 12:31

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 Oct 12, 2023

dulek suggested changes Oct 12, 2023

View reviewed changes

mdbooth reviewed Oct 13, 2023

View reviewed changes

controllers/openstackmachine_controller.go Outdated Show resolved Hide resolved

mikaelgron and others added 5 commits October 16, 2023 15:14

Add bool for floating ips on worker nodes

bf8a23c

Move floating IP control into machine

de9a1b2

Add missing check to get init functionality working

218c3db

Add latest crd after rebase

0875e8a

Stop removing FIP-assoc when worker is removed

8da55ac

mikaelgron force-pushed the worker-floating-ip branch from 9d64ad3 to 8da55ac Compare October 16, 2023 13:15

huxcrux mentioned this pull request Nov 15, 2023

Implementing IPAM for Floating IP (and potential other usage in the future) #1750

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024

mdbooth closed this Jan 30, 2024

huxcrux deleted the worker-floating-ip branch March 9, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Enable Worker Nodes to Associate with Floating IPs #1725

✨ Enable Worker Nodes to Associate with Floating IPs #1725

mikaelgron commented Oct 12, 2023

linux-foundation-easycla bot commented Oct 12, 2023 •

edited

Loading

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

netlify bot commented Oct 12, 2023 •

edited

Loading

dulek commented Oct 12, 2023

dulek Oct 12, 2023

mikaelgron Oct 16, 2023

dulek Oct 12, 2023

dulek Oct 12, 2023

mikaelgron Oct 16, 2023

dulek Oct 12, 2023

mdbooth left a comment

jichenjc commented Oct 17, 2023

k8s-ci-robot commented Oct 17, 2023

k8s-triage-robot commented Jan 22, 2024

dulek commented Jan 23, 2024

mdbooth commented Jan 30, 2024

✨ Enable Worker Nodes to Associate with Floating IPs #1725

✨ Enable Worker Nodes to Associate with Floating IPs #1725

Conversation

mikaelgron commented Oct 12, 2023

linux-foundation-easycla bot commented Oct 12, 2023 • edited Loading

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

netlify bot commented Oct 12, 2023 • edited Loading

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

dulek commented Oct 12, 2023

dulek Oct 12, 2023

Choose a reason for hiding this comment

mikaelgron Oct 16, 2023

Choose a reason for hiding this comment

dulek Oct 12, 2023

Choose a reason for hiding this comment

dulek Oct 12, 2023

Choose a reason for hiding this comment

mikaelgron Oct 16, 2023

Choose a reason for hiding this comment

dulek Oct 12, 2023

Choose a reason for hiding this comment

mdbooth left a comment

Choose a reason for hiding this comment

jichenjc commented Oct 17, 2023

k8s-ci-robot commented Oct 17, 2023

k8s-triage-robot commented Jan 22, 2024

dulek commented Jan 23, 2024

mdbooth commented Jan 30, 2024

linux-foundation-easycla bot commented Oct 12, 2023 •

edited

Loading

netlify bot commented Oct 12, 2023 •

edited

Loading