Improve URL format validation

[GeekArthur]

Signed-off-by: jingfu wang jingfu.j.wang@ibm.com

What type of PR is this?
/kind bug
/area devfile

What does does this PR do / why we need it:
This PR adds more requirements to URL format validation so that gate more invalid registry URLs

Which issue(s) this PR fixes:

Fixes #3451

PR acceptance criteria:

• Unit test

• Integration test

• I have read the test guidelines

How to test changes / Special notes to the reviewer:
Run odo registry add test <invalid URL>, for example run odo registry add test http://$$$$,,,,,@hdjh.hcgcyd

[GeekArthur]

/assign

[codecov]

Codecov Report

Merging #3900 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3900      +/-   ##
==========================================
- Coverage   44.55%   44.51%   -0.05%     
==========================================
  Files         142      142              
  Lines       13863    13885      +22     
==========================================
+ Hits         6177     6181       +4     
- Misses       7094     7112      +18     
  Partials      592      592              

Impacted Files	Coverage Δ
pkg/util/util.go	56.28% <100.00%> (+0.13%)	⬆️
pkg/devfile/parser/context/context.go	26.47% <0.00%> (-4.57%)	⬇️
pkg/occlient/utils.go	7.24% <0.00%> (-0.11%)	⬇️
pkg/devfile/parser/writer.go	60.00% <0.00%> (ø)
pkg/odo/cli/utils/convert.go	0.00% <0.00%> (ø)
pkg/sync/adapter.go	77.44% <0.00%> (+1.50%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6be3d28...f305bc3. Read the comment docs.

[dharmit]

@GeekArthur I'm able to add invalid registries like below:

$ odo registry list
NAME                       URL                                                                SECURE
test3                      https://)(url                                                      No
test2                      noprotocol://noregistry                                            No
test                       https://nosuchurlblah                                              No

While the last one could be possible in an internal environment (and hence a valid URL for some environment), the other two seem far-fetched and I tried them just for the sake of breaking it. I don't claim to know the definition of 100% correct URL/URI but unless user uses their commonsense, there's going to be ways break this.

However, while looking around, I did see examples where ParseRequestURI is used as @adisky suggested in her review. If you could make the change or comment why it's not reasonable to do that, this PR looks good to approve to me. 👍

[mohammedzee1000]

@GeekArthur I'm able to add invalid registries like below:

$ odo registry list
NAME                       URL                                                                SECURE
test3                      https://)(url                                                      No
test2                      noprotocol://noregistry                                            No
test                       https://nosuchurlblah                                              No

While the last one could be possible in an internal environment (and hence a valid URL for some environment), the other two seem far-fetched and I tried them just for the sake of breaking it. I don't claim to know the definition of 100% correct URL/URI but unless user uses their commonsense, there's going to be ways break this.

The only way you can validate the 3rd case, is to see if it is reachable or not, as technically, it is a valid url, just one that may not exist or even be reachable. Do we want to do this?

However, while looking around, I did see examples where ParseRequestURI is used as @adisky suggested in her review. If you could make the change or comment why it's not reasonable to do that, this PR looks good to approve to me.

+1

[GeekArthur]

@GeekArthur I'm able to add invalid registries like below:

$ odo registry list
NAME                       URL                                                                SECURE
test3                      https://)(url                                                      No
test2                      noprotocol://noregistry                                            No
test                       https://nosuchurlblah                                              No

While the last one could be possible in an internal environment (and hence a valid URL for some environment), the other two seem far-fetched and I tried them just for the sake of breaking it. I don't claim to know the definition of 100% correct URL/URI but unless user uses their commonsense, there's going to be ways break this.

However, while looking around, I did see examples where ParseRequestURI is used as @adisky suggested in her review. If you could make the change or comment why it's not reasonable to do that, this PR looks good to approve to me. 👍

We can use url.ParseRequestURI as url.Parse is loose version of URL parsing so that we can add customized URL parsing code, but given currently we only validate scheme and host components of the URL so we can use url.ParseRequestURI + customized host validation (url.ParseRequestURI doesn't fully cover host validation).

@dharmit Regarding the three cases you tested, https://)(url and https://nosuchurlblah should be valid URL as they follow the RFC 3986 (https://www.ietf.org/rfc/rfc3986.txt, FYI definition of 100% correct URL/URI is there but TL;DR), which is the latest global URL/URI standard. I know this one https://)(url is pretty tricky but if the user really wants to name the web host like that to use ( or ), I believe it works as those characters are not reserved characters for host component of URL according to RFC 3986. For noprotocol://noregistry, I don't think currently there is a supported scheme list so we just use the scheme validation from url.ParseRequestURI.

@mohammedzee1000

The only way you can validate the 3rd case, is to see if it is reachable or not, as technically, it is a valid url, just one that may not exist or even be reachable. Do we want to do this?

We decide to follow the design of yum-config-manager, which we don't validate the accessibility when adding the registry, we only validate the basic URL format on adding URL stage. So you are right, it's valid URL.

[GeekArthur]

@adisky @dharmit @mohammedzee1000 Comments addressed, please review again, thanks.

[GeekArthur]

/retest

[GeekArthur]

/retest

[GeekArthur]

/retest

[GeekArthur]

@dharmit All tests passed, can you review again?

[adisky]

/lgtm

[GeekArthur]

This PR also needs an approved label, @kadel can you help add that?

[kadel]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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:

• OWNERS [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment