@danwinship: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship

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

The pull request process is described here

is not only session affinity, is everything, right now kube-proxy doesn't consider if the endpoint is ready, as a consequence there are more issues like this

#105657

easy to reproduce with an init containers that holds the pod to be ready, the iptables rules are installed

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-deployment
  labels:
    app: test
spec:
  replicas: 1
  selector:
    matchLabels:
      app: test
  template:
    metadata:
      labels:
        app: test
        active: "yes"
    spec:
      initContainers:
        - name: pause-service
          image: busybox:stable
          command: ['sh', '-c', 'echo Pausing start. && sleep 100']
      containers:
      - name: test
        image: httpd
        ports:
        - containerPort: 80
---
apiVersion: v1
kind: Service
metadata:
  name: test
spec:
  type: ClusterIP
  selector:
    app: test
    active: "yes"
  ports:
    - protocol: TCP
      port: 80
      targetPort: 80

we also have the tests wrong

hmm, it seem I was wrong about that.
The rules are there but the service is not jumping to them

I think we need something like

diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go
index 79bb5b6c793..0b13ce46dd0 100644
--- a/pkg/proxy/iptables/proxier.go
+++ b/pkg/proxy/iptables/proxier.go
@@ -1002,7 +1002,19 @@ func (proxier *Proxier) syncProxyRules() {
                // Service does not have conflicting configuration such as
                // externalTrafficPolicy=Local.
                allEndpoints = proxy.FilterEndpoints(allEndpoints, svcInfo, proxier.nodeLabels)
-               hasEndpoints := len(allEndpoints) > 0
+               hasEndpoints := false
+               for _, e := range allEndpoints {
+                       if e.IsReady() {
+                               hasEndpoints = true
+                               break
+                       }
+                       if utilfeature.DefaultFeatureGate.Enabled(features.ProxyTerminatingEndpoints) {
+                               if svc.NodeLocalExternal() && e.IsServing() && e.IsTerminating() {
+                                       hasEndpoints = true
+                                       break
+                               }
+                       }
+               }
 
                svcChain := svcInfo.servicePortChainName
                if hasEndpoints {
diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go
index 85f40337ddf..81b142f1f25 100644
--- a/pkg/proxy/iptables/proxier_test.go
+++ b/pkg/proxy/iptables/proxier_test.go
@@ -2938,6 +2938,77 @@ COMMIT
        assert.NotEqual(t, expectedIPTablesWithSlice, fp.iptablesData.String())
 }
 
+func TestEndpointSliceE2EReject(t *testing.T) {
+       expectedIPTablesWithSlice := `*filter
+:KUBE-SERVICES - [0:0]
+:KUBE-EXTERNAL-SERVICES - [0:0]
+:KUBE-FORWARD - [0:0]
+:KUBE-NODEPORTS - [0:0]
+-A KUBE-SERVICES -m comment --comment "ns1/svc1 has no endpoints" -m tcp -p tcp -d 172.20.1.1/32 --dport 0 -j REJECT
+-A KUBE-FORWARD -m conntrack --ctstate INVALID -j DROP
+-A KUBE-FORWARD -m comment --comment "kubernetes forwarding rules" -m mark --mark 0x4000/0x4000 -j ACCEPT
+-A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
+-A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
+COMMIT
+*nat
+:KUBE-SERVICES - [0:0]
+:KUBE-NODEPORTS - [0:0]
+:KUBE-POSTROUTING - [0:0]
+:KUBE-MARK-MASQ - [0:0]
+-A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN
+-A KUBE-POSTROUTING -j MARK --xor-mark 0x4000
+-A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE
+-A KUBE-MARK-MASQ -j MARK --or-mark 0x4000
+-A KUBE-SERVICES -m comment --comment "kubernetes service nodeports; NOTE: this must be the last rule in this chain" -m addrtype --dst-type LOCAL -j KUBE-NODEPORTS
+COMMIT
+`
+
+       ipt := iptablestest.NewFake()
+       fp := NewFakeProxier(ipt)
+               AddressType: discovery.AddressTypeIPv4,
+               Endpoints: []discovery.Endpoint{{
+                       Addresses:  []string{"10.0.1.1"},
+                       Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)},
+                       NodeName:   utilpointer.StringPtr(testHostname),
+               }, {
+                       Addresses:  []string{"10.0.1.4"},
+                       Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)},
+                       NodeName:   utilpointer.StringPtr("node4"),
+               }},
+       }
+
+       fp.OnEndpointSliceAdd(endpointSlice)
+       fp.syncProxyRules()
+       assert.Equal(t, expectedIPTablesWithSlice, fp.iptablesData.String())
+}
+

/assign @aojea @thockin @robscott

and this to fix #105657

