manifest/subscription: support scheme specifier for subscription proxy url #2115
Conversation
supakeen
left a comment
supakeen
left a comment
There was a problem hiding this comment.
Note that this will not support just the proxy-host:8080 format, as it seems go doesn't have a great way to parse both easily (without doing some ugly string.contains("://")
This is fine but this value may come from blueprints that users already have lying around; it should be very clear why the URI is (now) invalid so perhaps change the message to include that it needs to be a 'compliant' URI or include the error which would likely also say something like 'missing protocol'.
ochosi
changed the title
There's an alternative we can consider, which is leaving the code as is and changing the way we set the proxy in the service. See HMS-9612 and the linked MR in app-interface. |
Hm, the proxy isn't part of the blueprint anywhere I think. It's set in image options, and the only active codepath is via the cloudapi currently. But I agree, let's include the error. |
There was a problem hiding this comment.
Thanks! Could you also rewrite the commit message to follow https://osbuild.org/docs/developer-guide/general/workflow/ 🥺
Just like manifest/subscription: …
There was a problem hiding this comment.
Thanks for fixing this.
This is a breaking change that will change behavior of ibcli registration. I suggest that a fallback mechanism of hostname:port is still a valid option. It is not currently the case I believe. It would be also quite nice to have this because the naming is pretty unclear and people who knows that rhsm config actually take hostname and not URL might accidentally use it in the future as well. The parsing code could actually form a nice getter on the type.
If we decide to go forward with this, please add one test case that also ensures this behavior is not removed in the future.
jlsherrill
changed the title
|
Updated it to support both the old format and the new format. This will ensure that if we ever need to switch to an https proxy in the future, we won't have any surprises. |
There was a problem hiding this comment.
Great, I think this nicely solves the problem. By the way this is a common issues across multiple packages I think I have seen the same issue with squid configuration years ago. Now that we support both, you do not need to even refactor the Proxy to ProxyURL since it actually supports both.
supakeen
left a comment
supakeen
left a comment
There was a problem hiding this comment.
We could be stricter in the non-schemaed case but it's fine.
lzap
left a comment
lzap
left a comment
There was a problem hiding this comment.
That is right, proxies typically do not hang out on 80/443 and if port was missing, we should probably either error out or use 3124 (or whatever is the squid default one I forgot). Erroring out would be likely more appropriate I think cos these ports are usually all over the place.
| splitUrl := strings.Split(proxyUrl, ":") | ||
| scheme = "http" // default to http | ||
| hostname = splitUrl[0] | ||
| if len(splitUrl) > 1 { | ||
| port = splitUrl[1] | ||
| } |
There was a problem hiding this comment.
Should we return an error here if len(splitURL) > 2?
There was a problem hiding this comment.
And what about IPv6 address literals? :>
There was a problem hiding this comment.
Replace this text with an IPv6 joke.
Yeah perhaps use URL parser for this case, I used this technique once. Just prepend http:// and let the stdlib to do heavylifting.
7 participants
and properly set it in subscription-manager (Fixing HMS-9612)
Currently in insights, we are using a full url (http://proxy-host:8080), but here we expect just a hostname and port (proxy-host:8080). This results in things not being set properly in sub-man. This fixes this by fully parsing the URL. Note that this will not support just the proxy-host:8080 format, as it seems go doesn't have a great way to parse both easily (without doing some ugly string.contains("://")