Improve validation of MigCluster Registry URL field

[pranavgaikwad]

Is your feature request related to a problem? Please describe.
Sometimes image migration may end up failing because of an extra trailing slash present in the URI of registry provided through MigCluster resource. We need to improve the backend validation to filter such bad inputs.

[sseago]

The format should just be host[:port] (with port optional). Should be pretty easy to add validation to complain if it's incorrect. Seems that the user confusion about this being a URL rather than a host+port remains.

[vlours]

Hey guys,

I will be interested to work on this issue. But I'm wondering if the issue is not related to some underline tools.
I notice the same behavior replicating the issue with skopeo:

### Test with external image:
# Wrong syntax with'//', it should display the "invalid reference format" error message
$ skopeo inspect docker://registry.fedoraproject.org//fedora:latest
# Right syntax with only one '/', it should only display the name of the image
$ skopeo inspect docker://registry.fedoraproject.org/fedora:latest | jq -r '.Name'

So do we need to put some validation/auto-fix steps in the code? Or would it be better to review deeper in the code to check which part of the process is failing due to the extra trailing slash present in the URI?

[sseago]

I'm not sure it matters which part is failing -- a trailing slash represents invalid input and the MigCluster validation should catch it. The registry field (it's not a URL field -- the github issue title is wrong and reflects the same confusion I referenced in my comment above) is a hostname with optional port. A slash is not part of a hostname or a port. The dockerImageReference is generated by combining the registry host with the namespace, name, and tag of the image.

[vlours]

I've prepared a fix:
https://github.com/konveyor/mig-controller/compare/master...vlours:issue-1154?expand=1#diff-67bd6592e42c39737a3840d985861eb133e8a8d9869ac3d1a6e5185b66a6de61

As the GetRegistryPath function is already splitting the field without a warning message, I assumed that we should remove the trailer the same way.
To which branch should I make my PR?

Cheers,

[pranavgaikwad]

@vlours Thank you for the fix. What you did in your fix makes sense and solves this particular issue. It would be great if we could also add an additional validation in MigCluster validation to address what Scott mentioned. I didn't know about the host[:port] validation. If you could address that in the same PR, that would be great. Totally fine if you only added this fix. All PRs go against the master branch.

[vlours]

Hi @pranavgaikwad,

I'm not sure how the validation is working.

Otherwise, please let me know which branch I should create the PR to?
Should I create the PR for a specific release/sprint branch, or directly to the master branch?

Cheers,

[sseago]

@pranavgaikwad Yes, it looks like we previously decided to strip off invalid input from the user instead of validating. I think as long as we're accepting invalid input before the host[:port], then stripping invalid input after is consistent here. If we're going to validate and complain about the trailing slash, we should probably also complain about "https://" in the field.

[pranavgaikwad]

@vlours Please create PR against master branch

[vlours]

PR created: #1158

[pranavgaikwad]

@vlours thank you for your contribution. Your PR is merged, I am still keeping this issue open for the other validation that we wanted to add.