diff --git a/pkg/proxy/endpoints.go b/pkg/proxy/endpoints.go
index 945237dd1ce..2a01b95d746 100644
--- a/pkg/proxy/endpoints.go
+++ b/pkg/proxy/endpoints.go
@@ -121,7 +121,13 @@ func (info *BaseEndpointInfo) Port() (int, error) {
 
 // Equal is part of proxy.Endpoint interface.
 func (info *BaseEndpointInfo) Equal(other Endpoint) bool {
-       return info.String() == other.String() && info.GetIsLocal() == other.GetIsLocal()
+       return info.String() == other.String() &&
+               info.GetIsLocal() == other.GetIsLocal() &&
+               info.GetNodeName() == other.GetNodeName() &&
+               info.GetZone() == other.GetZone() &&
+               info.IsReady() == other.IsReady() &&
+               info.IsTerminating() == other.IsTerminating() &&
+               info.IsServing() == other.IsServing()
 }
 
 // GetNodeName returns the NodeName for this endpoint.
@@ -561,8 +567,20 @@ func detectStaleConnections(oldEndpointsMap, newEndpointsMap EndpointsMap, stale
                        continue
                }
 
+               epReady := 0
+               for _, ep := range epList {
+                       if ep.IsReady() {
+                               epReady++
+                       }
+               }
+               oldEpReady := 0
+               for _, ep := range oldEndpointsMap[svcPortName] {
+                       if ep.IsReady() {
+                               oldEpReady++
+                       }
+               }
                // For udp service, if its backend changes from 0 to non-0. There may exist a conntrack entry that could blackhole traffic to the service.
-               if len(epList) > 0 && len(oldEndpointsMap[svcPortName]) == 0 {
+               if epReady > 0 && oldEpReady == 0 {
                        *staleServiceNames = append(*staleServiceNames, svcPortName)
                }
        }

ideally we should rewrite k8s.io/kubernetes/pkg/proxy/endpoints_test.go to test EndpointSlices instead of Endpoints and cover this new cases about Ready

Dan do you want to split the load?

cc @knabben

is not only session affinity, is everything

it's not everything...

hmm, it seem I was wrong about that.
The rules are there but the service is not jumping to them

Yes, because it writes out the chains before it figures out whether it's going to need to use the terminating endpoints. I have a fix for that coming later.

+                       if ep.IsReady() {
+                               epReady++

That's still not right, because it's not taking ProxyTerminatingEndpoints into account.

In fact, endpoints.go can't figure out on its own which endpoints are actually being used, because it doesn't know whether the service is externalTrafficPolicy: Local or not...

Dan do you want to split the load?

I'm in the middle of refactoring the iptables syncProxyRules to fix internalTrafficPolicy (#100313). I need to think about how to rearrange stuff to get some of the fixes in without refactoring first so it will be backportable.

/hold
may not make sense to merge this if other closely-related fixes are coming imminently...

In fact, endpoints.go can't figure out on its own which endpoints are actually being used, because it doesn't know whether the service is externalTrafficPolicy: Local or not...

And TopologyAwareHints screws this up as well. To compute the list of stale endpoints, you need information from both the EndpointSlices and the Services.

Except it's even worse than that, because we can now have different stale endpoints for internal and external connections:

• UDP Service X has externalTrafficPolicy: Local and (implicitly) internalTrafficPolicy: Cluster. It has both internal and external clients. ProxyTerminatingEndpoints is enabled.
• The sole endpoint for the service on Node Y gets deleted and its EndpointSlice becomes Ready: false, Serving: true, Terminating: true. It thus stops being a valid endpoint for internal (Cluster policy) traffic from Node Y, but does not stop being a valid endpoint for external (Local policy) traffic that reaches Node Y.
• At this point Node Y needs to delete conntrack entries from internal sources to the cluster IP or LB IP, but needs to not delete conntrack entries from external sources to the LB IP.

But we can't implement "invalidate conntrack entries from internal sources, but not conntrack entries from external sources", because the definition of "internal" and "external" is abstracted by the LocalDetector, which might distinguish internal from external using arbitrary iptables rules that have no equivalent in conntrack -D. (eg, matching by source interface name).

I think this is only a problem when ProxyTerminatingEndpoints is enabled; if it's disabled then the set of endpoints for a Local-traffic-policy service will always be a subset of the endpoints for a Cluster-traffic-policy service, so we can do the cleanup correctly. (We don't do the cleanup correctly currently, but we could, as opposed to in the ProxyTerminatingEndpoints case, where there is no easy fix at all.)

Ideas:

1. Drop ProxyTerminatingEndpoints, revert the associated code, go back to the drawing board...
2. Change the rules of ProxyTerminatingEndpoints so that it applies to Cluster services as well as Local ones so internal and external can never have disjoint endpoints. This is probably an API break, so we'd have to make it an opt-in feature on the service? (ie, you'd have to set publishTerminatingAddresses: true on the service)
3. We can fix the cleanup of "black hole" conntrack entries by always preceding every UDP -j REJECT rule with a -j NOTRACK rule so we don't end up creating the conntrack entries in the first place... That doesn't seem that bad in terms of either added iptables rules (most people won't have lots and lots of services with no endpoints) or added kernel filtering load (most people won't have lots and lots of clients trying to connect to services with no endpoints so the conntrack rules won't help much anyway). We can't NOTRACK the rules for accepted connections though, so I don't think this will completely solve the problem for us. (But maybe it's a good idea for the rejected connections anyway?)
4. We could make the proxy set conntrack labels on UDP traffic going through it, to record whether it came from an internal or external source, so that later we can use conntrack -D --label ... to delete conntrack entries from either only internal or only external sources. (This seems like it should work based on the iptables and conntrack man pages, but I'm not 100% sure.)

/hold cancel
There don't seem to be any other places in syncProxyRules itself that are confusing allEndpoints and readyEndpoints, so I think this fix (as a minimal, backport-able fix) is independent of any endpoint-tracking fixes, internalTrafficPolicy fixes, or ProxyTerminatingEndpoints fixes.

There is an additional bug, if a service has ONLY non-ready endpoints it doesn't insert the reject rule
#106030 (comment)

I think we can assume that the stale/conntrack problems were solved on the previous logic, if we are able to figure out how to solve the traffic locality problem that you mention , of course

At this point, ProxyTerminatingEndpoints is alpha so we have some room there ...

fixes #106182

I'm going to LGTM - we can do followups

/lgtm

I'm going to LGTM - we can do followups

we also have to review the in-flight keps, I don't know if it was Dan or you or both who commented it already, but reviewing them I'm really concerned about the future conflicts and interactions and the lots of KEPs that touch kube-proxy https://github.com/orgs/kubernetes/projects/10

• alpha Graceful Termination for Local External Traffic Policy Proxy Terminating Endpoints  enhancements#1669
• alpha Support of mixed protocols in Services with type=LoadBalancer Support of mixed protocols in Services with type=LoadBalancer enhancements#1435
• alpha Topology Aware Hints Topology Aware Routing enhancements#2433
• beta Tracking Terminating Endpoints Tracking Terminating Endpoints enhancements#1672
• beta Service Internal Traffic Policy Service Internal Traffic Policy enhancements#2086
• beta Optionally Disable NodePorts for Service Type=LoadBalancer Optionally Disable NodePorts for Service Type=LoadBalancer enhancements#1864
• beta Service Type=LoadBalancer Class Service Type=LoadBalancer Class  enhancements#1959

@aojea agree that we need to be careful here, there are a lot of changes coming to kube-proxy and the result could be inconsistencies like this fixes + non-iptables proxiers falling behind. #100313 covers the need for hints and traffic policy overlap, and @danwinship did a great job at describing the changes we'd need in that issue. With that said, this is all getting sufficiently complex that it's going to be difficult to keep all proxiers in sync.

how hard can be merge all the keps and break it down in different subtasks so we can work in different streams without breaking each others?

I can see clearly 2 themes:

• loadbalancer service type changes
• cluster traffic behavior (internal, external, ...)

At this point @danwinship has volunteered to take on #100313 which I think is really your second bullet point. I'm not sure who's working on the LoadBalancer/NodePort type changes.

if Dan is on it we are in good hands ;)

Hiya rob , good pt WRT other proxiers.. the userspace and windows proxiers will be alot better off in KPNG long term bc of what rob mentioned. We are always playing catch-up with those .

We're deprecating userspace now and we have MRs in flight for both of these to run in kpng:

I.e. here's our first windows one, kubernetes-sigs/kpng#83

And the userspace kubernetes-sigs/kpng#55

In that case the in tree proxiers could be whittled down to just iptables and IPVS which work quite well and aren't too hard to keep in sync.

@danwinship until which version do we have to backport this?

just 1.22; that's when terminating endpoint support was first added

(and only the last commit actually has bugfixes; the rest is all unit test changes, and I'm not sure if they'll backport easily, but it's not really necessary to backport them if they don't)

(well, ok, the "fix the metric" commit is a bugfix too, but it's not worth backporting)

let me know if you can do it, next friday is the last day for the cherry-pick

The cherry-pick deadline for the 1.22, 1.21 and 1.20 branches is next Friday, November 12, 2021, EOD PT.

let me know if you can do it, next friday is the last day for the cherry-pick

The cherry-pick deadline for the 1.22, 1.21 and 1.20 branches is next Friday, November 12, 2021, EOD PT.

cherry-pick to 1.22 #106373