fix:104 unsafe integer conversion in instance_utils.go #106
Conversation
Signed-off-by: gzb2025 <[email protected]>
Summary of ChangesHello @gzb2025, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an unsafe integer conversion vulnerability in GetPodComponentID by replacing strconv.Atoi with strconv.ParseInt and specifying a 32-bit size, which prevents potential overflows. The changes also correctly handle parsing of negative component IDs from pod names, a case that was previously failing. The addition of a comprehensive test suite in instance_utils_test.go is excellent and covers many edge cases. I have one suggestion to further improve the test coverage by adding a boundary case.
| tests := []struct { | ||
| name string // 测试用例名称 | ||
| pod *corev1.Pod // 输入的Pod对象 | ||
| expected int32 // 预期返回的ComponentID | ||
| }{ | ||
| // 场景1：通过标签获取ID（正常情况） | ||
| { | ||
| name: "valid id from label (positive)", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "123", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: 123, | ||
| }, | ||
| { | ||
| name: "valid id from label (negative)", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "-456", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: -456, | ||
| }, | ||
|
|
||
| // 场景2：标签存在但值不合法（非数字、超出范围） | ||
| { | ||
| name: "invalid non-numeric id in label", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "abc", | ||
| }, | ||
| }, | ||
| }, | ||
| expected: 0, // 错误时返回默认值0 | ||
| }, | ||
| { | ||
| name: "id in label exceeds int32 max", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "2147483648", // 比MaxInt32大1 | ||
| }, | ||
| }, | ||
| }, | ||
| expected: 0, | ||
| }, | ||
| { | ||
| name: "id in label less than int32 min", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "-2147483649", // 比MinInt32小1 | ||
| }, | ||
| }, | ||
| }, | ||
| expected: 0, | ||
| }, | ||
|
|
||
| // 场景3：标签不存在，从Pod名称解析ID（名称格式：前缀-组件名-ID） | ||
| { | ||
| name: "valid id from pod name (positive)", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "instance-web-789", // 最后一段为ID | ||
| Labels: map[string]string{}, // 无ComponentID标签 | ||
| }, | ||
| }, | ||
| expected: 789, | ||
| }, | ||
| { | ||
| name: "valid id from pod name (negative)", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "instance-db--987", // 最后一段为负ID | ||
| Labels: map[string]string{}, | ||
| }, | ||
| }, | ||
| expected: -987, | ||
| }, | ||
|
|
||
| // 场景4：标签不存在，名称解析不合法 | ||
| { | ||
| name: "invalid non-numeric id in pod name", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "instance-cache-xyz", // 最后一段非数字 | ||
| Labels: map[string]string{}, | ||
| }, | ||
| }, | ||
| expected: 0, | ||
| }, | ||
| { | ||
| name: "id in pod name exceeds int32 max", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "instance-store-2147483648", | ||
| Labels: map[string]string{}, | ||
| }, | ||
| }, | ||
| expected: 0, | ||
| }, | ||
| { | ||
| name: "id in pod name less than int32 min", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "instance-queue--2147483649", | ||
| Labels: map[string]string{}, | ||
| }, | ||
| }, | ||
| expected: 0, | ||
| }, | ||
|
|
||
| // 场景5：边界值测试（int32最大/最小值） | ||
| { | ||
| name: "int32 max value in label", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "2147483647", // math.MaxInt32 | ||
| }, | ||
| }, | ||
| }, | ||
| expected: math.MaxInt32, | ||
| }, | ||
| { | ||
| name: "int32 min value in label", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "-2147483648", // math.MinInt32 | ||
| }, | ||
| }, | ||
| }, | ||
| expected: math.MinInt32, | ||
| }, | ||
| { | ||
| name: "int32 max value in pod name", | ||
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "instance-max-2147483647", | ||
| Labels: map[string]string{}, | ||
| }, | ||
| }, | ||
| expected: math.MaxInt32, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The test suite is very comprehensive and covers a wide range of scenarios. To make it even more robust, I suggest adding a test case for the minimum int32 value parsed from a pod name. This would complete the boundary value testing for pod names, providing symmetry with the existing test for the maximum int32 value (int32 max value in pod name).
Here is a suggested test case to add:
{
name: "int32 min value in pod name",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-min--2147483648", // math.MinInt32
Labels: map[string]string{},
},
},
expected: math.MinInt32,
},
| "sigs.k8s.io/rbgs/api/workloads/v1alpha1" | ||
| ) | ||
|
|
||
| // TestGetPodComponentID 测试修复后的GetPodComponentID函数 |
There was a problem hiding this comment.
Please add comments in English.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes unsafe integer conversion issues in the GetPodComponentID function, addressing issue #104. The changes ensure proper int32 range validation and correct parsing of negative component IDs from pod names.
Key Changes
- Replaced
strconv.Atoiwithstrconv.ParseInt(componentId, 10, 32)to explicitly validate int32 range with proper error handling - Implemented logic to correctly parse negative IDs from pod names (e.g.,
instance-db--987→-987) by detecting double-dash patterns - Added comprehensive test coverage with 12 test scenarios covering positive/negative values, boundary cases, and invalid inputs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| pkg/reconciler/instance/utils/instance_utils.go | Enhanced GetPodComponentID function with safer integer parsing using ParseInt with 32-bit constraint and improved negative number detection logic for pod name parsing |
| pkg/reconciler/instance/utils/instance_utils_test.go | Added comprehensive test suite with 12 test cases covering valid/invalid IDs, boundary values (int32 max/min), and edge cases for both label-based and name-based ID extraction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pod: &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Labels: map[string]string{ | ||
| v1alpha1.InstanceComponentIDKey: "2147483648", // 比MaxInt32大1 |
There was a problem hiding this comment.
Chinese comment should be in English for consistency with the rest of the codebase.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an unsafe integer conversion and a bug in parsing negative IDs from pod names in instance_utils.go. The use of strconv.ParseInt with a specific bit size is a solid improvement for type safety. The logic to handle negative IDs from pod names is also well-implemented. Furthermore, the addition of a comprehensive test suite in instance_utils_test.go is excellent for ensuring the correctness and robustness of these changes. I have provided a couple of suggestions: one to refactor the main function for better code clarity and maintainability by removing duplicated logic, and another to add a missing boundary condition to the test suite to make it even more complete.
| componentId := pod.Labels[v1alpha1.InstanceComponentIDKey] | ||
| if len(componentId) != 0 { | ||
| id, _ := strconv.Atoi(componentId) | ||
| id, err := strconv.ParseInt(componentId, 10, 32) | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| return int32(id) | ||
| } | ||
| list := strings.Split(pod.Name, "-") | ||
| componentId = list[len(list)-1] | ||
| id, _ := strconv.Atoi(componentId) | ||
| lastIdx := len(list) - 1 | ||
| for lastIdx >= 0 && list[lastIdx] == "" { | ||
| lastIdx-- | ||
| } | ||
| if lastIdx < 0 { | ||
| return 0 | ||
| } | ||
| if lastIdx > 0 && list[lastIdx-1] == "" { | ||
| componentId = "-" + list[lastIdx] | ||
| } else { | ||
| componentId = list[lastIdx] | ||
| } | ||
| id, err := strconv.ParseInt(componentId, 10, 32) | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| return int32(id) |
There was a problem hiding this comment.
The logic for parsing the component ID string and handling errors is duplicated for both the label and pod name cases. You can refactor the function to first determine the componentId string (from either the label or the pod name), and then perform the parsing once at the end. This will make the code cleaner and adhere to the DRY (Don't Repeat Yourself) principle.
componentId := pod.Labels[v1alpha1.InstanceComponentIDKey]
if len(componentId) == 0 {
list := strings.Split(pod.Name, "-")
lastIdx := len(list) - 1
for lastIdx >= 0 && list[lastIdx] == "" {
lastIdx--
}
if lastIdx < 0 {
return 0
}
if lastIdx > 0 && list[lastIdx-1] == "" {
componentId = "-" + list[lastIdx]
} else {
componentId = list[lastIdx]
}
}
id, err := strconv.ParseInt(componentId, 10, 32)
if err != nil {
return 0
}
return int32(id)| }, | ||
| } |
There was a problem hiding this comment.
Great test coverage! To make it even more robust, consider adding a boundary test case for the minimum int32 value parsed from a pod name. You have a test for the max value from a pod name, and min value from a label, but the math.MinInt32 case from a pod name is missing.
expected: math.MaxInt32,
},
{
name: "int32 min value in pod name",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-min--2147483648", // math.MinInt32
Labels: map[string]string{},
},
},
expected: math.MinInt32,
},
}4 participants
Ⅰ. Motivation
The GetPodComponentID function in instance_utils.go had two main issues:
Unsafe integer conversion: Used strconv.Atoi (which returns int, size-dependent on system) and directly cast to int32, risking overflow on 64-bit systems.
Incorrect negative ID parsing from pod name: When parsing component ID from pod names (e.g., instance-db--987), the original logic failed to recognize negative signs, resulting in incorrect positive values.
These issues could lead to unexpected behavior in component ID resolution, affecting instance management and scaling logic.
Ⅱ. Modifications
Improved integer parsing for labels:
Replaced strconv.Atoi with strconv.ParseInt(componentId, 10, 32) to explicitly parse values into int32 range, with error handling to return 0 on failure.
Fixed negative ID parsing from pod names:
When labels are missing, instead of directly taking the last segment of strings.Split(pod.Name, "-"), added logic to:
Traverse backward to find the last non-empty segment (handling empty strings from -- splits).
Check if the previous segment is empty to detect negative signs (e.g., --987 becomes -987).
Used strconv.ParseInt with 32 bit size for consistent range checks, returning 0 on parsing errors.
Added comprehensive test cases (in instance_utils_test.go) to cover edge cases.
Ⅲ. Does this pull request fix one issue?
fixes #104
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added TestGetPodComponentID with 12 test scenarios:
Valid positive/negative IDs from labels.
Invalid label values (non-numeric, exceeding int32 max/min).
Valid positive/negative IDs from pod names (e.g., instance-db--987).
Invalid pod name values (non-numeric, exceeding int32 max/min).
Boundary values (int32 max: 2147483647, int32 min: -2147483648) in labels and pod names.
These tests verify correct parsing, error handling, and range checks.
Ⅴ. Describe how to verify it
Unit tests: Run go test ./pkg/reconciler/instance/utils/ -v -run TestGetPodComponentID to ensure all test cases pass.
Manual verification:
Create pods with labels containing valid/invalid component IDs (e.g., component-id: "-123", component-id: "2147483648").
Create pods with names containing negative IDs (e.g., instance-web--456) and non-numeric suffixes (e.g., instance-db-abc).
Call GetPodComponentID on these pods and verify returns match expectations (correct IDs for valid cases, 0 for invalid cases).
VI. Special notes for reviews
Checklist
make fmt.