Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Dec 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8d51dbb
Revamp selectors (again), convert organizations
zephraph Dec 13, 2022
129011e
Add project APIs, update routes
zephraph Dec 13, 2022
c29c67d
Fix clippy failures
zephraph Dec 14, 2022
599e6eb
Require less conversions when going from NameOrId to name
zephraph Dec 14, 2022
114d39a
Revamp selectors (again x2), convert projects
zephraph Dec 14, 2022
6ae7da3
Update nexus.json
zephraph Dec 14, 2022
63eddd7
Skip deprecated endpoints in auth check; update test to assert contents
zephraph Dec 14, 2022
8db8fa2
Fix authz test
zephraph Dec 14, 2022
bd1fafe
Merge branch 'main' into rfd-322-orgs-n-projs
zephraph Dec 14, 2022
f949c6c
Update nexus.json
zephraph Dec 14, 2022
bd033a0
format -_-
zephraph Dec 15, 2022
ae4ddcd
Merge branch 'main' into rfd-322-orgs-n-projs
zephraph Dec 15, 2022
69ac008
Add expanded error checking
zephraph Dec 15, 2022
1176211
Update nexus.json
zephraph Dec 16, 2022
5580d22
Merge branch 'main' into rfd-322-orgs-n-projs
zephraph Dec 19, 2022
79910eb
Merge branch 'main' into rfd-322-orgs-n-projs
zephraph Dec 20, 2022
3864a25
Remove duplicates?
zephraph Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions common/src/api/external/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,18 @@ impl TryFrom<String> for NameOrId {
}
}

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)
}
}
Comment on lines +291 to +301
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


