Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add ability to use node ip as LB target #590

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add ability to use node ip as LB target #590

wants to merge 1 commit into from
+32 −4

Conversation

blitss
Copy link

@blitss blitss commented Dec 15, 2023

(sorry for any possible mistakes in this PR, I'm not really familiar with Go)
In this PR I added the ability to use node ip as the target for the Hetzner LB. The motivation behind this is that current Robot implementation only adds public IPs from the Robot to the LB, and someone might have cluster setup like this, which uses internal IP from the vswitch.

k get node -o wide
NAME    STATUS   ROLES           AGE     VERSION   INTERNAL-IP   EXTERNAL-IP   OS-IMAGE                KERNEL-VERSION   CONTAINER-RUNTIME
node1   Ready    control-plane   5h51m   v1.28.3   10.0.1.3      <none>        Talos (v1.6.0-beta.1)   6.1.65-talos     containerd://1.7.10

which just doesn't work with following implementation. I also added warning for when the name in the Robot and k8s don't match. It's opt-in and you have to specify HCLOUD_LOAD_BALANCERS_USE_NODE_IP to use that.

I ran the controller like

 ROBOT_ENABLED=true \
HCLOUD_TOKEN= ROBOT_PASSWORD=ROBOT_USER="" \
KUBECONFIG=$HOME/.kube-old/config  \
HCLOUD_NETWORK=1047143 \
HCLOUD_NETWORK_ROUTES_ENABLED=false \
HCLOUD_NETWORK_DISABLE_ATTACHED_CHECK=true \
HCLOUD_LOAD_BALANCERS_USE_NODE_IP=true \
go run . --allow-untagged-cloud  --cloud-provider=hcloud --route-reconciliation-period=30s --webhook-secure-port=0 --leader-elect=false --kubeconfig=$HOME/.kube-old/config

and it worked like a charm for me. Result:

Pasted_Image_16_12_23__02_45

Since you're specifying a network I have to also use flags like HCLOUD_NETWORK_ROUTES_ENABLED and HCLOUD_NETWORK_DISABLE_ATTACHED_CHECK.

Would love to add test for it, but I think it's going to add a lot of complexities testing this along with a vswitch.

@blitss blitss requested a review from a team as a code owner December 15, 2023 23:48
@blitss blitss changed the title feat: add ability to use node ip for the dedicated servers feat: add ability to use node ip as LB target Dec 15, 2023
@apricote
Copy link
Member

Hey @blitss, thanks for the PR! I am currently on vacation, and will take a look at this in January.

@PReimers
Copy link

PReimers commented Feb 9, 2024

Any news here?

@github-actions GitHub Actions
Copy link
Contributor

This PR has been marked as stale because it has not had recent activity. The bot will close the PR if no further action occurs.

@github-actions github-actions bot added the stale label May 10, 2024
@github-actions github-actions bot closed this Jun 9, 2024
@jooola jooola reopened this Jun 10, 2024
@jooola jooola added pinned and removed stale labels Jun 10, 2024
@blitss
Copy link
Author

blitss commented Jun 10, 2024

@jooola do you want me to resolve that changes for you or will you check it out?

@jooola
Copy link
Member

jooola commented Jun 10, 2024

@blitss Sorry for the delay, we are currently low resource to work on this Pull Request. But we don't want this PR to vanish without giving you a proper review/answer, I will therefor pin the PR for now.

No, don't bother about the conflict for now.

jooola
jooola reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a very quick review, but I'll not be able to give the final word on this PR.

hcloud/instances_util.go
@@ -91,6 +92,7 @@ func getRobotServerByID(c robot.Client, id int, node *corev1.Node) (*hrobotmodel

// check whether name matches - otherwise this server does not belong to the respective node anymore
if server.Name != node.Name {
klog.Warningf("%s: server %d has name %q, but node %q has name %q. if you want node to be matched with node in Hetzner Robot you should rename it.", op, id, server.Name, node.Name, node.Name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to this PR, but might be a good addition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want this to be a separate PR or just a separate commit?

internal/config/config.go
@@ -31,6 +31,7 @@ const (
hcloudLoadBalancersNetworkZone = "HCLOUD_LOAD_BALANCERS_NETWORK_ZONE"
hcloudLoadBalancersDisablePrivateIngress = "HCLOUD_LOAD_BALANCERS_DISABLE_PRIVATE_INGRESS"
hcloudLoadBalancersUsePrivateIP = "HCLOUD_LOAD_BALANCERS_USE_PRIVATE_IP"
hcloudLoadBalancersUseNodeIP = "HCLOUD_LOAD_BALANCERS_USE_NODE_IP"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the naming or the option, this seems to target robot server only right?

Copy link
Author

@blitss blitss Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall but I think it does yes. But we can make it work with cloud servers too, is this the way?

Update: there's an option which uses private IP and I believe it is used in conjunction with cloud servers. Do we need this option for the cloud servers in that case?

@paprickar
Copy link

paprickar commented Sep 16, 2024 •
edited
Loading

Thanks @blitss for the work!
I need it now - and took your work https://github.com/paprickar/hcloud-cloud-controller-manager/tree/pr-590 until its merged.

FYI:
I most likely will add a docker build job to keep track of upstream changes and run the modified version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jooola jooola jooola left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
pinned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blitss @apricote @PReimers @jooola @paprickar