Skip to content

Conversation

jmpesp
Copy link
Contributor

@jmpesp jmpesp commented May 19, 2022

Global images should have version, according to RFD 4, plus OS
distribution, which is added with this commit. This allows for bucketing
what global images are available to use for booting. This commit
separates the param structs that are used to create global images from
project images.

This commit also adds a new error type called InterregnumError. It's
meant to be returned in cases where different Nexus are operating with
different types. The example in this PR is the difference between the
database enum variants and the external param enum variants. FreeBSD is
provided as an example where there's a database variant but not an
external enum variant. Note we also don't have FreeBSD cloud images
wired up yet for testing.

Note that ImageSource was also changed here to remove content from the
serde block, and change the enum variants to use field names. This was
done so that the JSON looked better.

Global images should have version, according to RFD 4, plus OS
distribution, which is added with this commit. This allows for bucketing
what global images are available to use for booting. This commit
separates the param structs that are used to create global images from
project images.

This commit also adds a new error type called InterregnumError. It's
meant to be returned in cases where different Nexus are operating with
different types. The example in this PR is the difference between the
database enum variants and the external param enum variants. FreeBSD is
provided as an example where there's a database variant but not an
external enum variant. Note we also don't have FreeBSD cloud images
wired up yet for testing.

Note that ImageSource was also changed here to remove `content` from the
serde block, and change the enum variants to use field names. This was
done so that the JSON looked better.
@david-crespo
Copy link
Contributor

How do we actually know the type mismatch is caused by an "interregnum" and not the user just passing an enum variant that doesn't exist?

Comment on lines 492 to 500
CREATE TYPE omicron.public.distribution AS ENUM (
'alpine',
'debian',
'ubuntu',
'rocky',
'centos',
'fedora',
'freebsd'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some hesitation about constraining a list of distributions in the database. I can see us having a table of distributions where we provide some defaults, but I don't think we generally want to be tasked with updating this everytime someone requests a new distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone's requesting a new Oxide supplied OS distribution to be packaged and distributed, I'd want that to be an explicit add. I'd rather that than a freeform string here.

If the customer wants to upload their own custom image, that is another deal entirely. Now that I'm typing this out, we're making a distinction between Oxide supplied global images and Customer created project images, but I think there's a fit for Customer created global images as well (a Debian base image that everyone should use, for example).

Choose a reason for hiding this comment

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

Why do we need to care about the difference between a customer created image and one that Oxide has created?

From an end user perspective, our specific Oxide things won't be particularly important versus whatever the project has standardized on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, let me write this differently - I'm not saying that Nexus has to care, or that there should be a distinction between them, but that we should write our authz policy to

  1. allow customers to POST to /images to create their own global base images
  2. not allow customers to delete our Oxide supplied global base images

Choose a reason for hiding this comment

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

I don't get why (2) should be true. Oxide supplied global base images don't come with the rack at all. Maybe one will be there to speed up the provisioning process; however, these are just something that we should think of as being able to import into the environment the same way that operators did. If an operator imported say alpine version x and ubuntu version y and then import version x+1 and y+1, they should certainly be able to remove alpine x and unbutu y from their rack/az. Similarly if they mis-import something they should be able to delete it as well. It seems weird to say oh, this came from Oxide, this space for something you never want to use or you never want projects to use shouldn't be reclaimable. Can you describe a bit more about why someone shouldn't be able to delete images they import from Oxide from their environment?

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 agree with the arguments here about why (2) shouldn't be true.

I'm imaging the root of the distinction between Oxide supplied images and user images being that Oxide images will be validated in some way that user images will not be, through something like a signature. Oxide images may also come to the rack through a different method, like from the update system instead of being uploaded by the user for example. I'm thinking that a distinction has to exist though maybe not applicable to authz policy - users could delete Oxide supplied global images or their own.

Choose a reason for hiding this comment

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

While it's possible they could come through the update system, I'm not sure that is what we want to pursue or not. It really ties into imho about what visibility to projects should be for images and related. As someone running a project, I may way to see the latest Oxide image and switch to that or I may want to coordinate that and use that to build a newer base image.

I guess I'd try to ask the question of how long can we go with whether Oxide supplied, some 3rd party, or user-generated being the same rather than special-casing us. I guess I'm not seeing where things like alternative update paths, signing (which if something we want, is something we'd want for 3rd parties, etc. to be able to do), or other things means that Oxide produced images should be unique in some regard in the rack. Let me know if it's useful to talk live about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

we really need Oxide supplied defaults. Operators will want to get a rack and have it usable to deploy VMs from day one. Just like other cloud providers provide base images for ubuntu, debian, common things, we too will do the same. The ownus should not be on the end user as that is a huge amount of time spent on their part to get images then before they can even deploy a VM.

Choose a reason for hiding this comment

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

I'm not saying we shouldn't have defaults here that folks can pull and use. The question I'm asking is should they actually be treated differently and be special from a database perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't have an opinion on that :)

@jmpesp
Copy link
Contributor Author

jmpesp commented May 19, 2022

How do we actually know the type mismatch is caused by an "interregnum" and not the user just passing an enum variant that doesn't exist?

My understanding is that a user can't pass in an enum variant that doesn't exist, that POST body would fail deserialization.

@david-crespo
Copy link
Contributor

Ah I see. It does exist in the application, just not in the database, which lets us safely assume it's because of a version mismatch.

Copy link
Contributor

@zephraph zephraph left a comment

Choose a reason for hiding this comment

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

This looks like a great starting point. I think there's still going to be some things to consider as the API develops, but I'd like to be able to go ahead and get started on wiring it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants