Validate Whereabouts IPAM CIDRs in NAD admission webhook#120
Open
sanjaytripathi97 wants to merge 2 commits into
Open
Validate Whereabouts IPAM CIDRs in NAD admission webhook#120sanjaytripathi97 wants to merge 2 commits into
sanjaytripathi97 wants to merge 2 commits into
Conversation
added 2 commits
June 9, 2026 20:46
Reject NetworkAttachmentDefinitions whose whereabouts ipam range, exclude, gateway, or ipRanges entries contain invalid CIDR or IP values during admission.
WalkthroughThis PR adds Whereabouts IPAM validation to the webhook admission controller. It implements validators for Whereabouts IP address management configurations, integrates validation into the NetworkAttachmentDefinition admission flow, and provides comprehensive unit and integration test coverage. ChangesWhereabouts IPAM Validation Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sanjaytripathi97 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
Contributor
|
Hi @sanjaytripathi97. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/webhook/ipam_validation.go`:
- Around line 55-77: The validators currently treat type mismatches as absent
and silently accept malformed Whereabouts IPAM fields; update the validation in
the ipam validation path to explicitly reject wrong JSON types instead of
returning nil. For each key referenced (range -> validateWhereaboutsRange,
range_start/range_end/gateway -> validateWhereaboutsStringIP, exclude ->
validateWhereaboutsExcludeList, and ipRanges -> the ipRangesRaw handling) detect
if the key exists but is the wrong type and return a descriptive error (e.g.,
"field X must be a string" or "field ipRanges must be an array") rather than
treating it as missing; keep calling the existing helper functions when types
are correct and only short-circuit to an error on type mismatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6745bcfc-b3f9-4061-9f68-68ef6df1df0f
📒 Files selected for processing (4)
pkg/webhook/ipam_validation.gopkg/webhook/ipam_validation_test.gopkg/webhook/webhook.gopkg/webhook/webhook_test.go
Comment on lines
+55
to
+77
| if rangeStr, ok := ipam["range"].(string); ok && rangeStr != "" { | ||
| if err := validateWhereaboutsRange(rangeStr); err != nil { | ||
| return fmt.Errorf("invalid whereabouts ipam range: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if err := validateWhereaboutsStringIP(ipam, "range_start"); err != nil { | ||
| return err | ||
| } | ||
| if err := validateWhereaboutsStringIP(ipam, "range_end"); err != nil { | ||
| return err | ||
| } | ||
| if err := validateWhereaboutsStringIP(ipam, "gateway"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := validateWhereaboutsExcludeList(ipam["exclude"]); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ipRangesRaw, ok := ipam["ipRanges"].([]interface{}) | ||
| if !ok { | ||
| return nil |
There was a problem hiding this comment.
Reject wrong JSON types for Whereabouts IPAM fields instead of silently skipping
Several validators treat type mismatch as “not present” and return nil (e.g., exclude, ipRanges, gateway/range_start/range_end, range). This lets malformed Whereabouts IPAM objects pass admission when fields exist but are non-string/non-array.
Suggested fix
func validateWhereaboutsIPAM(ipam map[string]interface{}) error {
- if rangeStr, ok := ipam["range"].(string); ok && rangeStr != "" {
- if err := validateWhereaboutsRange(rangeStr); err != nil {
- return fmt.Errorf("invalid whereabouts ipam range: %w", err)
- }
- }
+ if raw, exists := ipam["range"]; exists {
+ rangeStr, ok := raw.(string)
+ if !ok {
+ return fmt.Errorf("invalid whereabouts ipam range: must be a string")
+ }
+ if rangeStr != "" {
+ if err := validateWhereaboutsRange(rangeStr); err != nil {
+ return fmt.Errorf("invalid whereabouts ipam range: %w", err)
+ }
+ }
+ }
@@
- ipRangesRaw, ok := ipam["ipRanges"].([]interface{})
- if !ok {
- return nil
- }
+ rawIPRanges, exists := ipam["ipRanges"]
+ if !exists {
+ return nil
+ }
+ ipRangesRaw, ok := rawIPRanges.([]interface{})
+ if !ok {
+ return fmt.Errorf("invalid whereabouts ipam ipRanges: must be an array")
+ }
@@
func validateWhereaboutsExcludeList(excludeRaw interface{}) error {
- excludeList, ok := excludeRaw.([]interface{})
- if !ok || len(excludeList) == 0 {
+ if excludeRaw == nil {
+ return nil
+ }
+ excludeList, ok := excludeRaw.([]interface{})
+ if !ok {
+ return fmt.Errorf("invalid exclude list: must be an array")
+ }
+ if len(excludeList) == 0 {
return nil
}
@@
func validateWhereaboutsStringIP(ipam map[string]interface{}, field string) error {
- value, ok := ipam[field].(string)
- if !ok || value == "" {
+ raw, exists := ipam[field]
+ if !exists {
+ return nil
+ }
+ value, ok := raw.(string)
+ if !ok {
+ return fmt.Errorf("invalid whereabouts ipam %s: must be a string", field)
+ }
+ if value == "" {
return nil
}Also applies to: 101-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/webhook/ipam_validation.go` around lines 55 - 77, The validators
currently treat type mismatches as absent and silently accept malformed
Whereabouts IPAM fields; update the validation in the ipam validation path to
explicitly reject wrong JSON types instead of returning nil. For each key
referenced (range -> validateWhereaboutsRange, range_start/range_end/gateway ->
validateWhereaboutsStringIP, exclude -> validateWhereaboutsExcludeList, and
ipRanges -> the ipRangesRaw handling) detect if the key exists but is the wrong
type and return a descriptive error (e.g., "field X must be a string" or "field
ipRanges must be an array") rather than treating it as missing; keep calling the
existing helper functions when types are correct and only short-circuit to an
error on type mismatches.
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
1 participant
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.
Summary
The Multus admission controller accepts NetworkAttachmentDefinitions (NADs) with invalid Whereabouts IPAM values because validation only checks JSON syntax and basic CNI config shape. Invalid values such as
abc.169.1.0/24anda.b.c.d/23are currently allowed at admission time.This PR adds Whereabouts-specific IPAM validation during NAD admission and rejects configs containing invalid CIDR/IP values in:
ipam.rangeipam.excludeipam.gatewayipam.range_start/ipam.range_endipam.ipRanges[].range/ipam.ipRanges[].excludeValidation is scoped to
ipam.type: whereaboutsonly. Other IPAM plugins are unchanged.Problem
Invalid NAD example that is currently accepted: