🐛 properly trigger upgrades on version and klusterlet change #56
Conversation
WalkthroughAdd KlusterletHash to JoinedSpoke and FleetConfig CRD; vendor hashstructure/v2 and add ComputeHash; normalize bundle versions for comparisons; detect spoke changes by hashing klusterlet values; add version Normalize() and tests; bump clusteradm build ARGs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
223-246: Potential nil dereference when cluster-manager not found
getClusterManagerreturns(nil, nil)when the resource is not found. The code unconditionally dereferencescm.Spec.*, which will panic ifcm == nil(e.g., right after init or during transient states).Apply this diff to guard against nil and avoid a crash:
func hubNeedsUpgrade(ctx context.Context, fc *v1alpha1.FleetConfig, operatorC *operatorapi.Clientset) (bool, error) { @@ - cm, err := getClusterManager(ctx, operatorC) + cm, err := getClusterManager(ctx, operatorC) if err != nil { return false, err } + if cm == nil { + logger.V(0).Info("cluster-manager not found, skipping upgrade check") + return false, nil + } @@ activeBundleVersion, err := version.LowestBundleVersion(ctx, bundleSpecs)
🧹 Nitpick comments (5)
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_fleetconfigs.yaml (1)
2694-2697: Refine status.klusterletHash schema
Remove the
default: ""entry understatus.klusterletHashin
charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_fleetconfigs.yaml(around lines 2694–2697). Defaulting status fields isn’t necessary—controllers will always set this value, and an empty‐string default just adds noise.Add a validation pattern to ensure the field only contains lowercase hex digits. Under the same field, include:
pattern: '^[0-9a-f]+$'ComputeHash uses
strconv.FormatUint(..., 16)and emits a lowercase‐hex string (e.g. “ff23a1”) without any prefixes, so this regex will catch invalid values while remaining agnostic to length (fleetconfig-controller/internal/hash/hash.go:17).fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
247-252: Optional: normalize versions defensively to avoid spurious upgradesAssuming
version.LowestBundleVersionnow prefixes with "v", ensurefc.Spec.Hub.ClusterManager.Source.BundleVersionis normalized similarly (and case-consistently) before comparison to prevent false positives if users specify "0.6.0" vs "v0.6.0". If normalization is guaranteed upstream, ignore this.If not guaranteed, consider normalizing desired locally:
@@ - return activeBundleVersion != fc.Spec.Hub.ClusterManager.Source.BundleVersion, nil + desired := fc.Spec.Hub.ClusterManager.Source.BundleVersion + if desired != "default" && desired != "latest" && !strings.HasPrefix(desired, "v") { + desired = "v" + desired + } + return activeBundleVersion != desired, nilOr, better, expose a
version.Normalizehelper and use it here.fleetconfig-controller/internal/hash/hash.go (1)
10-19: Determinism looks good; add a small unit test to lock behaviorFormatV2 + unordered map handling gives stable hashes. Please add a unit test to assert:
- map order independence,
- nil pointer equals zero value when ZeroNil=true,
- slice order sensitivity (if that’s intended for values).
I can draft a quick table-driven test for ComputeHash against representative KlusterletChartConfig cases if you’d like.
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
153-156: Fix typo in error message: “compue” → “compute”Tiny polish for operator logs and user-facing conditions.
Apply this diff:
- if err != nil { - return fmt.Errorf("failed to compue hash of spoke %s klusterlet values: %w", spoke.Name, err) - } + if err != nil { + return fmt.Errorf("failed to compute hash of spoke %s klusterlet values: %w", spoke.Name, err) + }
219-227: Pointer to slice element return is okay here; consider returning value,bool for general safetygetJoinedSpoke returns a pointer into the provided slice. It’s read-only and used immediately so it’s fine. For broader reuse, returning (v1alpha1.JoinedSpoke, bool) avoids pointer aliasing to backing arrays.
I can refactor to return (JoinedSpoke, bool) and adjust call sites if you want to generalize this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
fleetconfig-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_fleetconfigs.yaml(1 hunks)fleetconfig-controller/go.mod(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/hub.go(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/spoke.go(5 hunks)fleetconfig-controller/internal/hash/hash.go(1 hunks)fleetconfig-controller/internal/version/version.go(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/LICENSE(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/errors.go(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/include.go(1 hunks)fleetconfig-controller/vendor/modules.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/include.go (1)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go (1)
Hash(98-129)
fleetconfig-controller/internal/hash/hash.go (1)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go (2)
Hash(98-129)FormatV2(56-56)
fleetconfig-controller/internal/controller/v1alpha1/hub.go (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Hub(175-207)ClusterManager(247-283)fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(49-63)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go (2)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/errors.go (3)
ErrFormat(18-18)ErrFormat(20-22)ErrNotStringer(8-10)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/include.go (3)
IncludableMap(13-15)Includable(6-8)Hashable(20-22)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-19)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (3)
Klusterlet(516-579)JoinedSpoke(444-466)Kubeconfig(300-316)
🪛 LanguageTool
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md
[grammar] ~3-~3: There might be a mistake here.
Context: ...library for creating a unique hash value for arbitrary values in Go. This can be...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...use case is comparing two values without sending data across the network, caching...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...nded you use the "v2" release since this fixes some significant hash collisions i...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...ons issues from v1. In practice, we used v1 for many years in real projects at Ha...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...t HashiCorp and never had issues, but it is highly dependent on the shape of the ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... the data you're hashing and how you use those hashes. When using v2+, you can s...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...l generate weaker v1 hashes by using the FormatV1 format when calling Hash. ...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (11)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/LICENSE (1)
1-22: Vendored MIT license included — good.Including upstream licensing alongside the vendored code is correct and keeps us compliant.
fleetconfig-controller/vendor/modules.txt (1)
211-213: Vendor manifest entry for hashstructure/v2 verifiedAll checks passed:
- No stray v1 imports detected outside of vendor (confirmed by ripgrep)
- Only
github.com/mitchellh/hashstructure/v2is imported ininternal/hash/hash.gofleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2directory exists- Version v2.0.2 in
vendor/modules.txtaligns withgo.modfleetconfig-controller/go.mod (1)
8-8: No legacy v1 imports found — v2 dependency is safeRan the provided ripgrep check across the repository and confirmed there are no imports of
github.com/mitchellh/hashstructurewithout the/v2suffix. You can proceed with thev2.0.2dependency without risk of split-brain.fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/errors.go (1)
1-22: Vendored errors: LGTMStandard upstream error types for hashstructure/v2. No local modifications warranted.
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md (1)
1-76: Vendored README: keep as-isDocumentation from upstream. No action needed.
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/include.go (1)
1-23: Vendored interfaces: LGTMInterfaces align with upstream API (Includable, IncludableMap, Hashable). Good to vendor without local edits.
fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
251-251: Correct inversion: upgrade when activeBundleVersion differs from desiredSwitching to
!=is the correct trigger for upgrades on version mismatch. Matches the PR intent.fleetconfig-controller/internal/hash/hash.go (1)
1-9: LGTM on minimal, focused wrapperStraightforward wrapper around hashstructure/v2; scoped import and API are clean.
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
180-191: Upgrade gating on version mismatch or hash change — solidTriggering upgrade on either desired version mismatch or klusterlet values hash change is correct and aligns with PR intent.
487-488: String equality compare assumes consistent “v” prefix; verify desired vs active normalizationactiveBundleVersion comes from version.LowestBundleVersion (now prefixed with “v”). If FleetConfig.Spec.Spokes[].Klusterlet.Source.BundleVersion can be specified without “v”, this comparison may always differ. Ensure inputs are normalized (both with or without “v”).
Would you like me to add a small helper normalizeVersion(string) that ensures a single “v” prefix and use it at comparison and argument construction sites?
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go (1)
1-475: Vendored upstream library — no changes requestedBringing in hashstructure/v2 with FormatV2 is appropriate for deterministic hashing of nested values. Avoid modifying vendored sources; all custom behavior should live in your internal/hash wrapper.
arturshadnik
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
201-208: JoinedSpoke.EnabledAddons is no longer populated; deregistration will skip addon disable and likely fail
deregisterSpoke()relies onspoke.EnabledAddonsto disable addons before unjoin (see Lines 623-641 in this file). With the current change,EnabledAddonsis never set on the status, so we won’t disable addons, and unjoin may fail due to remaining addon ManifestWorks. This regresses the unjoin flow.Set
EnabledAddonswhen appendingJoinedSpoke.Apply this diff:
js := v1alpha1.JoinedSpoke{ Name: spoke.Name, Kubeconfig: spoke.Kubeconfig, PurgeKlusterletOperator: spoke.Klusterlet.PurgeOperator, + EnabledAddons: enabledAddons, KlusterletHash: currKlusterletHash, }
🧹 Nitpick comments (7)
fleetconfig-controller/internal/version/version_test.go (1)
54-92: Add cases for pre-release/build metadata to pin intended semanticsIf you want Normalize to preserve pre-release/build segments exactly as parsed by Masterminds/semver, add a few cases (e.g., "v1.2.3-rc.1", "1.2.3+build.7", "1.2.3-rc.1+build.7") to guard against regressions.
fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
246-256: Normalize is the right move; consider semantic comparison to ignore build metadata diffsRight now we string-compare two normalized strings. If you intend to ignore build metadata (per SemVer precedence rules), compare semantically instead of by string. That avoids unnecessary upgrades on "1.2.3+buildA" vs "1.2.3+buildB".
Apply this minimal change here:
- return activeBundleVersion != requestedBundleVersion, nil + // Optional: compare semantically to ignore build metadata differences + equal, err := version.SemverEqual(activeBundleVersion, requestedBundleVersion) + if err != nil { + return false, err + } + return !equal, nilAnd add this helper in the version package (outside this file) to centralize semantics:
// in fleetconfig-controller/internal/version/version.go func SemverEqual(a, b string) (bool, error) { av, err := semver.NewVersion(a) if err != nil { return false, fmt.Errorf("invalid semver %q: %w", a, err) } bv, err := semver.NewVersion(b) if err != nil { return false, fmt.Errorf("invalid semver %q: %w", b, err) } // Masterminds/semver compares precedence (ignores build metadata) return av.Equal(bv), nil }fleetconfig-controller/internal/version/version.go (1)
48-55: Typo in comment: “stipped” → “stripped”Small polish to keep doc comments clean and searchable.
Apply this diff:
-// Normalize returns a semver string with the leading `v` prefix stipped off +// Normalize returns a semver string with the leading `v` prefix stripped offfleetconfig-controller/internal/controller/v1alpha1/spoke.go (4)
166-171: Fix typo in error string: “compue” → “compute”Minor but user-facing in logs; correct the spelling.
Apply this diff:
- if err != nil { - return fmt.Errorf("failed to compue hash of spoke %s klusterlet values: %w", spoke.Name, err) - } + if err != nil { + return fmt.Errorf("failed to compute hash of spoke %s klusterlet values: %w", spoke.Name, err) + }
175-181: Optional: log hash transition to aid debuggingWhen upgrades trigger on value changes, it helps to see the before/after hash and the decision.
Apply this diff:
hashChanged := false prevJs := getJoinedSpoke(fc.Status.JoinedSpokes, spoke.Name) if prevJs != nil { hashChanged = prevJs.KlusterletHash != currKlusterletHash } + prevHash := "" + if prevJs != nil { + prevHash = prevJs.KlusterletHash + } + logger.V(1).Info("klusterlet values hash check", + "spoke", spoke.Name, + "previousHash", prevHash, + "currentHash", currKlusterletHash, + "changed", hashChanged, + )
216-224: Helper looks good; optional: return (JoinedSpoke, ok) instead of pointerReturning a tuple avoids pointer semantics and nil checks. Not required, current approach is fine since the slice isn’t mutated before use.
479-489: Mirror hub: consider semantic version comparison instead of string inequalityBoth sides are normalized, but string inequality will still treat build metadata differences as upgrades. If you want to ignore build metadata, use the same
version.SemverEqualhelper suggested for hub.Apply this diff:
- return activeBundleVersion != requestedBundleVersion, nil + equal, err := version.SemverEqual(activeBundleVersion, requestedBundleVersion) + if err != nil { + return false, err + } + return !equal, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
fleetconfig-controller/internal/controller/v1alpha1/hub.go(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/spoke.go(5 hunks)fleetconfig-controller/internal/hash/hash.go(1 hunks)fleetconfig-controller/internal/version/version.go(1 hunks)fleetconfig-controller/internal/version/version_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/internal/hash/hash.go
🧰 Additional context used
🧬 Code graph analysis (3)
fleetconfig-controller/internal/version/version_test.go (1)
fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
fleetconfig-controller/internal/controller/v1alpha1/hub.go (2)
fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Hub(175-207)ClusterManager(247-283)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Klusterlet(516-579)JoinedSpoke(444-466)fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (4)
fleetconfig-controller/internal/version/version_test.go (1)
54-92: LGTM: solid table-driven coverage for Normalize()Covers v-prefixed, bare semver, and invalid input; assertions are correct and subtests read well.
fleetconfig-controller/internal/version/version.go (1)
48-55: Behavior of Normalize() is correct and consistent with LowestBundleVersion()Both paths use Masterminds/semver and return canonical strings without a leading "v", which keeps hub/spoke comparisons consistent.
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
30-30: Importing internal/hash is appropriate for value-change detectionDependency is minimal and the call site keeps the reconciling logic clean.
82-82: Range-based iteration reads cleaner and avoids index-only artifactsGood change; removes the need for an accumulator tied to an index.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
206-213: JoinedSpoke.EnabledAddons is never populated, breaking orderly unjoin/cleanupYou compute
enabledAddons(Lines 191–204) but do not persist it in the JoinedSpoke recorded to status.deregisterSpokerelies onspoke.EnabledAddonsto disable addons and wait for ManifestWorks cleanup. Without this, unjoin can fail or leave dangling resources.Set EnabledAddons on the recorded entry:
js := v1alpha1.JoinedSpoke{ Name: spoke.Name, Kubeconfig: spoke.Kubeconfig, PurgeKlusterletOperator: spoke.Klusterlet.PurgeOperator, + EnabledAddons: enabledAddons, KlusterletHash: currKlusterletHash, }
🧹 Nitpick comments (4)
fleetconfig-controller/internal/version/version.go (1)
48-55: Normalize: good addition; consider trimming whitespace and documenting the exact contractThe helper does what we need (strip optional "v" by leveraging semver parsing). Two small improvements:
- Trim whitespace to avoid surprises from values originating in YAML.
- Clarify in the comment that Normalize does not coerce partial versions and will return an error for non‑SemVer inputs; if you intend to accept partials, note that explicitly.
Apply:
-// Normalize returns a semver string with the leading `v` prefix stripped off +// Normalize returns the canonical semver string for v, with any leading 'v' stripped. +// It trims surrounding whitespace and returns an error if v is not a valid SemVer. func Normalize(v string) (string, error) { - sv, err := semver.NewVersion(v) + sv, err := semver.NewVersion(strings.TrimSpace(v)) if err != nil { return "", err } return sv.String(), nil }fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
82-84: Range loop over spokes is fine; be mindful of by-value copiesSwitching to
for _, spoke := range ...is cleaner. Just a reminder thatspokeis a copy; if any helper mutates fields for later persistence, those changes won’t reflect infc.Spec.Spokes. Current code only reads fromspoke, so this is fine.
221-229: getJoinedSpoke returns a pointer to a slice element; safe here but keep lifetime in mindReturning
&js[i]is fine while the underlying slice isn’t reallocated. Since you only read it immediately, risk is minimal. If you ever hold onto this pointer across appends tofc.Status.JoinedSpokes, it could become invalid. Alternative is to return(v1alpha1.JoinedSpoke, bool)or the index.
484-487: Consider using semantic version precedence for allbundleVersioncomparisons
The raw string checks will treat versions like1.2.3+build1and1.2.3+build2as different, even though their precedence is identical. To avoid unnecessary upgrades, it’s best to compare parsed semver objects by precedence.Locations needing an optional refactor:
- spoke.go (lines 446–452):
•if spoke.Klusterlet.Source.BundleVersion == "default"
•if spoke.Klusterlet.Source.BundleVersion == "latest"- spoke.go (lines 491–494):
•return activeBundleVersion != desiredBundleVersion, nil- hub.go (lines 214–220):
•if fc.Spec.Hub.ClusterManager.Source.BundleVersion == "default"
•if fc.Spec.Hub.ClusterManager.Source.BundleVersion == "latest"- hub.go (lines 253–256):
•return activeBundleVersion != desiredBundleVersion, nil- addon.go (line 506):
•installed.BundleVersion == bundleVersionOptional refactor snippet (example for spokes and hubs):
+ import ( + "fmt" + "github.com/Masterminds/semver" + ) @@ spoke.go, hub.go - return activeBundleVersion != desiredBundleVersion, nil + aVer, err := semver.NewVersion(activeBundleVersion) + if err != nil { + return false, fmt.Errorf("invalid active bundle version %q: %w", activeBundleVersion, err) + } + dVer, err := semver.NewVersion(desiredBundleVersion) + if err != nil { + return false, fmt.Errorf("invalid desired bundle version %q: %w", desiredBundleVersion, err) + } + return aVer.Compare(dVer) != 0, nilAnd for addon.go:
+ import "github.com/Masterminds/semver" @@ addon.go - return installed.BundleVersion == bundleVersion + iVer, err := semver.NewVersion(installed.BundleVersion) + if err != nil { + return false + } + bVer, err := semver.NewVersion(bundleVersion) + if err != nil { + return false + } + return iVer.Equal(bVer)The
"default"/"latest"literals can remain as string checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
fleetconfig-controller/internal/controller/v1alpha1/hub.go(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/spoke.go(5 hunks)fleetconfig-controller/internal/version/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/internal/controller/v1alpha1/hub.go
🧰 Additional context used
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Klusterlet(516-579)JoinedSpoke(444-466)fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
30-30: Hash import looks correct and scopedImporting the internal/hash package here is appropriate for computing the klusterlet values hash. No issues.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
546-552: Nil-pointer risk when logging joinedCondition fieldsWhen joinedCondition is nil, the log line dereferences joinedCondition.Message/Reason, causing a panic. Use safe defaults.
Apply this diff:
- if joinedCondition == nil || joinedCondition.Status != metav1.ConditionTrue { - logger.V(0).Info("skipping cleanup for unjoined spoke cluster", - "spoke", spoke.Name, "message", joinedCondition.Message, "reason", joinedCondition.Reason, - ) + if joinedCondition == nil || joinedCondition.Status != metav1.ConditionTrue { + var msg, reason string + if joinedCondition != nil { + msg, reason = joinedCondition.Message, joinedCondition.Reason + } + logger.V(0).Info("skipping cleanup for unjoined spoke cluster", + "spoke", spoke.Name, "message", msg, "reason", reason, + ) continue }
♻️ Duplicate comments (4)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (4)
179-183: Reduce log verbosity for per-spoke hash comparisonThe comparison log is V(0) and prints on every reconcile per spoke. Move it to V(1) (or V(2)) to keep default logs readable. See combined diff above.
300-309: Credentials leakage in logs: redact hub token and stop logging token payload
- getToken logs the full JSON output (includes hub-token). Avoid logging it.
- joinSpoke logs joinArgs, which includes the sensitive --hub-token value.
Implement a small redaction helper and sanitize logs. This prevents accidental credential exposure in aggregated controller logs.
Apply this diff:
@@ logger.V(1).Info("clusteradm get token", "args", tokenArgs) @@ - logger.V(1).Info("got join token", "output", string(stdout)) + logger.V(1).Info("got join token") // do not log the token payload@@ - logger.V(1).Info("clusteradm join", "args", joinArgs) + logger.V(1).Info("clusteradm join", "args", sanitizeArgs(joinArgs))And add the helper (place near other helpers in this file):
+// sanitizeArgs redacts values for known sensitive flags before logging. +func sanitizeArgs(args []string) []string { + out := append([]string(nil), args...) + for i := 0; i < len(out); i++ { + // redact known sensitive flags + if out[i] == "--hub-token" && i+1 < len(out) { + out[i+1] = "REDACTED" + i++ + } + } + return out +}Also applies to: 428-429
166-170: Don’t abort entire reconcile on a single spoke’s hash error; compute prev first and treat as “unchanged”Right now, a hash computation error for any spoke returns from handleSpokes and blocks reconciliation for all remaining spokes. Also, you compute the previous JoinedSpoke after hashing; compute it first so you can safely fall back to the previous hash when hashing fails. This keeps status stable and avoids accidental removal of the spoke from fc.Status.JoinedSpokes when continuing is desirable.
Apply this diff to make the error non-fatal for a single spoke and reduce noise:
@@ - // attempt an upgrade whenever the klusterlet's bundleVersion or values change - currKlusterletHash, err := hash.ComputeHash(spoke.Klusterlet.Values) - if err != nil { - return fmt.Errorf("failed to compute hash of spoke %s klusterlet values: %w", spoke.Name, err) - } + // attempt an upgrade whenever the klusterlet's bundleVersion or values change + // compute previous status entry first so we can fall back if hashing fails + prevJs, found := getJoinedSpoke(fc.Status.JoinedSpokes, spoke.Name) + currKlusterletHash, err := hash.ComputeHash(spoke.Klusterlet.Values) + if err != nil { + logger.Error(err, "failed to compute klusterlet values hash; treating as unchanged for this reconcile", "spoke", spoke.Name) + if found { + // fall back to previous hash value so we don't trigger a spurious upgrade + currKlusterletHash = prevJs.KlusterletHash + } else { + currKlusterletHash = "" + } + } @@ - hashChanged := false - prevJs, found := getJoinedSpoke(fc.Status.JoinedSpokes, spoke.Name) - if found { - hashChanged = prevJs.KlusterletHash != currKlusterletHash - logger.V(0).Info("comparing klusterlet values hash", - "spoke", spoke.Name, - "prevHash", prevJs.KlusterletHash, - "currHash", currKlusterletHash, - ) - } + hashChanged := found && prevJs.KlusterletHash != currKlusterletHash + logger.V(1).Info("klusterlet values hash comparison", "spoke", spoke.Name, "prevHash", prevJs.KlusterletHash, "currHash", currKlusterletHash)Also applies to: 175-185
206-211: Populate EnabledAddons in JoinedSpoke; otherwise unjoin/deregister will fail to clean addonsenabledAddons returned by handleSpokeAddons isn’t persisted into status. deregisterSpoke relies on JoinedSpoke.EnabledAddons to disable addons and to wait for manifestWorks cleanup. Leaving it empty can lead to unjoin failing with the AMW exists error.
Apply this diff:
@@ js := v1alpha1.JoinedSpoke{ Name: spoke.Name, Kubeconfig: spoke.Kubeconfig, PurgeKlusterletOperator: spoke.Klusterlet.PurgeOperator, + EnabledAddons: enabledAddons, KlusterletHash: currKlusterletHash, }Optional: store a stable, sorted slice to avoid noisy status diffs across reconciles:
- EnabledAddons: enabledAddons, + EnabledAddons: slices.Sorted(enabledAddons),If you prefer to avoid importing slices here, sort in-place before assignment: sort.Strings(enabledAddons).
I can send a follow-up cleanup to remove any leftover “allEnabledAddons” scaffolding if present elsewhere.
🧹 Nitpick comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
221-229: Avoid returning a pointer to a slice element (minor); return by valueReturning &js[i] points to an element in the passed slice. It’s safe in current usage (read-only, short-lived), but returning by value is simpler and avoids aliasing surprises if the caller ever stores the pointer.
-func getJoinedSpoke(js []v1alpha1.JoinedSpoke, spokeName string) (*v1alpha1.JoinedSpoke, bool) { +func getJoinedSpoke(js []v1alpha1.JoinedSpoke, spokeName string) (v1alpha1.JoinedSpoke, bool) { @@ - if i == -1 { - return nil, false - } - return &js[i], true + if i == -1 { + return v1alpha1.JoinedSpoke{}, false + } + return js[i], true }Then update call sites accordingly:
- prevJs, found := getJoinedSpoke(fc.Status.JoinedSpokes, spoke.Name) + prevJs, found := getJoinedSpoke(fc.Status.JoinedSpokes, spoke.Name)(no change at call site besides removing pointer derefs if any)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Klusterlet(516-579)JoinedSpoke(444-466)fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (3)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
30-30: Good addition: internal hash package import is appropriateBringing in the internal hash helper keeps hashing consistent across the codebase and avoids ad‑hoc implementations.
82-82: Loop style change to range is fineThe switch to a range-based loop improves readability and removes the unused index variable from the previous implementation.
484-487: No action needed:LowestBundleVersionalready returns a normalized SemVer stringThe implementation of
LowestBundleVersionbuilds*semver.Versioninstances withsemver.NewVersion(...)and then returnssemvers[0].String()(see version.go:45). TheString()method outputs the canonical form (e.g.1.28.0without a leading “v”), soactiveBundleVersionis already normalized. Comparing it directly against the normalizeddesiredBundleVersionis safe and will not misfire due to a stray “v” prefix.• Optional: you may still wrap
activeBundleVersionwithversion.Normalize(...)for explicitness, but it is redundant.
• No code changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
305-308: Redact secrets: token value and token-bearing args are logged and may leak credentialsTwo high-severity leaks:
- Line 309 logs the full stdout from clusteradm get token, which includes the hub token.
- Line 429 logs joinArgs containing --hub-token .
Also, the error path for getToken at Lines 305–308 includes “output” (stdout + stderr), potentially leaking the token into controller logs and conditions.
Harden logging as follows.
a) Don’t echo token payload; restrict error output to stderr:
@@ - stdout, stderr, err := exec_utils.CmdWithLogs(ctx, cmd, "waiting for 'clusteradm get token' to complete...") + stdout, stderr, err := exec_utils.CmdWithLogs(ctx, cmd, "waiting for 'clusteradm get token' to complete...") if err != nil { - out := append(stdout, stderr...) - return nil, fmt.Errorf("failed to get join token: %v, output: %s", err, string(out)) + // avoid leaking token from stdout + return nil, fmt.Errorf("failed to get join token: %v, stderr: %s", err, string(stderr)) } - logger.V(1).Info("got join token", "output", string(stdout)) + // avoid logging token payload entirely + logger.V(1).Info("got join token")b) Redact args where secrets appear:
- logger.V(1).Info("clusteradm join", "args", joinArgs) + logger.V(1).Info("clusteradm join", "args", sanitizeArgs(joinArgs))Add this helper (place near other helpers):
// sanitizeArgs redacts values for known sensitive flags before logging. func sanitizeArgs(args []string) []string { out := append([]string(nil), args...) for i := 0; i < len(out); i++ { switch out[i] { case "--hub-token": if i+1 < len(out) { out[i+1] = "REDACTED" i++ } } } return out }Optional: scan other logs for any future additions that might print sensitive values (e.g., env vars or kubeconfigs) and run them through the same sanitizer.
Also applies to: 309-312, 429-429
♻️ Duplicate comments (3)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
166-171: Don’t abort the entire reconcile on a single spoke’s hash error; log and continueA hashing failure for one spoke currently returns from handleSpokes, skipping remaining spokes. Prefer logging and continuing so one misconfigured spoke doesn’t block fleet-wide progress.
Apply this diff:
- currKlusterletHash, err := hash.ComputeHash(spoke.Klusterlet.Values) - if err != nil { - return fmt.Errorf("failed to compute hash of spoke %s klusterlet values: %w", spoke.Name, err) - } + currKlusterletHash, err := hash.ComputeHash(spoke.Klusterlet.Values) + if err != nil { + logger.Error(err, "failed to compute klusterlet values hash; skipping upgrade attempt for this spoke", "spoke", spoke.Name) + // Optionally: set a condition here to surface the issue per-spoke + continue + }
175-185: Reduce log verbosity for hash comparison detailsThe “comparing klusterlet values hash” message emits every reconcile at V(0). It’s noisy for controllers. Move to V(1) or V(2).
- logger.V(0).Info("comparing klusterlet values hash", + logger.V(1).Info("comparing klusterlet values hash", "spoke", spoke.Name, "prevHash", prevJs.KlusterletHash, "currHash", currKlusterletHash, )
206-214: EnabledAddons now recorded after enable — resolves earlier status driftYou now set EnabledAddons from the result of handleSpokeAddons before appending the JoinedSpoke. This fixes the previously reported issue where status had empty addons. Nice.
🧹 Nitpick comments (3)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
222-230: Avoid returning a pointer to a slice element; return by valueReturning &js[i] exposes an address into the backing array. While safe for read-only use here, returning by value eliminates aliasing concerns and is idiomatic.
-func getJoinedSpoke(js []v1alpha1.JoinedSpoke, spokeName string) (*v1alpha1.JoinedSpoke, bool) { +func getJoinedSpoke(js []v1alpha1.JoinedSpoke, spokeName string) (v1alpha1.JoinedSpoke, bool) { i := slices.IndexFunc(js, func(s v1alpha1.JoinedSpoke) bool { return spokeName == s.Name }) if i == -1 { - return nil, false + return v1alpha1.JoinedSpoke{}, false } - return &js[i], true + return js[i], true }Call site changes are minimal because field access is the same for values and pointers (no changes required to prevJs.KlusterletHash usage).
485-495: Version normalization before comparison: good fix; consider normalizing active side tooNormalizing desiredBundleVersion before comparing addresses formatting drift. Confirm that version.LowestBundleVersion returns a normalized string; otherwise normalize it too or compare as semver objects to be future-proof with pre-release/build metadata.
166-171: Nil/empty values hashing: confirm deterministic behavior across nil vs empty casesYou pass spoke.Klusterlet.Values directly to ComputeHash. With ZeroNil=true this should collapse nil pointers to zero values. Still, it’s worth a quick verification that:
- nil vs empty config hash the same, and
- map order within values does not affect the hash.
If not already covered by tests in internal/hash, consider adding them.
Do you want a small test added under internal/hash to assert determinism for:
- nil vs &KlusterletChartConfig{}
- maps with permuted key orders?
Also applies to: 731-746
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Klusterlet(516-579)JoinedSpoke(444-466)fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (2)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
30-33: Importing internal/hash is appropriate and used correctlyThe import is necessary for klusterlet values hashing and is used below. No issues.
82-84: Range loop over spokes: good simplificationSwitching to range improves readability and avoids index bugs. No capture issues here since no goroutines are spawned.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
309-309: Redact secrets in logs: token payload and --hub-token argument are leaked
- Line 309 logs the complete output of
clusteradm get token(includes hub token).- Line 429 logs
joinArgswhich contains--hub-token <secret>.Please redact before logging.
Minimal, safe changes:
- logger.V(1).Info("got join token", "output", string(stdout)) + logger.V(1).Info("got join token")- logger.V(1).Info("clusteradm join", "args", joinArgs) + logger.V(1).Info("clusteradm join", "args", sanitizeArgs(joinArgs))Add helper (outside this hunk):
// sanitizeArgs redacts values for known sensitive flags before logging. func sanitizeArgs(args []string) []string { out := append([]string(nil), args...) for i := 0; i < len(out); i++ { if out[i] == "--hub-token" && i+1 < len(out) { out[i+1] = "REDACTED" i++ } } return out }I understand this was deferred to a follow-up PR; leaving this as a reminder due to severity.
Also applies to: 429-429
🧹 Nitpick comments (2)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
222-230: Minor: O(n) scan per spoke; consider pre-indexing previous joined spokes if fleets grow
getJoinedSpokescans the slice on every lookup. For larger fleets, consider precomputing amap[string]v1alpha1.JoinedSpokeat the start ofhandleSpokesand using O(1) lookups. Not required for small N.Example outline (outside this hunk):
prevByName := make(map[string]v1alpha1.JoinedSpoke, len(fc.Status.JoinedSpokes)) for _, js := range fc.Status.JoinedSpokes { prevByName[js.Name] = js } // ... prevJs, found := prevByName[spoke.Name]
485-495: Add tests for pre-release/build metadata and guard against invalid bundle versionsThe current implementation of
NormalizeandLowestBundleVersioncorrectly strips the leadingvprefix and orders semver versions, but our existing tests only cover basic cases (e.g."v1.2.3","1.2.3", invalid strings). To ensure robustness—especially around build metadata and pre-release versions—and to avoid reconciler failures on bad input, I recommend:• Expand the version tests in
fleetconfig-controller/internal/version/version_test.goto include cases with build metadata and pre-releases. For example:{ name: "version with build metadata", version: "v1.2.3+build.45", want: "1.2.3+build.45", wantErr: false, }, { name: "pre-release version", version: "1.2.3-rc.1", want: "1.2.3-rc.1", wantErr: false, },And similarly, add a
LowestBundleVersionsuite that mixes specs like"fleet:v1.2.3+meta"and"fleet:v1.2.3-rc.2"to verify ordering and normalization.• Handle invalid desired versions gracefully in
spoke.go(around lines 485–495) instead of bubbling up an error and halting the entire reconcile. For example:desiredBundleVersion, err := version.Normalize(spoke.Klusterlet.Source.BundleVersion) - if err != nil { - return false, err - } + if err != nil { + logger.V(1).Info("invalid desired bundle version, skipping update check", "version", spoke.Klusterlet.Source.BundleVersion, "err", err) + // Set a status condition here if desired (e.g., .Status.Conditions = …) + return false, nil + }This approach lets the controller continue reconciling other spokes and surfaces the bad version in status rather than failing outright.
Implementing these optional refinements will help catch edge cases around metadata and improve the resiliency of your reconciliation loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Klusterlet(516-579)JoinedSpoke(444-466)fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (4)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (4)
30-30: LGTM: correct introduction of internal hash utilityUsing internal/hash centralizes hashing semantics and avoids ad-hoc hashing across the controller.
82-82: Range loop simplifies logic and removes fragile index couplingSwitching to
for _, spoke := range fc.Spec.Spokesreduces chances of index misuse and pairs well with per-spoke processing introduced below.
175-185: Hash comparison and logging level look good
- Comparing against previous
JoinedSpoke.KlusterletHashis correct.- Log moved to V(2), which is the right verbosity for per-reconcile diffs.
No change requested.
210-212: Status now reflects enabled addons and klusterlet hash — resolves earlier status driftPopulating
EnabledAddonsand recordingKlusterletHashat the time of status update fixes the previously reported issue where addons weren’t reflected. Good sequencing afterhandleSpokeAddons.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
fleetconfig-controller/build/Dockerfile.gke (1)
40-40: LGTM: Default ARG updated to 1.0.2; matches latest release.The value is correct and should resolve via install.sh (which auto-prefixes 'v'). (github.com, raw.githubusercontent.com)
Add a quick hint to avoid future “v” mistakes and to keep this in sync with hack/.versions.env:
-ARG CLUSTERADM_VERSION=1.0.2 +# Keep in sync with hack/.versions.env; pass bare x.y.z (install.sh adds 'v') +ARG CLUSTERADM_VERSION=1.0.2fleetconfig-controller/build/Dockerfile.devspace (1)
19-19: LGTM: Devspace image now pulls clusteradm 1.0.2.Correct default; install.sh handles the tag prefix. (github.com, raw.githubusercontent.com)
Add a small guardrail comment:
-ARG CLUSTERADM_VERSION=1.0.2 +# Pass bare x.y.z; install.sh prefixes 'v' at download time +ARG CLUSTERADM_VERSION=1.0.2fleetconfig-controller/build/Dockerfile.eks (1)
41-41: LGTM: EKS image defaulted to clusteradm 1.0.2.Matches the latest release; semantics with install.sh remain correct. (github.com, raw.githubusercontent.com)
Inline note to avoid accidental "v" prefix:
-ARG CLUSTERADM_VERSION=1.0.2 +# Keep in sync with hack/.versions.env; use bare version (install.sh adds 'v') +ARG CLUSTERADM_VERSION=1.0.2fleetconfig-controller/build/Dockerfile.base (1)
41-41: LGTM: Base image now installs clusteradm 1.0.2.Consistent with the repo-wide bump; correct usage with install.sh’s auto ‘v’ prefix. (github.com, raw.githubusercontent.com)
Add a short synch/comment:
-ARG CLUSTERADM_VERSION=1.0.2 +# Sync with hack/.versions.env; pass bare x.y.z (install.sh adds 'v') +ARG CLUSTERADM_VERSION=1.0.2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
fleetconfig-controller/build/Dockerfile.base(1 hunks)fleetconfig-controller/build/Dockerfile.devspace(1 hunks)fleetconfig-controller/build/Dockerfile.eks(1 hunks)fleetconfig-controller/build/Dockerfile.gke(1 hunks)fleetconfig-controller/hack/.versions.env(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T17:55:52.159Z
Learnt from: TylerGillson
PR: open-cluster-management-io/lab#51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.159Z
Learning: In the open-cluster-management-io/lab repository, chart versioning for fleetconfig-controller is handled automatically via GitHub release workflows, not through manual version bumps in Chart.yaml during regular PRs.
Applied to files:
fleetconfig-controller/build/Dockerfile.devspacefleetconfig-controller/build/Dockerfile.gkefleetconfig-controller/build/Dockerfile.eksfleetconfig-controller/hack/.versions.envfleetconfig-controller/build/Dockerfile.base
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (1)
fleetconfig-controller/hack/.versions.env (1)
3-3: LGTM: BumpCLUSTERADM_VERSIONto 1.0.2This aligns the default with the upstream clusteradm v1.0.2 release (Aug 27, 2025) and keeps the bare version—
install.shwill prefix it with “v” at runtime.• File:
fleetconfig-controller/hack/.versions.env
• Line 3: now readsCLUSTERADM_VERSION=1.0.2Nit (optional): document the “no v-prefix” nuance inline to prevent future confusion:
-CLUSTERADM_VERSION=1.0.2 +# Note: pass bare x.y.z; install.sh prefixes with “v” (e.g., 1.0.2 → v1.0.2) +CLUSTERADM_VERSION=1.0.2Verify no stale
1.0.1remains in the hack scripts:# look only for the old clusteradm default in hack/.versions.env rg -nP '(?<=^CLUSTERADM_VERSION=)1\.0\.1$' -C0 hack/.versions.env # optionally, ensure install.sh isn’t hard-coding the old version grep -R "CLUSTERADM_VERSION" -n hack/install.sh
|
/lgtm |
Bumps the github-actions group with 1 update: [actions/dependency-review-action](https://github.com/actions/dependency-review-action). Updates `actions/dependency-review-action` from 4.7.1 to 4.7.2 - [Release notes](https://github.com/actions/dependency-review-action/releases) - [Commits](actions/dependency-review-action@da24556...bc41886) --- updated-dependencies: - dependency-name: actions/dependency-review-action dependency-version: 4.7.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
arturshadnik
force-pushed
the
fix/klusterlet-upgrade
branch
from
2b07974 to
1f80679
Compare
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
arturshadnik
force-pushed
the
fix/klusterlet-upgrade
branch
from
f978bb2 to
dbae9ca
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arturshadnik, TylerGillson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
433-452: Fold hash computation and comparison into spokeNeedsUpgrade; handle failures without abortingComputing here enables: (a) single responsibility, (b) graceful degradation if hashing fails (skip hash-based check, still do version-based), and (c) returning the current hash for status.
-func spokeNeedsUpgrade(ctx context.Context, kClient client.Client, spoke v1alpha1.Spoke, joinedSpokes []v1alpha1.JoinedSpoke, currKlusterletHash string) (bool, error) { +func spokeNeedsUpgrade(ctx context.Context, kClient client.Client, spoke v1alpha1.Spoke, joinedSpokes []v1alpha1.JoinedSpoke) (bool, string, error) { logger := log.FromContext(ctx) logger.V(0).Info("spokeNeedsUpgrade", "spokeClusterName", spoke.Name) - hashChanged := false - prevJs, found := getJoinedSpoke(joinedSpokes, spoke.Name) - if found { - hashChanged = prevJs.KlusterletHash != currKlusterletHash - logger.V(2).Info("comparing klusterlet values hash", - "spoke", spoke.Name, - "prevHash", prevJs.KlusterletHash, - "currHash", currKlusterletHash, - ) - } - if hashChanged { - return true, nil - } + prevJs, found := getJoinedSpoke(joinedSpokes, spoke.Name) + prevHash := "" + if found { + prevHash = prevJs.KlusterletHash + } + currKlusterletHash, err := hash.ComputeHash(spoke.Klusterlet.Values) + if err != nil { + // Don't abort upgrades entirely; proceed with version-based checks. + logger.Error(err, "failed to compute klusterlet values hash; skipping hash-based check", "spoke", spoke.Name) + currKlusterletHash = prevHash // preserve existing status to avoid spurious diffs + } else if found && prevHash != currKlusterletHash { + logger.V(2).Info("comparing klusterlet values hash", + "spoke", spoke.Name, + "prevHash", prevHash, + "currHash", currKlusterletHash, + ) + return true, currKlusterletHash, nil + }
🧹 Nitpick comments (7)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md (4)
1-76: Vendored upstream docs: prefer ignoring vendor/ in linters over editing content.This README triggers markdownlint rules (MD007, MD014, MD040). Since it’s third‑party and vendored, don’t modify it; instead, exclude vendor/** from markdownlint to avoid churn.
You can add a markdownlint-cli2 config to ignore vendor:
{ "ignores": ["vendor/**", "**/vendor/**"] }Or, if you already have .markdownlint-cli2.jsonc, append the paths above.
32-34: Fenced code block is missing a language; also avoid “$” prompt to satisfy MD040/MD014.Only if you choose to lint vendor docs instead of ignoring them, apply:
-``` -$ go get github.com/mitchellh/hashstructure/v2 -``` +```sh +go get github.com/mitchellh/hashstructure/v2 +```
12-27: Unordered list indentation (MD007).If not ignoring vendor/, remove leading spaces before list bullets:
- * Hash any arbitrary Go value, including complex types. - * Tag a struct field to ignore it and not affect the hash value. - * Tag a slice type struct field to treat it as a set where ordering +* Hash any arbitrary Go value, including complex types. +* Tag a struct field to ignore it and not affect the hash value. +* Tag a slice type struct field to treat it as a set where ordering doesn't affect the hash code but the field itself is still taken into account to create the hash value. - * Optionally, specify a custom hash function to optimize for speed, collision +* Optionally, specify a custom hash function to optimize for speed, collision avoidance for your data set, etc. - * Optionally, hash the output of `.String()` on structs that implement fmt.Stringer, +* Optionally, hash the output of `.String()` on structs that implement fmt.Stringer, allowing effective hashing of time.Time - * Optionally, override the hashing process by implementing `Hashable`. +* Optionally, override the hashing process by implementing `Hashable`.
47-49: Doc link: prefer pkg.go.dev over godoc.org.Optional change if you decide to edit the README:
-For usage and examples see the [Godoc](http://godoc.org/github.com/mitchellh/hashstructure). +For usage and examples see [pkg.go.dev](https://pkg.go.dev/github.com/mitchellh/hashstructure/v2).fleetconfig-controller/internal/hash/hash.go (1)
12-14: Pad hex to fixed width for stable, comparable hashesZero-padding helps readability and lexicographic comparisons; no behavior change otherwise.
import ( - "strconv" + "fmt" + "strconv" @@ - hashStr := strconv.FormatUint(hash, 16) + hashStr := fmt.Sprintf("%016x", hash) return hashStr, nilAlso applies to: 20-21
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (2)
166-175: Centralize hash computation inside spokeNeedsUpgrade and return itKeeps all upgrade-decision inputs in one place and avoids duplicate logic. Also enables graceful handling when hashing fails.
- // attempt an upgrade whenever the klusterlet's bundleVersion or values change - currKlusterletHash, err := hash.ComputeHash(spoke.Klusterlet.Values) - if err != nil { - return fmt.Errorf("failed to compute hash of spoke %s klusterlet values: %w", spoke.Name, err) - } - upgrade, err := spokeNeedsUpgrade(ctx, kClient, spoke, fc.Status.JoinedSpokes, currKlusterletHash) + // attempt an upgrade whenever the klusterlet's bundleVersion or values change + upgrade, currKlusterletHash, err := spokeNeedsUpgrade(ctx, kClient, spoke, fc.Status.JoinedSpokes) if err != nil { return fmt.Errorf("failed to check if spoke cluster needs upgrade: %w", err) }
492-502: Normalize both active and desired versions before comparingPrevents mismatches due to “v” prefix or formatting drift.
- desiredBundleVersion, err := version.Normalize(spoke.Klusterlet.Source.BundleVersion) + desiredBundleVersion, err := version.Normalize(spoke.Klusterlet.Source.BundleVersion) if err != nil { - return false, err + return false, currKlusterletHash, err } + activeBundleVersionNorm, err := version.Normalize(activeBundleVersion) + if err != nil { + return false, currKlusterletHash, fmt.Errorf("failed to normalize active bundleVersion %q: %w", activeBundleVersion, err) + } logger.V(0).Info("found klusterlet bundleVersions", - "activeBundleVersion", activeBundleVersion, + "activeBundleVersion", activeBundleVersionNorm, "desiredBundleVersion", desiredBundleVersion, ) - return activeBundleVersion != desiredBundleVersion, nil + return activeBundleVersionNorm != desiredBundleVersion, currKlusterletHash, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
fleetconfig-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.github/workflows/dependency-review.yml(1 hunks)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go(1 hunks)fleetconfig-controller/build/Dockerfile.base(1 hunks)fleetconfig-controller/build/Dockerfile.devspace(1 hunks)fleetconfig-controller/build/Dockerfile.eks(1 hunks)fleetconfig-controller/build/Dockerfile.gke(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_fleetconfigs.yaml(1 hunks)fleetconfig-controller/go.mod(1 hunks)fleetconfig-controller/hack/.versions.env(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/hub.go(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/spoke.go(6 hunks)fleetconfig-controller/internal/hash/hash.go(1 hunks)fleetconfig-controller/internal/version/version.go(1 hunks)fleetconfig-controller/internal/version/version_test.go(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/LICENSE(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/errors.go(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go(1 hunks)fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/include.go(1 hunks)fleetconfig-controller/vendor/modules.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/LICENSE
🚧 Files skipped from review as they are similar to previous changes (15)
- fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/include.go
- fleetconfig-controller/internal/version/version_test.go
- fleetconfig-controller/go.mod
- fleetconfig-controller/hack/.versions.env
- fleetconfig-controller/build/Dockerfile.devspace
- fleetconfig-controller/internal/version/version.go
- fleetconfig-controller/internal/controller/v1alpha1/hub.go
- fleetconfig-controller/vendor/modules.txt
- fleetconfig-controller/api/v1alpha1/fleetconfig_types.go
- fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/errors.go
- fleetconfig-controller/build/Dockerfile.base
- fleetconfig-controller/build/Dockerfile.gke
- fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_fleetconfigs.yaml
- fleetconfig-controller/build/Dockerfile.eks
- fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go
🧰 Additional context used
🧬 Code graph analysis (2)
fleetconfig-controller/internal/hash/hash.go (1)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/hashstructure.go (3)
HashOptions(13-40)Hash(98-129)FormatV2(56-56)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (3)
Klusterlet(516-579)JoinedSpoke(444-466)Spoke(349-391)fleetconfig-controller/internal/version/version.go (1)
Normalize(49-55)
🪛 LanguageTool
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md
[grammar] ~3-~3: There might be a mistake here.
Context: ...library for creating a unique hash value for arbitrary values in Go. This can be...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...use case is comparing two values without sending data across the network, caching...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...nded you use the "v2" release since this fixes some significant hash collisions i...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...ons issues from v1. In practice, we used v1 for many years in real projects at Ha...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...t HashiCorp and never had issues, but it is highly dependent on the shape of the ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... the data you're hashing and how you use those hashes. When using v2+, you can s...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...l generate weaker v1 hashes by using the FormatV1 format when calling Hash. ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md
12-12: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (5)
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/README.md (1)
1-76: License and Vendoring Metadata Verified
- Upstream MIT License file is present at
fleetconfig-controller/vendor/github.com/mitchellh/hashstructure/v2/LICENSE.- The vendored version in
vendor/modules.txtis pinned togithub.com/mitchellh/hashstructure/v2 v2.0.2.- The same
v2.0.2reference exists ingo.mod.All compliance checks pass; no further action required.
.github/workflows/dependency-review.yml (1)
28-28: Dependency Review action bump looks good and is pinned to a commitUpgrading to v4.7.2 with a full-length SHA is a good, safe move.
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
82-90: Range loop and early-continue flow read cleanlyIteration and error-continue approach make sense here.
197-205: Good: status now records EnabledAddons and KlusterletHash after successful operationsThis fixes the earlier timing issue with EnabledAddons not reflecting post-enable state.
208-208: Confirm intended behavior: dropping prior JoinedSpoke entry on transient failuresBecause joinedSpokes is rebuilt, any spoke that hits a continue before appending will disappear from Status for this reconcile. If that's acceptable, ignore; otherwise consider preserving the previous entry on transient errors.
openshift-merge-bot
bot
merged commit 27523d6
into
open-cluster-management-io:main
2 participants
Sanity testing:
v0.16.0v1.0.0for hub and both spokes (hub-as-spoke, spoke)v1.0.0, and all pods were updated to use the new image.klusterlet.values.klusterlet.workConfiguration.kubeAPIQPSfrom50to60--kube-api-qps=60Summary by CodeRabbit
New Features
Bug Fixes
Chores