Revisit validation#1630
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the dataplane configuration “validated” model by removing separate Validated* wrapper types and switching validation to in-place enrichment (&mut self -> ConfigResult). It then updates router/mgmt/NAT/flow-filter call sites and tests to work with the new mutation-based validation flow.
Changes:
- Replace
ValidatedGwConfig/ValidatedOverlay/ValidatedVpcTable/ValidatedManifest/ValidatedExposewith runtime types (GwConfig,Overlay,VpcTable,VpcManifest,VpcExpose) that are validated/mutated in place. - Update mgmt/router/NAT/flow-filter modules and tests to call
validate()viamutvalues and to useGwConfig::from_validated(ExternalConfig). - Adjust config validation/enrichment logic (route table building, expose collapsing, underlay VTEP population) to happen during
validate().
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| validator/src/main.rs | Update validator to handle in-place validation API. |
| routing/src/router/rio.rs | Switch router runtime config storage to GwConfig. |
| routing/src/router/ctl.rs | Update router control channel messages to carry GwConfig. |
| routing/src/cli/handler.rs | Update CLI “show config” to use GwConfig. |
| nat/src/test.rs | Update NAT integration tests to validate Overlay in place. |
| nat/src/static_nat/test.rs | Update static NAT tests to work with GwConfig and in-place peering validation. |
| nat/src/static_nat/setup/mod.rs | Update NAT table builders to accept VpcTable/Peering/VpcExpose (no validated wrappers). |
| nat/src/portfw/portfwtable/setup.rs | Update port-forwarding ruleset builder to use non-validated overlay types. |
| nat/src/portfw/portfwtable/access.rs | Update port-forwarding table writer to accept VpcTable. |
| nat/src/masquerade/test.rs | Update masquerade tests to validate configs/overlays in place and use new accessors. |
| nat/src/masquerade/apalloc/test_alloc.rs | Update allocator tests to use validated-in-place VpcTable. |
| nat/src/masquerade/apalloc/setup.rs | Update allocator setup to use VpcManifest/VpcExpose and non-validated peerings. |
| nat/src/masquerade/allocator_writer.rs | Update masquerade config to accept VpcTable and non-validated peerings/exposes. |
| mgmt/src/tests/mgmt.rs | Update mgmt tests to build GwConfig via GwConfig::from_validated. |
| mgmt/src/processor/proc.rs | Switch mgmt processor pipeline to GwConfig and non-validated overlay types. |
| mgmt/src/processor/mgmt_client.rs | Update mgmt client API to return Arc<GwConfig>. |
| mgmt/src/processor/k8s_client.rs | Minor async call style change. |
| mgmt/src/processor/gwconfigdb.rs | Store applied config/history using GwConfig. |
| mgmt/src/processor/confbuild/router.rs | Update router config generation to accept Arc<GwConfig>. |
| mgmt/src/processor/confbuild/namegen.rs | Update name generation helpers to use Vpc. |
| mgmt/src/processor/confbuild/internal.rs | Update internal config builder to use GwConfig and non-validated overlay types. |
| flow-filter/src/tests.rs | Update flow-filter tests for in-place overlay validation. |
| flow-filter/src/setup.rs | Change flow-filter build API to accept &Overlay (no validated wrapper). |
| config/src/utils/overlap.rs | Update overlap helpers to operate on VpcExpose. |
| config/src/lib.rs | Export GwConfig instead of ValidatedGwConfig. |
| config/src/gwconfig.rs | Introduce GwConfig (wrapping ExternalConfig) and from_validated constructor. |
| config/src/external/underlay/mod.rs | Convert underlay validation to in-place enrichment (populate vtep). |
| config/src/external/overlay/vpcrouting.rs | Update VPC route table logic to use non-validated peerings/exposes and in-place validate. |
| config/src/external/overlay/vpcpeering.rs | Convert expose/manifest validation to in-place collapsing; remove validated expose/manifest types. |
| config/src/external/overlay/vpc.rs | Convert VPC/peering/table validation to in-place enrichment and build route tables during validate. |
| config/src/external/overlay/validation_tests.rs | Update overlay validation tests for new validate semantics. |
| config/src/external/overlay/tests.rs | Update overlay tests for new validate semantics. |
| config/src/external/overlay/mod.rs | Convert overlay validation to in-place (collect peerings + validate vpc table). |
| config/src/external/mod.rs | Convert ExternalConfig::validate to in-place enrichment and adjust helpers. |
| config/src/display.rs | Remove display impls for removed validated types; update overlay/VPC displays accordingly. |
Comments suppressed due to low confidence (1)
flow-filter/src/setup.rs:26
confidence: 7
tags: [logic]
`FlowFilterTable::build_from_overlay` relies on peerings having already been collected into each VPC (via `Overlay::validate()`), but the signature no longer enforces that. If a caller passes an unvalidated overlay with a non-empty `peering_table`, this will silently skip peerings and build an incomplete table.
A cheap runtime guard (plus clearer docs) avoids both silent misconfiguration and the overhead of cloning/validating here.
/// Build a [`FlowFilterTable`] from an overlay
pub fn build_from_overlay(overlay: &Overlay) -> Result<Self, ConfigError> {
let mut table = FlowFilterTable::new();
for vpc in overlay.vpc_table().values() {
for peering in vpc.peerings() {
table.add_peering(overlay, vpc, peering)?;
}
</details>
| /// FOR TESTS ONLY. Produces an enriched-but-not-collapsed expose (exclusion prefixes are | ||
| /// dropped without being applied), bypassing real validation. | ||
| #[cfg(feature = "testing")] | ||
| #[must_use] | ||
| #[allow(unsafe_code)] | ||
| unsafe fn fake_validated_expose(&self) -> ValidatedExpose { | ||
| ValidatedExpose { | ||
| unsafe fn fake_validated_expose(&self) -> VpcExpose { | ||
| VpcExpose { | ||
| default: self.default, | ||
| ips: self.ips.clone(), | ||
| nots: PrefixPortsSet::new(), | ||
| nat: self.nat.clone(), | ||
| } |
| /// Validate and enrich the external configuration in place (validating the underlay and | ||
| /// overlay and collecting peerings into each VPC). | ||
| /// | ||
| /// To obtain the runtime [`GwConfig`], validate then wrap: `cfg.validate()?; GwConfig::new(cfg)`. | ||
| /// | ||
| /// # Errors |
| /// Validate an [`ExternalConfig`] and wrap it into a runtime [`GwConfig`]. | ||
| /// | ||
| /// This is the only way to obtain a non-blank [`GwConfig`], which guarantees its | ||
| /// [`ExternalConfig`] has been validated. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns a [`ConfigError`] if validation fails. | ||
| pub fn from_validated(mut external: ExternalConfig) -> Result<Self, ConfigError> { | ||
| external.validate()?; | ||
| Ok(Self::new(external)) | ||
| } |
|
(I tagged to block the merge because I expect conflicts with #1618.) |
The vpcid map is a concept specific to a vpc table. Make it private, and construct it from a vpc table instead of an overlay. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Extend the VpcId map with Vnis and let peerings include the Vni of the remote VPC. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The builders will be used to represent default peering exposes. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The vpc routing module defines a vpc routing table which is an an alternate representation of the peerings of a given VPC. This representation is easier to validate and closer to what some NFs require to build their internal state. The validation of the vpc routing table includes now the constraint that remote destinations of a VPC that are allowed to overlap must be mapped to the same gateway group for failover to work correctly. This restriction is not one imposed by a single gateway but a fabric-wide one. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Build a vpc routing table from the set of validated peerings of a VPC and validate it, replacing the previous validation function. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Remove unnecessary derived impls as these impose unnecessary requirements on embedded types. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Store the vpc routing table in a ValidatedVpc for future use. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
6009f0f to
5818ca6
Compare
d3bb30b to
0af2025
Compare
Revert back to non-validated types Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
0af2025 to
5c59bf3
Compare
5818ca6 to
e8460cc
Compare
Notes