Skip to content

Conversation

tloesch
Copy link
Contributor

@tloesch tloesch commented Sep 25, 2025

What type of PR is this?

/kind feature
/area provider/hetzner

What this PR does / why we need it:

This PR enables the optional specification of a subnet where autoscaled nodes will be placed. Previously, it was only possible to define a network ID, which caused issues when multiple subnets existed within that network.

Without subnet specification, Hetzner would place nodes randomly in one of the available subnets, leading to unpredictable network configurations and potential connectivity problems. This feature provides more precise control over node placement.

The implementation ensures that nodes receive consistent IP addresses from the specified subnet through the improved hetzner cloud api that prevents IP conflicts during scaling operations.

Which issue(s) this PR fixes:

Fixes # #5263

Special notes for your reviewer:

The implementation was developed based on the following discussion and is now possible since the changes were made to the api of hetzner and the hetzner-go library (hetznercloud/hcloud-go#723 #8554) (recommandation from @lukasmetzner):
#8334

It is possible to define a global IPRange within the new ClusterConfig and/or define it on a pool basis.
If the global value is defined and the pool is not, the global IPRange will be used for that pool.
If nothing is defined, the system will behave as before.
For more information, see the README.md file.

Question:
Should the IPRange value validation be performed earlier? For example, within the BuildHetzner function in the hetzner_cloud_provider.go file?

Does this PR introduce a user-facing change?

feat(hetzner): add IP range configuration for private network

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area labels Sep 25, 2025
@k8s-ci-robot 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 Sep 25, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @tloesch. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot added area/cluster-autoscaler area/provider/hetzner Issues or PRs related to Hetzner provider labels Sep 25, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tloesch
Once this PR has been reviewed and has the lgtm label, please assign apricote 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:

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 k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/needs-area labels Sep 25, 2025
@lukasmetzner
Copy link
Contributor

@tloesch Thanks a lot for all your efforts in pushing this feature. Unfortunately, I won't have the time to take a look at it this week. I will do ASAP 🙏

@tloesch
Copy link
Contributor Author

tloesch commented Sep 26, 2025

@tloesch Thanks a lot for all your efforts in pushing this feature. Unfortunately, I won't have the time to take a look at it this week. I will do ASAP 🙏

@lukasmetzner No pressure. I am very grateful for the notification and the efforts to review it as quickly as possible. I am very satisfied with the cooperation so far.

@elmiko
Copy link
Contributor

elmiko commented Sep 26, 2025

/ok-to-test

@k8s-ci-robot 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 Sep 26, 2025
Copy link
Contributor

@lukasmetzner lukasmetzner left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution ^^ I have left a few suggestions and improvements.

Comment on lines 460 to 473
if nodeConfig, exists := clusterConfig.NodeConfigs[nodeId]; exists && nodeConfig.IPRange != "" {
_, ipNet, err := net.ParseCIDR(nodeConfig.IPRange)
if err != nil {
return nil, fmt.Errorf("failed to parse IP range %s for node %s: %v", nodeConfig.IPRange, nodeId, err)
}
return ipNet, nil
}
if clusterConfig.IPRange != "" {
_, ipNet, err := net.ParseCIDR(clusterConfig.IPRange)
if err != nil {
return nil, fmt.Errorf("failed to parse global IP range %s: %v", clusterConfig.IPRange, err)
}
return ipNet, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the parsing part into the BuildHetzner(...) cloudprovider.Cloudprovider function and add new fields of type *net.IPNet to hetznerManager and hetznerNodeGroup. The config is static during runtime of the autoscaler, so parsing them once is enough and provides immediate feedback to the user.

type hetznerManager struct {
    // ....
    defaultSubnetIPRange *net.IPNet
}
type hetznerNodeGroup struct {
    // ....
    subnetIPRange *net.IPNet
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attribute defaultSubnetIPRange within hetznerManager would not be used by anything if the processing is done within BuildHetzner. This is because we can set the default value immediately in the nodeGroups.

I am currently testing the changes. I will publish the changes afterwards.

Please give me feedback if necessary :)

Taints []apiv1.Taint
Labels map[string]string
ImagesForArch *ImageList
IPRange string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IPRange string
SubnetIPRange string

NodeConfigs map[string]*NodeConfig
IsUsingNewFormat bool
LegacyConfig LegacyConfig
IPRange string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IPRange string
DefaultSubnetIPRange string

**NOTE**: In contrast to `HCLOUD_CLUSTER_CONFIG`, this file is not base64 encoded.

The global `imagesForArch` configuration can be overridden on a per-nodepool basis by adding an `imagesForArch` field to individual nodepool configurations.
The `ipRange` configuration can be used to place nodes within a specific IP range. This only applies to private networks. Make sure that the IP range is within the configured private network IP Range. If you do not set this value, the default setting from Hetzner Cloud will be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs update when config values change. Please add a small notice, that the subnetwork needs to exist beforehand.

Comment on lines 477 to 487
func isIpRangeInNetwork(ipRange *net.IPNet, network *hcloud.Network) bool {
found := false
for _, nSubnet := range network.Subnets {
_, nSubnetRange, _ := net.ParseCIDR(nSubnet.IPRange.String())
if nSubnetRange.String() == ipRange.String() {
found = true
break
}
}
return found
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func isIpRangeInNetwork(ipRange *net.IPNet, network *hcloud.Network) bool {
found := false
for _, nSubnet := range network.Subnets {
_, nSubnetRange, _ := net.ParseCIDR(nSubnet.IPRange.String())
if nSubnetRange.String() == ipRange.String() {
found = true
break
}
}
return found
}
func isIpRangeInNetwork(ipRange *net.IPNet, network *hcloud.Network) bool {
return slices.ContainsFunc(network.Subnets, func(subnet hcloud.NetworkSubnet) bool {
if ipRange == nil || subnet.IPRange == nil {
return false
}
return subnet.IPRange.IP.Equal(ipRange.IP) && len(subnet.IPRange.Mask) == len(ipRange.Mask)
})
}

@jackfrancis
Copy link
Contributor

/release-note-edit

feat(hetzner): add IP range configuration for private network

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 2, 2025
@k8s-ci-robot 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 Oct 6, 2025
@tloesch tloesch requested a review from lukasmetzner October 6, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-autoscaler area/provider/hetzner Issues or PRs related to Hetzner provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants