Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix port when fall back to http #43620

Closed
wants to merge 0 commits into from
Closed

fix port when fall back to http #43620

wants to merge 0 commits into from

Conversation

dameng324
Copy link
Contributor

When fallback to http,the port should fallback to 80, if don't the port will be 443.

Already tested on my local machine.

Fixed: #43618 , dotnet/sdk-container-builds#588

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Sep 23, 2024
@@ -80,6 +80,7 @@ private static void FallbackToHttp(HttpRequestMessage request)
{
var uriBuilder = new UriBuilder(request.RequestUri!);
uriBuilder.Scheme = "http";
uriBuilder.Port = 80;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will explicitly set the port to 80 (i.e. http://whatever:80) - is that what's intended, or in that scenario would you want the implied default port (i.e. http://whatever)? If the latter, then you can set it to -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review.

Maybe I should change the port only when the port is 443, as the user's registry may specified the port.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested, If user specified a http port. the publish command will result an error:

Uploading layer 'sha256:28d4f52c4a876bde6af159c1e53d159d94edfab99bfdc6eda8b53be82ee29eb1' to 'dockerhub.shengguanda.com:80'
C:\Program Files\dotnet\sdk\8.0.401\Containers\build\Microsoft.NET.Build.Containers.targets(242,5): error CONTAINER1013: Failed to push to the output registry: The SSL connection could not be established, see inner excepti
on. 

So there is no way to specify a http port. It is another issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should defer to the user's registry configuration here:

  • if the registry url had a port, then we should use it
  • if the registry url did not have a port, then we should set -1 here, because per the docs

If the Port property is set to a value of -1, this indicates that the default port value for the protocol scheme will be used to connect to the host.

So the 'default' value of the http protocol would be used in this case.

@dameng324
Copy link
Contributor Author

dameng324 commented Sep 24, 2024

        if(uriBuilder.Port == 443) // default https port, change to http default port
            uriBuilder.Port = 80;

if uri.Port is not the default https port, we should not change it.
if it is 443, we set it to the default http port 80, The hard code 80 should be fine the , as the port 443 is hard code too.

if the code is

        if(uriBuilder.Port == 443) // default https port, change to http default port
            uriBuilder.Port = -1;

It looks weird. 443 is related to 80, not -1;

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dameng324 please change to use the approach described here - explicitly setting to port 80 may not 'look weird' to you, but it is not a sufficient solution for all of the use cases identified.

@dameng324
Copy link
Contributor Author

I checked the FallbackToHttpMessageHandler._port and uriBuilder.Port, they are all 443, not -1;
So I can not check whether does user set the port or not simply, as user may set 443 port for http insecure registry.

The only way is introducing a registryName parameter for FallbackToHttpMessageHandler. It may too complex for this simple issue. So maybe check port == 443 is enough.

The change has been committed. Thanks for your patients, Please check again.

@dameng324
Copy link
Contributor Author

BTW, I'm glad to test the package before releasing to avoid this bug.

I add insecure registry support in #39840, but saddly it's broken when 8.0.40x released. To avoid this happenning again, I can test this feature before dotnet team releasing it if needed.

@baronfel
Copy link
Member

@dameng324 I think we have enough information already in the FallbackToHttpMessageHandler to solve this correctly.

When the original registry name (possibly including a port) is converted to a Uri, the Uri contains flags that let us know if the Uri's port is the 'default port' or not.

> let reg = "localhost:5000";;
val reg: string = "localhost:5000"

> let regUri = Uri $"https://{reg}";;
val regUri: Uri = https://localhost:5000/

> regUri.Port, regUri.IsDefaultPort;;
val it: int * bool = (5000, false)

Similarly, if the registry data doesn't contain port information, we get that data as well:

> let reg = "registry.mycorp.com";;
val reg: string = "registry.mycorp.com"

> let regUri = Uri $"https://{reg}";;
val regUri: Uri = https://registry.mycorp.com/

> regUri.Port, regUri.IsDefaultPort;;
val it: int * bool = (443, true)

This data persists across usage of the UriBuilder:

> let reg = "registry.mycorp.com";;
val reg: string = "registry.mycorp.com"

> let regUri = Uri $"https://{reg}";;
val regUri: Uri = https://registry.mycorp.com/

> regUri.Port, regUri.IsDefaultPort;;
val it: int * bool = (443, true)

> let builder = UriBuilder regUri;;
val builder: UriBuilder = https://registry.mycorp.com:443/

> builder.Scheme <- "http";;
val it: unit = ()

> builder.Uri.Port, builder.Uri.IsDefaultPort;;
val it: int * bool = (443, false)

> builder.Port <- -1;;
val it: unit = ()

> builder.Uri.Port, builder.Uri.IsDefaultPort;;
val it: int * bool = (80, true)

Note that in this last example, setting the Port to the -1 value changed the Port and IsDefaultPort properties on the Uri being constructed by the builder.

Therefore, I think in this Fallback message handler, we can accurately detect if a Port was explicitly set by the user's registry domain and only 'reset' the port for the new http scheme if the Port was not explicitly set.

@dameng324
Copy link
Contributor Author

@baronfel Thanks for your explain.

I changed uriBuilder.Port==443 to request.RequestUri.IsDefaultPort and tested it. It works well.

@baronfel baronfel requested a review from a team September 25, 2024 14:49
@baronfel
Copy link
Member

@dotnet/product-construction had to rerun this PR due to the sdk-unified-build leg, I was told to ping you as this happens.

@dameng324
Copy link
Contributor Author

@baronfel I cannot open your comment on my commit 3fc207f. So I reply here:

For some reason, I tested uriBuilder.Uri.IsDefaultPort, It is false in this case. Maybe the bug is caused by this.

@tmds
Copy link
Member

tmds commented Oct 1, 2024

we can accurately detect if a Port was explicitly set by the user's registry domain and only 'reset' the port for the new http scheme if the Port was not explicitly set.

I don't think Uri supports seeing the difference between https://host:443 and https://host which is what we would need here to be able to fall back to http://host:443 for the first, and http://host for the latter.

I changed uriBuilder.Port==443 to request.RequestUri.IsDefaultPort and tested it. It works well.

These are functionally the same.

@baronfel
Copy link
Member

baronfel commented Oct 1, 2024

@tmds this is a great and very depressing point.

@tmds
Copy link
Member

tmds commented Oct 1, 2024

Actually, Uri.OriginalString will return the original string!

@tmds
Copy link
Member

tmds commented Oct 1, 2024

Actually, Uri.OriginalString will return the original string!

We also have the registryName nearby when we create the FallbackToHttpMessageHandler.

@tmds
Copy link
Member

tmds commented Oct 2, 2024

@dameng324 you should be able to Uri.OriginalString/registryName to derive the needed info and pass it to the FallbackToHttpMessageHandler constructor, like a fallbackPort argument.

@baronfel
Copy link
Member

baronfel commented Oct 7, 2024

I'm a little unclear on where we are with this PR - @dameng324 can you summarize quickly what you see any remaining issues being (if any?)

@dameng324
Copy link
Contributor Author

@baronfel this issue has been fixed except user set a 443 port insecure registry.

The solution provided by @tmds to fix the exception is good,but I am on vacation these days. If you need me to do this work, I can start tomorrow.

@baronfel
Copy link
Member

baronfel commented Oct 7, 2024

@dameng324 no rush, enjoy your vacation! We're locking down the SDK for 9.0.1xx quite soon, but this fix could be backported to the December servicing release as soon as it's merged, no problem at all.

@dameng324
Copy link
Contributor Author

@baronfel Hello, I rebase my code, and create a new PR(#44050) for this issue, would you have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default Uri is wrong when dotnet publish docker image to insecure registry
5 participants