Network Operator BGP API improvements #6362

internet-diglett · 2024-08-16T17:18:13Z

#6081

Refactored existing api to list bgp announce sets and added a new api to list announcements for an announce set

#6244 and #6245

Refactored bgp datastore methods to propagate additional error context as well as emit error types for the external API (404, 403, etc)

#6246

Two part fix:

Emit an error if operator attempts to create more than one bgp configuration for the same ASN
Emit an error if the operator tries to configure BGP peers for the same switch that point to bgp configs with different ASNs

#6349

Updated bgp datastore methods to always lookup the parent record (not just when the name needs to be resolved to a UUID). This guards us against situations where an invalid UUID is accidentally submitted.

sled03 ➜  oxide.rs git:(main) ✗ oxide --profile a4x2 system networking bgp announce-set list
[
  {
    "description": "Announce set for AS 47",
    "id": "4040ce02-f01d-449f-997e-aa5eb1ae1c8a",
    "name": "as47-announce",
    "time_created": "2024-08-19T20:49:49.040115Z",
    "time_modified": "2024-08-19T20:49:49.040115Z"
  },
  {
    "description": "Announce set for AS 48",
    "id": "c2e77d75-4c3d-4c61-8669-86e221bdaeca",
    "name": "as48-announce",
    "time_created": "2024-08-19T20:49:49.935591Z",
    "time_modified": "2024-08-19T20:49:49.935591Z"
  }
]


sled03 ➜  oxide.rs git:(main) ✗ oxide --profile a4x2 system networking bgp announcement list --name-or-id "as47-announce"
[
  {
    "address_lot_block_id": "4040ce02-f01d-449f-997e-aa5eb1ae1c8a",
    "announce_set_id": "4040ce02-f01d-449f-997e-aa5eb1ae1c8a",
    "network": "198.51.100.0/24"
  }
]

rcgoodfellow

Thanks Levon! Comments follow. Will take this for a test drive shortly.

rcgoodfellow · 2024-08-21T00:08:08Z

nexus/db-queries/src/db/datastore/bgp.rs

