Skip to content

Update HTTP fallback to better account for TLS timeout and previous attempts#10109

Merged
mxpv merged 2 commits intocontainerd:mainfrom
dmcgowan:fix-fallback-explicit-tls
Apr 23, 2024
Merged

Update HTTP fallback to better account for TLS timeout and previous attempts#10109
mxpv merged 2 commits intocontainerd:mainfrom
dmcgowan:fix-fallback-explicit-tls

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

A few relevant changes here to make the HTTP fallback more robust:

  • Handle TLS handshake timeout, as some HTTP servers may just not return anything to the TLS handshake attempt
  • If a previous fallback has already occurred with the same host using the same transport, immediately fallback to HTTP and do not copy the body
  • Avoid returning an empty TLS configuration when no TLS has been configured in ctr, this avoids a TLS configuration always set and forcing fallback when using ctr

Fixes #10014

@dmcgowan dmcgowan added cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Apr 23, 2024
@dmcgowan dmcgowan force-pushed the fix-fallback-explicit-tls branch from 4036eee to 501cb67 Compare April 23, 2024 01:37
@dmcgowan dmcgowan added the status/needs-major-release Can only be accepted into a major release label Apr 23, 2024
@dmcgowan dmcgowan added this to the 2.0 milestone Apr 23, 2024
@dmcgowan dmcgowan force-pushed the fix-fallback-explicit-tls branch from 501cb67 to 2a4c8e4 Compare April 23, 2024 01:48
@dmcgowan dmcgowan force-pushed the fix-fallback-explicit-tls branch from 2a4c8e4 to 5e470e1 Compare April 23, 2024 01:53
@dmcgowan
Copy link
Copy Markdown
Member Author

When backported to 1.6 and 1.7 I'll just keep the new function and the old type, just mark the old struct as deprecated. There is no need to keep the old struct since we will get this in before 2.0.

Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

return
}
go func() {
time.Sleep(time.Second)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hopefully, it's not flaky in CI env 😂 I mean that it maybe close before received timeout.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We should probably test for connection closed as well 😆

@mxpv mxpv added this pull request to the merge queue Apr 23, 2024
Merged via the queue into containerd:main with commit 444679c Apr 23, 2024
@dmcgowan dmcgowan added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch size/L status/needs-major-release Can only be accepted into a major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

localhost registry using nodeport gets upgraded to HTTPS and HTTP fallback does not work as expected

4 participants