Repository navigation
fix: detect image-selector and public-IP drift - #51
Merged
Merged
Conversation
Add two drift checks so NodeClass edits replace running nodes: - ImageSelectorDrift compares the NodeClaim's image against the NodeClass status.resolvedImages entry for its architecture, so editing spec.imageSelector churns nodes instead of only affecting future ones. Skipped when status has no usable entry — an empty status must never churn nodes. - PublicIPv4Drift (and the trivial IPv6 twin PublicIPv6Drift) fires when the server's primary public IP attachment disagrees with spec.enablePublicIPv4 / spec.enablePublicIPv6 in either direction. The existing server-image check stays first to catch out-of-band changes. Docs updated to match; the troubleshooting IPv4 paragraph claimed drift already fired on the field change — it does now. Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
Review follow-up: only nodes whose attachment disagrees with the new enablePublicIPv4 value are replaced, not every managed node. Also drop the dead resolved.Image nil guard — imageFromStatus never returns a ResolvedImage with a nil Image. Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
fakehcloud (test/e2e) serves public_net.ipv6.ip as a bare "::1", but the real API — and therefore hcloud-go's schema parser — treats that field as a CIDR, so the address is dropped at parse time and a two-way PublicIPv6Drift check misfires on every freshly created fake server (failing TestRealHetznerLifecycleConfiguration in CI). test/** is owned by parallel worktrees so the fake cannot be corrected here; the brief's default was to leave the IPv6 equivalent alone anyway. The required public-IPv4 check is unaffected — ipv4.ip is a plain address and round-trips through both the fake and the real API. Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
johnnyhuy
marked this pull request as ready for review
October 5, 2026 14:48
This was referenced Oct 4, 2026
johnnyhuy
added a commit
that referenced
this pull request
Oct 6, 2026
status.resolvedImages was re-resolved to the newest matching snapshot on every pass, so publishing any snapshot that matched the selector moved the resolution — and the image-selector drift check in #51 would have replaced the whole fleet five minutes later with no spec edit. Resolve now takes an optional preferred image and keeps it while it is still in the match set; the NodeClass controller passes the ID it already recorded. A selector edit (or a deleted/deprecated image) drops it out of the match set, so resolution moves exactly when the operator intended. Transient hcloud failures now keep the recorded images instead of clearing them, which had the same churn-through-a-blip failure mode. Also corrects the imageSelector.version docs: version and selector are AND-combined, not mutually exclusive. Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
johnnyhuy
added a commit
that referenced
this pull request
Oct 6, 2026
The fake answered with a bare "::1" for the primary IPv6. hcloud-go decodes that field with net.ParseCIDR and discards the error, so the address decoded to a nil IP and IsUnspecified() reported "no IPv6" for a server that has one. Hetzner itself returns a CIDR (its /64 block). Found while reviewing #51: its new public-IP drift checks made the e2e lifecycle test fail with PublicIPv6Drift on a freshly created server (real_hetzner_helpers_test.go:106). The fake now emits 2001:db8::1/64 and TestServerCreatePublicIPsRoundTrip pins the round-trip through the real SDK decode path so this cannot regress. Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
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.
What
Two new drift checks, both behaviour changes owned by this branch:
Image-selector drift (
DriftImageSelector) —IsDriftednow compares the NodeClaim'sstatus.imageIDagainst the NodeClass'sstatus.resolvedImagesentry for that NodeClaim's architecture. Previously only the running server's image was checked, so editingspec.imageSelectoron anHCloudNodeClassnever replaced existing nodes — only future nodes booted the new snapshot.imageSelectornow replaces running nodes. That is the product decision — the new snapshot should roll out to the whole fleet, not just new capacity.status.resolvedImageshas no entry for the NodeClaim's architecture, or the entry has a zero image ID, or the NodeClaim's image/arch is not yet observed. An empty NodeClass status must never churn nodes.DriftImage) stays first in the short-circuit order — it still catches out-of-band image changes on the running server.Public-IP drift (
DriftPublicIPv4) — fires when the spec disables public IPv4 (enablePublicIPv4: false) but the server still has a primary IPv4, or vice versa (enabled but missing). The docs previously claimed "Karpenter drift fires on the field change" for the private-network billing recipe — no such check existed until now, so the paragraph was corrected to describe the real behaviour including the churn.Drift check order (short-circuited)
Tests
TestIsDrifted_*coverage added against the httptest-backed hcloud fake: selector drift fires (andDriftImagedoes not shadow it), selector check skipped for empty/wrong-arch/zero-ID status, public-IPv4 drift fires when disabled-but-attached and when enabled-but-missing, and no drift when the public-IP state is consistent.TestIsDrifted_NoDriftnow also exercises a matchingresolvedImagesentry.mise run cigreen (lint 0 issues), includingtest/e2e(TestRealHetznerLifecycleConfiguration runs against fakehcloud in the default lane).Notes / deliberately out of scope
DriftPublicIPv6twin, but CI caught a fidelity gap:fakehcloudservespublic_net.ipv6.ipas a bare"::1"while the real API — and therefore hcloud-go's schema parser — treats that field as a CIDR, so the address is dropped at parse time and a two-way IPv6 check misfires on every freshly created fake server.test/e2e/fakehcloudbelongs to the parallel worktrees so the fake could not be corrected here; the brief's default was to leave the IPv6 equivalent alone. For the test owners: worth making the fake emit API-shaped CIDR values ("2001:db8::/64") so an IPv6 drift check can be added later.DISTRIBUTION.md, thehcloud image listrecipe,charts/**/templates/secret.yaml,.github/workflows/**,test/**.