RFD-322: Add `/v1/` endpoints for organizations and projects #2050

just-be-dev · 2022-12-13T15:09:07Z

Changes

v1 endpoints added for orgs
v1 endpoints added for projects
Selectors updated to be combinatorial such that they don't need to be fully reconstructed every time
Path and query param definitions for new endpoints centralized to params module
Added Name and Uuid to NameOrId converters to reduce some of the translation noise

just-be-dev · 2022-12-14T21:25:33Z

nexus/types/src/external_api/params.rs

-pub struct ProjectPath {
+pub struct OldProjectPath {


An unfortunate naming conflict, but this'll go away completely once we do #2055

just-be-dev · 2022-12-14T21:30:03Z

nexus/tests/output/uncovered-authz-endpoints.txt

+organization_delete                      (delete "/organizations/{organization_name}")
+project_delete                           (delete "/organizations/{organization_name}/projects/{project_name}")
+instance_delete                          (delete "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}")
+instance_view_by_id                      (get    "/by-id/instances/{id}")
+organization_view_by_id                  (get    "/by-id/organizations/{id}")
+project_view_by_id                       (get    "/by-id/projects/{id}")
 login_saml_begin                         (get    "/login/{silo_name}/saml/{provider_name}")
+organization_list                        (get    "/organizations")
+organization_view                        (get    "/organizations/{organization_name}")
+organization_policy_view                 (get    "/organizations/{organization_name}/policy")
+project_list                             (get    "/organizations/{organization_name}/projects")
+project_view                             (get    "/organizations/{organization_name}/projects/{project_name}")
+instance_list                            (get    "/organizations/{organization_name}/projects/{project_name}/instances")
+instance_view                            (get    "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}")
+instance_serial_console                  (get    "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/serial-console")
+instance_serial_console_stream           (get    "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/serial-console/stream")
+project_policy_view                      (get    "/organizations/{organization_name}/projects/{project_name}/policy")


There's work I could do here to get this test passing for all the old endpoints but I'm opting not to do that in favor of making process. The authz test is non-trivial and I'm trying to update it as little as possible until I can get a critical mass of APIs converted. I am going through and ensuring the new API endpoints are correctly tested though.

On that note, there is a slight issue in that you can only test and endpoint from the spec once. If you try to verify it more than once it freaks out on you. I don't have the exact error handy but it does this because it "consumes" routes from the spec and if it encounters something its already consumed then that's considered a failure. We actually need to be able to test a route at least twice (name path, id). Anyway, I'll add that to the top level issue to resolve.

just-be-dev · 2022-12-14T21:41:27Z

common/src/api/external/mod.rs

+impl From<Name> for NameOrId {
+    fn from(name: Name) -> Self {
+        NameOrId::Name(name)
+    }
+}
+
+impl From<Uuid> for NameOrId {
+    fn from(id: Uuid) -> Self {
+        NameOrId::Id(id)
+    }
+}


These greatly reduce a lot of conversion noise. We might not need them post v1 migration.

just-be-dev · 2022-12-14T21:41:49Z

nexus/db-model/src/name.rs

+impl From<Name> for external::NameOrId {
+    fn from(name: Name) -> Self {
+        Self::Name(name.0)
+    }
+}


Same as above

just-be-dev · 2022-12-14T21:42:53Z

nexus/src/app/organization.rs

-    pub async fn organization_fetch(
-        &self,
-        opctx: &OpContext,
-        organization_name: &Name,
-    ) -> LookupResult<db::model::Organization> {
-        let (.., db_organization) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .fetch()
-            .await?;
-        Ok(db_organization)
-    }
-
-    pub async fn organization_fetch_by_id(
-        &self,
-        opctx: &OpContext,
-        organization_id: &Uuid,
-    ) -> LookupResult<db::model::Organization> {
-        let (.., db_organization) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_id(*organization_id)
-            .fetch()
-            .await?;
-        Ok(db_organization)
-    }


@davepacheco you mentioned in #1957 that we could probably just ditch the fetch endpoints now that we're passing around lookups. I've been doing that.

just-be-dev · 2022-12-15T00:08:24Z

nexus/src/app/instance.rs

        opctx: &'a OpContext,
        instance_selector: &'a params::InstanceSelector,
    ) -> LookupResult<lookup::Instance<'a>> {
        match instance_selector {


You'll notice this selector logic is greatly reduced. It checks the name and ID case of the resource being selected against and delegates selection of sub resources to that resource's lookup.

That looks a lot cleaner. The error cases will likely need to be expanded depending on how that gets spec'd. Specifically the over-specification case will want a different error message.

For example:

InstanceSelector { project_selector: Some(project_selector), instance: NameOrId::Id(id) }

Right, yeah. @david-crespo mentioned in the RFD how he thought that case shouldn't be an error and I'm still undecided. Should we just ignore that case?

The ideal would be to allow valid over-specifications, but blanket allowing it would allow parameter combinations that contained mismatched parent identifiers. i.e. /{instance}?project={p_id}&organization={o_id} where p_id is not a child of o_id

Solving that would require also solving the id -> id lookup path.

It certainly could be defined via documentation that we ignore excess parameters, though changing that decision would be a breaking change across the API.

I think allowing valid over-specification and disallowing invalid would be ideal, but I do not know what the effort in that is.

Fair point. I think disallowing invalid combinations is the lowest common denominator. We could loosen that later and it would be non-breaking.

I've updated this in 69ac008 to have expanded error checks. Essentially I just added an extra case for instance and project.

just-be-dev · 2022-12-15T00:11:48Z

nexus/types/src/external_api/params.rs

+#[derive(Deserialize, JsonSchema)]
+pub struct OptionalOrganizationSelector {
+    #[serde(flatten)]
+    pub organization_selector: Option<OrganizationSelector>,
+}


The only reason this optional selector has to exist is because dropshot chokes on Query<Option<OrganizationalSelector>>. If that worked we could delete this.

The types actually work out fine, but the EXPECTORATE update fails.

Example error

thread 'integration_tests::commands::test_nexus_openapi' panicked at 'expected normal process exit with code 0, got Signaled(6) process stderr:thread 'main' panicked at 'invalid type Object( SchemaObject { metadata: Some( Metadata { id: None, title: Some( "Nullable_ProjectSelector", ), description: None, default: None, deprecated: false, read_only: false, write_only: false, examples: [], }, ), instance_type: None, format: None, enum_values: None, const_value: None, subschemas: Some( SubschemaValidation { all_of: Some( [ Object( SchemaObject { metadata: None, instance_type: None, format: None, enum_values: None, const_value: None, subschemas: None, number: None, string: None, array: None, object: None, reference: Some( "#/components/schemas/ProjectSelector", ), extensions: {}, }, ), ], ), any_of: None, one_of: None, not: None, if_schema: None, then_schema: None, else_schema: None, }, ), number: None, string: None, array: None, object: None, reference: None, extensions: { "nullable": Bool(true), }, }, )', /Users/just-be/.cargo/git/checkouts/dropshot-a4a923d29dccc492/2529126/dropshot/src/handler.rs:932:20 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ', nexus/tests/integration_tests/commands.rs:106:38 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I haven't tried converting all of it but you might be able to represent this as:

#[derive(Deserialize, JsonSchema)] pub struct OrganizationSelector { pub organization: Option<NameOrId>, } #[derive(Deserialize, JsonSchema)] pub struct ProjectSelector { #[serde(flatten)] pub organization_selector: Option<OrganizationSelector>, pub project: Option<NameOrId>, }

It would mean that the error message in _ => Err(Error::invalid_request(...) would need updating.

I'm also not positive on the open api spec that this will generate, it would need testing.

That's true. I made it more explicit because there are cases when we know the user must provide a path param (like the parent resource of a list endpoint) vs when it could be represented by the path. I think the extra code (which can be generated) is worth the trade off of a more correct spec.

just-be-dev · 2022-12-15T01:02:53Z

nexus/types/src/external_api/params.rs

+#[derive(Deserialize, JsonSchema)]
+pub struct OrganizationPath {
+    pub organization: NameOrId,
+}


Important thing to note is that I've relocated all of the /v1/ params here. This'll grow to be a bit more noisy and I think will be a good opportunity for macaronization.

karencfv

I'll leave the review of the internals for people who are more acquainted with the code base, but I played around with the progenitor generated client and it works beautifully!!!

Thanks for taking this on, the API is looking great! 🙇‍♀️

david-crespo · 2022-12-16T17:56:38Z

nexus/types/src/external_api/params.rs

+    pub pagination: PaginatedByNameOrId,
+    #[serde(flatten)]
+    pub organization: OrganizationSelector,
+}


I'd almost rather define this next to the endpoint handler since that's the only place it's called.

Yeah, I just grouped them together because I'm hoping to macroize it at some point.

@david-crespo I thought you were the one who split these out by type rather than by use?

david-crespo

Looks great. A bit noisy, but the noise is extremely straightforward so it's not a big deal. We'll have to see whether macros or openapi-lint is the better approach to avoiding mistakes.

just-be-dev · 2022-12-20T18:05:47Z

All right folks, I'm going to go ahead and merge. Your reviews are still welcome/greatly desired. I'll address any issues in a followup.

ahl

adding these as new endpoints rather than modifying existing ones make errors much harder to spot. When are we going to get rid of the non-v1 operations? presumably we'll be changing the operation names to remove v1?

ahl · 2022-12-20T19:05:15Z

nexus/tests/integration_tests/unauthorized_coverage.rs

+    // let expected_uncovered_endpoints =
+    //     std::fs::read_to_string("tests/output/uncovered-authz-endpoints.txt")
+    //         .expect("failed to load file of allowed uncovered endpoints");
+
+    // TODO: Update this to remove overwrite capabilities
+    // See https://github.com/oxidecomputer/expectorate/pull/12
+    assert_contents(
+        "tests/output/uncovered-authz-endpoints.txt",
+        uncovered_endpoints.as_str(),
+    );


what's the plan?

ahl · 2022-12-20T19:09:18Z

nexus/types/src/external_api/params.rs

+    #[serde(flatten)]
+    pub organization_selector: Option<OrganizationSelector>,


why not
pub organization: Option<NameOrId>,

Consistency mostly. I'll flatten it down.

ahl · 2022-12-20T19:10:08Z

nexus/types/src/external_api/params.rs

 #[derive(Deserialize, JsonSchema)]
 pub struct ProjectSelector {
+    #[serde(flatten)]
+    pub organization_selector: Option<OrganizationSelector>,


why not just organization: Option<NameOrId>

ahl · 2022-12-20T19:10:44Z

nexus/types/src/external_api/params.rs

+    pub pagination: PaginatedByNameOrId,
+    #[serde(flatten)]
+    pub organization: OrganizationSelector,
+}


@david-crespo I thought you were the one who split these out by type rather than by use?

just-be-dev · 2022-12-20T21:54:10Z

adding these as new endpoints rather than modifying existing ones make errors much harder to spot. When are we going to get rid of the non-v1 operations?

That's true, but it's generally also less disruptive. My goal is to not block folks working on other things while the transition is in progress. The loose order of operations is

Add the new endpoints
Update the docs and tests
Update the clients
Remove the old endpoints

You can see (or add) more specifics in the parent issue: #2028

presumably we'll be changing the operation names to remove v1?

Tentatively, yes. That seems to be the popular opinion. I think that that because we can have /v1/ and /v2/ of an endpoint then we should likely have that mirrored via the operation names. Even though it's more noisy, it's externally consistent. I'm flexible though.

just-be-dev added 4 commits December 13, 2022 10:08

Revamp selectors (again), convert organizations

8d51dbb

Add project APIs, update routes

129011e

Fix clippy failures

c29c67d

Require less conversions when going from NameOrId to name

599e6eb

just-be-dev force-pushed the rfd-322-orgs-n-projs branch from 73e0b67 to 599e6eb Compare December 14, 2022 04:43

just-be-dev added 3 commits December 14, 2022 15:22

Revamp selectors (again x2), convert projects

114d39a

Update nexus.json

6ae7da3

Skip deprecated endpoints in auth check; update test to assert contents

63eddd7

just-be-dev commented Dec 14, 2022

View reviewed changes

just-be-dev mentioned this pull request Dec 14, 2022

Update API to match resolution of RFD-322 #2028

Open

22 tasks

just-be-dev marked this pull request as ready for review December 14, 2022 21:40

just-be-dev requested review from augustuswm, davepacheco and smklein December 14, 2022 21:40

just-be-dev commented Dec 14, 2022

View reviewed changes

just-be-dev added 4 commits December 14, 2022 17:25

Fix authz test

8db8fa2

Merge branch 'main' into rfd-322-orgs-n-projs

bd1fafe

Update nexus.json

f949c6c

format -_-

bd033a0

just-be-dev commented Dec 15, 2022

View reviewed changes

karencfv reviewed Dec 15, 2022

View reviewed changes

just-be-dev added 2 commits December 15, 2022 15:38

Merge branch 'main' into rfd-322-orgs-n-projs

ae4ddcd

Add expanded error checking

69ac008

just-be-dev requested a review from ahl December 15, 2022 21:17

david-crespo reviewed Dec 16, 2022

View reviewed changes

david-crespo approved these changes Dec 16, 2022

View reviewed changes

just-be-dev added 4 commits December 16, 2022 13:16

Update nexus.json

1176211

Merge branch 'main' into rfd-322-orgs-n-projs

5580d22

Merge branch 'main' into rfd-322-orgs-n-projs

79910eb

Remove duplicates?

3864a25

just-be-dev merged commit 3e49d47 into main Dec 20, 2022

just-be-dev deleted the rfd-322-orgs-n-projs branch December 20, 2022 18:06

ahl reviewed Dec 20, 2022

View reviewed changes

		#[serde(flatten)]
		pub organization_selector: Option<OrganizationSelector>,

RFD-322: Add /v1/ endpoints for organizations and projects #2050

RFD-322: Add /v1/ endpoints for organizations and projects #2050

Uh oh!

Conversation

just-be-dev commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

augustuswm Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

just-be-dev commented Dec 20, 2022

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev commented Dec 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

RFD-322: Add `/v1/` endpoints for organizations and projects #2050

RFD-322: Add `/v1/` endpoints for organizations and projects #2050

just-be-dev commented Dec 13, 2022 •

edited

Loading

just-be-dev Dec 15, 2022 •

edited

Loading

augustuswm Dec 15, 2022 •

edited

Loading

just-be-dev Dec 15, 2022 •

edited

Loading