Add support for endpoint slices #5745

clayton-gonsalves · 2023-09-05T12:39:10Z

This PR adds support for Kubernetes endpoint slices.

codecov · 2023-09-05T12:51:39Z

Codecov Report

Merging #5745 (362a5ca) into main (8f6aff1) will increase coverage by 0.03%.
Report is 9 commits behind head on main.
The diff coverage is 77.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5745      +/-   ##
==========================================
+ Coverage   78.51%   78.55%   +0.03%     
==========================================
  Files         138      139       +1     
  Lines       19291    19602     +311     
==========================================
+ Hits        15146    15398     +252     
- Misses       3853     3901      +48     
- Partials      292      303      +11

Files	Coverage Δ
cmd/contour/servecontext.go	`83.63% <100.00%> (+0.07%)`	⬆️
...ovisioner/objects/rbac/clusterrole/cluster_role.go	`80.00% <100.00%> (+1.05%)`	⬆️
pkg/config/parameters.go	`85.93% <ø> (+0.13%)`	⬆️
internal/xdscache/v3/endpointstranslator.go	`88.64% <88.88%> (+0.40%)`	⬆️
cmd/contour/serve.go	`19.55% <0.00%> (-0.28%)`	⬇️
internal/xdscache/v3/endpointslicetranslator.go	`82.90% <82.90%> (ø)`

... and 4 files with indirect coverage changes

davinci26

lgtm, one question and one test request

davinci26 · 2023-09-13T14:25:26Z

internal/xdscache/v3/endpointslicetranslator.go

+	var healthCheckPort int32
+
+	for _, enpointSlice := range endpointSliceMap {
+		sort.Slice(enpointSlice.Endpoints, func(i, j int) bool {


why do we need to sort here?

This is from a carry over from the endpoint slice implementation. I think we can get rid of this.

I think it's desirable to sort, to produce stable results across runs. I forget the exact details but impacts performance and/or load balancing algorithms. So I'd say let's leave this in place.

just want to call out, that here the sorting happens only per slice, since we store all the slices in a map it's not possible to sort the combination of all the endpoints.

davinci26 · 2023-09-13T15:04:41Z

test/e2e/infra/endpointslice_test.go

+			},
+		}
+
+		f.CreateHTTPProxyAndWaitFor(p, e2e.HTTPProxyValid)


can we add a case where we scale and we also have not ready pods?

It would be hard to achieve a case where we can deterministically say during a test if the endpoints are ready or not. sure, w could remove the sleep function to catch some unready pods but that might lead to flakey tests

internal/xdscache/v3/endpointslicetranslator.go

clayton-gonsalves · 2023-09-19T10:15:40Z

@skriss @sunjayBhatia this is ready for the first pass

skriss

Thanks @clayton-gonsalves, this is looking pretty good - first pass just getting a few nits out of the way. Still looking into a few of the details

changelogs/unreleased/5745-clayton-gonsalves-minor.md

internal/xdscache/v3/endpointslicetranslator.go

cmd/contour/serve.go

internal/xdscache/v3/endpointslicetranslator.go

skriss · 2023-09-22T18:35:21Z

apis/projectcontour/v1alpha1/contourconfig.go

+	// useEndpointSlice configures contour to fetch endpoint data
+	// from k8s endpoint slices. defaults to false and reading endpoint
+	// data from the k8s endpoints.
+	UseEndpointSlice bool `json:"useEndpointSlice,omitempty"`


I'd be interested to hear what the other maintainers think about adding a more generic FeatureFlags field instead. It could be used for other things, and deprecating a value for it should be simpler than deprecating an entire field.

cc @sunjayBhatia @tsaarni

I think thats a good idea, rather than polluting the config every time we want to add/deprecate a field.

i made the change in 1fd6eea but will also wait for feedback from the other maintainers.

I was actually thinking a string slice, rather than a concrete field per feature, so that as features come and go it doesn't require any changes to the type itself, just to the values that are accepted in the field. But, would like to get a second opinion before making this change.

+1 for string slice, similar in spirit to the --disable-feature flag which is more generic than adding a specific flag per thing we want to disable

I guess that naming makes it sound more generic than it is, it's really for disabling reconciling some optional extended support CRDs, i dunno if we should repurpose the existing flag, have to think about it a bit

Yeah, I was looking at that flag as well for possible use, but since its semantics are opt-out rather than opt-in, it seemed hard to use here (would you opt out of using Endpoints which would result in opting into EndpointSlices? Seems unintuitive). I also don't quite remember why we went with a flag vs. a config file/ContourConfig field for that, will have to look back.

If we do just go ahead with a config file/ContourConfig field for opt-in features, then down the road we could always look at deprecating the --disable-feature flag and replacing with something more aligned with the opt-in feature flags. Or, alternately, I guess we could decide to not reconcile experimental GWAPI resources by default, and require them to be opted into (the opposite of what we have now). xref. #5801

added a string slice in ee4aa23

is this what y'all had in mind?

Looks reasonable to me!

cmd/contour/serve.go

internal/xdscache/v3/endpointslicetranslator.go

changelogs/unreleased/5745-clayton-gonsalves-minor.md

skriss · 2023-10-03T19:48:31Z

Modulo the feature flags spec, this is looking good to me, no further comments at the moment

sunjayBhatia · 2023-10-10T13:46:07Z

changelogs/unreleased/5745-clayton-gonsalves-minor.md

+## Add Kubernetes Endpoint Slice support
+
+This change optionally enables Contour to consume the kubernetes endpointslice API to determine the endpoints to configure Envoy with.
+Note: This change is off by default and is gated by the feature flag `useEndpointSlice`.


nit

Suggested change

Note: This change is off by default and is gated by the feature flag `useEndpointSlice`.

Note: This change is off by default and is gated by the feature flag `useEndpointSlices`.

sunjayBhatia · 2023-10-10T13:52:23Z

apis/projectcontour/v1alpha1/contourconfig.go

@@ -84,8 +84,19 @@ type ContourConfigurationSpec struct {

 	// Tracing defines properties for exporting trace data to OpenTelemetry.
 	Tracing *TracingConfig `json:"tracing,omitempty"`
+
+	// FeatureFlags defines toggle to enable new contour features.
+	// available toggles are


nit:

Suggested change

// available toggles are

// Available toggles are:

// useEndpointSlices - Configures contour to fetch endpoint data

// from k8s EndpointSlices instead of the default behavior which reads

// endpoint data from Endpoint resources.

skriss · 2023-10-10T19:24:33Z

@clayton-gonsalves unfortunately looks like there are a couple small merge conflicts to resolve

skriss

No further comments from me, ready for this to go once the merge conflicts and latest comments are resolved

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

tsaarni

Could you add the featureFlags also in Contour config file documentation here site/content/docs/main/configuration.md? It often tends to get out-of-sync and the naming for attributes is not very consistent (dash vs camelcase).

There is also example config in the same doc page, and same example config is in deployment manifest examples/contour/01-contour-config.yaml.

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

clayton-gonsalves · 2023-10-11T10:23:55Z

Could you add the featureFlags also in Contour config file documentation here site/content/docs/main/configuration.md? It often tends to get out-of-sync and the naming for attributes is not very consistent (dash vs camelcase).

There is also example config in the same doc page, and same example config is in deployment manifest examples/contour/01-contour-config.yaml.

I added it to the `configuration.md, I would prefer keeping the example configs as is because this feature is still opt in. I don't want folks copy pasting the config and enabling something by default.

clayton-gonsalves · 2023-10-13T14:32:46Z

@skriss @sunjayBhatia I think all comments are resolved. is this good to go?

tsaarni

Just couple more questions from me, nothing important though!

tsaarni · 2023-10-13T13:48:08Z

internal/xdscache/v3/endpointslicetranslator.go

+				// we can safely take the first element here.
+				// Refer to: https://issue.k8s.io/106267
+				addr := envoy_v3.SocketAddress(endpoint.Addresses[0], int(*p.Port))


Since the linked issue is not super trivial at a glance, maybe we could just refer to the description of kubernetes endpoint.Addresses where this assumption is also stated, including even the issue link.

tsaarni · 2023-10-13T14:03:52Z

internal/xdscache/v3/endpointslicetranslator.go

+		sort.Slice(endpointSlice.Endpoints, func(i, j int) bool {
+			return endpointSlice.Endpoints[i].Addresses[0] < endpointSlice.Endpoints[j].Addresses[0]
+		})


Question: Why do the addresses need to be sorted?

In the test there was also comment "addresses should be sorted". I know sorting was also done in endpointtranslator.go, where it likely found its way here as well, but it is not clear to me if sorting is necessary. Maybe the idea is just that the endpoints are potentially bit more stable over time that way? Also, the addresses are sorted as string, which is not the most logical order to sort IP addresses 😄.

There is a discussion around it here,.
I tried to follow the pattern from the endpoint implementation.

tsaarni · 2023-10-13T14:09:44Z

internal/xdscache/v3/endpointslicetranslator.go

+// endpointSliceMap may be nil, in which case, the result is also nil.
+func (c *EndpointSliceCache) RecalculateEndpoints(port, healthPort v1.ServicePort, endpointSliceMap map[string]*discoveryv1.EndpointSlice) []*LoadBalancingEndpoint {
+	var lb []*LoadBalancingEndpoint
+	uniqueEndpoints := make(map[string]struct{}, 0)


Question: why map of structs as a "flag" to keep track of unique keys, instead of map of bool?

It;s out of habit but I reason that a map[T]struct{} is intuitive as a set compared to a map[T]bool. A map[T]bool could have an element as false which doesn't make much sense and means you need to have an if check to be sure.

If I am not mistaken, an empty struct also doesn't allocate memory. although at the scale we are using it, it won't make that big of a difference. If there is a precedence to use map[T]bool in this project I am happy to change it.

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

skriss · 2023-10-19T20:10:58Z

@tsaarni @sunjayBhatia anything else blocking here? I'd definitely like to get this into the upcoming 1.27 RC. Can always address any remaining non-blocking items between RC and GA if needed.

skriss

LGTM, will get this into the upcoming 1.27 RC

sunjayBhatia · 2023-10-23T18:53:12Z

@tsaarni @sunjayBhatia anything else blocking here? I'd definitely like to get this into the upcoming 1.27 RC. Can always address any remaining non-blocking items between RC and GA if needed.

nothing blocking from me 👍🏽

clayton-gonsalves requested a review from a team as a code owner September 5, 2023 12:39

clayton-gonsalves requested review from tsaarni, stevesloka, skriss and sunjayBhatia and removed request for a team, tsaarni and stevesloka September 5, 2023 12:39

clayton-gonsalves added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Sep 5, 2023

clayton-gonsalves self-assigned this Sep 5, 2023

clayton-gonsalves force-pushed the use-endpoint-slices branch from bef44ee to eebadd6 Compare September 5, 2023 12:48

clayton-gonsalves requested a review from davinci26 September 5, 2023 13:08

clayton-gonsalves force-pushed the use-endpoint-slices branch from eebadd6 to 902e677 Compare September 5, 2023 14:28

clayton-gonsalves mentioned this pull request Sep 6, 2023

Design: Endpoint slice support #5703

Merged

davinci26 reviewed Sep 13, 2023

View reviewed changes

clayton-gonsalves force-pushed the use-endpoint-slices branch 4 times, most recently from ff1ed4f to 03a4e3d Compare September 19, 2023 09:47

skriss requested changes Sep 21, 2023

View reviewed changes

skriss requested changes Sep 22, 2023

View reviewed changes

internal/xdscache/v3/endpointslicetranslator.go Outdated Show resolved Hide resolved

cmd/contour/serve.go Outdated Show resolved Hide resolved

cmd/contour/serve.go Outdated Show resolved Hide resolved

internal/xdscache/v3/endpointslicetranslator.go Show resolved Hide resolved

skriss reviewed Sep 22, 2023

View reviewed changes

clayton-gonsalves force-pushed the use-endpoint-slices branch from 0ca48ab to 1fd6eea Compare September 26, 2023 11:06

clayton-gonsalves requested a review from skriss September 26, 2023 12:57

skriss reviewed Oct 3, 2023

View reviewed changes

cmd/contour/serve.go Outdated Show resolved Hide resolved

internal/xdscache/v3/endpointslicetranslator.go Show resolved Hide resolved

skriss mentioned this pull request Oct 3, 2023

clarify logic in endpointstranslator.go #5800

Closed

skriss requested changes Oct 3, 2023

View reviewed changes

changelogs/unreleased/5745-clayton-gonsalves-minor.md Outdated Show resolved Hide resolved

clayton-gonsalves force-pushed the use-endpoint-slices branch from d43f472 to ee4aa23 Compare October 6, 2023 17:08

clayton-gonsalves requested a review from skriss October 6, 2023 17:51

sunjayBhatia reviewed Oct 10, 2023

View reviewed changes

sunjayBhatia requested a review from tsaarni October 10, 2023 14:57

skriss reviewed Oct 10, 2023

View reviewed changes

clayton-gonsalves added 2 commits October 11, 2023 11:06

Add support for endpoint slices

24bd02f

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

address review comments

4fe26d8

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

clayton-gonsalves force-pushed the use-endpoint-slices branch from 9db55b9 to 4fe26d8 Compare October 11, 2023 09:18

make gen

d859606

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

tsaarni requested changes Oct 11, 2023

View reviewed changes

add featureFlags also in Contour config file documentation

7513f92

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

clayton-gonsalves requested review from skriss and tsaarni October 11, 2023 10:24

tsaarni reviewed Oct 13, 2023

View reviewed changes

update endpoint.address description

362a5ca

Signed-off-by: Clayton Gonsalves <clayton.gonsalves@reddit.com>

clayton-gonsalves requested review from tsaarni and sunjayBhatia October 13, 2023 15:51

skriss approved these changes Oct 23, 2023

View reviewed changes

skriss merged commit a216475 into projectcontour:main Oct 24, 2023
25 of 26 checks passed

skriss mentioned this pull request Oct 24, 2023

use EndpointSlices by default #5891

Closed

	Note: This change is off by default and is gated by the feature flag `useEndpointSlice`.
	Note: This change is off by default and is gated by the feature flag `useEndpointSlices`.

-	// available toggles are
+	// Available toggles are:
+	// useEndpointSlices - Configures contour to fetch endpoint data
+	// from k8s EndpointSlices instead of the default behavior which reads
+	// endpoint data from Endpoint resources.

Add support for endpoint slices #5745

Add support for endpoint slices #5745

Conversation

clayton-gonsalves commented Sep 5, 2023

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

davinci26 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clayton-gonsalves commented Sep 19, 2023

skriss left a comment

Choose a reason for hiding this comment

skriss Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Oct 10, 2023

skriss left a comment

Choose a reason for hiding this comment

tsaarni left a comment

Choose a reason for hiding this comment

clayton-gonsalves commented Oct 11, 2023

clayton-gonsalves commented Oct 13, 2023

tsaarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Oct 19, 2023 • edited Loading

skriss left a comment

Choose a reason for hiding this comment

sunjayBhatia commented Oct 23, 2023

codecov bot commented Sep 5, 2023 •

edited

Loading

skriss Sep 22, 2023 •

edited

Loading

skriss Oct 4, 2023 •

edited

Loading

skriss commented Oct 19, 2023 •

edited

Loading