fix(yurthub): use uint64 parsing for resourceVersion in completeListObjWithObjs to prevent 32-bit overflow - #2733
Open
nishantbkl3345-ship-it wants to merge 3 commits into
Conversation
added 3 commits
August 3, 2026 00:26
…LeaderNodeLabelSelector is nil
…key mismatch in enhancement mode In gcPodsWhenRestart, KeyBuildInfo for pods fetched from the apiserver was missing the Version field. In enhancement mode, this produced keys like 'pods..core' instead of 'pods.v1.core', causing them to never match the cached keys from ListResourceKeysOfComponent. As a result, all cached pods appeared deleted, triggering the safety guard that skips GC entirely — making pod GC silently non-functional on restart. Added Version: 'v1' to the KeyBuildInfo and created gc_test.go with unit tests that prove the mismatch and verify the fix.
…bjWithObjs to prevent overflow In completeListObjWithObjs, resourceVersion was parsed using strconv.Atoi into a signed Go int, ignoring parsing errors. On 32-bit platforms (such as ARM32 linux/arm/v7 edge devices), signed int is 32-bit (max 2147483647). Any resourceVersion above 2^31-1 caused strconv.Atoi to return ErrRange and clamp to 2147483647. This produced corrupted resourceVersion strings on synthesized list responses, causing relist storms and 410 Gone errors on edge devices. Switched to strconv.ParseUint(rvStr, 10, 64) with uint64 listRv and added unit tests covering 64-bit uint64 resourceVersions and edge cases.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens yurthub cached list resourceVersion synthesis by switching parsing from int to uint64, preventing 32-bit overflow/corruption on edge devices. It also includes two additional behavioral fixes in hubleader reconciliation and yurthub GC key construction.
Changes:
- Update
completeListObjWithObjsto parseresourceVersionviastrconv.ParseUint(..., 64)and emit viaFormatUint, with a new unit test covering large/invalid values. - Prevent a potential nil-map overwrite/panic in hubleader reconciliation when
LeaderNodeLabelSelectoris nil by copying selector labels into a fresh map. - Fix yurthub pod-GC key construction to include
Version: "v1"and add a regression test for disk-storage key consistency.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/yurtmanager/controller/hubleader/hubleader_controller.go | Avoids overwriting matchLabels with a potentially nil selector map; copies labels safely. |
| pkg/yurtmanager/controller/hubleader/hubleader_controller_test.go | Adds coverage for mark strategy when LeaderNodeLabelSelector is nil. |
| pkg/yurthub/gc/gc.go | Ensures pod GC key-building includes API version to match stored keys in enhancement mode. |
| pkg/yurthub/gc/gc_test.go | Adds regression tests for disk-storage key consistency / map lookup behavior. |
| pkg/yurthub/cachemanager/cache_manager.go | Switches list RV synthesis to uint64 parsing/formatting to prevent 32-bit overflow corruption. |
| pkg/yurthub/cachemanager/cache_manager_test.go | Adds unit test coverage for large/invalid resourceVersion handling. |
Suppressed comments (2)
pkg/yurthub/gc/gc_test.go:72
- This comment claims the fixed code includes "Version and Group", but the fixed KeyBuildInfo only sets Version (Group is defaulted internally). Adjust the wording to avoid confusion.
// Simulate the FIXED code (with Version and Group)
fixedKey, err := store.KeyFunc(storage.KeyBuildInfo{
pkg/yurthub/gc/gc_test.go:143
- This test ignores the error return from KeyFunc when building fixedKey. Failing fast on error keeps the test from passing with an unintended nil/zero-value key if KeyFunc behavior changes.
fixedKey, _ := store.KeyFunc(storage.KeyBuildInfo{
Component: "kubelet",
Namespace: "kube-system",
Name: "coredns-abc123",
Resources: "pods",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+335
to
+339
| rvUint, err := strconv.ParseUint(rvStr, 10, 64) | ||
| if err != nil { | ||
| klog.Warningf("failed to parse resourceVersion %q: %v", rvStr, err) | ||
| continue | ||
| } |
Comment on lines
176
to
+181
| // Set match labels | ||
| matchLabels := make(map[string]string) | ||
| if nodepool.Spec.LeaderElectionStrategy == string(appsv1beta2.ElectionStrategyMark) { | ||
| // Add mark strategy match labels | ||
| matchLabels = nodepool.Spec.LeaderNodeLabelSelector | ||
| // Add mark strategy match labels safely without overwriting matchLabels with a potential nil map | ||
| for k, v := range nodepool.Spec.LeaderNodeLabelSelector { | ||
| matchLabels[k] = v |
Comment on lines
+29
to
+35
| // TestGcPodKeyConsistency proves that the keys built in gcPodsWhenRestart | ||
| // (when constructing currentPodKeys from apiserver response) must include | ||
| // Version and Group to match the keys returned by ListResourceKeysOfComponent. | ||
| // | ||
| // Before the fix, gcPodsWhenRestart called KeyFunc without Version/Group, | ||
| // producing "pods..core" in enhancement mode instead of "pods.v1.core", | ||
| // causing all cached pod keys to appear deleted. |
Comment on lines
+129
to
+135
| buggyKey, _ := store.KeyFunc(storage.KeyBuildInfo{ | ||
| Component: "kubelet", | ||
| Namespace: "kube-system", | ||
| Name: "coredns-abc123", | ||
| Resources: "pods", | ||
| }) | ||
| currentPodKeysBuggy[buggyKey] = struct{}{} |
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 this PR does / why we need it
Fixes an integer overflow vulnerability and data corruption bug in
completeListObjWithObjswhen calculating the synthesizedresourceVersionfor cached list responses.Previously,
strconv.Atoiwas used to parse objectresourceVersionstrings into a signed Goint, ignoring parsing errors.resourceVersionvalues frequently exceed ,147,483,647$ (^{31}-1$).linux/arm/v7edge devices, an officially published release target for OpenYurt), signedintis 32-bit.resourceVersionabove ^{31}-1$ causedstrconv.Atoito returnstrconv.ErrRangeand clamp toMaxInt32($).resourceVersionis standardly defined as an unsigned 64-bit integer (uint64).This caused synthesized list responses to output corrupted
resourceVersionvalues on edge nodes, resulting in410 Goneerrors and relist storms across WAN links.This PR switches
completeListObjWithObjsto usestrconv.ParseUint(rvStr, 10, 64)withuint64forlistRv, safely skips invalid strings, and formats the output back withstrconv.FormatUint.Which issue(s) this PR fixes
N/A
Special notes for your reviewer
Test_completeListObjWithObjsinpkg/yurthub/cachemanager/cache_manager_test.gocovering 64-bituint64resourceVersions exceeding 32-bit max integer limits, empty, and invalid resourceVersions.go test -racepass cleanly.