EndpointSlice Conditions semantics: Ready, Serving and Terminating

[aojea]

Fix the usage of the EndpointSlice Conditions semantics: Ready, Serving and Terminating.

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• If your commit description contains a Fixes: <commit-id> tag, then
  please add the commit author[s] as reviewer[s] to this issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Thanks for contributing!

The EndpointSlice conditions describe 3 states for an Endpoint: Ready, Service and Terminating.

If the Endpoint is Ready it must leverage that value, independently of the other Conditions, this is the reason why the Kubernetes test mentioned in #18241 fails, since using service.Spec.publishNotReadyAddresses force this Ready state in the Endpoint.

In addition, Terminating endpoints must be included only of they are Serving, since that indicates that the Endpoint is valid, this state has to be introduced in the API for handling backward compatibility between versions adding an undesired complexity to consumers, so apologize for that 😄

In KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1669-proxy-terminating-endpoints we describe the behavior for Terminating Endpoints in case there are no existing Ready endpoints, to allow to implement zero downtime strategies during rolling updates.

Add a job to run Kubernetes sig-network tests, to guarantee that there is no regression and also benefit Cilium of these tests, that should guarantee a standard and conformance behavior with Kubernetes sig-network defined behaviors.

Fixes: #18241

Services backends with publishNotReadyAddresses are able to receive traffic independently if they are Terminating, since is the user intent to make them reachable despite its state.

[maintainer-s-little-helper]

Commit a2ec7c754c1196638ad54a2a5dfc7539741d6674 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[aojea]

/assign @aanm @squeed @aditighag

I've verified that this commits fixes #18241, the e2e passes with this patch.

However, I want to add some unit tests and a second commit to fix ProxyTerminatingEndpoints and implement https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1669-proxy-terminating-endpoints per Kubernetes KEP

I'm sending this PR just for testing the CI and for guidance and early feedback, since is my first contribution to cilium

Thanks

[sayboras]

Thanks for your PR.

You probably already see the below page. Also, you can mark this PR as draft if you want.

PS: Glad to see you here contributing to Cilium, we are heavily using Kind cluster as part our tests ;). The IPv6 related workflow was possible, thanks to your time helping me a few years back 👍.

https://docs.cilium.io/en/v1.13/contributing/development/contributing_guide/

[aanm]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 10.0.1.2:80 from testclient-host-bcqqr

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

[squeed]

The travis failure is legit:

    testcase.go:300: lbmap mismatch:
        --- /home/travis/gopath/src/github.com/cilium/cilium/test/controlplane/services/graceful-termination/lbmap2.golden
        +++ <actual>
        @@ -1,15 +1,15 @@
          - Services ---------------------------------------------------------------------
         | ID |                   Name |      Type |               Frontend | Backend IDs |
         |----+------------------------+-----------+------------------------+-------------
         |  0 |     default/kubernetes | ClusterIP |     10.96.0.1:443/NONE |           1 |
         |  1 | test/graceful-term-svc | ClusterIP | 10.96.116.33:8081/NONE |           0 |
          --------------------------------------------------------------------------------
         
        - - Backends -------------------------------------------------
        -| ID |              L3n4Addr |       State | Linked Services |
        -|----+-----------------------+-------------+-----------------
        -|  0 | 10.244.0.112:8081/TCP | terminating |               1 |
        -|  1 |   172.18.0.2:6443/TCP |      active |               0 |
        - ------------------------------------------------------------
        + - Backends --------------------------------------------
        +| ID |              L3n4Addr |  State | Linked Services |
        +|----+-----------------------+--------+-----------------
        +|  0 | 10.244.0.112:8081/TCP | active |               1 |
        +|  1 |   172.18.0.2:6443/TCP | active |               0 |
        + -------------------------------------------------------

(You can run these with go test ./test/controlplane) You might need to do a make -C test/controlplane update-golden

[squeed]

The affinity test is failing; probably need to do --helm-set sessionAffinity=true in the cilium install command.

[squeed]

I've flagged this PR to be backported; it will happen automatically unless it needs manual intervention.

[aditighag]

I agree that we can backport this to 1.12. @aojea AFAIK, the delta between the master and 1.12 implementation might not be too big. But in case there are conflicts, will you be able to help in resolving the conflicts?

[aojea]

will you be able to help in resolving the conflicts?

absolutely, I joined cilium slack so I can be reachable, but in case I have slow response Casey knows how to contact me

[xyll]

Hello!
Do you have maybe the ETA for the backport to the 1.12?
It is possible to get maybe nightly builds of 1.12 after the backport has happened?
Thanks!

[joestringer]

@xyll typically backporters will look at all PRs each week and start the backport. They will tag it so that it shows up in the breadcrumbs links here on this PR, so you should be able to find it by monitoring this PR. Typically after a couple of days it will be reviewed & merged, so a rough estimation would be late next week assuming something doesn't come up. At that point, it should be part of quay.io/cilium/cilium-ci:v1.12 and you could test out the image from there.

[gandro]

Unfortunately, this PR had some had some non-trivial conflicts against v1.12. This means that this needs PR needs to be backported manually to v1.12 from someone with expertise in the affected area.

@aojea Would you be able to open a PR against the v1.12 branch? Otherwise, we'll have to find a Cilium committer who is knowledgeable in the code to backport it.

[aojea]

@aojea Would you be able to open a PR against the v1.12 branch? Otherwise, we'll have to find a Cilium committer who is knowledgeable in the code to backport it.

I can do it, just need some guidance if there are some additional steps, or is just a normal cherry-pick and dealing with the conflicts?

[aojea]

hmm, it interferes with #21161, it seems that backporting requires to bring some code that is not necessary ... it seems that users will have to update to 1.13 :/

[gandro]

hmm, it interferes with #21161, it seems that backporting requires to bring some code that is not necessary ... it seems that users will have to update to 1.13 :/

Thanks for investigating. Yeah, let's remove the backport label for now then.

/cc @squeed

[ElvinEfendi]

Does this also fix #23744? It seems like it could.

[aojea]

Does this also fix #23744? It seems like it could.

I expect it, but I don't have time to test it so I can only guess 😄