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

Make IsConnectionReset work with more error implementations. #58520

Merged
merged 1 commit into from
Jan 20, 2018
Merged

Make IsConnectionReset work with more error implementations. #58520

merged 1 commit into from
Jan 20, 2018

Conversation

porridge
Copy link
Member

What this PR does / why we need it:
This fixes the code to correctly navigate error hierarchy, and actually
work.

Which issue(s) this PR fixes
An improvement for #55860

Special notes for your reviewer:

Integration-testing this code is somewhat hard. What I did to reproduce this
condition reliably was:

  1. use iptables to let the TCP handshake packets through but reject
    payload-carrying packets with:
sudo iptables -t raw -I PREROUTING -d localhost --protocol tcp --dport 443  -j NOTRACK
sudo iptables -t filter -I INPUT -d localhost --protocol tcp --dport 443 -m string --algo bm --string http  -j REJECT --reject-with tcp-reset
  1. start a dummy server with: nc -l -4 localhost 443
  2. make the client issue a GET on localhost:443

Then I added instrumentation to the place in k8s.io/client-go/rest/request.go
which calls this code, to discover the actual error hierarchy.

I think another way to test this would be to run a dummy server which would
listen() on a socket, accept() and then close() the incoming connection
straight away.

Release note:

Correctly handle transient connection reset errors on GET requests from client library.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 19, 2018
@porridge porridge mentioned this pull request Jan 19, 2018
Closed
@wojtek-t
Copy link
Member

/approve no-issue
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: porridge, wojtek-t

Associated issue: #55860

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018
@porridge
Copy link
Member Author

porridge commented Jan 19, 2018 via email

@wojtek-t
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit c9c6901 into kubernetes:master Jan 20, 2018
@porridge
Copy link
Member Author

@wojtek-t This should also apply cleanly to v1.6 v1.7 v1.8 and v1.9.
Can you add the milestones and the cherrypick-candidate label? I don't think I have the permissions.

@wojtek-t
Copy link
Member

@porridge - let's create the appropriate cherrypicks using the hack/cherypick-pull.sh script.
Also, 1.6 is no longer supported, and thus we shouldn't patch it.

@porridge
Copy link
Member Author

porridge commented Jan 23, 2018 via email •
edited
Loading

@wojtek-t
Copy link
Member

Yes - everyone has priviliges to run it. It needs to be approved by patch release manager then anyway.

This was referenced Jan 23, 2018
Merged
Merged
Merged
@porridge
Copy link
Member Author

OK, done, @wojtek-t can you rubber-stamp them all? They are linked here ^^.

k8s-github-robot pushed a commit that referenced this pull request Jan 23, 2018
Merge pull request #58702 from porridge/automated-cherry-pick-of-#585…
85cc15f 
…20-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #58520: Make IsConnectionReset work with more error implementations.

Cherry pick of #58520 on release-1.7.

#58520: Make IsConnectionReset work with more error implementations.
k8s-github-robot pushed a commit that referenced this pull request Jan 25, 2018
Merge pull request #58701 from porridge/automated-cherry-pick-of-#585…
1ad6bec 
…20-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #58520: Make IsConnectionReset work with more error implementations.

Cherry pick of #58520 on release-1.8.

#58520: Make IsConnectionReset work with more error implementations.
k8s-github-robot pushed a commit that referenced this pull request Jan 25, 2018
Merge pull request #58700 from porridge/automated-cherry-pick-of-#585…
705d9ed 
…20-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #58520: Make IsConnectionReset work with more error implementations.

Cherry pick of #58520 on release-1.9.

#58520: Make IsConnectionReset work with more error implementations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

@caesarxuchao caesarxuchao

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@porridge @wojtek-t @k8s-ci-robot @fejta-bot @k8s-github-robot @liggitt @caesarxuchao