Add routes to lookup resources by id #1266

zephraph · 2022-06-24T04:42:14Z

This will address #485

The intent is to provide a mechanism for fetching resources by id. I'd failed at previous attempts at this but #798 has made it much more feasible. In previous conversations we'd discussed using /by_id/<resource>/<id> as the general shape of the route which is represented here. Big shout out to @ahl for #1329 which helped to clean up the organization.

Note that while we've discussed adding PUT and DELETE endpoints as well, I'm not tackling that in this PR.

Requested feedback

~~/by_id/ or /by-id/~~? I've decided to use the dash. We have inconsistent usage of dashes and underscores but there's more of the former than the latter. I've got a openapi-lint issue open to help us enforce consistency here.
Should the resource names in the route stay plural or be changed to be singular?
I've spread the id implementations to be co-located near the associated GET method. Does that feel right or should I move back to having all the /by-id/ routes grouped together?

davepacheco · 2022-06-24T04:43:32Z

Nice! In terms of prioritizing review, what's the level of urgency on reviewing this? Is this blocking work for Tuesday?

nexus/src/db/lookup.rs

zephraph · 2022-06-24T04:47:26Z

I consider this low priority and isn't blocking anything that's critical to have on Tuesday.

nexus/src/external_api/http_entrypoints.rs

karencfv · 2022-06-27T23:53:16Z

This is real nice!!! Just popping in to say that looking up an item by ID is crucial imo. For example, when implementing the resource for VPCs in the Terraform provider I was already having to disable name updates https://github.com/oxidecomputer/terraform-provider-oxide-demo/blob/main/oxide/resource_vpc.go#L147-L154

zephraph · 2022-06-28T00:26:01Z

From @ahl in chat:

I was thinking about how this might translate to the CLI. I imagine we'd want to do something like oxide disk get --uuid or something as an alternative to specifying the org/project/disk
@zephraph if we wanted to support --uuid for other operations (modify, delete) would the cli just do one call to get the disk by name and then another call to modify or delete it?
it seems like this has the same raciness that @davepacheco alluded to, but perhaps it's no big deal if the initial lookup by uuid succeeds and the subsequent operation fails (i.e. those operations always race with delete anyways...)

I agree that it'd be nice to be able to supply something like --id to the regular commands and have that work in place of project/org. It seems like adding PUT and DELETE support for these endpoints would likely make sense. Aside from API bloat, is there any reason not to add them?

karencfv · 2022-06-28T01:00:52Z

I am 100% on board with adding PUT and DELETE. I'd even go as far as saying they are crucial as well. Everything should be identifiable by an immutable identifier imo

davepacheco · 2022-06-28T15:37:11Z

It seems like adding PUT and DELETE support for these endpoints would likely make sense. Aside from API bloat, is there any reason not to add them?

Yeah it makes sense to me to have the by-id endpoints function exactly like the corresponding ones in the by-name hierarchy (so yes, support PUT/DELETE, and eventually maybe have the same child resources too?), since it's really just another identifier path to the same HTTP resource.

ahl · 2022-06-28T23:17:07Z

A given entity can be named either by its UUID or by some Name tuple (agreed?). This may be a radical change, but another approach would be to dispense with the paths for the name tuple and instead prefer query/body params. In other words, for a single endpoint we could accept either the UUId or the appropriate collection of names. A GET for a disk, e.g., could take either a UUID or a org/instance/disk Name. This is as opposed to the alternative where every CRUD operation would effective have two versions: one by UUID and one by Name-tuple.

I don't claim to be an HTTP API aficionado so it may be that what I'm suggesting would be ugly or horrible.

From the perspective of the CLI, with either model of the API I imagine we'd give users the option of specifying, say, a disk either way. Approximately:

oxide disk --id <id> <cmd>
oxide disk --organization <org> --project <project> --disk <disk> <cmd>

where <cmd> may be: view, update, delete

ahl

This looks great. Two non-blocking thoughts:

We might want to name these <resource>_get_by_id as it looks like RFD 285 is going to change things to <resource>_<action> for the non-by-id versions
I don't think we need to do it in this PR, but would we want to do by_id versions for the other actions? Is it weird to double the size of the API?

david-crespo · 2022-06-30T02:38:57Z

Awesome. I'm with Adam about putting the resource name first.

