-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(hetzner): add IP range configuration for private network #8570
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.
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
base: master
Are you sure you want to change the base?
feat(hetzner): add IP range configuration for private network #8570
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tloesch 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 |
@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. |
/ok-to-test |
There was a problem hiding this 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.
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 | ||
} |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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 pasring 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IPRange string | |
SubnetIPRange string |
NodeConfigs map[string]*NodeConfig | ||
IsUsingNewFormat bool | ||
LegacyConfig LegacyConfig | ||
IPRange string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
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.
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
}) | |
} |
/release-note-edit
|
refactor some code update README.md
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 thehetzner_cloud_provider.go
file?Does this PR introduce a user-facing change?