Refactor Network Operator APIs #6016

internet-diglett · 2024-07-08T21:04:15Z

Summary

Our currently deployed APIs for operator networking aren't as user friendly as we'd like them to be. After landing these changes we hope to be able to start breaking ground on Terraform provider updates for rack / operator networking, which should increase user friendliness as well as make testing more streamlined in some scenarios. This will also allow us to migrate from using --json-object with Oxide cli commands, for example:

# Add a route
sled03 ➜  a4x2 git:(main) ✗ oxide --profile a4x2 system networking switch-port configuration route add  \
--configuration "default-uplink0" \
--dst 10.88.0.0/24 \
--gw 10.8.0.1 \
--interface qsfp0
{
  "dst": "10.88.0.0/24",
  "gw": "10.8.0.1/32",
  "interface_name": "qsfp0",
  "port_settings_id": "ec815e89-ff84-4e85-ad54-55020eb812c9"
}

# List routes
sled03 ➜  a4x2 git:(main) ✗ oxide --profile a4x2 system networking switch-port configuration route list \
--configuration "default-uplink0"
[
  {
    "dst": "10.88.0.0/24",
    "gw": "10.8.0.1/32",
    "interface_name": "qsfp0",
    "port_settings_id": "ec815e89-ff84-4e85-ad54-55020eb812c9"
  }
]

# Delete a route
sled03 ➜  a4x2 git:(main) ✗ oxide --profile a4x2 system networking switch-port configuration route remove  \
--configuration "default-uplink0" \
--dst 10.88.0.0/24 \
--gw 10.8.0.1 \
--interface qsfp0

# List routes again
sled03 ➜  a4x2 git:(main) ✗ oxide --profile a4x2 system networking switch-port configuration route list --configuration "default-uplink0"
[]

Changes

The following changes should mostly preserve backwards compatibility with the existing API, so we should be able to continue to use all of our existing automation / tooling / scripts for the time being.

Additional Acceptance Criteria

resolves Provide a mechanism for changing individual port settings #5343
~~- [ ] resolves need to validate bgp config UUID when adding/setting bgp peer #6462 ~~~~
~~- [ ] partially satisfies Audit network configuration error squashing. #6398~~
resolves BGP API: Rename name_or_id path param #6497

WIP: refactor address lot apis

ahl

You might think about generating the SDK and CLI as you evolve this API. In particular, I'd love to see this list of CLI commands that require the --json-body parameter to shrink: https://github.com/oxidecomputer/oxide.rs/blob/main/cli/tests/data/json-body-required.txt (note about half of them are networking-related)

ahl · 2024-07-15T21:09:42Z

openapi/nexus.json

@@ -9727,7 +9816,7 @@
          "last_address"
        ]
      },
-      "AddressLotBlockCreate": {
+      "AddressLotBlock2": {


just looking through this quickly, but I think we should avoid adding more of these name conflicting types that result in the 2 suffix.

internet-diglett · 2024-07-15T22:44:38Z

You might think about generating the SDK and CLI as you evolve this API. In particular, I'd love to see this list of CLI commands that require the --json-body parameter to shrink: https://github.com/oxidecomputer/oxide.rs/blob/main/cli/tests/data/json-body-required.txt (note about half of them are networking-related)

@ahl do we have a preferred pattern that we want to use to avoid this?

ahl · 2024-07-15T22:50:29Z

You might think about generating the SDK and CLI as you evolve this API. In particular, I'd love to see this list of CLI commands that require the --json-body parameter to shrink: https://github.com/oxidecomputer/oxide.rs/blob/main/cli/tests/data/json-body-required.txt (note about half of them are networking-related)

@ahl do we have a preferred pattern that we want to use to avoid this?

There are some structures that are challenging to generate, in particular deeply nested structs. Perhaps put another way: you might keep in mind how you'd like the CLI to work for a particular API concept and then organize the API in a way where you can imagine the direct translation e.g. to CLI arguments. There's more that progenitor could and can do, but some structures--such as SwitchPortSettingsCreate--are pretty hard to imagine translated into something rational at the CLI

internet-diglett · 2024-07-17T20:26:42Z

@ahl I generated a local CLI with this PR's openapi spec and it seems to avoid the need for --json-body:

For more information, try '--help'.
$ ./target/debug/oxide system networking address-lot create
error: the following required arguments were not provided:
  --description <description>
  --kind <kind>
  --name <name>

$ ./target/debug/oxide system networking address-lot block add
error: the following required arguments were not provided:
  --address-lot <address-lot>
  --first-address <first-address>
  --last-address <last-address>

Usage: oxide system networking address-lot block add --address-lot <address-lot> --first-address <first-address> --last-address <last-address>

ahl · 2024-07-17T21:01:43Z

well that's good news!

internet-diglett · 2024-07-17T21:49:12Z

Local testing against a running control plane:

$ ./target/debug/oxide system networking address-lot list
[
  {
    "description": "initial infrastructure ip address lot",
    "id": "4d2fc8ca-8759-46e4-9c36-7e9bbd14ecc1",
    "kind": "infra",
    "name": "initial-infra",
    "time_created": "2024-07-17T21:39:29.384680Z",
    "time_modified": "2024-07-17T21:39:29.384680Z"
  }
]

$ ./target/debug/oxide system networking address-lot block list --address-lot initial-infra
[
  {
    "first_address": "10.85.0.30",
    "id": "60efc85a-34d0-4e07-b3e2-c11f203e054d",
    "last_address": "10.85.0.30"
  }
]

$ ./target/debug/oxide system networking address-lot block add \
--address-lot initial-infra \
--first-address 10.100.0.0 \
--last-address 10.100.0.255
{
  "first_address": "10.100.0.0",
  "id": "5eff580c-9e96-4c01-a580-b9c782bc4196",
  "last_address": "10.100.0.255"
}

$ ./target/debug/oxide system networking address-lot block list --address-lot initial-infra
[
  {
    "first_address": "10.100.0.0",
    "id": "5eff580c-9e96-4c01-a580-b9c782bc4196",
    "last_address": "10.100.0.255"
  }, {
    "first_address": "10.85.0.30",
    "id": "60efc85a-34d0-4e07-b3e2-c11f203e054d",
    "last_address": "10.85.0.30"
  }
]

$ ./target/debug/oxide system networking address-lot block remove \
--address-lot initial-infra \
--first-address 10.100.0.0 \
--last-address 10.100.0.255

$ ./target/debug/oxide system networking address-lot block list --address-lot initial-infra
[
  {
    "first_address": "10.85.0.30",
    "id": "60efc85a-34d0-4e07-b3e2-c11f203e054d",
    "last_address": "10.85.0.30"
  }
]

david-crespo · 2024-08-30T21:09:42Z

While you're messing with these APIs... #6497

FelixMcFelix

Thanks for all the work here Levon! I spent some time reading over this; I really like the more granular updates we can have over switch port configuration. I've left some comments throughout.

common/src/api/external/mod.rs

nexus/types/src/external_api/params.rs

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

FelixMcFelix · 2024-09-11T13:21:22Z

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

+                    let port_settings_id =
+                        switch_port_configuration_id(&conn, identity).await?;
+
+                    let port_config = SwitchPortConfig {
+                        port_settings_id,
+                        geometry: new_geometry,
+                    };
+
+                    // create or update geometry
+                    diesel::insert_into(dsl::switch_port_settings_port_config)
+                        .values(port_config.clone())
+                        .on_conflict(dsl::port_settings_id)
+                        .do_update()
+                        .set(dsl::geometry.eq(new_geometry))
+                        .execute_async(&conn)
+                        .await?;


I think this ID check -> use pattern is safe, given that CRDB works with transations set as SERIALIZABLE. I'd have to defer to someone with more knowledge there though!

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

FelixMcFelix · 2024-09-11T14:41:15Z

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

+                        NameOrId::Id(id) => {
+                            // verify id is valid
+                            bgp_config::table
+                                .filter(bgp_config::time_deleted.is_null())
+                                .filter(bgp_config::id.eq(id))
+                                .select(bgp_config::id)
+                                .limit(1)
+                                .first_async::<Uuid>(&conn)
+                                .await
+                                .map_err(|e: diesel::result::Error| {
+                                    match e {
+                                        diesel::result::Error::NotFound => {
+                                            err.bail(Error::not_found_by_id(ResourceType::BgpConfig, &id))
+                                        },
+                                        _ => {
+                                            let message = "error while looking up bgp config for bgp peer";
+                                            error!(opctx.log, "{message}"; "error" => ?e);
+                                            err.bail(Error::internal_error(message))
+                                        },
+                                    }
+                                })
+                        },


Should most of the linked config elements here have a lookup_resource! definition? That doesn't give us LookupPath on a conn, but...

I'm also starting to wonder about the NameOrId resolution within each transaction -- I think that resolving to an ID should still occur only once outside the transaction, and then we {use, fetch, check liveness of} the target resource by ID as part of the transaction. I don't see a way that these transactions can cause a retry today, but if that changes I imagine you could hypothetically be working on renamed resources.

Do we allow renaming of resources today? It is my understanding that Name and ID are not changeable.

ID is immutable, yeah -- I had meant to select the NameOrId::Name match branch at the time.

My thinking is in case an update endpoint does crop up in future -- e.g., not all project scoped resources have update endpoints, but the resource name is almost always mutable when such an endpoint exists.

nexus/src/external_api/http_entrypoints.rs

nexus/types/src/external_api/params.rs

elaine-oxide

See comments. I did a quick review for consistency, but did not go deep into the DB transactions.

common/src/api/external/mod.rs

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

elaine-oxide · 2024-09-12T01:06:43Z

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

+                        keepalive: new_settings.keepalive.into(),
+                        remote_asn: new_settings.remote_asn.map(Into::into),
+                        min_ttl: new_settings.min_ttl.map(Into::into),
+                        md5_auth_key: new_settings.md5_auth_key,


Is a clone required?

Also check line 638.

Apologies, I am unsure of what you are referring to?

Oops, I should have noticed/clarified that line 638 was from a different file that has the same name but different directory: nexus/db-model/src/switch_port.rs:638

For example, nexus/db-queries/src/db/datastore/switch_port.rs:590 has code that looks like:

md5_auth_key: p.md5_auth_key.clone(),

I was raising the question of, for any line of code that has a style of md5_auth_key: xyz.md5_auth_key (includes this line here for this comment, and above other file line 638), whether to consider if it should actually be md5_auth_key: xyz.md5_auth_key.clone(), because I've seen usage of clone() for lines of code related to md5_auth_key in both the omicron and oxide.rs repositories in the past.
See:

https://github.com/oxidecomputer/oxide.rs/blob/e3e972bdc0d8d49c21b6b12c84b90c4d7a30eafc/cli/src/cmd_net.rs#L499

https://github.com/oxidecomputer/oxide.rs/blob/e3e972bdc0d8d49c21b6b12c84b90c4d7a30eafc/cli/src/cmd_net.rs#L1097

This was part of my quick checking for consistency throughout the code that I looked at.

Ah, ok! So generally clone() is required for any data you're "giving up ownership of" due to passing it to a different scope (like providing it as a parameter to a function), but you need to keep a copy of the value in the current scope for use later on. So if I wanted to pass that value to multiple function calls, I would need to clone it. Here, we don't need to hold on to this data, so it's okay that we give our only "copy" of it to the function that we pass the struct to.

Fortunately, if you ever miss a clone() the code will fail to compile due to violating ownership / borrow checker rules.

nexus/types/src/external_api/params.rs

internet-diglett · 2024-09-12T04:39:30Z

Many thanks for the reviews, going to chip away at these fixes!

* migrate settings apply / clear to configuration set / clear * add endpoint for viewing which configuration is active

internet-diglett · 2024-10-28T15:55:01Z

@taspelund do you mind kicking the tires on this and oxidecomputer/oxide.rs#817?

internet-diglett · 2024-10-31T19:02:40Z

Since this PR is already very big we'll perform fixes for #6462 and #6398 in a follow up PR.

elaine-oxide

See comments. I did a quick review for consistency, viewing only changes from commits since my last review. Did not check tests.

elaine-oxide · 2024-11-01T04:07:51Z

common/src/api/external/mod.rs

    pub remote_asn: Option<u32>,

-    /// Require messages from a peer have a minimum IP time to live field.
+    /// Require messages from this peer have a minimum IP time to live field.


Suggestion: "Require that messages...", to be consistent with line 2675.

elaine-oxide · 2024-11-01T04:20:33Z

nexus/external-api/output/nexus_tags.txt

@@ -129,6 +129,9 @@ snapshot_view                            GET      /v1/snapshots/{snapshot}

 API operations found with tag "system/hardware"
 OPERATION ID                             METHOD   URL PATH
+networking_switch_port_active_configuration_clear DELETE   /v1/system/hardware/racks/{rack_id}/switch/{switch}/switch-port/{port}/configuration


Weird column alignment compared to rest of file, but ok.

elaine-oxide · 2024-11-01T04:26:44Z

nexus/external-api/output/nexus_tags.txt

@@ -129,6 +129,9 @@ snapshot_view                            GET      /v1/snapshots/{snapshot}

 API operations found with tag "system/hardware"
 OPERATION ID                             METHOD   URL PATH
+networking_switch_port_active_configuration_clear DELETE   /v1/system/hardware/racks/{rack_id}/switch/{switch}/switch-port/{port}/configuration
+networking_switch_port_active_configuration_set PUT      /v1/system/hardware/racks/{rack_id}/switch/{switch}/switch-port/{port}/configuration
+networking_switch_port_active_configuration_view GET      /v1/system/hardware/racks/{rack_id}/switch/{switch}/switch-port/{port}/configuration
 networking_switch_port_apply_settings    POST     /v1/system/hardware/switch-port/{port}/settings


I'm looking at the diffs since my last review, and looking at this file as a whole, and looking in particular for consistency for the DELETE method (due to diff for line 195).

Are lines 50-51 similar to lines 135-136, in that a specific element is not specified at the end of the URL?

instance_ephemeral_ip_attach POST /v1/instances/{instance}/external-ips/ephemeral instance_ephemeral_ip_detach DELETE /v1/instances/{instance}/external-ips/ephemeral

There can only be one active configuration per switch port, so specifying which configuration to delete is unnecessary here.

elaine-oxide · 2024-11-01T04:31:15Z

nexus/external-api/src/lib.rs

    ) -> Result<HttpResponseDeleted, HttpError>;

    /// List switch port settings
    #[endpoint {
        method = GET,
-        path = "/v1/system/networking/switch-port-settings",
+        path = "/v1/system/networking/switch-port-configuration",


The ordering in nexus/external-api/output/nexus_tags.txt made this GET hard to find there, as it doesn't come right after the POST and DELETE in that file, although it seems to be sorted in alphabetical order by OPERATION ID there. Ok if no changes.

I believe that file is auto-generated

elaine-oxide · 2024-11-01T04:42:06Z

nexus/external-api/src/lib.rs

+    async fn networking_bgp_config_delete(
+        rqctx: RequestContext<Self::Context>,
+        sel: Path<params::BgpConfigSelector>,
+    ) -> Result<HttpResponseUpdatedNoContent, HttpError>;


Is HttpResponse type ok? Can check other places with this type.

This should be Deleted, will update

elaine-oxide · 2024-11-01T04:53:04Z

nexus/src/external_api/http_entrypoints.rs

+                    &opctx, &port, rack_id, switch, &settings,
+                )
+                .await?;
+            Ok(HttpResponseUpdatedNoContent {})


The syntax looks different here compared to other places (part of my consistency check). Could check other places too, as this appears in multiple places.

elaine-oxide · 2024-11-01T04:55:13Z

nexus/src/external_api/http_entrypoints.rs

@@ -4813,15 +3988,15 @@ async fn networking_switch_port_status(
    async fn networking_bgp_announce_set_update(
        rqctx: RequestContext<ApiContext>,
        config: TypedBody<params::BgpAnnounceSetCreate>,
-    ) -> Result<HttpResponseCreated<BgpAnnounceSet>, HttpError> {
+    ) -> Result<HttpResponseOk<BgpAnnounceSet>, HttpError> {


Is this HttpResponse type ok? In the rest of the code, it is mostly only used for list/view. Double check other places.

elaine-oxide · 2024-11-01T05:00:43Z

nexus/types/src/external_api/params.rs

@@ -2119,6 +2078,19 @@ pub struct SwitchPortApplySettings {
    pub port_settings: NameOrId,
 }

+/// Select a switch port by name


Check if this doc string needs to be updated, as there are other fields that are not just name.

elaine-oxide · 2024-11-01T05:04:11Z

openapi/nexus.json

        "tags": [
          "system/networking"
        ],
-        "summary": "Get information about a named set of switch-port-settings",
-        "operationId": "networking_switch_port_configuration_view",
+        "summary": "Delete switch port settings",


settings or configuration? (Fix in original code with docstring, check other places for consistency with new terminology)

elaine-oxide · 2024-11-01T05:08:40Z

openapi/nexus.json

@@ -19975,7 +20143,7 @@
        ]
      },
      "RouteAddRemove": {
-        "description": "A route to a destination network through a gateway address to add or remove to an interface.",
+        "description": "A network route to to add to or remove from an interface.",


"to to" (Fix in original code with docstring).

Levon Tarver and others added 4 commits July 4, 2024 11:22

WIP: refactor of operator api networking endpoints

cd6c856

WIP: refactor address lot apis

Merge branch 'main' into refactor-networking-apis

c5f042b

fix address lot testing

65b25dc

refactor address lot APIs to resemble ip pools

7bccb1b

ahl reviewed Jul 15, 2024

View reviewed changes

rename AddressLotBlock params

812e991

Merge branch 'main' into refactor-networking-apis

85abe0e

internet-diglett marked this pull request as ready for review July 17, 2024 22:34

internet-diglett added 9 commits July 23, 2024 21:35

WIP: allow partial update of switch port configuration

047c6fe

fixup! WIP: allow partial update of switch port configuration

8a807d9

plumb through switch port geometry

78033b5

fixup! plumb through switch port geometry

d8c2f42

list links

bca07fe

return structured data for geometry

30c217c

plumb through switch port link configuration

7000904

WIP: plumb switch port link create

1c695d0

create / delete link config

ba50716

internet-diglett changed the title ~~Refactor Address Lot APIs~~ Refactor Network Operator APIs Aug 19, 2024

internet-diglett added 3 commits August 27, 2024 16:08

Merge branch 'main' into refactor-networking-apis-2

d5b5553

WIP: interface address management

6eb4be4

WIP: interface address management

69ff1f0

internet-diglett mentioned this pull request Aug 29, 2024

Refactor network operator APIs oxidecomputer/oxide.rs#817

Draft

internet-diglett added 3 commits August 29, 2024 19:30

WIP: more refactor work

2e392d2

Make addresses list-able without knowing interface name

8cad50e

regen openapi

7a32cbd

internet-diglett added 3 commits August 30, 2024 22:39

WIP: bgp import/export/community add and list

1360445

WIP: finish roughing out bgp import/export/community endpoints

a392a80

WIP: breakout bgp peer import/export/communities

a1a70fd

rcgoodfellow added this to the 11 milestone Sep 6, 2024

rcgoodfellow assigned internet-diglett Sep 6, 2024

WIP unsquash switch port query errors

b1cbebe

FelixMcFelix reviewed Sep 11, 2024

View reviewed changes

elaine-oxide reviewed Sep 12, 2024

View reviewed changes

rcgoodfellow modified the milestones: 11, 12 Sep 26, 2024

internet-diglett added 10 commits October 15, 2024 17:15

WIP: update tests

822953e

add integration tests for new endpoints

9ef3456

change const to static in test fixtures

f65f6f9

Round 1 of PR review fixes

2225b77

Round 2 of PR review fixes

155e5d4

Endpoints for managing active switch port configs

36d700a

* migrate settings apply / clear to configuration set / clear * add endpoint for viewing which configuration is active

add tests for new active-configuration endpoints

4140aad

Merge branch 'main' into refactor-networking-apis

4edf90a

regen authz txt

7d8e38f

Merge branch 'main' into refactor-networking-apis

d1e4739

internet-diglett requested review from taspelund and smklein October 28, 2024 15:42

remove unused params

4f04635

elaine-oxide reviewed Nov 1, 2024

View reviewed changes

morlandi7 modified the milestones: 12, 13 Nov 22, 2024

Refactor Network Operator APIs #6016

Are you sure you want to change the base?

Refactor Network Operator APIs #6016

Conversation

internet-diglett commented Jul 8, 2024 • edited Loading

Summary

Changes

Additional Acceptance Criteria

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internet-diglett commented Jul 15, 2024

ahl commented Jul 15, 2024

internet-diglett commented Jul 17, 2024

ahl commented Jul 17, 2024

internet-diglett commented Jul 17, 2024

david-crespo commented Aug 30, 2024

FelixMcFelix 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

FelixMcFelix Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

elaine-oxide left a 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 Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

internet-diglett commented Sep 12, 2024

internet-diglett commented Oct 28, 2024

internet-diglett commented Oct 31, 2024

elaine-oxide left a 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

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 Jul 8, 2024 •

edited

Loading

FelixMcFelix left a comment •

edited

Loading

FelixMcFelix Oct 20, 2024 •

edited

Loading

internet-diglett Oct 22, 2024 •

edited

Loading