-
Notifications
You must be signed in to change notification settings - Fork 349
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
Bug: Skipper does not filter EndpointSlices without ports, causing backed connections to TCP port 0 #3151
Comments
Thanks for the bug report. If we follow the expected behavior then we would likely drop the endpointslice with Filtering Can you please also show your ingress configuration? |
The (redacted) ingress looks like:
That Ingress Is this what you were interested in? If not, let me know what information you'd like to review. |
yes exactly! |
I created a reproducer to check what kube-proxy's result would be. Setup apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: foo
component: app
name: foo-app
namespace: skipper-3151
spec:
replicas: 1
selector:
matchLabels:
app: foo
component: app
strategy: {}
template:
metadata:
labels:
app: foo
component: app
spec:
containers:
- command:
- sleep
- "99999"
image: busybox
name: busybox
ports:
- containerPort: 8000
name: http-app
resources:
limits:
memory: 100Mi
terminationGracePeriodSeconds: 0
---
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: foo
component: worker
name: foo-worker
namespace: skipper-3151
spec:
replicas: 1
selector:
matchLabels:
app: foo
component: worker
strategy: {}
template:
metadata:
labels:
app: foo
component: worker
spec:
containers:
- command:
- sleep
- "99999"
image: busybox
name: busybox
ports:
- containerPort: 8000
name: http-worker
resources:
limits:
memory: 100Mi
terminationGracePeriodSeconds: 0
---
apiVersion: v1
kind: Service
metadata:
labels:
app: foo
component: app
name: foo-app
namespace: skipper-3151
spec:
ports:
- name: "http" # changed from "80", because of https://github.com/zalando/skipper/issues/3151#issuecomment-2227940798 and because "80" would violate Kubernetes ingress jsonSchema and we would get error: `The Ingress "foo-named" is invalid: spec.rules[0].http.paths[0].backend.service.port.name: Invalid value: "80": must contain at least one letter (a-z)`
port: 80
protocol: TCP
targetPort: http-app # only used in the foo-app Deployment
selector:
app: foo # matching both foo-app and foo-worker Deployments
type: ClusterIP Confirmation that we see
kube-proxy figures out
What do we get back from:
So let's check skipper by creating 3 different ingress configurations targeting this service: apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
labels:
app: foo
name: foo-named
namespace: skipper-3151
spec:
rules:
- host: foo-named.example
http:
paths:
- backend:
service:
name: foo-app
port:
name: "http"
path: /
pathType: ImplementationSpecific
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
labels:
app: foo
name: foo-targeted-name
namespace: skipper-3151
spec:
rules:
- host: foo-targeted-name.example
http:
paths:
- backend:
service:
name: foo-app
port:
name: "http-app"
path: /
pathType: ImplementationSpecific
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
labels:
app: foo
name: foo-numbered
namespace: skipper-3151
spec:
rules:
- host: foo-numbered.example
http:
paths:
- backend:
service:
name: foo-app
port:
number: 80
path: /
pathType: ImplementationSpecific Running ingresses are:
Routing table output:
Testing response from endpoints seen in skipper routing table:
What do we get from skipper in 3 different configuration cases:
|
In general I think it's fair to assume we should drop |
fix #3151 unset ports in endpointslices should not be considered in our lb members list Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Thanks for the great bug report! |
Describe the bug
Skipper v0.21.146 using EndpointSlices as backend source (with
-enable-kubernetes-endpointslices=true
) will attempt to connect to backends on TCP port 0, when an Ingress Service has a EndpointSlice withports: null
.Skipper will complain like this:
I believe
skipper/dataclients/kubernetes/clusterclient.go
Line 551 in b7825a3
ports: null
is a rare, but state.When Skipper is using Endpoints and not EndpointSlice, this does not happen. Not sure why, but based on some tests Endpoints and Endpointslices are clearly updated differently upon pods phase changes.
I found this issue when I turned on the EndpointSlice feature and an application became 100% unavailable. The root cause found is a Service label selector matching two Deployments, in combination with a named targetPort that was only used in a single Deployment. Obviously the Service label selector should be fixed to only match a single Deployment. Easy. But IMHO Skipper should check EndpointSlices for set ports.
To Reproduce
Sure, what's needed:
The YAMLs to reproduce:
Applying these resource will result in a state like:
See that EndpointSlice with
<unset>
ports? Inspecting it you'll see theports: null
:Expected behavior
Skipper skips all endpoints from a EndpointSlice with
ports: null
.Observed behavior
It does not skip endpoints from a EndpointSlice with
ports: null
and will attempt to connect to a backend on TCP port 0.The text was updated successfully, but these errors were encountered: