Skip to content
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

feat(k8s): Switch from CoreV1 Endpoints to DiscoveryV1 EndpointSlice #643

Merged
merged 2 commits into from
May 10, 2023

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented May 7, 2023

EndpointSlice API is available as V1 since K8s 1.21. Let's do not rely on old Endpoint API in KTF.

It's been spotted during work on Kong/kubernetes-ingress-controller#3916.
TestValidationWebhook from KIC uses WaitForConnectionOnServicePort function from KTF. When KIC drops Endpoints in favour of EndpointSlice this test doesn't pass, because Endpoints are not created (and KTF expecting them).

Mirroring works in other way from Endpoints to EndpointSlices under circumstances described in docs. Thus after this change KTF should be still backward compatible with Endpoints and does not break someone's tests that rely on old Endpoints.

I've tested this change with KIC and everything passes

  • KIC with old Endpoints and KTF with EndpointSlice
  • KIC with EndpointSlice (WIP) and KTF with EndpointSlice

Thus this PR with high certainty (maybe some edge cases exist) shouldn't break someone's setup.

Other improvements - reduce coupling by replacing concrete K8s client as parameter with corresponding interface.

@programmer04 programmer04 temporarily deployed to gcloud May 7, 2023 11:09 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2023

Codecov Report

Patch coverage: 2.70% and project coverage change: +0.12 🎉

Comparison is base (c7c77f1) 58.40% compared to head (32b9c23) 58.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
+ Coverage   58.40%   58.53%   +0.12%     
==========================================
  Files          43       43              
  Lines        3455     3461       +6     
==========================================
+ Hits         2018     2026       +8     
- Misses       1173     1174       +1     
+ Partials      264      261       -3     
Flag Coverage Δ
integration-test 58.53% <2.70%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/utils/kubernetes/networking/services.go 13.20% <2.70%> (-0.80%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@programmer04 programmer04 temporarily deployed to gcloud May 7, 2023 13:13 — with GitHub Actions Inactive
@programmer04 programmer04 removed the WIP label May 7, 2023
@programmer04 programmer04 temporarily deployed to gcloud May 7, 2023 18:08 — with GitHub Actions Inactive
@programmer04 programmer04 self-assigned this May 8, 2023
@programmer04 programmer04 marked this pull request as ready for review May 9, 2023 13:01
@programmer04 programmer04 requested review from a team and shaneutt as code owners May 9, 2023 13:01
@programmer04 programmer04 enabled auto-merge (squash) May 9, 2023 13:02
@programmer04 programmer04 temporarily deployed to gcloud May 9, 2023 14:49 — with GitHub Actions Inactive
@programmer04 programmer04 requested a review from pmalek May 9, 2023 14:49
@programmer04 programmer04 temporarily deployed to gcloud May 10, 2023 07:42 — with GitHub Actions Inactive
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd work on the tests now - #649 - to ensure the changed functions still work 😅

@pmalek
Copy link
Member

pmalek commented May 10, 2023

@programmer04 You'll need to re-push the commits after you've signed them

@programmer04 programmer04 temporarily deployed to gcloud May 10, 2023 08:58 — with GitHub Actions Inactive
@programmer04 programmer04 merged commit c77a776 into main May 10, 2023
@programmer04 programmer04 deleted the switch-to-eps branch May 10, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants