Clear conntrack entries for externalIP and LoadBalancer IP #73323

prameshj · 2019-01-25T19:10:25Z

When an endpoint is deleted, the conntrack entries are cleared for
clusterIP but not for externalIP of the service. This change adds
that step.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
When an endpoint is deleted, the conntrack entries are cleared for clusterIP but not for externalIP of the service spec and LoadBalancer ip from service status. This PR adds that step.
Which issue(s) this PR fixes:

Fixes #65228

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

When an endpoint is deleted, the conntrack entries are cleared for clusterIP but not for externalIP of the service. This change adds that step.

prameshj · 2019-01-25T19:13:07Z

/assign @freehan

steven-sheehy · 2019-01-25T21:01:25Z

Thanks @prameshj for the fix. I can try to test locally if you have a docker image pushed somewhere.

Also, the fixes statement in the description has an extra slash, so referenced ticket won't get closed automatically.

hiep-huynh · 2019-01-30T20:11:06Z

pkg/proxy/iptables/proxier.go

@@ -613,6 +613,12 @@ func (proxier *Proxier) deleteEndpointConnections(connectionMap []proxy.ServiceE
 			if err != nil {
 				klog.Errorf("Failed to delete %s endpoint connections, error: %v", epSvcPair.ServicePortName.String(), err)
 			}
+			for _, extIP := range svcInfo.ExternalIPStrings() {
+				err := conntrack.ClearEntriesForNAT(proxier.exec, extIP, endpointIP, v1.ProtocolUDP)


This has the potential to indiscriminantly remove all sessions matching external IP to endpointIP regardless if it's stale or not. Can this be written to be more selective especially if we know the port(s) involved in the stale session?

There is a check above(L608) to delete stale connections based on the NodePort. If user is not using NodePort and has specified TargetPort by name, that could have a different value for each endPoint. ServiceEndpoint currently does not expose this value.

hiep-huynh

I repeatedly tested our apps using latest test build with your fix. I deleted the pods with UDP services in order to reproduce/cause stale sessions and verified that the stale sessions were removed immediately. I also verified that new UDP sessions and traffic were able to be created to the new pod instances.

In doing so I realized that my concerns about inadvertent removal of sessions for matching extIP, endpointIP, and of UDP type doesn't apply for the reason that pod instance endpointIP are unique and in deleting the instance, it's desirable to clear all sessions related to this endpointIP to prevent routing black holes. I'm able to verify that your code accomplishes this by clearing sessions from all possible extIPs to the removed pod instance endpointIP.

prameshj · 2019-02-11T20:38:36Z

I repeatedly tested our apps using latest test build with your fix. I deleted the pods with UDP services in order to reproduce/cause stale sessions and verified that the stale sessions were removed immediately. I also verified that new UDP sessions and traffic were able to be created to the new pod instances.

In doing so I realized that my concerns about inadvertent removal of sessions for matching extIP, endpointIP, and of UDP type doesn't apply for the reason that pod instance endpointIP are unique and in deleting the instance, it's desirable to clear all sessions related to this endpointIP to prevent routing black holes. I'm able to verify that your code accomplishes this by clearing sessions from all possible extIPs to the removed pod instance endpointIP.

Thanks for verifying! There could still be a conflict if someone is using 2 different UDP-based services with the same pods as endpoints right? But since this endpoint is stale anyway, it should be ok to delete those connections as well.

freehan · 2019-02-12T18:50:23Z

It would be great this PR can also cover the IP from LoadbalancerStatus. Just need to mirror the logic for that case

steven-sheehy · 2019-02-13T18:00:49Z

Need to update PR title & description to reflect LB change. Also, is there a relevant ticket for LB @freehan ?

prameshj · 2019-02-13T18:32:19Z

Need to update PR title & description to reflect LB change. Also, is there a relevant ticket for LB Minhan Xia ?
Can we reuse the same ticket for this too? We are just clearing conntrack entries for all IPs used to refer to the service, in addition to clusterIP

steven-sheehy · 2019-02-13T18:36:56Z

If you're asking me, I think it's fine to reuse ticket. I just wasn't sure if there was an existing issue for the LB IP that we can also link to.

freehan

/lgtm
/approve

prameshj · 2019-02-15T23:25:48Z

/assign @thockin

freehan · 2019-02-15T23:26:15Z

If you're asking me, I think it's fine to reuse ticket. I just wasn't sure if there was an existing issue for the LB IP that we can also link to.

I do not think there is an existing issue for LB IP. But I would imagine the same problem will hit UDP LBs.

thockin

/lgtm
/approve

k8s-ci-robot · 2019-02-19T23:01:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, prameshj, thockin

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:

~~pkg/proxy/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

prameshj · 2019-02-20T00:19:40Z

/retest

…23-upstream-release-1.13 Automated cherry pick of #73323: Clear conntrack entries for externalIP

Clear conntrack entries for externalIP

168602e

When an endpoint is deleted, the conntrack entries are cleared for clusterIP but not for externalIP of the service. This change adds that step.

k8s-ci-robot requested review from m1093782566 and smarterclayton January 25, 2019 19:10

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 Jan 25, 2019

Merge branch 'master' into clear-externalip-conntrack

5667ebd

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2019

k8s-ci-robot assigned freehan Jan 25, 2019

prameshj mentioned this pull request Jan 25, 2019

Download conntrack binary via configure scripts. #64640

Closed

fix import

cd2d33e

hiep-huynh suggested changes Jan 30, 2019

View reviewed changes

hiep-huynh approved these changes Feb 7, 2019

View reviewed changes

Remove conntrack entries from loadbalancer ip too.

24d3ab8

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 13, 2019

prameshj changed the title ~~Clear conntrack entries for externalIP~~ Clear conntrack entries for externalIP and LoadBalancer IP Feb 13, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2019

freehan approved these changes Feb 15, 2019

View reviewed changes

k8s-ci-robot assigned thockin Feb 15, 2019

thockin reviewed Feb 19, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2019

k8s-ci-robot merged commit 059d605 into kubernetes:master Feb 20, 2019

freehan mentioned this pull request Mar 14, 2019

Clear conntrack entries on 0 -> 1 endpoint transition with externalIPs #75265

Merged

prameshj mentioned this pull request Aug 19, 2019

Automated cherry pick of #73323: Clear conntrack entries for externalIP #81615

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 30, 2019

Merge pull request #81615 from prameshj/automated-cherry-pick-of-#733…

fccd9fb

…23-upstream-release-1.13 Automated cherry pick of #73323: Clear conntrack entries for externalIP

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear conntrack entries for externalIP and LoadBalancer IP #73323

Clear conntrack entries for externalIP and LoadBalancer IP #73323

prameshj commented Jan 25, 2019 •

edited

Loading

prameshj commented Jan 25, 2019

steven-sheehy commented Jan 25, 2019

hiep-huynh Jan 30, 2019

prameshj Feb 4, 2019

hiep-huynh left a comment

prameshj commented Feb 11, 2019 •

edited

Loading

freehan commented Feb 12, 2019

steven-sheehy commented Feb 13, 2019

prameshj commented Feb 13, 2019

steven-sheehy commented Feb 13, 2019

freehan left a comment

prameshj commented Feb 15, 2019

freehan commented Feb 15, 2019

thockin left a comment

k8s-ci-robot commented Feb 19, 2019

prameshj commented Feb 20, 2019

Clear conntrack entries for externalIP and LoadBalancer IP #73323

Clear conntrack entries for externalIP and LoadBalancer IP #73323

Conversation

prameshj commented Jan 25, 2019 • edited Loading

prameshj commented Jan 25, 2019

steven-sheehy commented Jan 25, 2019

hiep-huynh Jan 30, 2019

Choose a reason for hiding this comment

prameshj Feb 4, 2019

Choose a reason for hiding this comment

hiep-huynh left a comment

Choose a reason for hiding this comment

prameshj commented Feb 11, 2019 • edited Loading

freehan commented Feb 12, 2019

steven-sheehy commented Feb 13, 2019

prameshj commented Feb 13, 2019

steven-sheehy commented Feb 13, 2019

freehan left a comment

Choose a reason for hiding this comment

prameshj commented Feb 15, 2019

freehan commented Feb 15, 2019

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 19, 2019

prameshj commented Feb 20, 2019

prameshj commented Jan 25, 2019 •

edited

Loading

prameshj commented Feb 11, 2019 •

edited

Loading