FEATURE: Add ClusterConnection cluster-scoped resource - #549
FEATURE: Add ClusterConnection cluster-scoped resource#549niyomukiza-mechack wants to merge 4 commits into
Conversation
| - apiGroups: | ||
| - temporal.io | ||
| resources: | ||
| - clusterconnections |
There was a problem hiding this comment.
Are we sure clusterconnections belong here under the Role? Since its cluster scoped it seems like it should be associated only with a ClusterRole?
There was a problem hiding this comment.
Good catch! Should be fixed. clusterconnections was landing in the namespaced Role because hack/sync-rbac-rules.py filtered at block granularity, and controller-gen merged clusterconnections into the same block as the namespaced connections
jaypipes
left a comment
There was a problem hiding this comment.
Thanks @niyomukiza-mechack! I've left some thoughts for you inline. I think we want to think about the structure of the ConnectionRef object in a bit of a longer-term view.
| // +optional | ||
| // +kubebuilder:default=Connection | ||
| // +kubebuilder:validation:Enum=Connection;ClusterConnection | ||
| Kind string `json:"kind,omitempty"` |
There was a problem hiding this comment.
As of August, 2026 we only have a single API version (v1alpha1). In the future, however, we will likely have other API versions and potentially other API groups.
Best to think long-term and let's call this field ObjectRef and make it a pointer to a corev1.TypedObjectReference.
TypedObjectReference contains all the type information we might need in the future (for both cluster and namespace-scoped resources).
Another benefit to using TypedObjectReference here is that you can make the Name field optional and put a validation rule in place that says either Name or ObjectRef must be populated. And you can then remove some of the messy conditionals below in releaseConnectionFinalizerIfUnused where you are checking the Name field and trying to determine if the name refers to a Connection or a ClusterConnection...
There was a problem hiding this comment.
Super helpful and totally makes sense!
| ctx context.Context, | ||
| workerDeploy *temporaliov1alpha1.WorkerDeployment, | ||
| ) (temporaliov1alpha1.ConnectionSpec, client.Object, error) { | ||
| return r.getConnectionByRef(ctx, workerDeploy.Spec.WorkerOptions.ConnectionRef, workerDeploy.Namespace) |
There was a problem hiding this comment.
Is there a need for a resolveConnection helper method that is one line that calls the getConnectionByRef helper method? If not, recommend removing this level of indirection.
There was a problem hiding this comment.
removed
| // finalizer from the previously-referenced connection if no other WD uses it. | ||
| // The new connection is already protected by ensureConnectionFinalizer above, | ||
| // so the WD is never left unprotected. | ||
| current := workerDeploy.Spec.WorkerOptions.ConnectionRef |
There was a problem hiding this comment.
Note that you can avoid the below code with the following guard:
// If the desired state of the resource changed, check that any changes
// to the referenced connection type are handled.
if workerDeploy.Generation != workerDeploy.Status.ObservedGeneration {
}| } | ||
| // Preserve conditions that were set during this reconciliation | ||
| status.Conditions = workerDeploy.Status.Conditions | ||
| status.ObservedConnectionRef = workerDeploy.Spec.WorkerOptions.ConnectionRef.DeepCopy() |
| return nil | ||
| } | ||
|
|
||
| // removeConnectionFinalizerIfUnused releases the finalizer from the WD's current |
There was a problem hiding this comment.
I don't think there's a good reason to have this helper method that is just a one-liner that calls another (very similarly-named) helper method. I would prefer to just call releaseConnectionFinalizerIfUnused directly.
What was changed
api/v1alpha1/clusterconnection_types.go: Added a new cluster-scoped type reusingConnectionSpec and ConnectionStatus
workerdeployment_types.go: added optional Kind field to ConnectionReference(default: Connection, backward compatible)
worker_controller.go:resolveConnection()helper for kind-aware fetch,generalized
ensureConnectionFinalizerto client.Object, and cluster-wideremoveConnectionFinalizerIfUnused(cross-namespace list + kind-aware match), andfindTWDsUsingClusterConnectionmapper. Also added ClusterConnection watch + RBACWhy?
Context: #456
Today, TWC requires a Connection resource in every namespace. When there's many namespaces, there's a lot of duplication for Connection Resource.
ClusterConnectionlets you define the connection once, cluster-wide, and have WorkerDeployments in any namespace reference it. The credential Secret still lives per-namespace since a pod can only mount a Secret from its own namespace.Checklist
Closes
How was this tested:
Unit tests in
internal/controller/clusterconnection_test.goLocal Kind cluster:
##Migration path testing (the kind-change scenario):