VPC external endpoints #178
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ use super::schema::LookupByUniqueId; | |
| use super::schema::LookupByUniqueName; | ||
| use super::schema::LookupByUniqueNameInProject; | ||
| use super::schema::Project; | ||
| use super::schema::Vpc; | ||
| use super::sql::SqlSerialize; | ||
| use super::sql::SqlString; | ||
| use super::sql::SqlValueSet; | ||
|
|
@@ -731,4 +732,75 @@ impl DataStore { | |
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn project_list_vpcs( | ||
| &self, | ||
| project_id: &Uuid, | ||
| pagparams: &DataPageParams<'_, Name>, | ||
| ) -> ListResult<api::external::Vpc> { | ||
| let client = self.pool.acquire().await?; | ||
| sql_fetch_page_by::< | ||
| LookupByUniqueNameInProject, | ||
|
Collaborator
There was a problem hiding this comment. We're looking up VPCs by name here, but the name isn't explicitly a unique identifier for VPCs, is it? omicron/omicron-common/src/sql/dbinit.sql Lines 252 to 262 in da2071a The primary key is the UUID - shouldn't we be looking up VPCs by ID, not name? I get that the implementation of Caveat: It's totally possible I'm misinterpreting how omicron/omicron-nexus/src/db/schema.rs Lines 378 to 395 in da2071a But wouldn't we want a (FYI @davepacheco - I see
Collaborator
There was a problem hiding this comment. Follow-up: Is this relying on the unique index in omicron/omicron-common/src/sql/dbinit.sql Lines 265 to 268 in da2071a
Contributor
Author
There was a problem hiding this comment. Good news, I already wrote
Collaborator
There was a problem hiding this comment. Yeah, I saw the test, but I'm still trying to wrap my head around this - the omicron/omicron-nexus/src/db/sql_operations.rs Lines 624 to 656 in da2071a So I'm still kinda uncertain what provides the "names must be unique" property, if not the unique DB index.
Collaborator
There was a problem hiding this comment. Having some conversations in chat, but figured I'd post the TL;DR here too: If the Seems the approach taken by instances is creating a unique index by the tuple
Contributor
Author
There was a problem hiding this comment. Confirmed in the docs that the index is the official way to enforce uniqueness for a pair of columns:
Collaborator
There was a problem hiding this comment. I think y'all figured most of this out already! I'll try to explain a little more why things are the way they are. In terms of desired behavior, RFD 4 says this about the "name" property of something that has identity metadata:
RFD 21 explains that VPCs are scoped to projects. So yes, their name should be unique within the project, and not necessarily globally unique. In terms of implementing this: yes, we use CockroachDB unique indexes to guarantee uniqueness. That's the natural place to do it, since the database has such a facility and the behavior needs to be strongly consistent. As @smklein points out, the unique index for Instances is on the There's a close relationship between the fields in the unique indexes on a table, the @smklein wrote:
Yeah, so when the block comment on /// Idempotently insert a database record. This is intended for cases where the
/// record may conflict with an existing record with the same id, which should
/// be ignored, or with an existing record with the same name, which should
/// produce an [`Error::ObjectAlreadyExists`] error.
It might be more precise to say: ///
/// Idempotently insert a database record. More precisely, this is intended
/// for cases where:
///
/// * the caller wants to insert a database row for some particular object
/// * the object is identified by an immutable, unique id
/// * the caller wants the operation to succeed if a row already exists for this
/// object (i.e., a row exists with the same unique id, which is by definition
/// a row for the same object)
/// * the caller wants to fail if inserting the record would violate some other
/// constraint, usually a name conflict (which is necessarily a record
/// for a _different_ object, since if it were the same object, it would have
/// the same id, and the operation would succeed without inserting anything)
///
/// That might sound incredibly specific or even contrived, but it's the common
/// case for sagas that want to create database records for API resources. For
/// example, when provisioning an Instance, the provision saga will generate a
/// unique id and use this function to insert an initial Instance object into
/// the database. Since the id is generated in the saga, the only way to have a
/// conflict on the object's id is if the saga action is being rerun. Since
/// actions are supposed to be idempotent, the correct behavior is to succeed
/// despite the conflict. On the other hand, if the new Instance's name
/// conflicts with another one in the same project, that will raise a different
/// constraint violation that we do want to package up and bubble up to the
/// caller. This condition means somebody tried to create an Instance with the
/// same name as one that's already running, so we package this up as an
/// [`Error::ObjectAlreadyExists`] error.
///
This is a long way of explaining why the unique value is not used in the function aside from the error message. (Why bother putting it in the error message? The @smklein wrote:
I think you got this answered but I'm not sure? I wasn't quite sure why you expected to use a lookup by id. Is it just because that's the primary key in the database? Anyway, I expect API operations to do lookups by name rather than id. (RFD 4 is a little confusing in that it uses identifiers like "projectId" in all the URLs. But it also says "the endpoints that reference an individual resource work on either the uuid or the name." If I remember right, we only implement name lookups in Nexus for most resources today.) Given this, for any resource that's scoped to a project, I'd expect us to use LookupByUniqueNameInProject and not LookupByUniqueIdInProject. The general ideas here are:
I'd been assuming all this mostly based on past experience, but assuming it makes sense, we should probably document it. Does all that make sense?
Contributor
Author
There was a problem hiding this comment. That all makes sense. Most of it we were able to infer from the code, but it is helpful to see it spelled out. Especially:
I was mostly able to get there by copying the patterns I saw used for instances. |
||
| Vpc, | ||
| <Vpc as Table>::ModelType, | ||
| >(&client, (project_id,), pagparams, Vpc::ALL_COLUMNS) | ||
| .await | ||
| } | ||
|
|
||
| pub async fn project_create_vpc( | ||
| &self, | ||
| vpc_id: &Uuid, | ||
| project_id: &Uuid, | ||
| params: &api::external::VpcCreateParams, | ||
| ) -> Result<api::external::Vpc, Error> { | ||
| let client = self.pool.acquire().await?; | ||
| let now = Utc::now(); | ||
| let mut values = SqlValueSet::new(); | ||
| values.set("id", vpc_id); | ||
| values.set("time_created", &now); | ||
| values.set("time_modified", &now); | ||
| values.set("project_id", project_id); | ||
| params.sql_serialize(&mut values); | ||
|
|
||
| sql_insert_unique_idempotent_and_fetch::<Vpc, LookupByUniqueId>( | ||
| &client, | ||
| &values, | ||
| params.identity.name.as_str(), | ||
| "id", | ||
| (), | ||
| &vpc_id, | ||
| ) | ||
| .await | ||
| } | ||
|
|
||
| pub async fn vpc_fetch_by_name( | ||
| &self, | ||
| project_id: &Uuid, | ||
| vpc_name: &Name, | ||
| ) -> LookupResult<api::external::Vpc> { | ||
| let client = self.pool.acquire().await?; | ||
| sql_fetch_row_by::<LookupByUniqueNameInProject, Vpc>( | ||
| &client, | ||
| (project_id,), | ||
| vpc_name, | ||
| ) | ||
| .await | ||
| } | ||
|
|
||
| pub async fn project_delete_vpc(&self, vpc_id: &Uuid) -> DeleteResult { | ||
| let client = self.pool.acquire().await?; | ||
| let now = Utc::now(); | ||
| sql_execute_maybe_one( | ||
| &client, | ||
| format!( | ||
| "UPDATE {} SET time_deleted = $1 WHERE \ | ||
| time_deleted IS NULL AND id = $2", | ||
| Vpc::TABLE_NAME, | ||
| ) | ||
| .as_str(), | ||
| &[&now, &vpc_id], | ||
| || Error::not_found_by_id(ResourceType::Vpc, vpc_id), | ||
| ) | ||
| .await | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right. It needs to be
(project_id, name)rather than(name, project_id). This is critical to be able to use this index to list the VPCs in a Project. (Think of it this way: we want the database to be able to quickly find the first the VPC in the Project. If the index is sorted by project_id first, that's easy. Once it finds that record, the next N values in the index will be the N VPCs in that project, sorted by name -- perfect. If the index is sorted by(name, project_id), this doesn't work, and there's no efficient way to find the first VPC (or any other VPC) in a Project.)Edit: I really wish there were some way to automatically test this. This is probably working in your change because CockroachDB is doing a table scan rather than using this index. I kind of want CockroachDB to refuse to do queries that always necessitate table scans. I'm not really sure how best to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Didn't realize the index works like a nested dictionary.