oxidecomputer / oxidecomputer/omicron

In-memory result of inserting multiple `BgpPeerConfig`s with unnumbered peers may have incorrect communities and import/export policies

Open
#10,151 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

When performing the database inserts for a SwitchPortSettingsCreate request, we assemble an in-memory map keyed by IP address: https://github.com/oxidecomputer/omicron/blob/eb0bb82820e46f2b114f3ae0b620333bfe0109bc/nexus/db-queries/src/db/datastore/switch_port.rs#L1410-L1411

For unnumbered peers, we use the sentinel value 0.0.0.0 as the key: https://github.com/oxidecomputer/omicron/blob/eb0bb82820e46f2b114f3ae0b620333bfe0109bc/nexus/db-queries/src/db/datastore/switch_port.rs#L1416-L1419

After performing the inserts (which do record the correct data for each peer - this bug only affects the in-memory result we return), we reassemble the communities and import/export policies by matching up the inserted row against its corresponding entry in peer_by_addr: https://github.com/oxidecomputer/omicron/blob/eb0bb82820e46f2b114f3ae0b620333bfe0109bc/nexus/db-queries/src/db/datastore/switch_port.rs#L1535-L1561

However, unnumbered peers all share the same 0.0.0.0 key, so we only have one possible value, and we reuse that for all unnumbered peers. If we insert two different unnumbered peers associated with two different links and those two peers have different communities or import/export policies, the result we return will be incorrect: we'll return the same communities and import/export policies for all unnumbered peers (the exact value being whichever was inserted into the peer_by_addr map last).

This came up during cleanup work for #9832, when reworking the database representations for unnumbered peers to get rid of the need for a sentinel value. The upcoming PR that does that rework will fix this.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in nexus/db-queries/src/db/datastore/switch_port.rs at the peer_by_addr assembly around lines 1410-1419 and the result reconstruction around lines 1535-1561. Trace SwitchPortSettingsCreate with two unnumbered peers on different links and different communities or import/export policies. Done means the returned in-memory result preserves each peer's own values, while the database rows remain correct.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.