*: upgrade Golang to 1.25.8, dependencies, and fix lint issues#10315
*: upgrade Golang to 1.25.8, dependencies, and fix lint issues#10315JmPotato wants to merge 2 commits intotikv:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
dco-signoff: yes
JmPotato
changed the title
ti-chi-bot
bot
added
release-note-none
📝 WalkthroughWalkthroughGo toolchain directives bumped from 1.25.6 → 1.25.8 across multiple go.mod files. Linter config and installer version updated. Several small code tweaks: safer loop bounds to avoid OOB and replacing fmt.Sprintf + WriteString with fmt.Fprintf to write into builders. No public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
JmPotato
changed the title
JmPotato
changed the title
ti-chi-bot
bot
added
size/M
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Line 82: The G120 exclusion removes the linter check that forces body size
limits and can expose admin endpoints to DoS; instead of keeping that exclusion,
wrap relevant HTTP handlers (e.g., PD admin handlers/ServeHTTP implementations
or specific handler funcs that serve internal admin endpoints) with
http.MaxBytesReader (or enforce request.Body read limits in their
ServeHTTP/handler functions) so each handler caps request body size, and remove
the global G120 exception from .golangci.yml; update any middleware or top-level
router (e.g., admin middleware, PDAdminHandler, or equivalent handler functions)
to apply the MaxBytesReader wrapper consistently for all admin/internal
endpoints.
- Line 86: The code unsafely indexes it.iters[0] without guarding against an
empty slice, relying on upstream callers (Handler.GetAllRequestHistoryHotRegion)
to provide defaults; fix by adding an explicit defensive check in the function
that accesses it.iters (verify len(it.iters) > 0 and return an error or default
behavior if empty) or, if you decide to accept the current pattern, restrict the
G602 suppression to only the specific function by adding a function-level nolint
comment for G602 rather than a global exclusion; update the function that uses
it.iters[0] to either validate input or document and narrowly suppress the
linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88c503c2-84ee-41e9-ac75-38181a56905d
📒 Files selected for processing (13)
.golangci.ymlMakefileclient/go.modgo.modpkg/mcs/scheduling/server/apis/v1/api.gopkg/mock/mockcluster/mockcluster.gopkg/schedule/checker/merge_checker_test.gopkg/schedule/labeler/labeler.gopkg/schedule/labeler/rules.goserver/api/region.gotests/integrations/go.modtests/server/api/region_label_test.gotools/go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- go.mod
- tools/go.mod
- client/go.mod
- tests/integrations/go.mod
| - G115 | ||
| - G115 # Integer overflow in type conversions — too many false positives in Go's type system | ||
| - G118 # context cancel not called — can't trace cancel through goroutines/sync.Once/structs/returns | ||
| - G120 # Form data without body size limit — PD HTTP endpoints are internal admin APIs |
There was a problem hiding this comment.
G120 exclusion may leave endpoints vulnerable to DoS.
Even internal admin APIs should enforce body size limits. An internal attacker or misconfigured client could exhaust server memory. Consider adding explicit http.MaxBytesReader wrappers to HTTP handlers instead of globally excluding this check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml at line 82, The G120 exclusion removes the linter check that
forces body size limits and can expose admin endpoints to DoS; instead of
keeping that exclusion, wrap relevant HTTP handlers (e.g., PD admin
handlers/ServeHTTP implementations or specific handler funcs that serve internal
admin endpoints) with http.MaxBytesReader (or enforce request.Body read limits
in their ServeHTTP/handler functions) so each handler caps request body size,
and remove the global G120 exception from .golangci.yml; update any middleware
or top-level router (e.g., admin middleware, PDAdminHandler, or equivalent
handler functions) to apply the MaxBytesReader wrapper consistently for all
admin/internal endpoints.
| - G123 # TLS session resumption may bypass VerifyPeerCertificate — CN check is additional to standard TLS | ||
| - G402 # TLS MinVersion — PD supports configurable TLS for backward compatibility | ||
| - G404 # Insecure random source — already banned by depguard, uses math/rand/v2 | ||
| - G602 # Slice index out of range — high false positive rate, Go has runtime bounds checking |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how many G602 warnings exist in production code vs tools
echo "=== G602 occurrences in production code (pkg/, server/) ==="
rg -l "iters\[0\]|keys\[0\]|vals\[0\]" --type go -g '!tools/**' -g '!*_test.go' | head -20
echo ""
echo "=== Checking hot_region_storage.go for unguarded slice access ==="
rg -n -B2 -A2 'iters\[0\]' pkg/storage/hot_region_storage.goRepository: tikv/pd
Length of output: 634
🏁 Script executed:
#!/bin/bash
# Check the second file with unguarded slice access
echo "=== Checking core.go for unguarded slice access ==="
rg -n -B3 -A3 'keys\[0\]|vals\[0\]' pkg/dashboard/keyvisual/input/core.go
echo ""
echo "=== Checking HotRegionStorageIterator constructor ==="
rg -n -B5 -A10 'func.*HotRegionStorageIterator' pkg/storage/hot_region_storage.go | head -50
echo ""
echo "=== Check if iterator can be created with empty slices ==="
rg -n 'HotRegionStorageIterator' pkg/storage/hot_region_storage.go | head -20Repository: tikv/pd
Length of output: 2121
🏁 Script executed:
#!/bin/bash
# Check callers of NewIterator
echo "=== Callers of NewIterator ==="
rg -n 'NewIterator' --type go pkg/ server/ | grep -v 'func.*NewIterator' | head -20
echo ""
echo "=== Callers of GetKeys ==="
rg -n 'GetKeys()' --type go pkg/ server/ | head -20
echo ""
echo "=== Check if requireTypes parameter validation exists ==="
rg -n -B10 'NewIterator' pkg/storage/hot_region_storage.go | grep -A10 'func.*NewIterator'Repository: tikv/pd
Length of output: 3562
🏁 Script executed:
#!/bin/bash
# Check the server handler to see if hotRegionTypes validation exists
echo "=== Checking server/handler.go for hotRegionTypes validation ==="
rg -n -B10 -A5 'hotRegionTypes.*NewIterator' server/handler.go
echo ""
echo "=== Check initialization of hotRegionTypes ==="
rg -n -B15 'hotRegionTypes' server/handler.go | head -40Repository: tikv/pd
Length of output: 1940
🏁 Script executed:
#!/bin/bash
# Check what storage.HotRegionTypes contains
echo "=== Checking storage.HotRegionTypes definition ==="
rg -n 'var.*HotRegionTypes\|HotRegionTypes\s*=' pkg/storage/ --type go | head -20
echo ""
echo "=== Get full definition ==="
rg -n -A5 'var.*HotRegionTypes\s*=' pkg/storage/ --type go
echo ""
echo "=== Check core.go RegionsInfo type and usage ==="
rg -n -B5 'type RegionsInfo' pkg/dashboard/keyvisual/input/core.goRepository: tikv/pd
Length of output: 716
🏁 Script executed:
#!/bin/bash
# Verify the GetKeys() method in core.go and check its actual callers
echo "=== Full GetKeys() implementation in core.go ==="
rg -n -B2 -A15 'func.*GetKeys.*RegionsInfo' pkg/dashboard/keyvisual/input/core.go
echo ""
echo "=== Check if GetKeys is only on RegionsInfo or also on other types ==="
rg -n 'func.*GetKeys' pkg/dashboard/keyvisual/input/core.go
echo ""
echo "=== Search for actual calls to RegionsInfo.GetKeys ==="
rg -n 'RegionsInfo.*GetKeys\|\.GetKeys.*region' pkg/ server/ --type go | grep -v test | head -10Repository: tikv/pd
Length of output: 273
🏁 Script executed:
#!/bin/bash
# Check if there are any cases where empty hotRegionTypes could reach NewIterator
echo "=== Check test cases for NewIterator with empty requireTypes ==="
rg -n -B5 -A5 'NewIterator.*\[\]' pkg/storage/ server/ --type go | head -40
echo ""
echo "=== Verify the handler logic more carefully ==="
rg -n -B20 -A10 'GetHistoryHotRegionIter' server/handler.go | head -60
echo ""
echo "=== Check if any code path could pass empty hotRegionTypes ==="
rg -n 'GetAllRequestHistoryHotRegion\|GetHistoryHotRegionIter' server/handler.go -A20 | head -40Repository: tikv/pd
Length of output: 5748
G602 exclusion silences real bug patterns, though currently mitigated by caller validation.
The unguarded it.iters[0] access at pkg/storage/hot_region_storage.go:364 is a genuine code pattern that G602 would flag. However, the actual risk is lower than stated because the Handler.GetAllRequestHistoryHotRegion() method (lines 142-146 in server/handler.go) always ensures non-empty hotRegionTypes by defaulting to storage.HotRegionTypes when the request is empty.
That said, the function signature allows callers to pass empty slices without validation, and the code accesses iters[0] immediately without bounds checks. This is fragile defensive coding—while currently safe due to caller discipline, it could break with future refactoring.
Consider either:
- Adding validation to reject empty slices upfront, or
- Using file-level exclusions for this specific function rather than silencing all G602 warnings globally
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml at line 86, The code unsafely indexes it.iters[0] without
guarding against an empty slice, relying on upstream callers
(Handler.GetAllRequestHistoryHotRegion) to provide defaults; fix by adding an
explicit defensive check in the function that accesses it.iters (verify
len(it.iters) > 0 and return an error or default behavior if empty) or, if you
decide to accept the current pattern, restrict the G602 suppression to only the
specific function by adding a function-level nolint comment for G602 rather than
a global exclusion; update the function that uses it.iters[0] to either validate
input or document and narrowly suppress the linter.
close tikv#10314 Signed-off-by: JmPotato <github@ipotato.me>
- Add documented gosec exclusions (G118, G120, G123, G602, G702, G704, G705, G706) for rules with high false-positive rates in PD's context as an internal distributed systems component - Fix 4 potential slice out-of-bounds in key-pair iteration loops by changing `i < len(keys)` to `i+1 < len(keys)` - Replace 4 `WriteString(fmt.Sprintf(...))` with `fmt.Fprintf()` to avoid intermediate string allocations - Bump golangci-lint to v2.11.2 and Go directive to 1.25.8 Signed-off-by: JmPotato <github@ipotato.me>
JmPotato
force-pushed
the
upgrade-go-1.25.7-and-tidb-dashboard
branch
from
9b350e1 to
1d3db52
Compare
|
@JmPotato: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.golangci.yml (1)
81-90:⚠️ Potential issue | 🟠 MajorScope these new gosec suppressions to the packages that actually need them.
Most of the justifications here are package-specific (
internal admin APIs,dev tools,CLI tools), butlinters.settings.gosec.excludesdisables the rules for the entire repository. That means futureG118/G120/G702/G704/G705/G706findings in unrelated production code will never surface. Please move these to path-level exclusions or inline#nosec///gosec:disableon the audited call sites instead. golangci-lint and gosec both support narrower false-positive handling for exactly this case. (golangci-lint.run)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 81 - 90, The current .golangci.yml places many gosec rule IDs (G118, G120, G123, G402, G404, G602, G702, G704, G705, G706) into the global linters.settings.gosec.excludes which disables them repository-wide; instead, move each suppression into package/path-scoped exclusions or local inline disables at the audited call sites. Edit linters.settings.gosec.excludes to remove the global entries and add targeted exclusions for only the packages that legitimately need them (or use golangci-lint path-level exclude patterns or gosec inline comments like // `#nosec` or //gosec:disable on the specific functions/files), and ensure you reference the exact rule IDs (e.g., G118, G120, etc.) in those targeted excludes so unrelated production code keeps those checks enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.golangci.yml:
- Around line 81-90: The current .golangci.yml places many gosec rule IDs (G118,
G120, G123, G402, G404, G602, G702, G704, G705, G706) into the global
linters.settings.gosec.excludes which disables them repository-wide; instead,
move each suppression into package/path-scoped exclusions or local inline
disables at the audited call sites. Edit linters.settings.gosec.excludes to
remove the global entries and add targeted exclusions for only the packages that
legitimately need them (or use golangci-lint path-level exclude patterns or
gosec inline comments like // `#nosec` or //gosec:disable on the specific
functions/files), and ensure you reference the exact rule IDs (e.g., G118, G120,
etc.) in those targeted excludes so unrelated production code keeps those checks
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 082602f1-cac5-4929-9337-b1ae415ba327
📒 Files selected for processing (13)
.golangci.ymlMakefileclient/go.modgo.modpkg/mcs/scheduling/server/apis/v1/api.gopkg/mock/mockcluster/mockcluster.gopkg/schedule/checker/merge_checker_test.gopkg/schedule/labeler/labeler.gopkg/schedule/labeler/rules.goserver/api/region.gotests/integrations/go.modtests/server/api/region_label_test.gotools/go.mod
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/server/api/region_label_test.go
- pkg/schedule/labeler/labeler.go
- pkg/mcs/scheduling/server/apis/v1/api.go
- client/go.mod
- go.mod
- pkg/schedule/checker/merge_checker_test.go
- server/api/region.go
- tests/integrations/go.mod
- pkg/mock/mockcluster/mockcluster.go
- Makefile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10315 +/- ##
==========================================
+ Coverage 78.78% 78.90% +0.11%
==========================================
Files 527 527
Lines 70916 70920 +4
==========================================
+ Hits 55870 55956 +86
+ Misses 11026 10966 -60
+ Partials 4020 3998 -22
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
What problem does this PR solve?
Issue Number: Close #10314
What is changed and how does it work?
Check List
Tests
Code changes
Release note
Summary by CodeRabbit
Chores
Bug Fixes
Tests