Skip to content

v1 instance APIs as per RFD-322 #1957

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

Merged
merged 35 commits into from
Dec 8, 2022
Merged

v1 instance APIs as per RFD-322 #1957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
233dc6d
Test impl of new endpoint format
just-be-dev Nov 17, 2022
0be1778
ResourceIdentifier -> NameOrId
just-be-dev Nov 17, 2022
6e3210f
Add docs, remove unneeded chunk
just-be-dev Nov 17, 2022
a2681b9
Use v1 prefix, convert instance delete
just-be-dev Nov 17, 2022
18bc9cf
Push up diplay error
just-be-dev Nov 17, 2022
07ec450
ReferenceIdentifer -> NameOrId in nexus.json
just-be-dev Nov 17, 2022
5b2f217
Update nexus.json to fix build issues
just-be-dev Nov 17, 2022
8631e9f
Change v1 to a prefix instead of postfix
just-be-dev Nov 17, 2022
41672aa
Normalize id lookups and path selection
just-be-dev Nov 19, 2022
954ffdb
Add v1 create instance api
just-be-dev Nov 20, 2022
c8d191b
Add instance list endpoint
just-be-dev Nov 21, 2022
6708bac
Add instance migrate endpoint
just-be-dev Nov 22, 2022
26b5651
Add instance reboot endpoint
just-be-dev Nov 22, 2022
f70f84f
Add instance start endpoint
just-be-dev Nov 22, 2022
e6374fe
Add instance stop endpoint
just-be-dev Nov 22, 2022
cdaa977
Add serial console endpoints
just-be-dev Nov 22, 2022
d0a19c6
Workaround for multiple emits
just-be-dev Nov 22, 2022
f439f59
Implement NameOrId for display to try resolve dropshot error
just-be-dev Nov 22, 2022
b4e818a
Flip the order of id/name in the NameOrId enum
just-be-dev Nov 23, 2022
7bdfeb0
Add untagged to NameOrId
just-be-dev Nov 25, 2022
cf5f540
Update NameOrId to be labeled
just-be-dev Nov 29, 2022
8c7ce21
RFD-322 PoC Revision 2 (#1995)
just-be-dev Nov 30, 2022
87f4d25
Move version to postfix
just-be-dev Nov 30, 2022
393283d
Merge main int rfd-322-poc
just-be-dev Dec 1, 2022
2a87558
Always do a lookup, even when only handling UUID
just-be-dev Dec 2, 2022
0b227b0
Merge branch 'main' into rfd-322-poc
just-be-dev Dec 2, 2022
78962c2
Merge branch 'main' into rfd-322-poc
just-be-dev Dec 5, 2022
ce7da01
Fix clippy failures
just-be-dev Dec 6, 2022
2092012
Merge branch 'main' into rfd-322-poc
just-be-dev Dec 6, 2022
96165f5
Update unauthz coverage
just-be-dev Dec 6, 2022
e616c03
Fix up some failed testcases
just-be-dev Dec 6, 2022
3c13edc
Add view by id to skip list
just-be-dev Dec 7, 2022
f26c18a
Merge branch 'main' into rfd-322-poc
just-be-dev Dec 7, 2022
12bc365
Update lookups to return authz refs instead of uuids
just-be-dev Dec 7, 2022
fc517af
Address PR feedback
just-be-dev Dec 8, 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
42 changes: 42 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 @@ -268,6 +268,48 @@ impl Name {
}
}

#[derive(Serialize, Deserialize, Display, Clone)]
#[display("{0}")]
#[serde(untagged)]
pub enum NameOrId {
Id(Uuid),
Name(Name),
}

impl TryFrom<String> for NameOrId {
type Error = String;

fn try_from(value: String) -> Result<Self, Self::Error> {
if let Ok(id) = Uuid::parse_str(&value) {
Ok(NameOrId::Id(id))
} else {
Ok(NameOrId::Name(Name::try_from(value)?))
}
}
}

