-
-
Notifications
You must be signed in to change notification settings - Fork 509
Fix load balancer IP configuration for hcloud provider 1.56+ #1954
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?
Fix load balancer IP configuration for hcloud provider 1.56+ #1954
Conversation
- Change IP allocation from hardcoded 254 to -2 (last usable IP in subnet) - Remove 'ip' from ignore_changes lifecycle to allow proper management with hcloud provider 1.56+ - Add comments explaining the changes
Summary of ChangesHello @arbianshkodra, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue with load balancer IP configuration when using Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request correctly addresses the load balancer IP configuration issue with hcloud provider v1.56+ by using a dynamic IP from cidrhost and allowing the provider to manage the IP lifecycle. The changes are well-documented with comments and the PR description provides clear migration notes. My main feedback is to apply the same fix to a similar resource for the control plane load balancer, which appears to have been overlooked, to ensure consistency and prevent the same issue from occurring there.
| ip = cidrhost( | ||
| ( | ||
| length(hcloud_network_subnet.agent) > 0 | ||
| ? hcloud_network_subnet.agent.*.ip_range[0] | ||
| : hcloud_network_subnet.control_plane.*.ip_range[0] | ||
| ) | ||
| , 254) | ||
| , -2) |
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.
This change to use -2 in cidrhost is correct and makes the IP allocation more robust for different subnet sizes.
I noticed that a similar resource, hcloud_load_balancer_network.control_plane in control_planes.tf, still uses the hardcoded value 254 and also has ip in its lifecycle.ignore_changes block. This will likely cause the same issue this PR is fixing, but for the control plane load balancer.
Here is the relevant code from control_planes.tf:
resource "hcloud_load_balancer_network" "control_plane" {
// ...
ip = cidrhost(hcloud_network_subnet.control_plane.*.ip_range[0], 254)
lifecycle {
ignore_changes = [ip]
}
}To ensure consistency and prevent future errors, I recommend applying the same fix to the hcloud_load_balancer_network.control_plane resource in control_planes.tf as part of this PR.
- Change IP allocation from hardcoded 254 to -2 for control_plane load balancer - Remove 'ip' from ignore_changes lifecycle block - Fix network_gw_ipv4 variable definition (default -> description) - Ensures consistency across all load balancer network resources
|
/gemini review |
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.
Code Review
This pull request correctly addresses the load balancer IP configuration issue with hcloud provider v1.56+ by updating the cidrhost function to use the last available IP and by allowing the provider to manage the IP address. The changes are well-documented with comments and the fix in modules/host/variables.tf is also a good improvement. I've found one potential issue in control_planes.tf where enable_public_interface has been added to ignore_changes, which would prevent a configurable variable from being updated. My suggestion is to remove it to ensure expected behavior.
| lifecycle { | ||
| ignore_changes = [ip] | ||
| # Removed 'ip' from ignore_changes to allow proper management with hcloud provider 1.56+ | ||
| ignore_changes = [enable_public_interface] | ||
| } |
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.
While removing ip from ignore_changes is correct for the hcloud provider v1.56+ update, replacing it with enable_public_interface seems unintended. This change will cause Terraform to ignore any future modifications to the var.control_plane_lb_enable_public_interface variable, which is likely not the desired behavior for a configurable parameter. If the intention was solely to remove ip from this lifecycle block, ignore_changes should be an empty list.
lifecycle {
# Removed 'ip' from ignore_changes to allow proper management with hcloud provider 1.56+
ignore_changes = []
}
The original control_planes.tf only had 'ip' in ignore_changes, not 'enable_public_interface'. Setting to empty list to avoid ignoring future changes to the configurable parameter.
|
/gemini review |
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.
Code Review
This pull request correctly addresses an incompatibility with the hcloud provider v1.56.0+ by updating the load balancer network IP configuration. The changes are correct and consistently applied. I've added two comments: one suggesting a code simplification in control_planes.tf, and another regarding an unrelated but correct fix that was included in this PR. Overall, this is a solid improvement.
| lifecycle { | ||
| ignore_changes = [ip] | ||
| # Removed 'ip' from ignore_changes to allow proper management with hcloud provider 1.56+ | ||
| ignore_changes = [] | ||
| } |
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.
| variable "network_gw_ipv4" { | ||
| type = string | ||
| default = "Default IPv4 gateway address for the node's primary network interface" | ||
| type = string | ||
| description = "Default IPv4 gateway address for the node's primary network interface" | ||
| } |
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.
This is a good correction, changing default to description. The previous implementation was using the default attribute to hold a descriptive string, which is not its intended purpose.
However, this change is unrelated to the main goal of this pull request (fixing the load balancer IP configuration) and is not mentioned in the summary. To improve clarity and make reviews easier, it's best practice to keep pull requests focused on a single logical change. Please consider moving unrelated fixes like this to a separate PR in the future.
Summary
Fixes #1953
This PR addresses the load balancer network attachment IP issue that occurs when upgrading to hcloud provider v1.56.0+. The provider now validates that the load balancer IP is within the assigned subnet range and properly manages the IP lifecycle.
Changes
-2(last usable IP in subnet) instead of hardcoded254incidrhost()functionipfromignore_changesblock to allow proper IP management with hcloud provider 1.56+Background
With hcloud provider v1.56.0, load balancers can now be assigned to specific network subnets, and the provider validates that attachment IPs are within the subnet IP range. The previous hardcoded value of
254could fall outside the actual subnet range, causing the error:Attachment IP (10.254.0.1) is outside subnet IP range (10.0.0.0/16)
Using
-2withcidrhost()ensures we always get the last usable IP within the actual subnet, regardless of subnet size.Testing
terraform planno longer shows warnings or errors about IP rangesMigration Notes
For existing deployments upgrading to this version with hcloud provider 1.56+:
The problem is that hcloud provider 1.56 won't let us fix the bad state through normal plan/apply - it validates and rejects the change. This is why you need to remove it from state first: