[Issue 246]: Introduce ConnectionProvider seam (step 1: controller-as-library) by eniko-dif · Pull Request #597 · temporalio/temporal-worker-controller

eniko-dif · 2026-09-16T14:16:17Z

Summary

Related to issue #246

This is step 1 of making the worker controller usable as a library: it introduces a clean seam between the WorkerDeployment reconciler and the connection resources it references, so the OSS controller no longer hardcodes the temporal.io/Connection and temporal.io/ClusterConnection kinds.

A wrapper binary can now register additional connection kinds at process start — without upstream CRD changes — by appending its own ConnectionProvider to the startup-built slice. The reconciler resolves a ConnectionReference to a registered provider, which supplies the referenced object (for finalizer management), a cached SDK client, and a fingerprint (for drift detection).

What changed

New seam: `internal/controller/connectionprovider`

ConnectionProvider — the group-kind-level factory. One instance is registered per GroupKind at startup; the default registry ships Connection and ClusterConnection. Provides Fetch, NewObject, IsClusterScoped, and GroupKind.
ResolvedConnection — the per-reconcile handle bound to a fetched object. Carries the behavior for that connection: Object(), GetClient(), Fingerprint(), ApplyWorkerPodSpec(), Evict().
Error taxonomy mapping to distinct status conditions: *UnknownKindError → ReasonUnknownConnectionKind, *AuthError → ReasonAuthSecretInvalid, *DialError → ReasonTemporalClientCreationFailed.
Identity by name + GroupKind (not hardcoded kind names), applied consistently across the watch mapper, finalizer release, and sameConnectionRef — so a namespaced Connection "foo" and a cluster-scoped connection "foo" are distinct.

Default provider: `internal/controller/clientpool/default_provider.go`

DefaultProvider implements ConnectionProvider for the two built-in kinds, sharing a ClientPool.
defaultResolved implements ResolvedConnection; the ConnectionSpec is opaque to the reconciler and never leaves this package.

Reconciler wiring

WorkerDeploymentReconciler now holds a startup-built Providers []ConnectionProvider (never mutated) instead of a concrete ClientPool. main.go wires clientpool.NewDefaultProviders.
Watches are set up per registered kind (skipping cluster-scoped kinds when the controller is namespace-scoped), so wrapper-registered kinds get the watch for free.
connection_helpers.go centralizes finalizer add/release logic, scoped correctly per kind.

API

ConnectionReference gains an ObjectRef (full type info: apiGroup + kind + name) alongside the shorthand Name form. CEL validation enforces exactly-one-of and apiGroup == temporal.io; kind is intentionally not validated at admission — an unregistered kind surfaces as ReasonUnknownConnectionKind on the next reconcile.

Design notes

Fingerprint / ApplyWorkerPodSpec invariant: ApplyWorkerPodSpec writes Fingerprint() into the connection-spec-hash annotation, so the two cannot drift apart — the planner compares Fingerprint() against that annotation to decide whether an in-place pod-spec update is needed. The default provider guarantees this by delegating to Fingerprint(); the contract is documented on both methods and locked by a test.
PodSpecApplyOpts: the per-worker context (TemporalNamespace, WorkerDeploymentName, BuildID) is passed as a struct rather than positional strings, removing the risk of swapping indistinguishable arguments at call sites.
Unknown kinds are rejected before the WD finalizer is added, so a bad connectionRef never wedges deletion.

Why "step 1"

The concrete ConnectionSpec still lives in api/v1alpha1 and the default provider is the only one shipped. This PR establishes the interface and the registration mechanism; subsequent steps will let a wrapper binary supply its own provider implementations and (eventually) its own connection CRDs without forking the controller.

Test coverage

connectionprovider: LookupProvider (unknown/found/empty), RefGroupKind, RefName, UnknownKindError.Is.
clientpool (default provider): Fetch (namespaced/cluster-scoped/not-found), GetClient (auth/dial/cache-hit), Fingerprint stability, ApplyWorkerPodSpec (inject + idempotent + annotation-equals-fingerprint), Evict, NewObject, IsClusterScoped, NewDefaultProviders.
Existing reconciler/planner/k8s tests updated for the new signatures.

go build ./..., go vet, and gofmt are clean; affected package tests pass.

Checklist

Builds with go build ./...
gofmt clean
Tests added/updated for new behavior

CLAassistant · 2026-09-16T14:16:39Z

All committers have signed the CLA.

Replace the deprecated mgr.GetEventRecorderFor (returns record.EventRecorder) with mgr.GetEventRecorder (returns events.EventRecorder). The new API's Eventf signature adds 'related' and 'action' parameters; 'related' is nil for all call sites and 'action' reuses the reason string (already UpperCamelCase). Test fake recorder switched from record.NewFakeRecorder to events.NewFakeRecorder.

jaypipes

Phew, this is a lot to digest :) Some suggestions inline for you to take a look at. Will do a more in-depth review later on.

jaypipes · 2026-09-18T10:41:07Z

+	// IsClusterScoped reports whether this kind is cluster-scoped. Used to
+	// scope the watch mapper: a namespaced kind lists WorkerDeployments in the
+	// connection's own namespace; a cluster-scoped kind lists across all.
+	IsClusterScoped() bool


I don't believe this interface method is necessary. It's possible to ask the discovery client whether a GroupKind is cluster-scoped or not:

