Skip to content

BGP unnumbered peers: make database representation consistent and more strict#10155

Open
jgallagher wants to merge 21 commits intojohn/stronger-unnumbered-types-2from
john/networking-db-cleanup
Open

BGP unnumbered peers: make database representation consistent and more strict#10155
jgallagher wants to merge 21 commits intojohn/stronger-unnumbered-types-2from
john/networking-db-cleanup

Conversation

@jgallagher
Copy link
Contributor

Staged on top of #10122, which strengthened the types used for BGP peers in the external API.

This PR is pretty big, but the actual prod code changes are more like +600/-300 (there are a bunch of new tests and a bunch of SQL migration Stuff pushing the total size up). I'd recommend starting with the changes to dbinit.sql, which should be in line with the plan @internet-diglett and I discussed. Summarizing #9832 (comment):

  • Always represent unnumbered peers as a NULL address
  • Add CHECK constraints that prevent storing the addresses 0.0.0.0 or :: to ensure we don't have any leftover sentinel values
  • For the three tables that required the address to be non-NULL because it was part of a composite primary key, add a new synthetic id primary key (a random UUID)

The database migrations are similar in spirit to #9976: while we're not correcting mismatches between diesel and CRDB here, we are introducing migrations that could fail if real databases have data we don't expect (e.g., an address of 0.0.0.0 in the table where we expect unnumbered peers to be represented as NULL). The tactic I went with in the migration is:

  • If there's only one unnumbered peer (treating NULL, 0.0.0.0, :: all as possibilities), convert it to NULL.
  • If there are multiple, keep one (preferring NULL then 0.0.0.0) and delete the rest. This shouldn't delete anything unless someone has some already-invalid-at-runtime data.

There's a data migration test that should cover a sample of each kind of possibly-invalid input and confirm the above behavior.

One other thing that snuck in here is a bugfix for #10151; that's in 70ae97e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant