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"
  }
]

internet-diglett · 2024-08-30T20:56:51Z

I think we will need to un-nest the communities and import / export policies if we want to be able to add peers without using --json-body.

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.

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!

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

WIP: 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 6 commits August 30, 2024 00:51

fix address remove logic

2377850

WIP: route add / remove

8416cac

WIP: route add / remove

d991823

WIP: route add / remove / list

de92b30

add bgp peer list / add / remove

22fe92a

don't require entire peer config for peer removal

193bcaf

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 5 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Network Operator APIs #6016

Refactor Network Operator APIs #6016

internet-diglett commented Jul 8, 2024 •

edited

Loading

ahl left a comment

ahl Jul 15, 2024

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

internet-diglett commented Aug 30, 2024

david-crespo commented Aug 30, 2024

FelixMcFelix left a comment •

edited

Loading

FelixMcFelix Sep 11, 2024

FelixMcFelix Sep 11, 2024

internet-diglett Oct 18, 2024

FelixMcFelix Oct 20, 2024 •

edited

Loading

elaine-oxide left a comment

elaine-oxide Sep 12, 2024

internet-diglett Oct 18, 2024

elaine-oxide Oct 18, 2024

internet-diglett commented Sep 12, 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

ahl Jul 15, 2024

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

internet-diglett commented Aug 30, 2024

david-crespo commented Aug 30, 2024

FelixMcFelix left a comment • edited Loading

Choose a reason for hiding this comment

FelixMcFelix Sep 11, 2024

Choose a reason for hiding this comment

FelixMcFelix Sep 11, 2024

Choose a reason for hiding this comment

internet-diglett Oct 18, 2024

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

elaine-oxide Sep 12, 2024

Choose a reason for hiding this comment

internet-diglett Oct 18, 2024

Choose a reason for hiding this comment

elaine-oxide Oct 18, 2024

Choose a reason for hiding this comment

internet-diglett commented Sep 12, 2024

internet-diglett commented Jul 8, 2024 •

edited

Loading

FelixMcFelix left a comment •

edited

Loading

FelixMcFelix Oct 20, 2024 •

edited

Loading