karmadactl: validate --cert-external-ip to avoid silently using 127.0…#7656
karmadactl: validate --cert-external-ip to avoid silently using 127.0…#7656Anand-240 wants to merge 1 commit into
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 |
Summary of ChangesHello, 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 a bug in 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 the 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 counterproductive. 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. Footnotes
|
karmada-bot
added
the
size/S
![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 adds validation for the cert-external-ip parameter in karmadactl init by splitting the input comma-separated string and validating each IP address, along with adding corresponding unit tests. The review feedback points out a consistency issue where netutils.ParseIPSloppy is used for validation (which trims spaces), but downstream certificate generation uses net.ParseIP (which does not trim spaces). This mismatch could lead to silent failures or unexpected fallbacks if users include spaces in their IP list, so switching to net.ParseIP for validation is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| if i.ExternalIP != "" { | ||
| for _, ip := range strings.Split(i.ExternalIP, ",") { | ||
| if netutils.ParseIPSloppy(ip) == nil { |
There was a problem hiding this comment.
There is a potential consistency issue here. netutils.ParseIPSloppy(ip) tolerates leading and trailing spaces by trimming them internally. However, utils.StringToNetIP (called via utils.FlagsIP during certificate generation) uses net.ParseIP which does not tolerate spaces and will return nil, silently falling back to 127.0.0.1.
If a user provides --cert-external-ip "192.168.1.1, 192.168.1.2", the validation will pass because ParseIPSloppy successfully parses the trimmed IP. But during certificate generation, utils.StringToNetIP will fail to parse " 192.168.1.2" and silently fall back to 127.0.0.1.
To prevent this, we should use net.ParseIP(ip) directly for validation to ensure strict alignment with utils.StringToNetIP's parsing behavior, or ensure that utils.StringToNetIP also uses netutils.ParseIPSloppy.
| if netutils.ParseIPSloppy(ip) == nil { | |
| if net.ParseIP(ip) == nil { |
There was a problem hiding this comment.
Good catch, thanks. You're right that ParseIPSloppy was more lenient than the actual consumption path. I switched the validation to net.ParseIP so it matches exactly how utils.StringToNetIP parses the value during cert generation, and added a test for the spaced input ("192.0.2.1, 192.0.2.2") which is now correctly rejected. Updated in the latest push.
There was a problem hiding this comment.
Pull request overview
This PR fixes a karmadactl init bug where invalid --cert-external-ip values were silently converted to 127.0.0.1 during certificate SAN generation, leading to confusing TLS errors later.
Changes:
- Added explicit validation for
--cert-external-ipinCommandInitOption.Validate()by splitting on commas and parsing each IP. - Added regression tests covering invalid and valid
cert-external-ipinputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/karmadactl/cmdinit/kubernetes/deploy.go | Adds validation for ExternalIP to fail fast on invalid IP values. |
| pkg/karmadactl/cmdinit/kubernetes/deploy_test.go | Adds unit tests for valid/invalid cert-external-ip validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if i.ExternalIP != "" { | ||
| for _, ip := range strings.Split(i.ExternalIP, ",") { | ||
| if netutils.ParseIPSloppy(ip) == nil { | ||
| return fmt.Errorf("cert-external-ip %q is not a valid IP address", ip) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Done, switched to strings.SplitSeq to match how utils.FlagsIP splits the value. Thanks!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7656 +/- ##
==========================================
+ Coverage 42.04% 42.06% +0.01%
==========================================
Files 879 879
Lines 54827 54831 +4
==========================================
+ Hits 23053 23064 +11
+ Misses 30027 30022 -5
+ Partials 1747 1745 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
….0.1 Signed-off-by: Anand-240 <srivastavaanandprakash16@gmail.com>
Anand-240
force-pushed
the
fix-cert-external-ip-validation
branch
from
4f416af to
f2a4a7d
Compare
karmada-bot
added
size/M
What type of PR is this?
/kind bug
What this PR does / why we need it:
karmadactl initaccepts the--cert-external-ipflag but never validates it. The value is passed toutils.FlagsIP->utils.StringToNetIP, which silently falls back to127.0.0.1for any value it can't parse as an IP. So an invalid--cert-external-ipslips through and the certificate ends up signed for127.0.0.1instead of the address the user asked for, which surfaces later as confusing TLS errors.This PR validates
--cert-external-ipinValidate()(splitting on comma and parsing each entry withnetutils.ParseIPSloppy), the same way--karmada-apiserver-advertise-addressis already validated, and returns a clear error on invalid input.Which issue(s) this PR fixes:
Fixes #7655
Special notes for your reviewer:
Added regression tests (
Invalid cert-external-ip,Valid cert-external-ip) toTestCommandInitOption_Validate. The validation splits exactly the wayFlagsIPconsumes the value, so anything that passes validation is guaranteed to produce real IPs in the cert SANs.gofmt,go vet,go build, the package tests, and the import-aliases check all pass.Does this PR introduce a user-facing change?: