rbg icon indicating copy to clipboard operation
rbg copied to clipboard

fix:104 unsafe integer conversion in instance_utils.go

Open gzb2025 opened this issue 1 month ago • 3 comments

Ⅰ. 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

  • [✅] Format your code make fmt.
  • [✅] Add unit tests or integration tests.
  • [ ] Update the documentation related to the change.

gzb2025 avatar Nov 18 '25 15:11 gzb2025

Summary of Changes

Hello @gzb2025, I'm Gemini Code Assist[^1]! 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 GetPodComponentID function, ensuring robust and accurate component ID resolution for instance management. By implementing safer integer parsing and correcting the interpretation of negative IDs from pod names, the changes prevent potential data overflows and misinterpretations, thereby enhancing the stability and reliability of the system's instance identification logic.

Highlights

  • Safe Integer Conversion: Replaced strconv.Atoi with strconv.ParseInt(componentId, 10, 32) in GetPodComponentID to explicitly parse values into int32 range, preventing potential overflows on 64-bit systems and adding error handling.
  • Correct Negative ID Parsing: Improved logic for parsing component IDs from pod names to correctly handle negative signs, especially in cases like instance-db--987, where a double hyphen indicates a negative number.
  • Comprehensive Test Coverage: Added extensive unit tests in instance_utils_test.go covering various scenarios, including valid/invalid IDs from labels and pod names, boundary values (MaxInt32, MinInt32), and out-of-range values.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 :thumbsup: and :thumbsdown: 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.

[^1]: Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

gemini-code-assist[bot] avatar Nov 18 '25 15:11 gemini-code-assist[bot]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests.

:loudspeaker: Thoughts on this report? Let us know!

codecov-commenter avatar Nov 19 '25 01:11 codecov-commenter

/gemini review

cheyang avatar Nov 20 '25 13:11 cheyang