Repository navigation
feat(hetzner): ksail-owned floating IP primitives for a stable control-plane endpoint - #5699
Merged
Merged
Conversation
…ntrol-plane endpoint Adds EnsureFloatingIP (get-by-name, create-if-absent, ksail.owned labels, <cluster>-floating-ip naming), idempotent AttachFloatingIPToServer / DetachFloatingIP, and floating-IP cleanup in deleteInfrastructure so cluster delete never leaks a billed reserved address. Nothing calls EnsureFloatingIP yet — endpoint/config wiring is the next slice. Foundation for devantler-tech/platform#2120 (Talos hcloud VIP). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds Hetzner floating IP support: lookup-or-create, attach/detach, deletion, infrastructure cleanup wiring, ownership constants and errors, a test-only delete helper, and expanded tests for ownership and error handling. ChangesHetzner Floating IP Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant HetznerAPI
Provider->>HetznerAPI: EnsureFloatingIP(clusterName, homeLocation)
HetznerAPI-->>Provider: GetByName result
alt not found
Provider->>HetznerAPI: Create floating IP
HetznerAPI-->>Provider: created floating IP
else owned match
Provider-->>Provider: return existing floating IP
else unowned collision
Provider-->>Provider: return ErrFloatingIPNotOwned
end
Provider->>HetznerAPI: AttachFloatingIPToServer or DetachFloatingIP
alt already in desired state
Provider-->>Provider: no-op
else
Provider->>HetznerAPI: Assign or Unassign action
HetznerAPI-->>Provider: action result
end
Provider->>HetznerAPI: deleteFloatingIP(clusterName)
alt ksail-owned and present
Provider->>HetznerAPI: Delete floating IP
else missing or unowned
Provider-->>Provider: skip
end
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
1 of 4 tasks
Contributor
✅MegaLinter analysis: Success✅ Linters with no issuesactionlint, bash-exec, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint Notices📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining See detailed reports in MegaLinter artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/svc/provider/hetzner/floating_ip_test.go (2)
31-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a params struct for the fake server constructor.
newFloatingIPServertakes seven parameters, most of which are counter pointers; grouping them into a*floatingIPCounters(already used elsewhere as a return type) would reduce parameter-list noise.♻️ Proposed refactor
-func newFloatingIPServer( - t *testing.T, - listJSON string, - createCalls, deleteCalls, assignCalls, unassignCalls *int32, - lastCreateBody *atomic.Value, -) *httptest.Server { +func newFloatingIPServer( + t *testing.T, + listJSON string, + counters *floatingIPCounters, +) *httptest.Server {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/svc/provider/hetzner/floating_ip_test.go` around lines 31 - 78, `newFloatingIPServer` has an overly long parameter list made up mostly of counter pointers, which is noisy and hard to maintain. Refactor the test helper to accept a single `*floatingIPCounters` (the same struct already used elsewhere) plus the remaining inputs, and update the call sites accordingly. Keep the helper behavior the same by reading the individual counters from that struct inside `newFloatingIPServer`, `registerFloatingIPActionHandlers`, and the `/floating_ips` handlers.
176-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing nil-client coverage for Attach/Detach.
AttachFloatingIPToServerandDetachFloatingIPboth returnprovider.ErrProviderUnavailablewhenp.client == nil(per the upstream implementation), mirroringEnsureFloatingIP. OnlyEnsureFloatingIPhas aTestEnsureFloatingIP_NilClientcounterpart; the attach/detach nil-client branches are untested.✅ Suggested additional tests
func TestAttachFloatingIPToServer_NilClient(t *testing.T) { t.Parallel() prov := hetzner.NewProvider(nil) err := prov.AttachFloatingIPToServer( t.Context(), &hcloudtest.FloatingIP{ID: 7}, &hcloudtest.Server{ID: 9}, ) require.ErrorIs(t, err, provider.ErrProviderUnavailable) } func TestDetachFloatingIP_NilClient(t *testing.T) { t.Parallel() prov := hetzner.NewProvider(nil) err := prov.DetachFloatingIP(t.Context(), &hcloudtest.FloatingIP{ID: 7}) require.ErrorIs(t, err, provider.ErrProviderUnavailable) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/svc/provider/hetzner/floating_ip_test.go` around lines 176 - 245, Add nil-client test coverage for the floating IP attach/detach paths: `AttachFloatingIPToServer` and `DetachFloatingIP` should be verified to return `provider.ErrProviderUnavailable` when the provider is created with a nil client, matching the existing `EnsureFloatingIP_NilClient` behavior. Add focused tests alongside the existing `TestAttachFloatingIPToServer_*` and `TestDetachFloatingIP_*` cases, using `hetzner.NewProvider(nil)` and asserting the error with `require.ErrorIs`.pkg/svc/provider/hetzner/floating_ip.go (2)
130-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDelete call isn't retried like the preceding lookup.
GetByNameis wrapped inretryTransientHetznerOperation, but theDeletecall directly below is not, despitedeleteInfrastructureaborting the whole cleanup chain on any error here. For consistency with the retry pattern already established in this function (and elsewhere in this file's cleanup path), consider wrappingDeletesimilarly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/svc/provider/hetzner/floating_ip.go` around lines 130 - 133, The delete path in `deleteFloatingIP` skips the same transient retry handling used by `GetByName`, so wrap `p.client.FloatingIP.Delete` with `retryTransientHetznerOperation` just like the lookup above. Keep the existing error wrapping with the `floatingIPName` context, and ensure the cleanup flow in `deleteInfrastructure` benefits from the retry behavior when deleting the floating IP.
40-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
hcloud.Ptrinstead ofnew(...)for the pointer conversion.Line 42's
new(floatingIPName)relies on the Go 1.26 language change that letsnewtake an expression (not just a type) — valid given this repo's declared 1.26.1+ toolchain, but it's a very recent, less-familiar idiom that diverges from the hcloud-go library's own convention (hcloud.Ptr), which is used for this exact purpose elsewhere in the SDK.♻️ Proposed alignment with library convention
result, _, err := p.client.FloatingIP.Create(ctx, hcloud.FloatingIPCreateOpts{ Type: hcloud.FloatingIPTypeIPv4, - Name: new(floatingIPName), + Name: hcloud.Ptr(floatingIPName), HomeLocation: &hcloud.Location{Name: homeLocation}, Labels: ResourceLabels(clusterName), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/svc/provider/hetzner/floating_ip.go` around lines 40 - 45, Replace the pointer conversion in the FloatingIP creation path with the hcloud library’s pointer helper to match its convention. In the floating IP creation call inside the relevant provider function, swap out the current name pointer expression for hcloud.Ptr so the code aligns with other hcloud-go usages and avoids the newer, less familiar idiom.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/svc/provider/hetzner/floating_ip.go`:
- Around line 30-38: Ensure EnsureFloatingIP does not adopt a floating IP solely
by name; after p.client.FloatingIP.GetByName returns a match, verify the
ksail.owned label before returning it. If the label is absent or false, treat it
as not owned and continue creating/handling a new floating IP instead of reusing
the user-managed one. Use the existing ownership pattern from deleteFloatingIP
as the reference for the label check.
- Around line 108-124: The lookup in floatingIPByName is swallowing every
GetByName failure after retries, which can hide real API problems and leave a
billed address behind. Update the logic around retryTransientHetznerOperation in
floating_ip.go to distinguish a true “not found” case from other errors: only
return nil for the missing-resource case, and propagate or surface any other
error instead of treating it as absent. Also add an actual log call in the
ignored-path so the skipped floating IP can be observed.
---
Nitpick comments:
In `@pkg/svc/provider/hetzner/floating_ip_test.go`:
- Around line 31-78: `newFloatingIPServer` has an overly long parameter list
made up mostly of counter pointers, which is noisy and hard to maintain.
Refactor the test helper to accept a single `*floatingIPCounters` (the same
struct already used elsewhere) plus the remaining inputs, and update the call
sites accordingly. Keep the helper behavior the same by reading the individual
counters from that struct inside `newFloatingIPServer`,
`registerFloatingIPActionHandlers`, and the `/floating_ips` handlers.
- Around line 176-245: Add nil-client test coverage for the floating IP
attach/detach paths: `AttachFloatingIPToServer` and `DetachFloatingIP` should be
verified to return `provider.ErrProviderUnavailable` when the provider is
created with a nil client, matching the existing `EnsureFloatingIP_NilClient`
behavior. Add focused tests alongside the existing
`TestAttachFloatingIPToServer_*` and `TestDetachFloatingIP_*` cases, using
`hetzner.NewProvider(nil)` and asserting the error with `require.ErrorIs`.
In `@pkg/svc/provider/hetzner/floating_ip.go`:
- Around line 130-133: The delete path in `deleteFloatingIP` skips the same
transient retry handling used by `GetByName`, so wrap
`p.client.FloatingIP.Delete` with `retryTransientHetznerOperation` just like the
lookup above. Keep the existing error wrapping with the `floatingIPName`
context, and ensure the cleanup flow in `deleteInfrastructure` benefits from the
retry behavior when deleting the floating IP.
- Around line 40-45: Replace the pointer conversion in the FloatingIP creation
path with the hcloud library’s pointer helper to match its convention. In the
floating IP creation call inside the relevant provider function, swap out the
current name pointer expression for hcloud.Ptr so the code aligns with other
hcloud-go usages and avoids the newer, less familiar idiom.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81881cb7-942a-4963-b39f-336c0aa058a7
📒 Files selected for processing (5)
pkg/svc/provider/hetzner/export_test.gopkg/svc/provider/hetzner/floating_ip.gopkg/svc/provider/hetzner/floating_ip_test.gopkg/svc/provider/hetzner/infrastructure.gopkg/svc/provider/hetzner/labels.go
Contributor
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
…ookup errors on delete EnsureFloatingIP no longer silently adopts a user-managed reserved address that merely shares the conventional name — it now requires the ksail.owned label (same guard deleteFloatingIP applies) and surfaces the name collision as ErrFloatingIPNotOwned. deleteFloatingIP now propagates lookup failures after transient retries instead of treating every error as "not found", so a persistent API failure can no longer silently leak a billed reserved address (GetByName reports absence as nil/nil, never an error). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
devantler
marked this pull request as ready for review
July 2, 2026 15:50
This was referenced Jul 2, 2026
Closed
devantler
added a commit
that referenced
this pull request
Jul 2, 2026
…g for Talos × Hetzner (#5719) * feat(hetzner): floating-IP config surface + stable API endpoint wiring for Talos × Hetzner Adds the opt-in config surface for the stable-API-endpoint lane (platform#2120 option A): floatingIPEnabled + floatingIPLocation on OptionsHetzner. When enabled, cluster create ensures the cluster-owned floating IP (EnsureFloatingIP, #5699), attaches it to the first control-plane server, and renders it as the cluster endpoint with the floating IP plus every control-plane node IP in the certificate SANs (node IPs must stay verifiable — readiness checks and break-glass access dial nodes directly). Disabled (default) renders byte-identical configs. Node-side ownership handover (Talos VIP block) ships separately (#5718); until then the address must be claimed via a user Talos patch. Fixes #5714 * chore: sync modules and update generated files --------- Co-authored-by: devantler <26203420+devantler@users.noreply.github.com>
This was referenced Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5698. Foundation for devantler-tech/platform#2120 (Option A — Talos-managed hcloud floating-IP VIP, per the maintainer's on-issue choice).
What
Slice 1 of the stable control-plane endpoint work — the floating-IP primitives, following the provider's existing idempotent ensure pattern (
EnsureNetwork/EnsureFirewall):EnsureFloatingIP— get-by-name (<cluster>-floating-ip), create-if-absent (IPv4, homed in the cluster's location,ksail.ownedlabels).AttachFloatingIPToServer/DetachFloatingIP— idempotent assign/unassign (no-op when already in the desired state), so create-retries re-assert instead of erroring.deleteInfrastructurenow releases the cluster's floating IP — but only when it carries theksail.ownedlabel: a user-managed reserved address that merely shares the name is never released (a released floating IP is unrecoverable). Server deletion only unassigns, so without thiscluster deletewould leak a billed address once slice 2 starts creating them.No create-path behavior change yet: nothing calls
EnsureFloatingIPin this slice. Slice 2 (follow-up child of #5698's plan) adds thespec.provider.hetznerconfig surface, endpoint computation from the floating IP, and the Talos VIP machine-config/cert-SAN rendering.Design decision expressed here (platform#2120)
platform#2120's open question — where is the floating IP reserved declaratively? — is decided as (A) the KSail provisioner: KSail already owns the Hetzner servers/network/firewall via native hcloud-go (the standing native-Go-SDK provisioning direction), so the floating IP is a sibling resource; a whole Crossplane hcloud provider for one resource would be disproportionate. If you'd rather have (B), redirect here before promotion.
Validation
go test ./pkg/svc/provider/hetzner/— 11 new tests (httptest-backed hcloud API fake, same pattern as the network-reconcile tests): ensure create/adopt/nil-client, attach no-op/assign/reassign, detach no-op/unassign, delete owned/unowned-skip/absent.golangci-lint run ./pkg/svc/provider/hetzner/...— clean (aftercache clean; the repo-wide local run only reports the known stale-worktree cache + local-version drift, CI is authoritative).go build+ fullgo test ./...— green except the knownpkg/cli/cmd/open/chatparallel-run flake (127/127 pass alone).