cloudapi: make ImageRequests tied to their UploadOptions #3018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, one could pass GCPUploadOptions to an aws image request because the spec technically allows that. I haven't found any validation in our code, so I presume that the result would be pretty undefined. A real-life example: If someone wanted an aws image but didn't specify `share_with_accounts`, this is valid according to AWSS3UploadOptions. Composer doesn't perform any additional checks, so it would just proceed to building such an image, and uploading it to our account where no one can use access it. This commit makes ImageRequest be one of AWSEC2ImageRequest, AzureImageRequest etc. XYZImageRequest always requires its `upload_options` to be XYZUploadOptions. As all of these ImageRequests contain some common fields, I created AbstractImageRequest to deduplicate them. One thing that I don't like is that image types called AWSEC2ImageRequestImageTypeAws are long, and in most cases we need to cast them to `string`, which loses a bit of type safety. What I like is that the API spec is now more self-explanatory and helpful for composer's users. I also had to adjust several tests that build aws images without setting `share_with_accounts`. This breaks backward compatibility but the only broken requests are non-sense ones. Signed-off-by: Ondřej Budai <[email protected]>
thozza
approved these changes
Sep 23, 2022
There was a problem hiding this comment.
Overall, this IMO makes sense and looks good 👍 .
I don't like the many occurrences of casting to string, but I don't have any better suggestion since the code is generated from the API spec. I also think that the code may get uglier if we ever decide to allow uploading images to S3, in addition to their native upload target, but that is something for our future selfs... 😅
Comment on lines
+839
to
+840
| - edge-container | ||
| - iot-container |
There was a problem hiding this comment.
Are you sure that nobody relies on uploading these also to S3? 🤔 Just checking...
thozza
added a commit
to thozza/koji-osbuild
that referenced
this pull request
Nov 2, 2022
It turned out, that the upload options for AWS EC2 or GCP with just the required properties specified, would match both schemas. This is causing the validation fail with `oneOf` used for `upload_options`. This will be fixed in osbuild-composer PR#3018. However, we can't use the same approach for koji-osbuild, while keeping the schema backward compatible and sane. Another discussed option would be to define `upload_options` as an empty object with `additionalProperties` set to `True`. This would effectively mean no validation of `upload_options`. None of the plugins actually modify the `upload_options` in any way. It is passed as provided to `osbuild-composer`. I think that the change in this commit is a compromise. The validation of the `upload_options` schema is kept, but it is relaxed to `anyOf`. [1] osbuild/osbuild-composer#3018 Signed-off-by: Tomáš Hozza <[email protected]>
thozza
added a commit
to thozza/koji-osbuild
that referenced
this pull request
Nov 11, 2022
It turned out, that the upload options for AWS EC2 or GCP with just the required properties specified, would match both schemas. This is causing the validation fail with `oneOf` used for `upload_options`. This will be fixed in osbuild-composer PR#3018. However, we can't use the same approach for koji-osbuild, while keeping the schema backward compatible and sane. Another discussed option would be to define `upload_options` as an empty object with `additionalProperties` set to `True`. This would effectively mean no validation of `upload_options`. None of the plugins actually modify the `upload_options` in any way. It is passed as provided to `osbuild-composer`. I think that the change in this commit is a compromise. The validation of the `upload_options` schema is kept, but it is relaxed to `anyOf`. [1] osbuild/osbuild-composer#3018 Signed-off-by: Tomáš Hozza <[email protected]>
thozza
added a commit
to thozza/koji-osbuild
that referenced
this pull request
Nov 15, 2022
It turned out, that the upload options for AWS EC2 or GCP with just the required properties specified, would match both schemas. This is causing the validation fail with `oneOf` used for `upload_options`. This will be fixed in osbuild-composer PR#3018. However, we can't use the same approach for koji-osbuild, while keeping the schema backward compatible and sane. Another discussed option would be to define `upload_options` as an empty object with `additionalProperties` set to `True`. This would effectively mean no validation of `upload_options`. None of the plugins actually modify the `upload_options` in any way. It is passed as provided to `osbuild-composer`. I think that the change in this commit is a compromise. The validation of the `upload_options` schema is kept, but it is relaxed to `anyOf`. [1] osbuild/osbuild-composer#3018 Signed-off-by: Tomáš Hozza <[email protected]>
ochosi
pushed a commit
to thozza/koji-osbuild
that referenced
this pull request
Nov 17, 2022
It turned out, that the upload options for AWS EC2 or GCP with just the required properties specified, would match both schemas. This is causing the validation fail with `oneOf` used for `upload_options`. This will be fixed in osbuild-composer PR#3018. However, we can't use the same approach for koji-osbuild, while keeping the schema backward compatible and sane. Another discussed option would be to define `upload_options` as an empty object with `additionalProperties` set to `True`. This would effectively mean no validation of `upload_options`. None of the plugins actually modify the `upload_options` in any way. It is passed as provided to `osbuild-composer`. I think that the change in this commit is a compromise. The validation of the `upload_options` schema is kept, but it is relaxed to `anyOf`. [1] osbuild/osbuild-composer#3018 Signed-off-by: Tomáš Hozza <[email protected]>
ondrejbudai
pushed a commit
to osbuild/koji-osbuild
that referenced
this pull request
Nov 21, 2022
It turned out, that the upload options for AWS EC2 or GCP with just the required properties specified, would match both schemas. This is causing the validation fail with `oneOf` used for `upload_options`. This will be fixed in osbuild-composer PR#3018. However, we can't use the same approach for koji-osbuild, while keeping the schema backward compatible and sane. Another discussed option would be to define `upload_options` as an empty object with `additionalProperties` set to `True`. This would effectively mean no validation of `upload_options`. None of the plugins actually modify the `upload_options` in any way. It is passed as provided to `osbuild-composer`. I think that the change in this commit is a compromise. The validation of the `upload_options` schema is kept, but it is relaxed to `anyOf`. [1] osbuild/osbuild-composer#3018 Signed-off-by: Tomáš Hozza <[email protected]>
|
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 30+7 days with no activity. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, one could pass GCPUploadOptions to an aws image request because the spec technically allows that. I haven't found any validation in our code, so I presume that the result would be pretty undefined.
A real-life example: If someone wanted an aws image but didn't specify
share_with_accounts, this is valid according to AWSS3UploadOptions. Composer doesn't perform any additional checks, so it would just proceed to building such an image, and uploading it to our account where no one can use access it.This commit makes ImageRequest be one of AWSEC2ImageRequest, AzureImageRequest etc. XYZImageRequest always requires its
upload_optionsto be XYZUploadOptions. As all of these ImageRequests contain some common fields, I created AbstractImageRequest to deduplicate them.One thing that I don't like is that image types called AWSEC2ImageRequestImageTypeAws are long, and in most cases we need to cast them to
string, which loses a bit of type safety.What I like is that the API spec is now more self-explanatory and helpful for composer's users.
I also had to adjust several tests that build aws images without setting
share_with_accounts.This breaks backward compatibility but the only broken requests are non-sense ones.
This is actually a prequel to #3006. A suggestion from @thozza was to make AWSEC2UploadOptions specified like this:
The issue is that, the spec validator will happily validate
AWSEC2UploadOptionswithout neither of these fields set using theAWSS3UploadOptionsspec. I did not like that, so I began thinking how to make the schema more error-proof and came out with this.I was thinking if this is fine for future extensions: One extension that I can think of is to add the ability to upload any image to an S3-like storage. I think this is fine in the spec because we just make all image requests uploadable to both their target and to S3. A bit of code-duplication can happen but with
oneOfandallOf, we should be just fine.Signed-off-by: Ondřej Budai [email protected]
This pull request includes: