pass over comments about api endpoints #773

jessfraz · 2022-03-15T23:22:56Z

part of #772

jessfraz · 2022-03-15T23:23:33Z

nexus/src/external_api/http_entrypoints.rs

+ * Returns a list of all organizations in the system. The organizations are returned
+ * in sorted order, with the most recent organizations appearing first.
+ *
+ * TODO: figure out if the above is correct or what the order actually is.


@davepacheco @ahl I could find it in the code likely but are all endpoints returned in the same order by default and what is that order

jessfraz · 2022-03-15T23:24:06Z

nexus/src/external_api/http_entrypoints.rs

+ * unchanged.
+ * Leaving this comment here just to ping dave or whoever wrote it to let them know.
+ * Then I will delete.
+ * - Jess


@davepacheco @ahl before I deleted the comment just wanted to let you know, you are correct and this is fine behavior lots of APIs do this

So I was going to make a point of cleaning up these comments everywhere, if that's cool

jessfraz · 2022-03-15T23:50:12Z

nexus/src/external_api/http_entrypoints.rs

+ * 201 means its actually been created aka. synchronous
+ * 202 means its being created aka. asynchronous
+ * At least from the APIs I've used.
+ * - Jess


I left another note here as well @davepacheco

jessfraz · 2022-03-16T00:09:09Z

nexus/src/external_api/http_entrypoints.rs

@@ -2196,6 +2365,9 @@ async fn hardware_sleds_get_sled(

 /**
 * Refresh update metadata
+ *
+ * Refreshes the update metadata. If there is a new software update, then the server
+ * will be aware of it and can optionally perform the update.


@iliana likely you know more of the details we can provide here as to the behavior of this endpoint, or should we hide this endpoint from external users.

Might make sense to hide it for now, but long-term I think operators should have a "Check for new updates" button in the console that would trigger a metadata refresh and possibly rebuilding the update plan. (See also #761)

ah okay dope

jessfraz · 2022-03-16T00:09:54Z

nexus/src/external_api/http_entrypoints.rs

+ *
+ * Replaces the firewall rules in a VPC with the specified rules. Any existing
+ * firewall rules are overwritten by the new rules.
+ * TODO: write a bit about the semantics of this operation since it is finiky.


cc @davepacheco mostly looking for a blurb about what happens if the rules are changed before this API call is run and what if the etags differ I guess.

jessfraz · 2022-03-16T00:43:53Z

also adding @rmustacc to make sure I didn't mess up IP behavior etc

rzezeski · 2022-03-16T05:15:22Z

nexus/src/external_api/http_entrypoints.rs

+ * Permanently delete a VPC from a project. This operation cannot be undone. Any subnets
+ * or other resources in the VPC will be deleted as well.


I'm assuming, for the sake of brevity, these comments are meant to briefly describe the common, successful case and not go into all the gory semantic details of a call? E.g. I would hope that if the VPC contains in-use subnets/interfaces that we return a result indicating the operation cannot be performed.

How about as a compromise there is a parameter force such that if ?force=true it cleans up all the child resources. This would help the console as well as prevent someone from fucking something up accidentally. And in the case the person actually wants to delete everything they can change a param and boom versus writing a bunch more code for dependencies which is always super annoying. And the console shows you everything you will delete in deleting a resource and takes the effort off them to do the child calls as well. cc @zephraph to verify console use case

It's not just child resources that would need cleaning. E.g., this VPC might include a subnet with N interfaces. Those N interfaces are used by N running guests. Those interfaces cannot go away while the guests are running. So not only would we have to cleanup all child resources, we have to also find, stop, and delete all other resources using these child resources. I mentions guests here because it's the obvious thing that comes to mind, but perhaps there are other non-child resources that would need to be dealth with as well. So should deleting a VPC cause stopping/deletion of other things that are not under the VPC umbrella, but are instead consumers of these resources?

But that aside, in the case of not passing that flag we still have an error to deal with. My larger question was if we intend to document that stuff here or if these comments are only for the obvious working case, and we'll elaborate all the gritty details in some other place?

Ah for sure can elaborate on the instances in that case I would even expect force to fail.

My larger question was if we intend to document that stuff here or if these comments are only for the obvious working case, and we'll elaborate all the gritty details in some other place?

For consumers of the API, these comments are being populated by the docs, so it would be really nice to get all details here. since it will show up here: https://docs.oxide.computer/api/delete-a-vpc-from-a-project as they are implementing stuff. Also the way I wrote all the clients these comments propagate to the clients so if they use godoc or rust docs its right there in the code as well. Best to be as verbose as possible. We will obviously have other tutorials but being as verbose as possible here would be really nice!

Maybe we dont even need force in this case, basically I'm trying to account for a VPC with things in it that arent being used and someone just wants to clean up. Like for instance for terraform etc, would be nice to do one call versus n+1

That being said, these docs are just a first pass it doesn't need to be ship-worthy but also when will we have time to do ship worthy things haha another question

In the case of a failure to delete by in-use resources, we really just want to make sure we can communicate what needs to be cleaned up or removed from use before the desired user action can happen.

rzezeski · 2022-03-16T05:35:49Z

nexus/src/external_api/http_entrypoints.rs

- * Create a VPC Router
+ * Create a router
+ *
+ * Creates a new router in a VPC.
 */


This should specify VPC Custom Router. There are two types of VPC routers: System and Custom. The System Router is implicitly created as part of the VPC and can neither be created nor destroyed explicitly. As new VPC Subnets are added, routes are added to the System Router implicitly. However, in order to refine routing policy a Custom Router can be created and associated with a VPC Subnet. Only one Custom Router per VPC Subnet. This is discussed in RFD 63 §2.3.

So I believe this API comment should specify VPC Custom Router to make it clear what it applies to. The same goes for the other router APIs.

will elaborate

Only one Custom Router per VPC Subnet. This is discussed in RFD 63 §2.3.

one question if there is only one vpc router per subnet should this route be instead under /subnets/{subnet_name} for the endpoint path? to declare that subnet as the parent or it is one per VPC.

It's a good question. The way RFD 21 is worded I get the impression that the custom routers live under the VPC itself, and the VPC subnet is an argument/parameter passed as part of creation.

rzezeski · 2022-03-16T06:07:31Z

nexus/src/external_api/http_entrypoints.rs

+ * Delete a route
+ *
+ * Permanently deletes the specified route from the specified router.
+ * TODO: We might want some more detail here on the behavior of this endpoint.


Yea, related to my other comment we can only add/delete routes from VPC Custom Routers.

will elaborate

nexus/src/external_api/http_entrypoints.rs

Signed-off-by: Jess Frazelle <[email protected]>

davepacheco

@jessfraz Thanks for doing this! It's a big improvement in consistency and conciseness, especially the summaries.

I can't reply to any of the threads you started with questions for me because GitHub says they're "outdated" now. But I think I addressed those in other comments.

davepacheco · 2022-03-18T20:47:51Z

rustfmt.toml

@@ -4,3 +4,8 @@
 max_width = 80
 use_small_heuristics = "max"
 edition = "2021"
+
+
+# Temp unstable features


You don't want to check this in.

davepacheco · 2022-03-18T20:49:06Z

nexus/src/external_api/http_entrypoints.rs

-/// List all organizations.
+/// List organizations
+///
+/// Returns a list of all organizations in the system. The organizations are returned


The default sort order for most things is ascending order by name.

davepacheco · 2022-03-18T20:51:45Z

nexus/src/external_api/http_entrypoints.rs

-/// Delete a specific organization.
+/// Delete an organization
+///
+/// Permanently deletes an organization. It cannot be undone. This will not remove any


Under RFD 234, users and groups are under the Silo anyway.

Note that for most things today, the implementation will not let you remove a container that has children (e.g., an Organization with Projects in it or a Project with Instances in it). This is changeable (to have it recursively remove everything) but it's a little tricky and I'd love to defer that for MVP.

I think we want that behavior for orgs, really the only one I want the behavior of nuke the world is for projects. Since test projects are going to be very common. Almost in every other scenario you don't want to nuke the world

Happy to put that on the list of "MVP" things ... but either way, it's not what the code does today and I think it's better to have the docs reflect reality and update them when we change things.

(In terms of doing this: to do an async removal correctly, I think we'd need to (1) mark the project deleted so that you can't create new things in it, (2) go crawl the thing and remove everything in it, which can take arbitrarily long and get interrupted, so presumably this has to happen in a saga, (3) go remove the Project itself. It's an open question (to me) whether we'd let you access things in the Project while that's happening. And what if we run into something we can't delete, like a running Instance? These are all the reasons I'd love to punt if we can.)

oh for sure ill make this match reality sorry I've just been doing other stuff will update this

yeah mostly what I was saying is in testing we likely will script this anyways so might be worth doing for reals, but also not to bad to script through the API , I did similar in the cli repo (minus sagas of course but just recursively calling API endpoints to delete) for the cleanup of tests)

In azure, one of the things i really liked was that deleting the higher level folder (ie projects) would delete everything even running instances from rfd 4 https://github.com/oxidecomputer/rfd/tree/master/rfd/0004/#22-projects

davepacheco · 2022-03-18T20:53:44Z

nexus/src/external_api/http_entrypoints.rs

 // TODO-correctness: Is it valid for PUT to accept application/json that's a
 // subset of what the resource actually represents?  If not, is that a problem?
 // (HTTP may require that this be idempotent.)  If so, can we get around that
 // having this be a slightly different content-type (e.g.,
 // "application/json-patch")?  We should see what other APIs do.
+// Other APIS work like the docs above, so it's fine. As any any parameters not passed are left


This is covered under #333 and described in detail in the second paragraph of this section of RFD 4. The short summary is that I think we probably do want to use a different content-type here but it will work the way people expect.

davepacheco · 2022-03-18T20:54:56Z

nexus/src/external_api/http_entrypoints.rs

-/// List all projects.
+/// List projects
+///
+/// Returns a list of all projects in the organization. This includes only the subset


This includes only the subset of projects that the caller has access to.

That's not true today, and we'd talked about not doing this for the MVP.

The projects are sorted by creation date, with the most recently created projects appearing first.

Today, these are sorted in ascending order by name.

I guess I'm curious of the complexity here, would it not be like a join of the database tables being the scenes?

I assume you're talking about filtering the Projects by access? It is just a complicated database query, but it's potentially very complicated. You have access to a Project if any of these is true:

you have any of the roles that grant you read-only access to everything in the system

the Project is in an Organization on which you have been assigned any of roles that grant you read access

you have been assigned any of the roles that grant you read access directly to the Project

and that's just Projects. For Instances, there's another level of hierarchy you can be assigned to. And that's just for predefined roles. If/when we support custom roles/policies, we will see if you've been assigned any role on any resource that's linked to the target resource in such a way that you get the permission on the target resource by virtue of having the role on the other resource.

The good news is that Oso supports this use case in other languages. They call it data filtering. So for example we define a policy file that says things like "anybody with 'collaborator' role on an Organization can 'read' all Instances in all Projects in the Organization". Normally we ask Oso "do $these_roles grant $permission on this $resource?" and it answers "yes" or "no". But the Oso engine also supports saying "build me a logical expression that describes all $resources on which these $roles have permission $permission", and then you can turn that into SQL and query the database with it. So one idea is to use that for this use case. But (1) they haven't implemented this for Rust, and (2) it's not yet clear to me how we can ensure that the resulting queries will always be indexed, which is critical for scale. Both of these are definitely solvable -- just not trivial.

As a side point: the more I dug into other systems, the more I found they don't seem to do this. For example, if I give you access to a Google Drive directory, I think I'm implicitly granting you access to view everything in it. You don't just see a partial view of the documents that you have access to. Similarly, GitHub doesn't seem to have hidden Organizations -- everybody always sees all Organizations. (They do have private Repos, of course.) I believe @ahl told me that in AWS, usually you get access to a "List" API and then you can see everything -- it doesn't filter based on what you can subsequently do something with. I may have misunderstood that one, though.

This is not at all to say it's not a good idea -- I think it is -- just that maybe it's not critical for minimum-viable.

makes sense as long as i can get the list!

davepacheco · 2022-03-18T20:55:31Z

nexus/src/external_api/http_entrypoints.rs

+/// Delete a project
+///
+/// Permanently deletes the project with the specified name in the specified organization.
+/// It cannot be undone. This also deletes all the project's resources.


This also deletes all the project's resources.

Mentioned above: that's not the way the delete operations work today.

yeah I guess the only problem w delaying it is that like in terraform, in the cole etc, I will be implementing all this logic for us versus just differing it to one place where it is implemented. Granted I'd have to do it terraform regardless but the console has a bunch of plans for dependencies of things.

Also I feel like as we get more and more testing going and people are creating lots of tests resources the need for this becomes more and more acute. We might just do it out of our own pain ;)

Don't Terraform providers already have a notion of what resources depend on which other resources and remove them in reverse-dependency order?

Anyway, I wasn't saying we shouldn't do this. I mentioned above what's involved in doing it. I'm just saying that's not the way it works today, and I think the docs should reflect the current state.

Pretty sure i have to do it manually unless they've updated the SDK but its not much to do

im happy to create the pr i might just need some help to make sure I don't mess up the sagas

i edited the comment tho to reflect reality today

davepacheco · 2022-03-18T20:57:19Z

nexus/src/external_api/http_entrypoints.rs

 // TODO-correctness This is supposed to be async.  Is that right?  We can create
 // the instance immediately -- it's just not booted yet.  Maybe the boot
 // operation is what's a separate operation_id.  What about the response code
 // (201 Created vs 202 Accepted)?  Is that orthogonal?  Things can return a
 // useful response, including an operation id, with either response code.  Maybe
 // a "reboot" operation would return a 202 Accepted because there's no actual
 // resource created?
+//
+//
+// 201 means its actually been created aka. synchronous


That's my experience as well. The comment is about whether it's "201 Created" with an Instance in the "Starting" state vs. "202 Accepted" with no Instance having been created yet. I think we actually want the first one of these here.

davepacheco · 2022-03-18T20:57:53Z

nexus/src/external_api/http_entrypoints.rs