+                        .await?;
+
+                    if !configs_with_asn.is_empty() {
+                        error!(opctx.log, "config for asn already exists"; "asn" => ?config.asn, "configs" => ?configs_with_asn);


maybe the name of this method should be bgp_config_create instead of bgp_config_set as the latter implies we'll update/overwrite the existing config if it exists.

rcgoodfellow · 2024-08-21T00:28:04Z

nexus/db-queries/src/db/datastore/bgp.rs

+                if let Some(err) = err.take() {
+                    error!(opctx.log, "{msg}"; "error" => ?err);
+                    err
+                } else {


Can you provide a comment on why non "takable" errors result in a 500?

rcgoodfellow · 2024-08-21T01:00:01Z

nexus/db-queries/src/db/datastore/bgp.rs

+                                match e {
+                                    diesel::result::Error::NotFound => err
+                                        .bail(Error::not_found_by_id(
+                                            ResourceType::BgpConfig,


Should this be ResourceType::BgpAnnounceSet?

rcgoodfellow · 2024-08-21T01:00:26Z

nexus/db-queries/src/db/datastore/bgp.rs

+                                        diesel::result::Error::NotFound => err
+                                            .bail(Error::not_found_by_name(
+                                                ResourceType::BgpConfig,
+                                                &name,


Same as above, should probably be ResourceType::BgpAnnounceSet.

rcgoodfellow · 2024-08-21T01:05:08Z

nexus/db-queries/src/db/datastore/bgp.rs

+                                match e {
+                                    diesel::result::Error::NotFound => err
+                                        .bail(Error::not_found_by_id(
+                                            ResourceType::BgpConfig,


ResourceType::BgpAnnounceSet

rcgoodfellow · 2024-08-21T01:05:31Z

nexus/db-queries/src/db/datastore/bgp.rs

+                                    match e {
+                                        diesel::result::Error::NotFound => err
+                                            .bail(Error::not_found_by_name(
+                                                ResourceType::BgpConfig,


ResourceType::BgpAnnounceSet

rcgoodfellow · 2024-08-21T01:18:33Z

nexus/db-queries/src/db/datastore/switch_port.rs

+                            }
+                        })?;
+
+                    // if we're setting a port settings id (and therefore activating a configuration


Good catch! I guess this is one of the consequences of being able to build up arbitrary configs, but only have some of them active - we have to validate that anything that is being activated is valid against the rest of the active set.

internet-diglett · 2024-08-21T02:42:02Z

Some more test output from a4x2:

Error un-squashing for bgp:

sled03 ➜  oxide.rs git:(main) ✗ oxide --profile a4x2 system networking bgp config create --asn 49 --bgp-announce-set-id "32ff136e-2b11-47d6-9f23-85eb278095be" --description "BGP config for AS 49" --name as49
error
Error Response: status: 404 Not Found; headers: {"content-type": "application/json", "x-request-id": "5cbf4d5c-7005-43e9-8eac-c132dba884b4", "content-length": "185", "date": "Wed, 21 Aug 2024 02:38:02 GMT"}; value: Error { error_code: Some("ObjectNotFound"), message: "not found: bgp-announce-set with id \"32ff136e-2b11-47d6-9f23-85eb278095be\"", request_id: "5cbf4d5c-7005-43e9-8eac-c132dba884b4" }

Not allowing multiple configs for a bgp asn:

sled03 ➜  oxide.rs git:(main) ✗ oxide --profile a4x2 system networking bgp config create --asn 48 --bgp-announce-set-id "86afcc48-933e-4048-ab9b-0442aaea4efc" --description "another BGP config for AS 48" --name as48-2
error
Error Response: status: 409 Conflict; headers: {"content-type": "application/json", "x-request-id": "b14be4d5-1f08-4550-9d92-3368317a36d9", "content-length": "150", "date": "Wed, 21 Aug 2024 02:39:35 GMT"}; value: Error { error_code: Some("Conflict"), message: "cannot have more than one configuration per ASN", request_id: "b14be4d5-1f08-4550-9d92-3368317a36d9" }
Error Response: status: 409 Conflict; headers: {"content-type": "application/json", "x-request-id": "b14be4d5-1f08-4550-9d92-3368317a36d9", "content-length": "150", "date": "Wed, 21 Aug 2024 02:39:35 GMT"}; value: Error { error_code: Some("Conflict"), message: "cannot have more than one configuration per ASN", request_id: "b14be4d5-1f08-4550-9d92-3368317a36d9" }

internet-diglett · 2024-08-21T19:26:11Z

nexus/db-queries/src/db/datastore/bgp.rs

+                    // TODO: remove once per-switch-multi-asn support is added
+                    // Bail if an existing config for this ASN already exists.
+                    // This is a temporary measure until multi-asn-per-switch is supported.
+                    let configs_with_asn: Vec<BgpConfig> = dsl::bgp_config
+                        .filter(dsl::asn.eq(config.asn))
+                        .filter(dsl::time_deleted.is_null())
+                        .select(BgpConfig::as_select())
+                        .load_async(&conn)
+                        .await?;


So I found something that needs to be changed here. Elsewhere in Nexus our logic relies on this code being idempotent, so I'm reworking this to allow and exact match to return Ok(config) but otherwise bail with the error.

This still might not be fixed, doing a bit more testing

I think I have it sorted, it was another diesel-ism

It's been happy for multiple a4x2 laps now, auto-merging

internet-diglett added 3 commits August 13, 2024 21:36

WIP: network db queries: don't squash all errors into a 500

db20b83

Merge branch 'main' into bugfix-6244

efbe3a6

WIP: improve error detection and messages for BGP workflows

76a6f9c

internet-diglett changed the title ~~Network Operator API improvements~~ Network Operator BGP API improvements Aug 16, 2024

internet-diglett added 3 commits August 16, 2024 21:36

WIP: improve error detection and messages for BGP workflows

de821f8

provide more useful error messages / types from bgp datastore methods

308a600

add api endpoint for listing announce sets

e1be886

internet-diglett added 2 commits August 20, 2024 01:02

regen openapi and update test

7676834

fixes for #6246

62ceb10

internet-diglett marked this pull request as ready for review August 20, 2024 18:57

internet-diglett added this to the 10 milestone Aug 20, 2024

internet-diglett requested review from rcgoodfellow, smklein and davepacheco August 20, 2024 19:03

rcgoodfellow reviewed Aug 21, 2024

View reviewed changes

internet-diglett added 3 commits August 21, 2024 02:58

pr comment fixes

c044b8c

unbreak the thing

be60f15

Merge branch 'main' into bugfix-6244

20a98a1

internet-diglett commented Aug 21, 2024

View reviewed changes

restore idempotent behavior

57c3c23

rcgoodfellow approved these changes Aug 21, 2024

View reviewed changes

internet-diglett added 2 commits August 22, 2024 00:47

diesel-isms

2b87c42

Merge branch 'main' into bugfix-6244

6a4fef0

internet-diglett enabled auto-merge (squash) August 22, 2024 01:54

fixup! Merge branch 'main' into bugfix-6244

ebbee8e

internet-diglett disabled auto-merge August 22, 2024 02:32

internet-diglett enabled auto-merge (squash) August 22, 2024 02:32

internet-diglett merged commit f1e1aff into main Aug 22, 2024
23 checks passed

internet-diglett deleted the bugfix-6244 branch August 22, 2024 03:48

internet-diglett mentioned this pull request Aug 28, 2024

sync with omicron/r10 nexus.json oxidecomputer/oxide.rs#813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network Operator BGP API improvements #6362

Network Operator BGP API improvements #6362

internet-diglett commented Aug 16, 2024 •

edited

Loading

internet-diglett commented Aug 19, 2024

rcgoodfellow left a comment •

edited

Loading

rcgoodfellow Aug 21, 2024

rcgoodfellow Aug 21, 2024

rcgoodfellow Aug 21, 2024

rcgoodfellow Aug 21, 2024

rcgoodfellow Aug 21, 2024

rcgoodfellow Aug 21, 2024

rcgoodfellow Aug 21, 2024

internet-diglett commented Aug 21, 2024

internet-diglett Aug 21, 2024

internet-diglett Aug 21, 2024

internet-diglett Aug 22, 2024

internet-diglett Aug 22, 2024

Network Operator BGP API improvements #6362

Network Operator BGP API improvements #6362

Conversation

internet-diglett commented Aug 16, 2024 • edited Loading

#6244 and #6245

Related

internet-diglett commented Aug 19, 2024

rcgoodfellow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internet-diglett commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internet-diglett commented Aug 16, 2024 •

edited

Loading

rcgoodfellow left a comment •

edited

Loading