impl JsonSchema for NameOrId {
fn schema_name() -> String {
"NameOrId".to_string()
}

fn json_schema(
gen: &mut schemars::gen::SchemaGenerator,
) -> schemars::schema::Schema {
schemars::schema::SchemaObject {
subschemas: Some(Box::new(schemars::schema::SubschemaValidation {
one_of: Some(vec![
label_schema("id", gen.subschema_for::<Uuid>()),
label_schema("name", gen.subschema_for::<Name>()),
]),
..Default::default()
})),
..Default::default()
}
.into()
}
}

/// Name for a built-in role
#[derive(
Clone,
Expand Down
185 changes: 92 additions & 93 deletions nexus/src/app/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use omicron_common::api::external::InstanceState;
use omicron_common::api::external::InternalContext;
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 omicron_common::api::external::Vni;
use omicron_common::api::internal::nexus;
Expand All @@ -53,18 +54,84 @@ use uuid::Uuid;
const MAX_KEYS_PER_INSTANCE: u32 = 8;

impl super::Nexus {
pub async fn instance_lookup(
Copy link
Collaborator

@davepacheco davepacheco Dec 9, 2022

Choose a reason for hiding this comment

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

Is the thought that we'll eventually create a foo_lookup() for every type of object? And that it would includes a match block with all possible combinations of the query params? It seems like there's a lot that can go wrong there. But it feels like this is the kind of problem we solved (or tried to solve?) with LookupPath and I wonder if we can clean this up by passing around lookup::Instance or the like (discussed a little here, which I see you considered). What if we had the http_entrypoints functions use LookupPath directly and pass the lookup::Foo into app-level functions? I think that would make them more composeable, so that instance_lookup() could use project_lookup() (which would use organization_lookup()) rather than duplicating that logic. What do you think?

Copy link
Contributor Author

@just-be-dev just-be-dev Dec 9, 2022

Choose a reason for hiding this comment

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

Yeah, I like that idea a lot. I'll explore it a little more. There've been some iterations on this that have slowly started to feel better. It went from names, to ids, to authz objects. Maybe the lookups are the last step to really make it compositional. Thanks for the suggestion.

&self,
opctx: &OpContext,
instance_selector: params::InstanceSelector,
) -> LookupResult<authz::Instance> {
match instance_selector {
params::InstanceSelector { instance: NameOrId::Id(id), .. } => {
// TODO: 400 if project or organization are present
Copy link
Collaborator

@davepacheco davepacheco Dec 9, 2022

Choose a reason for hiding this comment

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

For what it's worth, I think these cases are valuable to fix early because they can help catch otherwise nasty client bugs, and also fixing them later seems likely a breaking change.

let (.., authz_instance) =
LookupPath::new(opctx, &self.db_datastore)
.instance_id(id)
.lookup_for(authz::Action::Read)
.await?;
Ok(authz_instance)
}
params::InstanceSelector {
instance: NameOrId::Name(instance_name),
project: Some(NameOrId::Id(project_id)),
..
} => {
// TODO: 400 if organization is present
let (.., authz_instance) =
LookupPath::new(opctx, &self.db_datastore)
.project_id(project_id)
.instance_name(&Name(instance_name))
.lookup_for(authz::Action::Read)
.await?;
Ok(authz_instance)
}
params::InstanceSelector {
instance: NameOrId::Name(instance_name),
project: Some(NameOrId::Name(project_name)),
organization: Some(NameOrId::Id(organization_id)),
} => {
let (.., authz_instance) =
LookupPath::new(opctx, &self.db_datastore)
.organization_id(organization_id)
.project_name(&Name(project_name))
.instance_name(&Name(instance_name.clone()))
.lookup_for(authz::Action::Read)
.await?;
Ok(authz_instance)
}
params::InstanceSelector {
instance: NameOrId::Name(instance_name),
project: Some(NameOrId::Name(project_name)),
organization: Some(NameOrId::Name(organization_name)),
} => {
let (.., authz_instance) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(&Name(organization_name))
.project_name(&Name(project_name))
.instance_name(&Name(instance_name.clone()))
.lookup_for(authz::Action::Read)
.await?;
Ok(authz_instance)
}
// 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(),
}),
}
}

