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.

Sign up for GitHub

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

Jump to bottom

[release/1.6] Fix ambiguous tls fallback #9300

Merged
merged 2 commits into from Oct 29, 2023
Merged

[release/1.6] Fix ambiguous tls fallback #9300

merged 2 commits into from Oct 29, 2023

Conversation

dmcgowan
Copy link
Member

Backport

@dmcgowan
Avoid TLS fallback when protocol is not ambiguous
51df21d 
The TLS fallback should only be used when the protocol is ambiguous due
to provided TLS configurations and defaulting to http. Do not add TLS
configurations when defaulting to http. When the port is 80 or will be
defaulted to 80, there is no protocol ambiguity and TLS fallback should
not be used.

Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit d48ceb6)
Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan
Check scheme and host of request on push redirect
5dd6430 
When the HTTP fallback is used, the scheme changes from HTTPS to HTTP
which can cause a mismatch on redirect, causing the authorizer to get
stripped out. Since the redirect host must match the redirect host in
this case, credentials are only sent to the same origin host that
returned the redirect.

This fixes an issue for a push getting a 401 unauthorized on the PUT
request even though credentials are available.

Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit 466ee87)
Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan dmcgowan changed the base branch from main to release/1.6 October 26, 2023 04:49
@dmcgowan dmcgowan closed this Oct 26, 2023
@dmcgowan dmcgowan reopened this Oct 26, 2023
@containerd containerd deleted a comment from k8s-ci-robot Oct 26, 2023
@samuelkarp
Copy link
Member

/retest

@samuelkarp
Copy link
Member

k8s-ci-robot now reruns GitHub Actions with the retest command? That's new. I just wanted the one aborted prow job to rerun...

/test pull-containerd-node-e2e

@k8s-ci-robot
Copy link

@samuelkarp: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-containerd-build
  • /test pull-containerd-node-e2e-1-6

Use /test all to run all jobs.

In response to this:

k8s-ci-robot now reruns GitHub Actions with the retest command? That's new. I just wanted the one aborted prow job to rerun...

/test pull-containerd-node-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@samuelkarp samuelkarp mentioned this pull request Oct 26, 2023
Closed
@akhilerm
Copy link
Member

akhilerm commented Oct 26, 2023 •
edited

k8s-ci-robot now reruns GitHub Actions with the retest command?

@samuelkarp It was added so that the github workflows could be retriggered by the author of the PR. Otherwise, only org members/owners could retrigger a github workflow

Also, the aborted job is for the main branch. In release/1.6 we are using the pull-containerd-node-e2e-1-6 job.

akhilerm
akhilerm approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

/lgtm

@samuelkarp
Copy link
Member

Also, the aborted job is for the main branch. In release/1.6 we are using the pull-containerd-node-e2e-1-6 job.

Looks like it got triggered because of this?

dmcgowan changed the base branch from main to release/1.6

@AkihiroSuda
Copy link
Member

/test all

AkihiroSuda
AkihiroSuda approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM if green

@akhilerm
Copy link
Member

The integration test failure seems unrelated to the changes in this PR. Can the job be retriggered and checked?

@kzys
Copy link
Member

kzys commented Oct 27, 2023

/test all

@kzys
Copy link
Member

kzys commented Oct 27, 2023

pull-containerd-node-e2e is showing

Test started last Wednesday at 9:49 PM is still running after 42h38m9s. (more info)

So we may need to do something on Prow side. I don't think I can cancel the job. Can I?

@akhilerm
Copy link
Member

pull-containerd-node-e2e is showing

Test started last Wednesday at 9:49 PM is still running after 42h38m9s. (more info)

So we may need to do something on Prow side. I don't think I can cancel the job. Can I?

@kzys It can be ignored, since that test is not supposed to run on this branch(release/1.6). I am not sure if github allows to merge by overriding the failing check.

@samuelkarp samuelkarp merged commit e63636a into containerd:release/1.6 Oct 29, 2023
36 of 37 checks passed
@samuelkarp
Copy link
Member

I am not sure if github allows to merge by overriding the failing check.

I merged it anyway. I don't think there's a way to clear pull-containerd-node-e2e and we can't rerun it (since it doesn't actually run for the release/1.6 branch).

@vsiravar vsiravar mentioned this pull request Oct 31, 2023
Closed
@akhilerm akhilerm mentioned this pull request Nov 19, 2023
Merged
@AbrohamLincoln AbrohamLincoln mentioned this pull request Feb 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelkarp samuelkarp samuelkarp approved these changes

@estesp estesp estesp approved these changes

@akhilerm akhilerm akhilerm approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dmcgowan @samuelkarp @k8s-ci-robot @akhilerm @AkihiroSuda @kzys @estesp