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] Support endpoint slices #1608

Closed

Conversation

scottd018
Copy link

@scottd018 scottd018 commented Jul 30, 2021

What this PR does / why we need it:

Fixes #1172 to address endpoint slices in Kubernetes.

Which issue this PR fixes:

fixes #1172, adds endpoint slices as an optional configuration when starting the controllers.

Special notes for your reviewer:

Support endpoint slices

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • [ x ] the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Dustin Scott added 8 commits July 29, 2021 17:34
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
This is required when the CLI input is set to
--use-endpoint-slices.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
This check ensures that the version of k8s is compatible when
using the --use-endpoint-slices feature.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
This refactor allows the parser to select between
'legacy' endpoints and newer endpointslices and
handles them appropriately.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
@CLAassistant
Copy link

CLAassistant commented Jul 30, 2021

CLA assistant check
All committers have signed the CLA.

@scottd018
Copy link
Author

@mflendrich I have a decent start to add endpoint slices, before a final refactor. I'm having some problems running integration tests in my environment, however I suspect the issue to be environmental. Was hoping I can let the pipeline take care of this check as to save some time in troubleshooting, however it looks as if I need to have the workflow manually kicked off because I am a first time contributor.

Are you able to help here? Thanks!

@scottd018 scottd018 changed the title Feature support endpoint slices [feat]: Support endpoint slices Jul 30, 2021
@mflendrich
Copy link
Contributor

Are you able to help here? Thanks!

Done 👍

Feel free to file a bug whenever you think that the test does not work in your environment - maybe that's something we can (or even should) fix.

@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #1608 (c4ad2d9) into next (0a503e5) will increase coverage by 0.00%.
The diff coverage is 36.03%.

❗ Current head c4ad2d9 differs from pull request most recent head f0f6e56. Consider uploading reports for the commit f0f6e56 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             next    #1608   +/-   ##
=======================================
  Coverage   46.13%   46.14%           
=======================================
  Files          71       71           
  Lines        6745     6833   +88     
=======================================
+ Hits         3112     3153   +41     
- Misses       3262     3312   +50     
+ Partials      371      368    -3     
Flag Coverage Δ
integration-test 46.14% <36.03%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
internal/manager/setup.go 54.12% <8.69%> (-12.54%) ⬇️
internal/store/store.go 45.09% <33.33%> (-0.52%) ⬇️
internal/parser/parser.go 55.88% <42.85%> (-4.00%) ⬇️
internal/manager/config.go 92.62% <100.00%> (+0.12%) ⬆️
internal/sendconfig/common_workflows.go 65.95% <100.00%> (ø)
internal/proxy/clientgo_cached_proxy_resolver.go 61.58% <0.00%> (+1.32%) ⬆️
...trollers/configuration/zz_generated_controllers.go 34.63% <0.00%> (+2.03%) ⬆️
internal/parser/ingressrules.go 66.66% <0.00%> (+7.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a503e5...f0f6e56. Read the comment docs.

shaneutt and others added 8 commits July 30, 2021 10:43
This patch reduces the packages we want coverage
for to pkg/ and internal/ (where all our code lives
now minus scripts and some build tools) and turns
on unit tests during the integration test runs to
capture more coverage data.
This will ensure the validation of kubernetes version
for using endpoint slices returns appropriately.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
This moves the validation logic into its own function
for code coverage.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
@scottd018 scottd018 changed the title [feat]: Support endpoint slices [feat] Support endpoint slices Jul 30, 2021
@scottd018
Copy link
Author

Are you able to help here? Thanks!

Done 👍

Feel free to file a bug whenever you think that the test does not work in your environment - maybe that's something we can (or even should) fix.

Thanks @mflendrich ! Looks like the integration test did run and pass (without the new endpoint slice logic), so I think this is probably something specific to my environment. I've pushed up the new integration test. If you can, kick it off again and in the meantime I can see about filing an issue for user-specific environments and push that up as well.

In another upcoming change we're switching testing
of older kubernetes releases onto GKE (from KIND)
so that they're running against conformant production
quality cluster configurations. This patch moves
the tests which must remain in kind (for now) into
their own job as a blocker for releases.
This was needed to support running against standard
GKE clusters which have a tiny default upper bound
on the number of Ingress resources that can be present.

For the same reason this patch also reduces the overall
number of Ingresses tested.
This is a step in the direction of making the integration
test suite capable of running in parallel.
Dustin Scott added 20 commits July 31, 2021 09:22
This will ensure the validation of kubernetes version
for using endpoint slices returns appropriately.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
This moves the validation logic into its own function
for code coverage.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Pre-check script would pass and fail in the middle of the
integration test due to missing helm and kubectl binaries.
Added checkes for both and re-organized the print statement
so that it does not run too far to the right in the
user terminal.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Rather than create an entirely separate test for endpoint
slices, piggyback on the 2 integration test for PG vs
dbless and use endpoint slices for dbless and endpoints
for PG.  This keeps the codebase simpler while still
providing code coverage for endpoints vs. endpoint slices.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Previous assumption about the version API was incorrect.
Had to create a new clientset to consume the server version
to ensure that a consumer is not trying to use endpoint
slices against an invalid version.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
This fixes a missing logic assumption that did not
understand that an EndpointSlice object did not match
the exact name of a Service resource like Endpoints do.
Rather, we look at the labels of the EndpointSlice resource
to match the name to ensure that it belongs to the service.
Additionally, added some missing methods that were left out
of prior commits.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Removed the hard-coded versioning logic and moved the
logic into a constant.  Use a float rather than nested
integer checks.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
…tion

Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
Signed-off-by: Dustin Scott <sdustin@vmware.com>
@scottd018 scottd018 marked this pull request as ready for review July 31, 2021 18:11
@scottd018 scottd018 requested a review from a team as a code owner July 31, 2021 18:11
@scottd018
Copy link
Author

@mflendrich This is ready for code review. Please let me know if there is anything you'd like a discussion or clarification on, or anything that you feel needs fixed. Happy to push new commits to satisfy your requirements.

Signed-off-by: Dustin Scott <sdustin@vmware.com>
@mflendrich mflendrich removed the request for review from a team August 2, 2021 08:52
@mflendrich mflendrich deleted the branch Kong:next August 6, 2021 18:30
@mflendrich mflendrich closed this Aug 6, 2021
@mflendrich
Copy link
Contributor

mflendrich commented Aug 6, 2021

@scottd018

GH has automatically closed this PR because we've deleted the next branch.

Could you please change the target branch of this PR from next to main and reopen it? 🙏 Only the author can do it.
The closure of this PR is NOT because we don't want to merge this PR.

edit: apparently, in this case GH doesn't allow for changing the target branch. If you are unable to set the target branch to main, could you please recreate this PR, targetting main this time? Thank you!

@scottd018
Copy link
Author

@mflendrich requested a new PR #1678 for this. Also refactored a bit to meet the changes in the main branch.

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.

5 participants