Conversation
Track failing (CAS, PAS) hop in aspVerify and expose it as the aspa/invalid-hop tag; warn once per direction when peer ASN is unknown. Accept rpki-client's `providers` key (emits [0] for AS0) alongside Routinator's `provider_asids`. Add end-to-end tests covering file load, cache swap, ROV/ASPA actions, and reload. Combined with the n-2 valley-free relaxation already on dev0424.
There was a problem hiding this comment.
Pull request overview
This PR updates the RPKI ASPA implementation to (a) support multiple JSON ASPA schemas, (b) enhance ASPA verification results with failing-hop details, and (c) add end-to-end integration tests for ROV/ASPA behavior.
Changes:
- Extend JSON file parsing to accept both
provider_asids(Routinator) andproviders(rpki-client) ASPA formats, including AS0 handling. - Update ASPA verification to return failing-hop details and tag INVALID paths with
aspa/invalid-hop. - Add integration tests that exercise file loading, cache swapping, pipe wiring, and ROV/ASPA actions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| stages/rpki/validate.go | Updates ASPA validation call site to the new validateAspa(m) signature. |
| stages/rpki/rpki.go | Adds per-direction sync.Once to rate-limit “peer ASN unknown” warnings. |
| stages/rpki/integration_test.go | Adds end-to-end tests covering file load → pipe → ROV/ASPA actions. |
| stages/rpki/file_test.go | Adds a JSON parsing test for both ASPA provider key variants and AS0 encoding. |
| stages/rpki/file.go | Accepts providers key in ASPA JSON entries and routes providers to nextASPA. |
| stages/rpki/aspa_test.go | Updates tests for new aspVerify return signature and downstream behavior cases. |
| stages/rpki/aspa.go | Changes aspVerify to return failing hop, adds aspa/invalid-hop tagging, and refactors validateAspa. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if maxUp+maxDown < n-2 { | ||
| // NB: a >1-pair gap means both ramps hit asp_not; report the | ||
| // down-ramp failure (closer to the peer) if available. | ||
| if dnCAS != 0 { | ||
| return aspa_invalid, dnCAS, dnPAS |
There was a problem hiding this comment.
In downstream mode, the n-2 threshold makes 2-hop paths (len(path)==2) always return aspa_valid, even when the only hop is definitively asp_not (both ramps can break with maxUp=maxDown=0, but 0 < n-2 is false). This also prevents returning aspa_unknown for 2-hop paths with no attestations. Consider special-casing n <= 2 (or using requiredPairs := n-1 unless n >= 3, where you allow the single central uncovered pair) so 2-hop paths still validate the single pair.
| u := &m.Update | ||
| tags := pipe.UseTags(m) | ||
|
|
There was a problem hiding this comment.
tags := pipe.UseTags(m) is computed unconditionally, but tags is only used when s.aspa_tag is true. To avoid allocating/initializing the tag map for deployments that run ASPA validation with --aspa-tag=false, move the pipe.UseTags(m) call inside the if s.aspa_tag { ... } block.
| case aspa_invalid: | ||
| tags["aspa/status"] = "INVALID" | ||
| if failCAS != 0 { | ||
| tags["aspa/invalid-hop"] = fmt.Sprintf("%d %d", failCAS, failPAS) | ||
| } |
There was a problem hiding this comment.
The PR introduces the new aspa/invalid-hop tag for INVALID results, but there is no test asserting it is populated when aspVerify fails with a definitive asp_not. Adding a unit/integration test that triggers an aspa_invalid via aspVerify (not first-hop mismatch / AS_SET) and checks pipe.UseTags(m)["aspa/invalid-hop"] would prevent regressions and ensure the tag stays accurate.
No description provided.