Implement silo level images #2772

zephraph · 2023-04-05T21:22:22Z

Implements silo level images as a replacement for global images.

This implementation preserves a single API endpoint /v1/images and a single database table to store all images. It uses database views like #2767 to map the notion of ProjectImage and SiloImage to the images table. From an authz perspective there are actually three resources represented:SiloImage, ProjectImage, and Image. As the names suggest, SiloImage is the child of a silo and ProjectImage is the child of a project. Image occupies an odd space where it's currently considered the child of a Silo though it technically straddles both positions in the hierarchy.

david-crespo · 2023-04-12T18:19:11Z

nexus/types/src/external_api/views.rs

-    #[serde(flatten)]
-    pub parent_id: SiloOrProjectId,
+    /// ID of the parent project if the image is a project image
+    pub project_id: Option<Uuid>,


Probably worth going all the way and saying ", otherwise null", or even "On silo-scoped images this will be null.`

david-crespo · 2023-04-12T18:38:51Z

nexus/src/app/image.rs

+                    silo.lookup_for(authz::Action::CreateChild).await?;
+                (authz_silo, None)
+            }
+        };


It helps avoid repeating the rather complex logic contained below.

zephraph · 2023-04-17T17:28:32Z

common/src/sql/dbinit.sql

+    silo_id,
+    project_id,


Note that project images also include silo_id. This is necessary to convert them back to the base image type.

zephraph · 2023-04-17T17:31:09Z

nexus/authz-macros/src/lib.rs

+        // If this resource is directly inside a Silo, we only need to define
+        // permissions that are contingent on having roles on that Silo.
+        (PolarSnippet::InSilo, _) => format!(
+            r#"
+                resource {} {{
+                    permissions = [
+                        "list_children",
+                        "modify",
+                        "read",
+                        "create_child",
+                    ];
+
+                    relations = {{ containing_silo: Silo }};
+                    "list_children" if "viewer" on "containing_silo";
+                    "read" if "viewer" on "containing_silo";
+                    "modify" if "collaborator" on "containing_silo";
+                    "create_child" if "collaborator" on "containing_silo";
+                }}
+
+                has_relation(parent: Silo, "containing_silo", child: {})
+                    if child.silo = parent;
+            "#,
+            resource_name, resource_name,
+        ),


I patterned this off of InProject to avoid have to create a one-off authz rule for SiloImage. In the future if there's a normal resource that's a child of the silo we can use this polar snippet to avoid having to define custom rules for it. I haven't looked too closely at the other resources that live under the silo to see if any of them could be updated to use this snippet instead.

zephraph · 2023-04-17T17:36:39Z

nexus/db-model/src/image.rs

+}
+
+#[derive(
+    Queryable, Selectable, Clone, Debug, Resource, Serialize, Deserialize,


It's notable that because project_image and silo_image are based on views they're not insertable resources (as noted by @luqmana in #2767)

zephraph · 2023-04-17T17:41:55Z

nexus/db-queries/src/authz/api_resources.rs

+// This resource is a collection of _all_ images in a silo, including project images.
+authz_resource! {
+    name = "Image",
+    parent = "Silo",
+    primary_key = Uuid,
+    roles_allowed = false,
+    polar_snippet = InSilo,
+}


It's notable here that I've used the InSilo polar snippet to define auth for the base image when that's not strictly true. The only place Image is referred to in the auth system is when we're looking up an image based on ID. In that case, we don't know if it's a project or silo image so we must use the generic form of the image lookup. For the current implementation that means a user must have read permissions on the Silo to even be able to look up a project image by ID. Subsequent authz checks validates the appropriate permissions on the project (if it's a project image) so we get the right checks on the resource eventually.

nexus/db-queries/src/db/datastore/image.rs

zephraph · 2023-04-17T17:59:17Z

nexus/db-queries/src/db/lookup.rs

@@ -453,7 +483,7 @@ impl<'a> Root<'a> {
 lookup_resource! {
    name = "Silo",
    ancestors = [],
-    children = [ "IdentityProvider", "SamlIdentityProvider", "Project" ],
+    children = [ "IdentityProvider", "SamlIdentityProvider", "Project", "SiloImage" ],


Should Image be added here? cc @davepacheco

I believe the only purpose of putting things here is to generate selectors on this lookup type that will look up child resources. e.g., if you wanted to do LookupPath(...).silo_id(...).image_name(...), then you'd want to put that here. I haven't looked at the rest of this PR yet to know if you'd want that here but it seems like not since you should already have SiloImage. But I think it's safe to leave it out if you're not sure.

This base Image type will only ever be used to access an image by ID so it seems fine.

zephraph · 2023-04-17T18:00:21Z

nexus/db-queries/src/db/lookup.rs

@@ -540,6 +579,15 @@ lookup_resource! {

 lookup_resource! {
    name = "Image",
+    ancestors = ["Silo"],
+    children = [],
+    lookup_by_name = false,


It's notable that this resource is only used for ID lookups. If there's a name it's always either SiloImage or ProjectImage instead.

zephraph · 2023-04-17T18:00:43Z

nexus/db-queries/src/db/lookup.rs

+// Helpers for unifying the interfaces around images
+
+pub enum ImageLookup<'a> {
+    ProjectImage(ProjectImage<'a>),
+    SiloImage(SiloImage<'a>),
+}
+
+pub enum ImageParentLookup<'a> {
+    Project(Project<'a>),
+    Silo(Silo<'a>),
+}
+


This might not really be the best place for these little helper enums, but I wasn't sure where else to put them.

zephraph · 2023-04-17T18:01:32Z

nexus/src/app/image.rs

-    pub fn image_lookup<'a>(
+    pub async fn image_lookup<'a>(


This is unique in that it's the only lookup that's async

zephraph · 2023-04-17T18:02:38Z

nexus/src/app/image.rs

        &'a self,
        opctx: &'a OpContext,
        image_selector: params::ImageSelector,
-    ) -> LookupResult<lookup::Image<'a>> {
+    ) -> LookupResult<ImageLookup<'a>> {


The return type here is an enum that wraps either a ProjectImage or a SiloImage. It's important that we returned the appropriate type for authz purposes.

zephraph · 2023-04-17T18:04:18Z

nexus/src/app/image.rs

+                let (.., db_image) = LookupPath::new(opctx, &self.db_datastore)
+                    .image_id(id).fetch().await?;
+                let lookup = match db_image.project_id {
+                    Some(_) => ImageLookup::ProjectImage(LookupPath::new(opctx, &self.db_datastore)
+                        .project_image_id(id)),
+                    None => {
+                        ImageLookup::SiloImage(LookupPath::new(opctx, &self.db_datastore)
+                            .silo_image_id(db_image.silo_id))
+                    },
+                };


This is a bit unique. Because there's only a single endpoint for lookup images by id (/v1/images/{image_id}) we don't actually know if it's a silo or project image when we get the ID. We first have to lookup the image and see if project_id is populated. If it is, we know it's a project id. Otherwise it's a silo id.

zephraph · 2023-04-17T18:09:48Z

nexus/src/app/silo.rs

+    pub fn current_silo_lookup<'a>(
+        &'a self,
+        opctx: &'a OpContext,
+    ) -> LookupResult<lookup::Silo<'a>> {
+        let silo = opctx
+            .authn
+            .silo_required()
+            .internal_context("looking up current silo")?;
+        let silo = self.silo_lookup(opctx, NameOrId::Id(silo.id()))?;
+        Ok(silo)
+    }


This is just a helpful utility to be able to quickly construct a silo lookup just given the opctx.

…e image to a silo image)

nexus/db-model/src/image.rs

nexus/types/src/external_api/params.rs

Co-authored-by: David Crespo <david-crespo@users.noreply.github.com>

david-crespo

Looks great. We went over this in detail on a call. This has been a long journey.

co-authored-by: David Crespo <david@oxidecomputer.com>

davepacheco · 2023-04-19T17:35:22Z

nexus/db-queries/src/db/datastore/image.rs

+            silo_id,
+            diesel::insert_into(dsl::image)
+                .values(image)
+                .on_conflict(dsl::id)


This looks more like an "upsert" than "create". I'm not sure if that's necessary here?

Yeah, you're right, this should be more of a regular insert. I'll create an issue to follow up on this.

The project images look like they're in the same boat

Post the merge of #2772 we want to remove the concept of system level images. This PR takes on that work.

zephraph added 12 commits April 5, 2023 11:51

Add image views

f3c30a8

WIP impl project/silo images

fa120f8

Merge branch 'main' into image-silo-views

3d470c8

Merge branch 'main' into image-silo-views

f6a99d5

Put a pin in current progress

3343ead

Add silo image id, name lookups

094ec23

Fixup some lookup issues

974f31e

Fix more import/life cycle/lookup references

8c05776

Merge branch 'main' into image-silo-views

1f77553

Merge branch 'main' into image-silo-views

532e6e5

Fixup selectors

7016701

Update image result type for project_id to be optional

053d14e

david-crespo reviewed Apr 12, 2023

View reviewed changes

Add image promote

96ab341

david-crespo reviewed Apr 12, 2023

View reviewed changes

zephraph added 7 commits April 12, 2023 16:57

Enable listing silo images alongside project images

acdb77e

Merge branch 'main' into image-silo-views

420af77

Merge branch 'main' into image-silo-views

4c58909

Fix incorrect error message matching in test

3b1c576

Merge branch 'main' into image-silo-views

5c34397

Attempt to resolve the stub test failure

68e649a

Fix incorrect project image lookup

e74f595

zephraph changed the title ~~[WIP] Implement silo level images~~ Implement silo level images Apr 17, 2023

zephraph marked this pull request as ready for review April 17, 2023 15:09

Fixup authz test for silo images

239c37a

zephraph commented Apr 17, 2023

View reviewed changes

Remove insertable from the resources based off of db views

7fd9329

zephraph commented Apr 17, 2023

View reviewed changes

nexus/db-queries/src/db/datastore/image.rs Outdated Show resolved Hide resolved

zephraph commented Apr 17, 2023

View reviewed changes

zephraph added 2 commits April 17, 2023 14:37

Fixup promotion to have the correct authz (and to actually convert th…

abfb634

…e image to a silo image)

Ensure project image list w/ silos is scoped to the silo

093f09b

david-crespo reviewed Apr 17, 2023

View reviewed changes

nexus/db-model/src/image.rs Show resolved Hide resolved

Update image create test; default include_silo_images to false

c2a605e

david-crespo reviewed Apr 17, 2023

View reviewed changes

nexus/types/src/external_api/params.rs Outdated Show resolved Hide resolved

zephraph and others added 6 commits April 17, 2023 17:23

Update nexus/types/src/external_api/params.rs

fd48b21

Co-authored-by: David Crespo <david-crespo@users.noreply.github.com>

Fix parsing issues w/ include_silo_images flag

d48460b

Change promote query to an update

da9d1e7

Add silo image creation test

ae67793

Fix image promotion process

5d9dd3f

Co-authored-by: David Crespo <david-crespo@users.noreply.github.com>

Remove unused import

28c87cb

david-crespo approved these changes Apr 18, 2023

View reviewed changes

zephraph and others added 3 commits April 18, 2023 16:04

Add module docs for image models

c96d484

co-authored-by: David Crespo <david@oxidecomputer.com>

Fix clippy issues

c288e85

Add promote endpoint to authz coverage

2f9ef0c

zephraph merged commit 9fafa2b into main Apr 19, 2023

zephraph deleted the image-silo-views branch April 19, 2023 01:26

zephraph mentioned this pull request Apr 19, 2023

Remove global images #2870

Merged

davepacheco reviewed Apr 19, 2023

View reviewed changes

zephraph mentioned this pull request Apr 19, 2023

Silo/Project image create queries are upserts when they should be inserts #2878

Open

zephraph added a commit that referenced this pull request May 9, 2023

Remove global images (#2870)

f574b3c

Post the merge of #2772 we want to remove the concept of system level images. This PR takes on that work.

david-crespo mentioned this pull request Aug 25, 2023

Silo-scoped IP pools #3926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement silo level images #2772

Implement silo level images #2772

zephraph commented Apr 5, 2023 •

edited

Loading

david-crespo Apr 12, 2023

david-crespo Apr 12, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

davepacheco Apr 18, 2023

zephraph Apr 18, 2023 •

edited

Loading

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023

zephraph Apr 17, 2023 •

edited

Loading

david-crespo left a comment

davepacheco Apr 19, 2023

zephraph Apr 19, 2023

zephraph Apr 19, 2023

Implement silo level images #2772

Implement silo level images #2772

Conversation

zephraph commented Apr 5, 2023 • 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

zephraph Apr 18, 2023 • 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

zephraph Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

david-crespo 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

zephraph commented Apr 5, 2023 •

edited

Loading

zephraph Apr 18, 2023 •

edited

Loading

zephraph Apr 17, 2023 •

edited

Loading