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

Correctly Clear conntrack entry on endpoint changes when using nodeport #71573

Merged
merged 1 commit into from
Dec 7, 2018

Conversation

JacobTanenbaum
Copy link
Contributor

@JacobTanenbaum JacobTanenbaum commented Nov 29, 2018

When using NodePort to connect to an endpoint using UDP, if the endpoint is deleted on
restoration of the endpoint traffic does not flow. This happens because conntrack holds
the state of the connection and the proxy does not correctly clear the conntrack entry
for the stale endpoint.

Introduced a new function to conntrack ClearEntriesForPortNAT that uses the endpointIP
and NodePort to remove the stale conntrack entry and allow traffic to resume when
the endpoint is restored.

Signed-off-by: Jacob Tanenbaum jtanenba@redhat.com

release note

Clear UDP conntrack entry on endpoint changes when using nodeport 

/kind bug

fixes #59368

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 29, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @JacobTanenbaum. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 29, 2018
@JacobTanenbaum
Copy link
Contributor Author

@knobunc @squeed @dcbw PTAL

@dcbw
Copy link
Member

dcbw commented Nov 30, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 30, 2018
@dcbw
Copy link
Member

dcbw commented Nov 30, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2018
@@ -107,3 +107,19 @@ func ClearEntriesForNAT(execer exec.Interface, origin, dest string, protocol v1.
}
return nil
}

// ClearEntriesForNATPort uses the conntrack tool to delete the contrack entries
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: docblock function name mismatch

@dcbw
Copy link
Member

dcbw commented Dec 1, 2018

/retest

When using NodePort to connect to an endpoint using UDP, if the endpoint is deleted on
restoration of the endpoint traffic does not flow. This happens because conntrack holds
the state of the connection and the proxy does not correctly clear the conntrack entry
for the stale endpoint.

Introduced a new function to conntrack ClearEntriesForPortNAT that uses the endpointIP
and NodePort to remove the stale conntrack entry and allow traffic to resume when
the endpoint is restored.

Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2018
@JacobTanenbaum
Copy link
Contributor Author

/retest

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @JacobTanenbaum

@dcbw
Copy link
Member

dcbw commented Dec 6, 2018

@JacobTanenbaum can you add the release note to the PR description per #71573 (comment) ?

Copy link
Member

@dcbw dcbw left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2018
@dcbw
Copy link
Member

dcbw commented Dec 7, 2018

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Dec 7, 2018
@dcbw
Copy link
Member

dcbw commented Dec 7, 2018

@smarterclayton can you take a quick look at this one?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 7, 2018
@JacobTanenbaum JacobTanenbaum changed the title Correctly Clear conntrack entrty on endpoint changes when using nodeport Correctly Clear conntrack entry on endpoint changes when using nodeport Dec 7, 2018
@dcbw
Copy link
Member

dcbw commented Dec 7, 2018

@bowei any thoughts on this one?

@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JacobTanenbaum, smarterclayton

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel 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 Dec 7, 2018
@k8s-ci-robot k8s-ci-robot merged commit f0bae6e into kubernetes:master Dec 7, 2018
ravens added a commit to ravens/kubernetes that referenced this pull request Nov 5, 2020
A previous PR (kubernetes#71573) intended to clear conntrack entry on endpoint
changes when using nodeport by introducing a dedicated function to
remove the stale conntrack entry on the node port and allow traffic to
resume. By doing so, it has introduced a nodeport specific bug where the
conntrack entries related to the ClusterIP does not get clean if
endpoint is changed (issue kubernetes#96174). We fix by doing ClusterIP cleanup in
all cases.
jingxu97 pushed a commit to jingxu97/kubernetes that referenced this pull request Nov 7, 2020
A previous PR (kubernetes#71573) intended to clear conntrack entry on endpoint
changes when using nodeport by introducing a dedicated function to
remove the stale conntrack entry on the node port and allow traffic to
resume. By doing so, it has introduced a nodeport specific bug where the
conntrack entries related to the ClusterIP does not get clean if
endpoint is changed (issue kubernetes#96174). We fix by doing ClusterIP cleanup in
all cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kube-proxy/iptables, UDP conntrack data not removed on endpoint changes
6 participants