zephraph · 2022-07-01T14:54:05Z

I'm going to gate this behind #1329. Having that in will make it easier to organize this work I believe.

zephraph · 2022-07-07T19:40:10Z

nexus/tests/integration_tests/unauthorized.rs

+    /// populates `id` params for later route lookups
+    id_routes: Vec<&'static str>,


This could've been more generic to allow arbitrarily mapping fields to params but it makes the types a lot harder. Given it's hard for me to imagine a usecase outside of needing to do id lookups, I just focused it on solving that problem.

This seems like a great simplification.

nexus/tests/integration_tests/unauthorized.rs

zephraph · 2022-07-07T21:11:22Z

The authorization tests are still failing. I think it's likely happening on the first /by-id/ call. It's a panic, so I'm still trying to work through that.

thread 'integration_tests::unauthorized::test_unauthorized' panicked at 'called `Result::unwrap()` on an `Err` value: expected status code 403 Forbidden, found 404 Not Found

zephraph · 2022-07-08T22:39:54Z

Okay, I finally got things worked out. I was having a few problems...

In VerifyEndpoint I had the by-id routes listed as Visibility::Public (meaning they yielded a 403 instead of a 404). Switching them to Visibility::Private fixed the above error.
The creation of project images and snapshots is unimplemented so I changed the by-id routes of both to also return unimplemented
NICs were tricky b/c technically the NIC is already created. I could've done extra work to create a new NIC, but it didn't feel worth the effort. Instead I modified the SetupReq to do either GET or POST and be able to associate that to any id_routes. It's likely this will be handy in the future for any resources that aren't directly created.

zephraph · 2022-07-08T22:56:09Z

nexus/tests/integration_tests/endpoints.rs

+            url: "/by-id/images/{id}",
+            visibility: Visibility::Protected,
+            allowed_methods: vec![
+                AllowedMethod::GetUnimplemented,


This lookup isn't implemented due to the underlying create/fetch not being implemented. Project images are still stubs at this point.

zephraph · 2022-07-08T22:56:22Z

nexus/tests/integration_tests/endpoints.rs

+            url: "/by-id/snapshots/{id}",
+            visibility: Visibility::Protected,
+            allowed_methods: vec![
+                AllowedMethod::GetUnimplemented,


Same as above

zephraph · 2022-07-08T22:57:00Z

nexus/tests/integration_tests/endpoints.rs

@@ -579,6 +579,15 @@ lazy_static! {
                )
            ],
        },
+
+        VerifyEndpoint {
+            url: "/by-id/organizations/{id}",


Note the {id} is important here. That's the only way the id will get substituted.

zephraph · 2022-07-08T22:58:55Z

nexus/tests/integration_tests/unauthorized.rs

+        // Lookup the previously created NIC
+        SetupReq::Get {
+            url: &*DEMO_INSTANCE_NIC_URL,
+            id_routes: vec!["/by-id/network-interfaces/{id}"],
        },


This is the (first) usecase of somewhere we may lookup a resource instead of creating a new resource mainly for the purpose of populating the id param.

zephraph · 2022-07-08T23:00:25Z

nexus/tests/integration_tests/unauthorized.rs

+            None => endpoint
+                .url
+                .replace("{id}", "00000000-0000-0000-0000-000000000000"),


In this case there was no entry in setup_requests for some given endpoint that included {id}. This is true in the case of unimplemented endpoints like images and snapshots right now.

karencfv

Thanks for taking this on. Really looking forward to having these endpoints available!

karencfv · 2022-07-11T21:30:27Z

nexus/tests/output/nexus_tags.txt

@@ -4,6 +4,7 @@ disk_create                              /organizations/{organization_name}/proj
 disk_delete                              /organizations/{organization_name}/projects/{project_name}/disks/{disk_name}
 disk_list                                /organizations/{organization_name}/projects/{project_name}/disks
 disk_view                                /organizations/{organization_name}/projects/{project_name}/disks/{disk_name}
+disk_view_by_id                          /by-id/disks/{id}


What do you think about flipping the paths around like this:

Suggested change

disk_view_by_id /by-id/disks/{id}

disk_view_by_id /disks/by-id/{id}

It follows path convention a little closer and it would make command generation a little easier.

Would the idea be that you'd have /disks, /projects, /instances, and all the others at the top level, and each of these would contain only one HTTP resource beneath them which was by_id?

You'd have to, which I think is why Justin didn't do it that way

I'm not sure if we are planning to have more global actions on resources? For example, we might want to have batch actions in the future where several resources are acted on.

In that case having the by-id bit of the path wouldn't make too much sense any more, the way I'm proposing 🤔

Also, we'll probably want to have an endpoint for attaching disks with an ID as well. That would make it instances/{id}/disks/attach/{id}

I'm OK with either way we go :) If the majority prefer having by-id at the top level I'm ok with that :)

I guess, what I'm saying is that we might have other global actions for endpoints where being under by-id/ might not make sense, and if this scheme fits with that

I don't think flipping it gains us much. With by_id at the front we have fewer top level routes overall which keeps things a bit cleaner. It'll be slightly easier for us to filter down on id only endpoints.

My mental model of this is that it's actually just all one endpoint with different parameters:

/by-id/{resource}/{id}

From an API design perspective I believe the fewer top level endpoints we have, the better off we are. This is largely related to #1383 where we actually have different contexts for top level apis (e.g. if they run inside the context of a silo or not) and introducing more top level apis would make that even harder to document.

As for batching, I think the current endpoint could still support that. Something like /by-id/{resource}?ids[]=1,2,3,4 is a rough approximation of how it could work.

Ha! I had not seen #1383. Sounds good to me then :)

davepacheco

Nice! This looks like the sort of thing that looks simple and clean now but probably involved a lot of grind to get here. Thanks for doing it!

In regards to your specific feedback:

Should the resource names in the route stay plural or be changed to be singular?

I'd go with plural, and that matches the existing stuff.

I've spread the id implementations to be co-located near the associated GET method. Does that feel right or should I move back to having all the /by-id/ routes grouped together?

What you've got feels right to me.

nexus/src/external_api/http_entrypoints.rs

davepacheco · 2022-07-11T23:22:28Z

nexus/tests/integration_tests/unauthorized.rs

-    /// body of the `POST` request
-    body: serde_json::Value,
+/// The setup phase takes a list of `SetupReq` enums and issues a `GET` or `POST`
+/// request to each one's `url`. The `id_routes` field is a list of URLs that result


typo here? ("that result the result")

Cool, yeah. I improved it.

davepacheco · 2022-07-11T23:26:27Z

nexus/tests/integration_tests/unauthorized.rs

+    /// populates `id` params for later route lookups
+    id_routes: Vec<&'static str>,


This seems like a great simplification.

ahl

I realized that I neglected to click submit earlier. Belated, but LGTM!! Nice work

first pass at implementing /by_id/ routes

81369d7

zephraph requested a review from davepacheco June 24, 2022 04:42

zephraph changed the title ~~Add /by_id/ routes for common~~ Add /by_id/ routes for common resources Jun 24, 2022

zephraph commented Jun 24, 2022

View reviewed changes

nexus/src/db/lookup.rs Outdated Show resolved Hide resolved

zephraph commented Jun 24, 2022

View reviewed changes

nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved

zephraph commented Jun 24, 2022

View reviewed changes

nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved

zephraph added 5 commits June 24, 2022 01:03

Restore text that failed to get written when I ran out of space

9d78737

Add docstrings

3712cd9

Fixup formatting

6b99acf

Fix compiler errors

2a1b9e2

Update config

eaac639

zephraph requested a review from ahl June 24, 2022 17:20

This was referenced Jun 28, 2022

Include display names in resource policy response #1207

Open

Add VPC, Subnet columns to network interface table oxidecomputer/console#983

Closed

ahl reviewed Jun 29, 2022

View reviewed changes

Merge branch 'main' into fetch-by-id

81bd09d

zephraph added 4 commits July 1, 2022 10:58

Reverse by_id/resource name order

6632e19

Merge branch 'main' into fetch-by-id

a175c1b

reorganize imports

e42bfaf

Reorganize id lookup implementation code

b59d183

zephraph changed the title ~~Add /by_id/ routes for common resources~~ Add routes to lookup resources by id Jul 6, 2022

zephraph added 3 commits July 7, 2022 14:25

Add mechanism to valid auth endpoints with ids

77d3a50

Merge branch 'main' into fetch-by-id

ff96503

Add coverage for by-id unauthed tests

cb95f77