pub async fn project_create_instance(
self: &Arc<Self>,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
authz_project: &authz::Project,
params: &params::InstanceCreate,
) -> CreateResult<db::model::Instance> {
let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.lookup_for(authz::Action::CreateChild)
.await?;
opctx.authorize(authz::Action::CreateChild, authz_project).await?;
Copy link
Collaborator

@davepacheco davepacheco Dec 9, 2022

Choose a reason for hiding this comment

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

I gather we've basically decoupled the lookup (which is now done by the caller) from the authz check. It makes sense, just a little sad because coupling those was intentional to make it harder to mess up the authz part. (And I see now why you were talking about wanting to express the authz checks that were done in the type.)

Actually, I think if you have the foo_lookup() functions return lookup::Foo, that will address this problem too because the top-level caller doesn't have to decide which authz check to do.

As this is now, I think we'll also wind up doing 2x the authz checks now and we may wind up wanting to explore caching those. As I think about it, what we probably want to cache are the roles and not the result of the authz check itself. That's because when you call foo_lookup(), you're only checking for read (because the caller doesn't know what authz checks will be needed by other functions it's going to call later), but later functions may wind up needing other permissions, so the authz checks won't look exactly the same.


// Validate parameters
if params.disks.len() > MAX_DISKS_PER_INSTANCE as usize {
Expand Down Expand Up @@ -133,8 +200,6 @@ impl super::Nexus {

let saga_params = sagas::instance_create::Params {
serialized_authn: authn::saga::Serialized::for_opctx(opctx),
organization_name: organization_name.clone().into(),
project_name: project_name.clone().into(),
project_id: authz_project.id(),
create_params: params.clone(),
};
Expand Down Expand Up @@ -195,15 +260,10 @@ impl super::Nexus {
pub async fn project_list_instances(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
authz_project: &authz::Project,
pagparams: &DataPageParams<'_, Name>,
) -> ListResultVec<db::model::Instance> {
let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.lookup_for(authz::Action::ListChildren)
.await?;
opctx.authorize(authz::Action::ListChildren, authz_project).await?;
self.db_datastore
.project_list_instances(opctx, &authz_project, pagparams)
.await
Expand All @@ -212,26 +272,10 @@ impl super::Nexus {
pub async fn instance_fetch(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
) -> LookupResult<db::model::Instance> {
let (.., db_instance) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.instance_name(instance_name)
.fetch()
.await?;
Ok(db_instance)
}

pub async fn instance_fetch_by_id(
&self,
opctx: &OpContext,
instance_id: &Uuid,
authz_instance: &authz::Instance,
Copy link
Collaborator

@davepacheco davepacheco Dec 9, 2022

Choose a reason for hiding this comment

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

I think this pattern of function makes a lot less sense after this change. (That's not a bad thing.) We presumably just did a lookup of the Instance, dropped the database part, and now we're doing a whole other lookup just to fetch the database part again. It seems like wherever we're using foo_fetch() now we should just have it use LookupPath()...fetch() instead of what it's currently doing, which must be LookupPath()...lookup(). This might be tricky if we're using the new Nexus::foo_lookup() pattern, but I think that's another reason to have those functions return lookup::Foo instead of authz::Foo (so the caller can decide if they want to use lookup() or fetch()).

) -> LookupResult<db::model::Instance> {
let (.., db_instance) = LookupPath::new(opctx, &self.db_datastore)
.instance_id(*instance_id)
.instance_id(authz_instance.id())
.fetch()
.await?;
Ok(db_instance)
Expand All @@ -243,20 +287,12 @@ impl super::Nexus {
pub async fn project_destroy_instance(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
) -> DeleteResult {
// TODO-robustness We need to figure out what to do with Destroyed
// instances? Presumably we need to clean them up at some point, but
// not right away so that callers can see that they've been destroyed.
let (.., authz_instance, _) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.instance_name(instance_name)
.fetch()
.await?;
opctx.authorize(authz::Action::Delete, authz_instance).await?;

self.db_datastore
.project_delete_instance(opctx, &authz_instance)
Expand All @@ -274,17 +310,10 @@ impl super::Nexus {
pub async fn project_instance_migrate(
self: &Arc<Self>,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
params: params::InstanceMigrate,
) -> UpdateResult<db::model::Instance> {
let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.instance_name(instance_name)
.lookup_for(authz::Action::Modify)
.await?;
opctx.authorize(authz::Action::Modify, authz_instance).await?;

// Kick off the migration saga
let saga_params = sagas::instance_migrate::Params {
Expand Down Expand Up @@ -338,9 +367,7 @@ impl super::Nexus {
pub async fn instance_reboot(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
) -> UpdateResult<db::model::Instance> {
// To implement reboot, we issue a call to the sled agent to set a
// runtime state of "reboot". We cannot simply stop the Instance and
Expand All @@ -355,9 +382,7 @@ impl super::Nexus {
// running.
let (.., authz_instance, db_instance) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.instance_name(instance_name)
.instance_id(authz_instance.id())
.fetch()
.await?;
let requested = InstanceRuntimeStateRequested {
Expand All @@ -378,15 +403,11 @@ impl super::Nexus {
pub async fn instance_start(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
) -> UpdateResult<db::model::Instance> {
let (.., authz_instance, db_instance) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.instance_name(instance_name)
.instance_id(authz_instance.id())
Copy link
Collaborator

@davepacheco davepacheco Dec 9, 2022

Choose a reason for hiding this comment

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

Same here as above -- we already did a lookup in the caller. It'd be nice to just use that here instead of doing another lookup. (You could even wind up with a different result here than the caller got.)

.fetch()
.await?;
let requested = InstanceRuntimeStateRequested {
Expand All @@ -407,15 +428,11 @@ impl super::Nexus {
pub async fn instance_stop(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
) -> UpdateResult<db::model::Instance> {
let (.., authz_instance, db_instance) =
LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
.project_name(project_name)
.instance_name(instance_name)
Copy link
Contributor

@david-crespo david-crespo Nov 29, 2022

Choose a reason for hiding this comment

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

This is all a big improvement, thanks for showing what this would look like.

.instance_id(authz_instance.id())
.fetch()
.await?;
let requested = InstanceRuntimeStateRequested {
Expand Down Expand Up @@ -1110,19 +1127,10 @@ impl super::Nexus {
pub(crate) async fn instance_serial_console_data(
&self,
opctx: &OpContext,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
params: &params::InstanceSerialConsoleRequest,
) -> Result<params::InstanceSerialConsoleData, Error> {
let db_instance = self
.instance_fetch(
opctx,
organization_name,
project_name,
instance_name,
)
.await?;
let db_instance = self.instance_fetch(opctx, authz_instance).await?;

let sa = self.instance_sled(&db_instance).await?;
let data = sa
Expand All @@ -1146,18 +1154,9 @@ impl super::Nexus {
&self,
opctx: &OpContext,
conn: dropshot::WebsocketConnection,
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
authz_instance: &authz::Instance,
) -> Result<(), Error> {
let instance = self
.instance_fetch(
opctx,
organization_name,
project_name,
instance_name,
)
.await?;
let instance = self.instance_fetch(opctx, authz_instance).await?;
let ip_addr = instance
.runtime_state
.propolis_ip
Expand Down
Loading