Skip to content

Add library for manually syncing EndpointSlices - #180

Open
andrewstucki wants to merge 7 commits into
mainfrom
as/portmapper
Open

Add library for manually syncing EndpointSlices#180
andrewstucki wants to merge 7 commits into
mainfrom
as/portmapper

Conversation

@andrewstucki

Copy link
Copy Markdown
Contributor

This adds a library-style Kubernetes controller that publishes the EndpointSlices backing "colocated" Services: Services whose ports are served by overlapping-but-distinct subsets of a shared pod pool.

It replaces the native EndpointSlice controller for Services that deliberately define no selector. Where the native
controller aligns Pods to Services with spec.selector, this library aligns them through a configurable label or annotation, and a pluggable membership check decides — per pod, per port — which pods back which ports:

Service: my-service
  ├── port 8080 (http)  → pods A, B, C
  └── port 8443 (https) → pods B, C, D

Usage is found in the example file included, but roughly corresponds to:

	mapper, _ := portmapper.New(portmapper.Config{
		...
		Membership: portmapper.PerPort(map[string]portmapper.Checker{
			"http":  portmapper.HTTPGet("/healthz", time.Second),
			"https": portmapper.TCPDial(time.Second),
			"someother": portmapper.All(
				portmapper.PodReady(),
				portmapper.PortNames(portmapper.AnnotationKey(portsKey)),
			),
		}, nil),
		...
	})
	_ = mapper.SetupWithManager(mgr)

Custom membership mapping functions (with an optional tristate membership-determination interface for ambiguous responses) are supported as well.

@secpanda

secpanda commented Aug 28, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@chrisseto chrisseto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just did a quick pass but overall LGTM. For an MVP in cloud this looks great. If we want to expose this degree of configuration more generally, the wiring up will likely get pretty nasty. Happy to do a more thorough pass if you'd like.

Comment thread portmapper/portmapper.go
// slices. Cleanup only acts when those leftovers are actually present,
// so a long-migrated Service costs nothing. Set this when something else
// legitimately manages those objects.
DisableNativeCleanup bool

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: DisableMigrationCleanup may be more clear

Comment thread portmapper/reconciler.go

lookupCtx, cancel := context.WithTimeout(ctx, nodeLookupTimeout)
var node corev1.Node
err := r.client.Get(lookupCtx, client.ObjectKey{Name: nodeName}, &node)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be more efficient to perform a List? I can't think of any real benefit to perform name specific look ups.

Comment thread portmapper/reconciler.go
Comment on lines +371 to +374
// nodeLookupTimeout bounds each Node cache lookup: the first Get lazily
// starts a Node informer, and without Node RBAC its cache never syncs -- an
// unbounded Get would wedge the reconcile worker forever instead of
// degrading.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this true? I was under the impression that an error would be returned if the underlying cache/informer hadn't yet been started up.

Comment thread portmapper/reconciler.go
Comment on lines +454 to +456
// One check budget for the whole render: membership probes can block up
// to their timeout apiece, so fan-out is bounded per reconcile.
sem := make(chan struct{}, r.cfg.MaxConcurrentChecks)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like a perfect use case for errgroup.Group.SetLimit?

Comment thread portmapper/reconciler.go
Comment on lines +675 to +677
if !sliceChanged(current, want) {
continue
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason to not rely on SSA to perform this check for you?

@chrisseto

Copy link
Copy Markdown
Contributor

Ah, last thought: We'll need to test how this performs in each cloud. It would not surprise me if certain configurations bypassed kube-proxy and/or ignored endpoint slices.

Huh... I wonder if Cilium would respect this.

@andrewstucki

Copy link
Copy Markdown
Contributor Author

@chrisseto yeah -- I've been going back and forth about whether we should actually maintain both EndpointSlices and deprecated Endpoints directly. I recall there being some oddities around DNS resolution in stock CoreDNS for certain configurations where we were trying to populate EndpointSlices without creating the underlying legacy Endpoints as well. Definitely worth trying out/testing in each major cloud just to make sure that we don't have to do a bunch of legacy stuff too.

@chrisseto

Copy link
Copy Markdown
Contributor

Before I forget: We should probably respect publishNotReady (I don't think I saw that anywhere)

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.

3 participants