func kindIsNamespaced( disco discovery.CachedDiscoveryInterface, // See: https://github.com/kubernetes/client-go/blob/master/discovery/cached/disk/cached_discovery.go#L45 gvr schema.GroupVersionResource, // See: https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime/schema#GroupVersionResource ) bool { apiResources, err := disco.ServerResourcesForGroupVersion( gvr.GroupVersion().String(), ) if err != nil { return false } for _, apiResource := range apiResources.APIResources { if apiResource.Name == gvr.Resource { return apiResource.Namespaced } } return false }

jaypipes · 2026-09-18T10:44:31Z

+	// this by delegating to Fingerprint(). A provider whose kind owns pod config
+	// out of band implements the mutation as a no-op but must still write the
+	// fingerprint annotation.
+	ApplyWorkerPodSpec(podSpec *corev1.PodSpec, annotations map[string]string, opts PodSpecApplyOpts) error


Do we need to pass the annotations map? How about just passing the fingerprint that we will set on the PodSpec?

jaypipes · 2026-09-18T10:48:18Z

+func (r *WorkerDeploymentReconciler) findTWDsUsingConnection(ctx context.Context, tc client.Object) []reconcile.Request {
+	return r.findTWDsUsingConnectionKind(connectionprovider.RefGroupKind(temporaliov1alpha1.ConnectionReference{Name: tc.GetName()}), false)(ctx, tc)
+}
+
+func (r *WorkerDeploymentReconciler) findTWDsUsingClusterConnection(
+	ctx context.Context,
+	cc client.Object,
+) []reconcile.Request {
+	return r.findTWDsUsingConnectionKind(connectionprovider.RefGroupKind(temporaliov1alpha1.ConnectionReference{ObjectRef: &corev1.TypedObjectReference{APIGroup: ptr(temporaliov1alpha1.GroupVersion.Group), Kind: "ClusterConnection", Name: cc.GetName()}}), true)(ctx, cc)
+}
+
+// getConnectionByRef returns the underlying object for ref via the provider
+// registry, for tests that need the concrete object.
+func (r *WorkerDeploymentReconciler) getConnectionByRef(
+	ctx context.Context,
+	ref temporaliov1alpha1.ConnectionReference,
+	namespace string,
+) (client.Object, error) {
+	prov, err := connectionprovider.LookupProvider(r.Providers, ref)
+	if err != nil {
+		return nil, err
+	}
+	connection, err := prov.Fetch(ctx, ref, namespace)
+	if err != nil {
+		return nil, err
+	}
+	return connection.Object(), nil
+}


Any reason these methods are defined on WorkerDeploymentReconciler in the clusterconnection_test.go file instead of a non-test file like the newly-added internal/controller/connection_helpers.go file?

jaypipes · 2026-09-18T10:51:18Z

-					Namespace: twd.Namespace,
-				},
-			})
+// findTWDsUsingConnectionKind returns a mapper that enqueues every WorkerDeployment


This should probably be moved to the new internal/controller/connection_helpers.go file along with the other connection-related helpers.

jaypipes · 2026-09-18T10:52:42Z

 	sigs.k8s.io/yaml v1.6.0 // indirect
 )
+
+replace github.com/temporalio/temporal-worker-controller => ../..


jaypipes · 2026-09-18T10:53:39Z

 // If an update is required, it rebuilds the deployment spec and returns a pointer to that Deployment.
 // If no update is needed or the Deployment does not exist, it returns nil.
 func checkAndUpdateDeploymentPodTemplateSpec(
+	ctx context.Context,


Any reason to add the context parameter here? I don't see it being used anywhere.

jaypipes · 2026-09-18T11:02:14Z

 				continue
 			}
-			r.Recorder.Eventf(workerDeploy, corev1.EventTypeWarning, ReasonGateWorkflowFailed,
+			r.Recorder.Eventf(workerDeploy, nil, corev1.EventTypeWarning, ReasonGateWorkflowFailed, ReasonGateWorkflowFailed,


Hmmm.... I'm wondering how this code was previously compiling. It seems we were not calling the EventRecorder.Eventf interface method properly... there should have been a compile-time failure.

eniko-dif requested review from a team and jlegrone as code owners September 16, 2026 14:16

eniko-dif changed the title ~~Introduce ConnectionProvider seam (step 1: controller-as-library)~~ [Issue 246]: Introduce ConnectionProvider seam (step 1: controller-as-library) Sep 16, 2026

eniko-dif added 2 commits September 17, 2026 15:58

Support custom Temporal connections

f4f63ed

Cluster connection fix

b09ab37

eniko-dif force-pushed the eniko/custom-clients branch from e1fda24 to f49a562 Compare September 17, 2026 14:03

eniko-dif force-pushed the eniko/custom-clients branch from f49a562 to 7361be9 Compare September 17, 2026 14:07

jaypipes reviewed Sep 18, 2026

View reviewed changes

Navigation Menu

[Issue 246]: Introduce ConnectionProvider seam (step 1: controller-as-library) - #597

[Issue 246]: Introduce ConnectionProvider seam (step 1: controller-as-library)#597
eniko-dif wants to merge 3 commits into
temporalio:mainfrom
eniko-dif:eniko/custom-clients

eniko-dif commented Sep 16, 2026 •

edited

Loading

Uh oh!

CLAassistant commented Sep 16, 2026 •

edited

Loading

Uh oh!

jaypipes left a comment

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

jaypipes Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

eniko-dif commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

New seam: internal/controller/connectionprovider

Default provider: internal/controller/clientpool/default_provider.go

Reconciler wiring

API

Design notes

Why "step 1"

Test coverage

Checklist

Uh oh!

CLAassistant commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jaypipes Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eniko-dif commented Sep 16, 2026 •

edited

Loading

New seam: `internal/controller/connectionprovider`

Default provider: `internal/controller/clientpool/default_provider.go`

CLAassistant commented Sep 16, 2026 •

edited

Loading