Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect determination of active filtering state for BGP peers #6316

Closed
rcgoodfellow opened this issue Aug 13, 2024 · 2 comments · Fixed by #6318
Closed

Incorrect determination of active filtering state for BGP peers #6316

rcgoodfellow opened this issue Aug 13, 2024 · 2 comments · Fixed by #6318
Labels
customer For any bug reports or feature requests tied to customer requests networking Related to the networking.
Milestone

Comments

@rcgoodfellow
Copy link
Contributor

In the following code

let active = peer_dsl::switch_port_settings_bgp_peer_config
.filter(db_peer::port_settings_id.eq(port_settings_id))
.select(db_peer::allow_export_list_active)
.limit(1)
.first_async::<bool>(&conn)
.await?;

If there are two peers configured within the same switch port settings, one with filtering and one without, this code will pick up the active property of whichever peer comes first in the database table. If the peers have different active states for export filtering, this results in one peer picking up the other's active state.

@rcgoodfellow
Copy link
Contributor Author

rcgoodfellow commented Aug 13, 2024

I have reproduced this in omicron main as of 4a6be3a.

The reproducer is as follows.

  1. Launch a4x2 with the PoP topology.
  2. Run the following command.
oxide system networking bgp filter \
    --rack `oxide system hardware rack list | jq -r .[0].id` \
    --switch switch0 \
    --port qsfp0 \
    --peer 169.254.10.1 \
    --direction 'export' \
    --allowed 198.51.100.0/24

Then, on switch0, observe that the expected filter is applied to the first peer, and the second peer has an empty allow list (this means allow no prefixes to export) instead of NoFiltering

root@oxz_switch:~# mgadm bgp c r l
[
    Router {
        asn: 65547,
        graceful_shutdown: false,
        id: 65547,
        listen: "[::]:179",
    },
]
root@oxz_switch:~# export ASN=65547
root@oxz_switch:~# mgadm bgp c n l
[
    Neighbor {
        allow_export: Allow(
            [
                V4(
                    Prefix4 {
                        length: 24,
                        value: 198.51.100.0,
                    },
                ),
            ],
        ),
        allow_import: NoFiltering,
        asn: 65547,
        communities: [],
        connect_retry: 3,
        delay_open: 3,
        enforce_first_as: false,
        group: "qsfp0",
        hold_time: 6,
        host: "169.254.10.1:179",
        idle_hold_time: 3,
        keepalive: 2,
        local_pref: None,
        md5_auth_key: None,
        min_ttl: None,
        multi_exit_discriminator: None,
        name: "169.254.10.1",
        passive: false,
        remote_asn: None,
        resolution: 100,
        vlan_id: None,
    },
    Neighbor {
        allow_export: Allow(
            [],
        ),
        allow_import: NoFiltering,
        asn: 65547,
        communities: [],
        connect_retry: 3,
        delay_open: 3,
        enforce_first_as: false,
        group: "qsfp0",
        hold_time: 6,
        host: "169.254.30.1:179",
        idle_hold_time: 3,
        keepalive: 2,
        local_pref: None,
        md5_auth_key: None,
        min_ttl: None,
        multi_exit_discriminator: None,
        name: "169.254.30.1",
        passive: false,
        remote_asn: None,
        resolution: 100,
        vlan_id: None,
    },
]

@rcgoodfellow
Copy link
Contributor Author

rcgoodfellow commented Aug 14, 2024

Confirmed that #6318 fixes the behavior described above.

internet-diglett added a commit that referenced this issue Aug 14, 2024
…ve` (#6318)

This change updates the queries to lookup peers by
`switch_port_settings_id` AND peer `address` when checking
`allow_export_list_active` column.

Related
---
#6316
@twinfees twinfees added customer For any bug reports or feature requests tied to customer requests and removed customer For any bug reports or feature requests tied to customer requests labels Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer For any bug reports or feature requests tied to customer requests networking Related to the networking.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants