Allow Community images #8
Conversation
|
This sounds good for situations in which a pool uses image names instead of IDs. I think that if we add the extra step of sending a list of allowed owners, it's probably easier to just use image IDs instead of image names. The image ID will only ever match one image. I have nothing against adding this. I think you need to rebase. Some commits here seem to be shared with the PR I just merged. |
client/client.go
Outdated
| }); err != nil { | ||
| return nil, fmt.Errorf("failed to get image with name or id %s: %w", nameOrID, err) | ||
| } | ||
| // try again but look for community images |
There was a problem hiding this comment.
I think there is no good way to search for public AND `community' visibility. so we have to duplicate the code ..
but we could also simply use images.ImageVisibility("all") ?! What do you think ?
There was a problem hiding this comment.
I think that's a great idea. Better still, we could make this a flag in the config/extra-specs search-all-images or something. By default limit to public images, and we switch to all if the flag is set to true. We must take care because (I think) if we mistakingly use credentials for an admin user, all can truly include all images.
There was a problem hiding this comment.
Okay .. I'll change this.
The visibility thing is really tricky .. you have public, private, shared and community .. but the visibility is only important for listing the images not necessarily for using. E.g. you can boot from any community image .. and you can also boot from a shared image which you didn't accept (yet) or which you explicitly rejected.
Hence the allowed owners check is very crucial
There was a problem hiding this comment.
visibility can now be explicitly set in config and/or extra specs
.. this is exactly our use-case .. we have multiple pools with the same image, but as we have many openstacks we have the same image with multiple different ids. We really would stick to image names as ids are not human friendly and having the same image with different ids can drive you crazy. :) |
Ahh! That makes perfect sense! |
provider/provider.go
Outdated
| } | ||
|
|
||
| // verify owner | ||
| if spec.AllowedImageOwners != nil && len(spec.AllowedImageOwners) > 0 { |
There was a problem hiding this comment.
I think we can drop the nil check here. The nil value of a slice is an empty slice, even if we explicitly set it to nil. So len() should be fine.
| m.UseConfigDrive = *spec.UseConfigDrive | ||
| } | ||
|
|
||
| if spec.AllowedImageOwners != nil { |
There was a problem hiding this comment.
It should be fine to remove the if and just assign the value.
Wait, this is also a config value. Forget what I said.
|
This looks great. Let me know when you're done and I can merge it. |
I think you could have a look again now .. I also started adding some minimal unit tests. |
|
Lgtm. Merging. We can iterate. |
Openstack offers multiple visibility levels. These level define if and how images can be shared between openstack projects.
This PR allows to also use images to create instances if visibility is
community.Unfortunately, there seems to be no
VisibilityAlloption, hence we search for public images first and then we search for community images in a 2nd step.Consuming shared images may introduce security issues. For that reason we add a
AllowedImageOwnersconfig flag. If it is unset we allow all images, but one can also define a listallowed ownersto protect against images of untrusted origin.