Fix epslices

[mneverov]

What kind of PR is this?

Bugfix

Why this PR is needed / What this PR do?

First commit addresses a situation when an endpoint slice is not ready, but it was not set.
Second commit complements #520 (which can be closed in favor of this PR). The port mapping should be done based on field precedence: 1. port.Name; 2. port.TargetPortName; 3. port.TargetPort (see the adjusted test).

Which issue(s) this PR fixes?

Fixes #
Following tests pass:

• EndpointSlice [It] should support a Service with multiple ports specified in multiple EndpointSlices
• EndpointSlice [It] should support a Service with multiple endpoint IPs specified in multiple EndpointSlices
• EndpointSliceMirroring [It] should mirror a custom Endpoint with multiple subsets and same IP address

Additional information about this PR

[jayunit100]

i think we can merge this now. it looks good to me.
we can test/look at it together tomorrow
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jayunit100, mneverov

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:

• OWNERS [jayunit100]

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

[jayunit100]

i wonder if in general , KPNG NEEDS to be dependent on port names for routing rules. To me it shoudlnt.

i.e. if

• service 10.1.2.3:8080 points to my hamburger (tpn: hport) SAAS
• service 10.1.2.3:9090 points to my pizza (tpn:pPort) SAAS

Why cant we blindly just write routing rules to 8080 and 9090. Who cares what the names are. After all, the 10.1.2.3:8080 and 10.1.2.3:9090 represent unique primary keys, and are complete in terms of the layer 4 information content required to distinguish these backends.? right?

[jayunit100]

Ah got it.
Kubernetes supports this odd thing where targetPort=80 and portName=port8080 ... In that case ,

• port name overrides targetPort
  but KPNG discarded that name info and then forwareded to the incorrect, targetPort 80 which was supposed to be replaced w 8080

[mcluseau]

umm isn't it what we provide with port overrides in endpoints? I mean, the PortMapping(...) call in api/localv1/endpoint.go

[mneverov]

@mcluseau previously, the PortMapping only worked if TargetPortName was specified. This PR added port name check too: if port.Name != "" ....

[jayunit100]

@mcluseau maybe suggesting there a possible brain fix here ???

[mcluseau]

well, the idea is still to keep k8s business logic outside of the backends. Otherwise, we're just doing that https://xkcd.com/927/ ;-)
As I'm not very available on the project nowadays, I couldn't dig this a lot, but I can see that @mneverov actually did some work in the file I mentioned, so he can have missed it :-) Now, I just wonder why there's a second round of interpretation has, from my "old" memories, the idea of the PortMappings() calls was to directly provide the "final form" of argument enpointPortMap argument here in iptables.go.

[mneverov]

well, the idea is still to keep k8s business logic outside of the backends

I was not aware of it, got it.

the idea of the PortMappings() calls was to directly provide the "final form" of argument enpointPortMap argument here in iptables.go

From my perspective this PR did not change it.

My interpretation of how port mapping works:

K8s

Endpoint slice reconciler creates endoint slices with port names taken from a corresponding service port name. A Service port binds targetName to the Pod port if the targetName is a string (as it is described in the documentation). The same logic is used in the endpoints controller. If a Service port targetName is specified as a string and it does not match to any Pod port names then the controllers create an EndpointSlice (or legacy Endpoints) without this port. In case if it is the only port specified in a Service then EndpoinSlice will not have ports at all. If the targetName is an integer (i.e. a port 8080), then it is copied to the EndpointSlice port definition.

KPNG

KPNGs frontend kube2store watches api-server and maps service ports to the internal PortMapping representation. The only difference from k8s is that PortMapping has a separate field TargetPortName for the case when the service port is a string.

So, actually this check doesn't make sense and should be removed.

For services without selectors the mapping rules are almost the same:

1. If a Service specifies a port name, then it has to match with the EndpointSlice port name. If it doesn't - no routing. One corner case is when Service doesn't specify port name, but EndpointSlice does. In this scenario again, no routing.
2. If a Service specifies targetPort (doesn't matter if it is as a string or actual port) it is not part of the decision, i.e. it can be any bogus port and as long as there is a properly defined EndpointSlice, the routing will be created.

apiVersion: v1
kind: Service
metadata:
  name: nginx-service
spec:
  ports:
    - port: 80
      protocol: TCP
      targetPort: anything-here

---

apiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
  name: nginx-service-1
  labels:
    kubernetes.io/service-name: nginx-service
addressType: IPv4
ports:
  - name: ''
    protocol: TCP
    port: 80
endpoints:
  - addresses:
      - "10.244.0.5" # ip of the nginx port