chore: Discard reserved IPs from the config entities#1648
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVpcExpose validation rejects special-use IPv4 and IPv6 prefixes in exposed and translated ranges. ConfigError reports the rejected prefix and reason, while overlay tests cover reserved, multicast, loopback, link-local, and excluded-fragment cases. ChangesSpecial-use prefix validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds validation to reject special-use/reserved IP prefixes from VpcExpose endpoint ranges, and enables previously-ignored tests that assert those prefixes are rejected.
Changes:
- Introduces a special-use prefix list and validation helper to reject reserved IPv4/IPv6 ranges in
VpcExpose. - Adds a
ConfigError::SpecialUsePrefixerror variant for clearer failures. - Un-ignores and extends overlay validation tests for reserved/multicast/loopback/link-local prefixes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| config/src/external/overlay/vpcpeering.rs | Adds special-use prefix list + validation in VpcExpose::validate() to reject reserved ranges. |
| config/src/external/overlay/validation_tests.rs | Enables reserved-prefix validation tests and adds link-local coverage. |
| config/src/errors.rs | Adds SpecialUsePrefix error variant to report reserved prefix rejection. |
![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 `@config/src/external/overlay/vpcpeering.rs`:
- Around line 18-52: The SPECIAL_USE_PREFIXES table in special_use_class lacks
IPv6 loopback and multicast reservations. In
config/src/external/overlay/vpcpeering.rs lines 18-52, add ::1/128 classified as
loopback and ff00::/8 classified as multicast; in
config/src/external/overlay/validation_tests.rs lines 165-177, add rejection
tests covering both prefixes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caf8f0c8-bdaf-4a3b-b6e2-45ff34be74e5
📒 Files selected for processing (3)
config/src/errors.rsconfig/src/external/overlay/validation_tests.rsconfig/src/external/overlay/vpcpeering.rs
bdc0c80 to
8139d85
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/src/external/overlay/validation_tests.rs (1)
127-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen assertions to match the specific error variant.
The special-use prefix tests use
assert!(expose.validate().is_err()), which only verifies that validation fails. Since the upstream validation returnsConfigError::SpecialUsePrefix(prefix, class)for these cases (pervpcpeering.rslines 352-358 anderrors.rslines 69-70), asserting the specific variant would catch regressions where validation fails for a different reason (e.g., an unrelated check accidentally rejecting a valid prefix).♻️ Proposed fix: assert SpecialUsePrefix variant
fn test_reserved_ipv4_zero_rejected() { let expose = VpcExpose::empty().ip("0.0.0.0/32".into()); - assert!(expose.validate().is_err()); + let result = expose.validate(); + assert!( + matches!(result, Err(ConfigError::SpecialUsePrefix(_, "unspecified"))), + "{result:?}" + ); }Apply the same pattern to the remaining special-use tests, matching the expected class string (
"loopback","link-local","multicast","reserved/broadcast","unspecified").Also applies to: 153-178
🤖 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 `@config/src/external/overlay/validation_tests.rs` around lines 127 - 137, Strengthen the special-use prefix assertions in test_reserved_ipv4_zero_rejected, test_reserved_ipv6_zero_rejected, and the other remaining special-use tests to match ConfigError::SpecialUsePrefix with the expected prefix and class string, rather than only checking validation failure. Use the appropriate classes, including “reserved/broadcast” for 0.0.0.0/32 and “unspecified” for ::/128, while preserving each test’s existing input.
🤖 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.
Nitpick comments:
In `@config/src/external/overlay/validation_tests.rs`:
- Around line 127-137: Strengthen the special-use prefix assertions in
test_reserved_ipv4_zero_rejected, test_reserved_ipv6_zero_rejected, and the
other remaining special-use tests to match ConfigError::SpecialUsePrefix with
the expected prefix and class string, rather than only checking validation
failure. Use the appropriate classes, including “reserved/broadcast” for
0.0.0.0/32 and “unspecified” for ::/128, while preserving each test’s existing
input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cf55e06-dbec-4055-965e-9a3006ac4732
📒 Files selected for processing (3)
config/src/errors.rsconfig/src/external/overlay/validation_tests.rsconfig/src/external/overlay/vpcpeering.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- config/src/external/overlay/vpcpeering.rs
- config/src/errors.rs
8139d85 to
00e44c8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/src/external/overlay/validation_tests.rs (1)
127-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse specific error variant assertions for special-use prefix tests.
The new tests use
assert!(expose.validate().is_err()), which only verifies that validation fails — not that it fails withConfigError::SpecialUsePrefix. An unrelated validation failure (e.g., a future regression in a different check) would still pass these tests, reducing their diagnostic value. Other tests in this file already usematches!with specificConfigErrorvariants.Consider asserting against the expected variant:
- assert!(expose.validate().is_err()); + let result = expose.validate(); + assert!( + matches!(result, Err(ConfigError::SpecialUsePrefix(_, _))), + "{result:?}" + );This applies to all newly enabled/added tests in this range (
test_reserved_ipv4_zero_rejected,test_reserved_ipv6_zero_rejected,test_reserved_ipv4_broadcast_rejected,test_multicast_prefix_rejected,test_loopback_prefix_rejected,test_ipv4_link_local_rejected,test_ipv6_link_local_rejected,test_ipv6_loopback_rejected,test_ipv6_multicast_rejected, andtest_reserved_fragment_after_exclusion_rejected).🤖 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 `@config/src/external/overlay/validation_tests.rs` around lines 127 - 202, Update the listed special-use prefix tests to assert that validate() returns ConfigError::SpecialUsePrefix using the file’s existing matches! assertion pattern, rather than only checking is_err(). Apply this to all ten named tests, including the post-exclusion fragment case, while preserving their current inputs and setup.
🤖 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.
Nitpick comments:
In `@config/src/external/overlay/validation_tests.rs`:
- Around line 127-202: Update the listed special-use prefix tests to assert that
validate() returns ConfigError::SpecialUsePrefix using the file’s existing
matches! assertion pattern, rather than only checking is_err(). Apply this to
all ten named tests, including the post-exclusion fragment case, while
preserving their current inputs and setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5016a542-c81d-44ca-aba1-3f6a1a41d316
📒 Files selected for processing (3)
config/src/errors.rsconfig/src/external/overlay/validation_tests.rsconfig/src/external/overlay/vpcpeering.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- config/src/errors.rs
- config/src/external/overlay/vpcpeering.rs
c7e94e2 to
e3dcd17
Compare
e3dcd17 to
d2a148f
Compare
| #[error("Prefix {0} is a special-use/reserved range ({1}) and cannot be used in a VpcExpose")] | ||
| SpecialUsePrefix(Prefix, &'static str), |
| fn reject_special_use(prefixes: &PrefixPortsSet) -> ConfigResult { | ||
| for prefix_with_ports in prefixes { | ||
| let prefix = prefix_with_ports.prefix(); | ||
| if let Some(class) = special_use_class(&prefix) { | ||
| return Err(ConfigError::SpecialUsePrefix(prefix, class)); | ||
| } |
d2a148f to
c6cd898
Compare
| /// Reason a prefix is reserved, or `None` if it is usable. | ||
| /// | ||
| /// Rejects only a prefix wholly inside a reserved block, so the roots `0.0.0.0/0` and `::/0` | ||
| /// (which contain reserved blocks but are not contained by one) stay valid. | ||
| fn special_use_class(prefix: &Prefix) -> Option<&'static str> { | ||
| SPECIAL_USE_PREFIXES | ||
| .iter() | ||
| .find(|(special, _)| special.covers(prefix)) | ||
| .map(|(_, class)| *class) | ||
| } |
There was a problem hiding this comment.
The doc comment for this function is not accurate, it doesn't reject anything - it only tells whether the prefix uses any of the special-use prefixes, right? Can you please fix?
(I'm not sure we need it as a separate function by the way, I'd probably inline it inside of reject_special_use(), but I don't mind much.)
| static SPECIAL_USE_PREFIXES: LazyLock<Vec<(Prefix, &'static str)>> = LazyLock::new(|| { | ||
| [ | ||
| (IpAddr::V4(Ipv4Addr::UNSPECIFIED), 8, "unspecified"), | ||
| (IpAddr::V4(Ipv4Addr::new(127, 0, 0, 0)), 8, "loopback"), |
There was a problem hiding this comment.
Nit: you could use the IpAddr::V4 wrapping part to the .map() at the end of the function, or even use Prefix::from_str("127.0.0.0/8") directly instead of IpAddr::V4(Ipv4Addr::new(127, 0, 0, 0)), 8, it would be easier to read?
| ( | ||
| IpAddr::V4(Ipv4Addr::new(240, 0, 0, 0)), | ||
| 4, | ||
| "reserved (240/4)", |
There was a problem hiding this comment.
| "reserved (240/4)", | |
| "reserved (240.0.0.0/4)", |
Not much longer, and more explicit in debug messages
| // Re-check the effective (post-exclusion) sets: collapsing can carve a broad allowed prefix | ||
| // down to a fragment that lands wholly inside a reserved range. | ||
| reject_special_use(&clone.ips)?; | ||
| if let Some(nat) = &clone.nat { | ||
| reject_special_use(&nat.as_range)?; | ||
| } |
There was a problem hiding this comment.
I don't see the point in checking before and after the exclusion prefix collapsing. Just keep this one after the collapse. This way:
- We avoid checking twice
- If an allowed prefix is covered by a special prefix, but there exists an exclusion prefix removing it, then it's an acceptable configuration because we won't end up using the prefix covered by a special prefix. So the early check is not entirely correct, only this one should be kept.
| /// Reserved prefixes that are never valid tenant endpoints, with the reason they are rejected. | ||
| static SPECIAL_USE_PREFIXES: LazyLock<Vec<(Prefix, &'static str)>> = LazyLock::new(|| { | ||
| [ | ||
| (IpAddr::V4(Ipv4Addr::UNSPECIFIED), 8, "unspecified"), |
There was a problem hiding this comment.
Can we change the string for this one? In logs it will read like no reason was specified. Could be INADDR_ANY, or maybe just 0.0.0.0?
| fn special_use_class(prefix: &Prefix) -> Option<&'static str> { | ||
| SPECIAL_USE_PREFIXES | ||
| .iter() | ||
| .find(|(special, _)| special.covers(prefix)) |
There was a problem hiding this comment.
Shouldn't this be collides_with() instead of covers()? If the special prefix does not fully cover the allowed prefix, then:
-
In theory, we could have the non-covered part of the allowed prefix covered by some other special-use prefix, so maybe there's no valid IP left in the allowed prefix anyway; but we don't know, because we check coverage from individual special prefixes only, rather than checking coverage by the union of these prefixes. I don't think we have contiguous prefix in
SPECIAL_USE_PREFIXESso it's probably not an issue in practice but still something we'd have to keep in mind if we were to expand the list for whatever reason. -
If we accept prefixes that contains IPs from some special-use ranges, in particular for
as_rangeprefixes, we risk using these reserved IPs in practice, for example by using them for masquerading in the allocator.
So instead, I think we should reject if any IP from Prefix is also in one of the special use prefixes? If, for example, the user wants 0.0.0.0/8 without breaking it down into a list of smaller prefixes to avoid including 0.0.0.0, they can always add a not: 0.0.0.0/32 to comply?
There was a problem hiding this comment.
Maybe I can use collides_with() only for as ranges? I think covers() for ips is fine
| matches!(result, Err(ConfigError::SpecialUsePrefix(..))), | ||
| "{result:?}" |
There was a problem hiding this comment.
Maybe check the exact message for this one, to make sure you don't get the one for 240.0.0.0/4
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergey Matov <sergey.matov@githedgehog.com>
c6cd898 to
aa6696e
Compare
| OverlappingPrefixes(PrefixWithOptionalPorts, PrefixWithOptionalPorts), | ||
| #[error("Inconsistent IP version in VpcExpose: {0}")] | ||
| InconsistentIpVersion(Box<VpcExpose>), | ||
| #[error("Prefix {0} is a special-use/reserved range ({1}) and cannot be used in a VpcExpose")] |
| // A broad prefix that only overlaps reserved space passes the raw check, but excluding the | ||
| // non-reserved part leaves a wholly-reserved fragment. The post-exclusion re-check must reject | ||
| // it. Here 224.0.0.0/3 minus 240.0.0.0/4 collapses to multicast 224.0.0.0/4. |
No description provided.