impl JsonSchema for NameOrId {
fn schema_name() -> String {
"NameOrId".to_string()
Expand Down
6 changes: 6 additions & 0 deletions nexus/db-model/src/name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ use serde::{Deserialize, Serialize};
#[display("{0}")]
pub struct Name(pub external::Name);

impl From<Name> for external::NameOrId {
fn from(name: Name) -> Self {
Self::Name(name.0)
}
}
Comment on lines +40 to +44
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above


NewtypeFrom! { () pub struct Name(external::Name); }
NewtypeDeref! { () pub struct Name(external::Name); }

Expand Down
56 changes: 18 additions & 38 deletions nexus/src/app/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,56 +62,36 @@ impl super::Nexus {
instance_selector: &'a params::InstanceSelector,
) -> LookupResult<lookup::Instance<'a>> {
match instance_selector {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@zephraph zephraph Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

params::InstanceSelector { instance: NameOrId::Id(id), .. } => {
// TODO: 400 if project or organization are present
params::InstanceSelector {
project_selector: None,
instance: NameOrId::Id(id),
} => {
let instance =
LookupPath::new(opctx, &self.db_datastore).instance_id(*id);
Ok(instance)
}
params::InstanceSelector {
instance: NameOrId::Name(instance_name),
project: Some(NameOrId::Id(project_id)),
..
project_selector: Some(project_selector),
instance: NameOrId::Name(name),
} => {
// TODO: 400 if organization is present
let instance = LookupPath::new(opctx, &self.db_datastore)
.project_id(*project_id)
.instance_name(Name::ref_cast(instance_name));
let instance = self
.project_lookup(opctx, project_selector)?
.instance_name(Name::ref_cast(name));
Ok(instance)
}
params::InstanceSelector {
instance: NameOrId::Name(instance_name),
project: Some(NameOrId::Name(project_name)),
organization: Some(NameOrId::Id(organization_id)),
project_selector: Some(_),
instance: NameOrId::Id(_),
} => {
let instance = LookupPath::new(opctx, &self.db_datastore)
.organization_id(*organization_id)
.project_name(Name::ref_cast(project_name))
.instance_name(Name::ref_cast(instance_name));
Ok(instance)
Err(Error::invalid_request(
"when providing instance as an ID, project should not be specified",
))
}
params::InstanceSelector {
instance: NameOrId::Name(instance_name),
project: Some(NameOrId::Name(project_name)),
organization: Some(NameOrId::Name(organization_name)),
} => {
let instance = LookupPath::new(opctx, &self.db_datastore)
.organization_name(Name::ref_cast(organization_name))
.project_name(Name::ref_cast(project_name))
.instance_name(Name::ref_cast(instance_name));
Ok(instance)
_ => {
Err(Error::invalid_request(
"instance should either be UUID or project should be specified",
))
}
// TODO: Add a better error message
_ => Err(Error::InvalidRequest {
message: "
Unable to resolve instance. Expected one of
- instance: Uuid
- instance: Name, project: Uuid
- instance: Name, project: Name, organization: Uuid
- instance: Name, project: Name, organization: Name
"
.to_string(),
}),
}
}

Expand Down
78 changes: 33 additions & 45 deletions nexus/src/app/organization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use crate::authz;
use crate::context::OpContext;
use crate::db;
use crate::db::lookup;
use crate::db::lookup::LookupPath;
use crate::db::model::Name;
use crate::external_api::params;
Expand All @@ -18,10 +19,32 @@ use omicron_common::api::external::DeleteResult;
use omicron_common::api::external::Error;
use omicron_common::api::external::ListResultVec;
use omicron_common::api::external::LookupResult;
use omicron_common::api::external::NameOrId;
use omicron_common::api::external::UpdateResult;
use ref_cast::RefCast;
use uuid::Uuid;

impl super::Nexus {
pub fn organization_lookup<'a>(
&'a self,
opctx: &'a OpContext,
organization_selector: &'a params::OrganizationSelector,
) -> LookupResult<lookup::Organization<'a>> {
match organization_selector {
params::OrganizationSelector { organization: NameOrId::Id(id) } => {
let organization = LookupPath::new(opctx, &self.db_datastore)
.organization_id(*id);
Ok(organization)
}
params::OrganizationSelector {
organization: NameOrId::Name(name),
} => {
let organization = LookupPath::new(opctx, &self.db_datastore)
.organization_name(Name::ref_cast(name));
Ok(organization)
}
}
}
pub async fn organization_create(
&self,
opctx: &OpContext,
Expand All @@ -30,30 +53,6 @@ impl super::Nexus {
self.db_datastore.organization_create(opctx, new_organization).await
}

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)
}
Comment on lines -33 to -55
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


pub async fn organizations_list_by_name(
&self,
opctx: &OpContext,
Expand All @@ -73,27 +72,20 @@ impl super::Nexus {
pub async fn organization_delete(
&self,
opctx: &OpContext,
organization_name: &Name,
organization_lookup: &lookup::Organization<'_>,
) -> DeleteResult {
let (.., authz_org, db_org) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.fetch()
.await?;
let (.., authz_org, db_org) = organization_lookup.fetch().await?;
self.db_datastore.organization_delete(opctx, &authz_org, &db_org).await
}

pub async fn organization_update(
&self,
opctx: &OpContext,
organization_name: &Name,
organization_lookup: &lookup::Organization<'_>,
new_params: &params::OrganizationUpdate,
) -> UpdateResult<db::model::Organization> {
let (.., authz_organization) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.lookup_for(authz::Action::Modify)
.await?;
organization_lookup.lookup_for(authz::Action::Modify).await?;
self.db_datastore
.organization_update(
opctx,
Expand All @@ -108,12 +100,10 @@ impl super::Nexus {
pub async fn organization_fetch_policy(
&self,
opctx: &OpContext,
organization_name: &Name,
organization_lookup: &lookup::Organization<'_>,
) -> LookupResult<shared::Policy<authz::OrganizationRole>> {
let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.lookup_for(authz::Action::ReadPolicy)
.await?;
let (.., authz_org) =
organization_lookup.lookup_for(authz::Action::ReadPolicy).await?;
let role_assignments = self
.db_datastore
.role_assignment_fetch_visible(opctx, &authz_org)
Expand All @@ -128,13 +118,11 @@ impl super::Nexus {
pub async fn organization_update_policy(
&self,
opctx: &OpContext,
organization_name: &Name,
organization_lookup: &lookup::Organization<'_>,
policy: &shared::Policy<authz::OrganizationRole>,
) -> UpdateResult<shared::Policy<authz::OrganizationRole>> {
let (.., authz_org) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.lookup_for(authz::Action::ModifyPolicy)
.await?;
let (.., authz_org) =
organization_lookup.lookup_for(authz::Action::ModifyPolicy).await?;

let role_assignments = self
.db_datastore
Expand Down
Loading