Skip to content

fix port error when fallback to http#44050

Merged
baronfel merged 4 commits intodotnet:mainfrom
dameng324:main
Oct 16, 2024
Merged

fix port error when fallback to http#44050
baronfel merged 4 commits intodotnet:mainfrom
dameng324:main

Conversation

@dameng324
Copy link
Copy Markdown
Contributor

@dameng324 dameng324 commented Oct 10, 2024

As discussed in previous PR: #43620,

I pass the original registryName to FallbackToHttpMessageHandler and check it if contains a port or not. If not, set the uriBuilder.Port = -1.

For summary. It should behave like this:

  • If user set an insecure registry without port, It will fallback to http default port 80.
  • If user set an insecure registry with a port, even 443(the default port of https), It will maintain the orginal port.

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

@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Oct 10, 2024
Copy link
Copy Markdown
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.

Thanks for the rebase - this seems very clear. Do you have any idea how this could be tested via unit tests? Maybe via the pattern that @tmds did for this auth handler PR - a mock HttpHandler?

Comment thread src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs Outdated
Comment thread src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs Outdated
@dameng324 dameng324 requested a review from a team as a code owner October 10, 2024 13:21
Copy link
Copy Markdown
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.

Thanks for adding the tests!

@dameng324
Copy link
Copy Markdown
Contributor Author

@baronfel the work is done,can you spare some time to check?

@baronfel
Copy link
Copy Markdown
Member

This LGTM, thanks @dameng324! I kicked CI to see if those two failing legs can get green. If so, we'll merge and I'll backport the fix to 8.0.400 and 9.0.100 and work on getting approval for those.

@baronfel
Copy link
Copy Markdown
Member

/backport to release/8.0.4xx

@baronfel
Copy link
Copy Markdown
Member

/backport to release/9.0.1xx

@github-actions
Copy link
Copy Markdown
Contributor

Started backporting to release/8.0.4xx: https://github.com/dotnet/sdk/actions/runs/11370820390

@github-actions
Copy link
Copy Markdown
Contributor

Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/11370821680

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 Area-Infrastructure 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

4 participants