OpenStack: support "file" scheme for custom os image urls #3273
Conversation
openshift-ci-robot
added
the
size/M
Fedosin
changed the title
|
Can I still refrence the file with SHA Checksum so the installer checks the integrity of the file provided ? file:///path/to/file?sha256=xxxxx ? |
|
@rheinzma unfortunately no, but I think checksum validation doesn't make sense here, since you normally trust files on your local file system. If necessary you can check it before the installation with |
|
/retest |
|
/retest |
|
/uncc @jstuever |
openshift-ci-robot
requested review from
iamemilio
and removed request for
jstuever
|
/retest |
|
/lgtm |
|
/lgtm |
pkg/rhcos/openstack.go
Outdated
| _, err := url.ParseRequestURI(rhcosImage) | ||
| // We support only 'http(s)' and 'file' schemes, URLs with other schemes are considered as Glance image | ||
| // names. | ||
| url, err := url.Parse(rhcosImage) | ||
| if err != nil { | ||
| return rhcosImage, false | ||
| } | ||
|
|
||
| if url.Scheme != "http" && url.Scheme != "https" && url.Scheme != "file" { | ||
| return rhcosImage, false | ||
| } | ||
|
|
There was a problem hiding this comment.
technically one could provide a ftp URI and that would still work, but now providing something like that means it's going to try to assume that as glance image id.
I think there needs to be better validation here in terms of whats allowed and what's not.
There was a problem hiding this comment.
Now the installation fails if the user provides an ftp or any other non-http(s) url, because we will try to download the file using http protocol. This is wrong behavior.
With this patch I limit the supported schemes to http(s) and file, as it is said in the documentation. URLs with other schemes will be considered as Glance image names.
There was a problem hiding this comment.
but this function will return result for ftp as if that's an glance image though still.. will is incorrect.
we should proabably have validation to restrict the scheme to these values or empty for glance image.
There was a problem hiding this comment.
In accordance with the documentation and as originally planned:
- A valid
http(s)URL translates to the address from where we're going to download the file. - A valid
fileURL translates to the image file location on the local file system. - All other strings are translated to the Glance image name, even if they are
ftpors3URLs.
As an alternative solution, I can offer to limit the schemes and return an error if the scheme is not http or file. That is:
- A valid
http(s)URL translates to the address from where we're going to download the file. - A valid
fileURL translates to the image file location on the local file system. - Any other URL-like string leads to the error: "Unsupported URL scheme".
- All other strings are translated to the Glance image name.
Fedosin
force-pushed
the
file_locations
branch
2 times, most recently
from
02a5be4
to
a252113
Compare
|
/test e2e-openstack |
|
/retest |
Now we support http(s) schemes only, but for disconnected installs it's very convenient to specify the local file path to the image file. This commit adds "file" scheme support, so users can set the location as "file:///path/to/image".
| } | ||
| case "file": | ||
| localFilePath = filepath.FromSlash(url.Path) | ||
| default: |
There was a problem hiding this comment.
Personally I would have liked it to be part of validation of the install-config.yaml
But this is fine too.
/approve
openshift-ci-robot
added
the
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-openstack |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@Fedosin: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Now we support http(s) schemes only, but for disconnected installs it's very convenient to specify the local file path to the image file.
This commit adds "file" scheme support, so users can set the location as "file:///path/to/image".