zephraph marked this pull request as ready for review July 7, 2022 19:34

zephraph requested a review from jmpesp July 7, 2022 19:34

Fix comment

07112fd

zephraph commented Jul 7, 2022

View reviewed changes

david-crespo reviewed Jul 7, 2022

View reviewed changes

nexus/tests/integration_tests/unauthorized.rs Outdated Show resolved Hide resolved

Fix unauth tests

4284067

Merge branch 'main' into fetch-by-id

b35e82d

zephraph commented Jul 8, 2022

View reviewed changes

karencfv reviewed Jul 11, 2022

View reviewed changes

karencfv approved these changes Jul 11, 2022

View reviewed changes

david-crespo approved these changes Jul 11, 2022

View reviewed changes

davepacheco reviewed Jul 11, 2022

View reviewed changes

zephraph added 2 commits July 11, 2022 19:39

Restore test that was added by @ahl in #751

39c658b

Improve the setup req comment

67e1c8d

davepacheco approved these changes Jul 12, 2022

View reviewed changes

zephraph enabled auto-merge (squash) July 12, 2022 00:30

zephraph merged commit 4640dd3 into main Jul 12, 2022

zephraph deleted the fetch-by-id branch July 12, 2022 00:53

ahl reviewed Jul 12, 2022

View reviewed changes

zephraph mentioned this pull request Jul 15, 2022

Add VPC, Subnet to NIC table oxidecomputer/console#1047

Merged

zephraph mentioned this pull request Jul 22, 2022

Enable retrieving and modifying a resource by id #485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add routes to lookup resources by id #1266

Add routes to lookup resources by id #1266

zephraph commented Jun 24, 2022 •

edited

Loading

davepacheco commented Jun 24, 2022

zephraph commented Jun 24, 2022

karencfv commented Jun 27, 2022

zephraph commented Jun 28, 2022

karencfv commented Jun 28, 2022

davepacheco commented Jun 28, 2022

ahl commented Jun 28, 2022 •

edited

Loading

ahl left a comment

david-crespo commented Jun 30, 2022

zephraph commented Jul 1, 2022

zephraph Jul 7, 2022

davepacheco Jul 11, 2022

zephraph commented Jul 7, 2022 •

edited

Loading

zephraph commented Jul 8, 2022

zephraph Jul 8, 2022

zephraph Jul 8, 2022

zephraph Jul 8, 2022

zephraph Jul 8, 2022

zephraph Jul 8, 2022

karencfv left a comment

karencfv Jul 11, 2022 •

edited

Loading

davepacheco Jul 11, 2022

david-crespo Jul 11, 2022

karencfv Jul 11, 2022 •

edited

Loading

karencfv Jul 11, 2022

zephraph Jul 11, 2022

zephraph Jul 11, 2022

karencfv Jul 11, 2022

davepacheco left a comment

davepacheco Jul 11, 2022

zephraph Jul 11, 2022

davepacheco Jul 11, 2022

ahl left a comment

		/// populates `id` params for later route lookups
		id_routes: Vec<&'static str>,

	disk_view_by_id /by-id/disks/{id}
	disk_view_by_id /disks/by-id/{id}

Add routes to lookup resources by id #1266

Add routes to lookup resources by id #1266

Conversation

zephraph commented Jun 24, 2022 • edited Loading

Requested feedback

davepacheco commented Jun 24, 2022

zephraph commented Jun 24, 2022

karencfv commented Jun 27, 2022

zephraph commented Jun 28, 2022

karencfv commented Jun 28, 2022

davepacheco commented Jun 28, 2022

ahl commented Jun 28, 2022 • edited Loading

ahl left a comment

Choose a reason for hiding this comment

david-crespo commented Jun 30, 2022

zephraph commented Jul 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zephraph commented Jul 7, 2022 • edited Loading

zephraph commented Jul 8, 2022

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

karencfv left a comment

Choose a reason for hiding this comment

karencfv Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karencfv Jul 11, 2022 • 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

davepacheco 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

ahl left a comment

Choose a reason for hiding this comment

zephraph commented Jun 24, 2022 •

edited

Loading

ahl commented Jun 28, 2022 •

edited

Loading

zephraph commented Jul 7, 2022 •

edited

Loading

karencfv Jul 11, 2022 •

edited

Loading

karencfv Jul 11, 2022 •

edited

Loading