-/// Delete an instance from a project.
+/// Delete an instance
+///
+/// Permanently deletes the instance with the specified name in the specified project and


This also requires that the Instance be stopped.

nexus/src/external_api/http_entrypoints.rs

davepacheco · 2022-03-18T21:01:30Z

nexus/src/external_api/http_entrypoints.rs

-/// Fetch a specific built-in system user
+/// Get a user
+///
+/// Returns details of the specified user. If you pass `me` as the `user_name`,


Is that true?

What if another user's login is me?

I think we can reserve it, a bunch of apis I've used have this and it's very nice, or at least some way of introspecting yourself

The string itself could be like _self _ me whatever

I mean im fine with /session/mew as long as it has more info about the user, but its nice if its somehow under the same path as other user controls

I don't see how we can reserve it because we don't control it -- it comes from the IdP. If someone signs up with their IdP with username "me", then I think people just won't be able to interact with their user in our product if we assume "me" is special.

But I think this comment might not be right about the current implementation of this endpoint.

thats fair, im fine w session/me then

davepacheco · 2022-03-18T21:09:19Z

Oh, another general note: I always refer to API resources in title case, like Organizations, Projects, Instances, Disks, etc. because so many of these are common English words and could easily be confused. I think this might be a good style rule for our own docs. What do folks think?

bnaecker · 2022-03-18T22:27:37Z

Oh, another general note: I always refer to API resources in title case, like Organizations, Projects, Instances, Disks, etc. because so many of these are common English words and could easily be confused. I think this might be a good style rule for our own docs. What do folks think?

Yes please! I also have been referring to "VPC Subnets" to distinguish the API resource from an IP subnetwork. This is important today, as all VPC Subnets have both an IPv4 and IPv6 subnetwork assigned to them.

jessfraz · 2022-03-19T02:28:23Z

Makes sense, will update!

Co-authored-by: David Pacheco <[email protected]>

Signed-off-by: Jess Frazelle <[email protected]>

jessfraz · 2022-03-24T16:51:37Z

okay i think i have fixed all the term casing and comment to reflect reality

david-crespo · 2023-03-15T19:03:05Z

Closing because these are pretty out of date by now and will need to be redone, but most endpoints have no summaries and could probably use one.

jessfraz requested review from davepacheco and ahl March 15, 2022 23:22

jessfraz commented Mar 15, 2022

View reviewed changes

jessfraz commented Mar 16, 2022

View reviewed changes

jessfraz force-pushed the pass-over-code-comments branch from ed62684 to 6d82d5d Compare March 16, 2022 00:10

jessfraz marked this pull request as ready for review March 16, 2022 00:20

jessfraz mentioned this pull request Mar 16, 2022

[nexus] Add stubs for image APIs #749

Merged

rzezeski reviewed Mar 16, 2022

View reviewed changes

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

jessfraz force-pushed the pass-over-code-comments branch from c233195 to 06bd2d4 Compare March 16, 2022 16:09

jessfraz added 2 commits March 16, 2022 09:33

initial pass

5fd29d5

update spec

9080ebf

Signed-off-by: Jess Frazelle <[email protected]>

jessfraz force-pushed the pass-over-code-comments branch from 61179a2 to 9080ebf Compare March 16, 2022 16:33

davepacheco reviewed Mar 18, 2022

View reviewed changes

jessfraz and others added 5 commits March 18, 2022 19:46

Update nexus/src/external_api/http_entrypoints.rs

ba9d949

Co-authored-by: David Pacheco <[email protected]>

make requested changes

efd548d

Signed-off-by: Jess Frazelle <[email protected]>

update json

9af812a

Signed-off-by: Jess Frazelle <[email protected]>

fix casing

7c18743

Signed-off-by: Jess Frazelle <[email protected]>

update spec

ee3fe5e

Signed-off-by: Jess Frazelle <[email protected]>

ahl self-assigned this Aug 17, 2022

david-crespo closed this Mar 15, 2023

david-crespo deleted the pass-over-code-comments branch March 15, 2023 19:03

		* Permanently delete a VPC from a project. This operation cannot be undone. Any subnets
		* or other resources in the VPC will be deleted as well.

pass over comments about api endpoints #773

pass over comments about api endpoints #773

Conversation

jessfraz commented Mar 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessfraz commented Mar 16, 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

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

jessfraz Mar 16, 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

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

jessfraz Mar 19, 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

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

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 commented Mar 18, 2022

bnaecker commented Mar 18, 2022

jessfraz commented Mar 19, 2022

jessfraz commented Mar 24, 2022

david-crespo commented Mar 15, 2023

jessfraz commented Mar 15, 2022 •

edited

Loading

jessfraz Mar 16, 2022 •

edited

Loading

jessfraz Mar 19, 2022 •

edited

Loading