From 3d9ef8948ae98cc92c3325f8440328c669972ee3 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Thu, 23 Jul 2026 09:12:11 -0400 Subject: [PATCH] clean up auth mode and secret name handling This commit is the first of a series that attempts to clean up the Connection CRD and handling of Connection CRs in the client pool and controller packages. This moves the `AuthMode` type out of `internal/clientpool` and into the `api/v1alpha1` package but *does not* add a field to `ConnectionSpec` for auth mode. Instead, we add a `ConnectionSpec.AuthMode()` method that returns the calculate authentication mode after looking at the different fields of `ConnectionSpec`. Similarly, we add a `ConnectionSpec.SecretName()` method that returns the name of the Kubernetes Secret that holds the authentication information for the Connection. We clean up code in clientpool and controller by now calling these methods on `ConnectionSpec`. Signed-off-by: Jay Pipes --- api/v1alpha1/connection_types.go | 44 +++++++++++++++++++ internal/controller/clientpool/clientpool.go | 41 +++++++---------- .../controller/clientpool/clientpool_test.go | 32 +++++++------- internal/controller/reconciler_events_test.go | 2 +- internal/controller/worker_controller.go | 44 ++++--------------- 5 files changed, 86 insertions(+), 77 deletions(-) diff --git a/api/v1alpha1/connection_types.go b/api/v1alpha1/connection_types.go index 4c11bb11..f3dc1e50 100644 --- a/api/v1alpha1/connection_types.go +++ b/api/v1alpha1/connection_types.go @@ -5,10 +5,22 @@ package v1alpha1 import ( + "errors" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// AuthMode is the mode for authenticating to a Temporal server. +type AuthMode string + +const ( + AuthModeTLS AuthMode = "TLS" + AuthModeAPIKey AuthMode = "API_KEY" + AuthModeNoCredentials AuthMode = "NO_CREDENTIALS" + // Add more auth modes here as they are supported +) + // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. // SecretReference contains the name of a Secret resource in the same namespace. @@ -59,6 +71,38 @@ type ConnectionSpec struct { APIKeySecretRef *corev1.SecretKeySelector `json:"apiKeySecretRef,omitempty"` } +// AuthMode returns the authentication mode for the ConnectionSpec. +func (s ConnectionSpec) AuthMode() AuthMode { + switch { + case s.MutualTLSSecretRef != nil: + return AuthModeTLS + case s.APIKeySecretRef != nil: + return AuthModeAPIKey + default: + return AuthModeNoCredentials + } +} + +// SecretName extracts the secret name from the ConnectionSpec, returning an +// error if the secret name is missing and the authentication mode requires it. +func (s ConnectionSpec) SecretName() (string, error) { + authMode := s.AuthMode() + switch authMode { + case AuthModeTLS: + if s.MutualTLSSecretRef == nil || s.MutualTLSSecretRef.Name == "" { + return "", errors.New("TLS secret name is not set") + } + return s.MutualTLSSecretRef.Name, nil + case AuthModeAPIKey: + if s.APIKeySecretRef == nil || s.APIKeySecretRef.Name == "" { + return "", errors.New("API key secret name is not set") + } + return s.APIKeySecretRef.Name, nil + default: + return "", nil + } +} + func (s ConnectionSpec) TLSServerName() string { if s.TLS == nil { return "" diff --git a/internal/controller/clientpool/clientpool.go b/internal/controller/clientpool/clientpool.go index bfc8e169..19b5f2bb 100644 --- a/internal/controller/clientpool/clientpool.go +++ b/internal/controller/clientpool/clientpool.go @@ -22,21 +22,12 @@ import ( runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) -type AuthMode string - -const ( - AuthModeTLS AuthMode = "TLS" - AuthModeAPIKey AuthMode = "API_KEY" - AuthModeNoCredentials AuthMode = "NO_CREDENTIALS" - // Add more auth modes here as they are supported -) - type ClientPoolKey struct { HostPort string TLSServerName string - Namespace string // Temporal namespace - SecretName string // Include secret name in key to invalidate cache when the secret name changes - AuthMode AuthMode // Include auth mode in key to invalidate cache when the auth mode changes for the secret + Namespace string // Temporal namespace + SecretName string // Include secret name in key to invalidate cache when the secret name changes + AuthMode v1alpha1.AuthMode // Include auth mode in key to invalidate cache when the auth mode changes for the secret } type MTLSAuth struct { @@ -45,7 +36,7 @@ type MTLSAuth struct { } type ClientAuth struct { - mode AuthMode + mode v1alpha1.AuthMode mTLS *MTLSAuth // non-nil when mode == AuthMTLS, nil when mode == AuthAPIKey } @@ -100,7 +91,7 @@ func (cp *ClientPool) GetSDKClient(key ClientPoolKey) (sdkclient.Client, bool) { return nil, false } - if key.AuthMode == AuthModeTLS { + if key.AuthMode == v1alpha1.AuthModeTLS { // Check if any certificate is expired expired, err := isCertificateExpired(info.auth.mTLS.expiryTime) if err != nil { @@ -184,10 +175,10 @@ func (cp *ClientPool) fetchClientUsingMTLSSecret(secret corev1.Secret, opts NewC TLSServerName: tlsServerName, Namespace: opts.TemporalNamespace, SecretName: opts.Spec.MutualTLSSecretRef.Name, - AuthMode: AuthModeTLS, + AuthMode: v1alpha1.AuthModeTLS, } auth := ClientAuth{ - mode: AuthModeTLS, + mode: v1alpha1.AuthModeTLS, mTLS: &MTLSAuth{tlsConfig: clientOpts.ConnectionOptions.TLS, expiryTime: expiryTime}, } return &clientOpts, &key, &auth, nil @@ -217,10 +208,10 @@ func (cp *ClientPool) fetchClientUsingAPIKeySecret(opts NewClientOptions) (*sdkc TLSServerName: tlsServerName, Namespace: opts.TemporalNamespace, SecretName: opts.Spec.APIKeySecretRef.Name, - AuthMode: AuthModeAPIKey, + AuthMode: v1alpha1.AuthModeAPIKey, } auth := ClientAuth{ - mode: AuthModeAPIKey, + mode: v1alpha1.AuthModeAPIKey, mTLS: nil, } @@ -244,10 +235,10 @@ func (cp *ClientPool) fetchClientUsingNoCredentials(opts NewClientOptions) (*sdk TLSServerName: tlsServerName, Namespace: opts.TemporalNamespace, SecretName: "", - AuthMode: AuthModeNoCredentials, + AuthMode: v1alpha1.AuthModeNoCredentials, } auth := ClientAuth{ - mode: AuthModeNoCredentials, + mode: v1alpha1.AuthModeNoCredentials, mTLS: nil, } @@ -257,7 +248,7 @@ func (cp *ClientPool) fetchClientUsingNoCredentials(opts NewClientOptions) (*sdk func (cp *ClientPool) ParseClientSecret( ctx context.Context, secretName string, - authMode AuthMode, + authMode v1alpha1.AuthMode, opts NewClientOptions, ) (*sdkclient.Options, *ClientPoolKey, *ClientAuth, error) { // Fetch the secret from k8s cluster, if it exists. Otherwise, create a connection with the server without using any credentials. @@ -273,21 +264,21 @@ func (cp *ClientPool) ParseClientSecret( // Check the secret type switch authMode { - case AuthModeTLS: + case v1alpha1.AuthModeTLS: if secret.Type != corev1.SecretTypeTLS && secret.Type != corev1.SecretTypeOpaque { err := fmt.Errorf("secret %s must be of type kubernetes.io/tls or Opaque", secret.Name) return nil, nil, nil, err } return cp.fetchClientUsingMTLSSecret(secret, opts) - case AuthModeAPIKey: + case v1alpha1.AuthModeAPIKey: if secret.Type != corev1.SecretTypeOpaque { err := fmt.Errorf("secret %s must be of type kubernetes.io/opaque", secret.Name) return nil, nil, nil, err } return cp.fetchClientUsingAPIKeySecret(opts) - case AuthModeNoCredentials: + case v1alpha1.AuthModeNoCredentials: return cp.fetchClientUsingNoCredentials(opts) default: @@ -305,7 +296,7 @@ func (cp *ClientPool) DialAndUpsertClient(clientOpts sdkclient.Options, clientPo // (non-namespace-scoped) RPC that fails with namespace-scoped API keys // on Temporal Cloud. This is safe because client.Dial already calls // GetSystemInfo internally, which is a superset of CheckHealth. - if clientAuth.mode != AuthModeAPIKey { + if clientAuth.mode != v1alpha1.AuthModeAPIKey { if _, err := c.CheckHealth(context.Background(), &sdkclient.CheckHealthRequest{}); err != nil { c.Close() return nil, fmt.Errorf("temporal server health check failed: %w", err) diff --git a/internal/controller/clientpool/clientpool_test.go b/internal/controller/clientpool/clientpool_test.go index 7f17e2b5..a26cf799 100644 --- a/internal/controller/clientpool/clientpool_test.go +++ b/internal/controller/clientpool/clientpool_test.go @@ -230,8 +230,8 @@ func TestFetchMTLS_ValidCert_Succeeds(t *testing.T) { clientOpts, key, auth, err := cp.fetchClientUsingMTLSSecret(secret, makeOpts("localhost:7233")) require.NoError(t, err) - assert.Equal(t, AuthModeTLS, key.AuthMode) - assert.Equal(t, AuthModeTLS, auth.mode) + assert.Equal(t, temporaliov1alpha1.AuthModeTLS, key.AuthMode) + assert.Equal(t, temporaliov1alpha1.AuthModeTLS, auth.mode) assert.NotNil(t, auth.mTLS) assert.NotNil(t, clientOpts.ConnectionOptions.TLS) assert.Len(t, clientOpts.ConnectionOptions.TLS.Certificates, 1) @@ -306,7 +306,7 @@ func TestFetchNoCredentials_TLSServerNameOverride(t *testing.T) { require.NotNil(t, clientOpts.ConnectionOptions.TLS) assert.Equal(t, "temporal-cloud.example.com", clientOpts.ConnectionOptions.TLS.ServerName) assert.Equal(t, "temporal-cloud.example.com", key.TLSServerName) - assert.Equal(t, AuthModeNoCredentials, auth.mode) + assert.Equal(t, temporaliov1alpha1.AuthModeNoCredentials, auth.mode) } func newTestPoolWithFakeClient(objects ...runtime.Object) *ClientPool { @@ -349,8 +349,8 @@ func TestFetchAPIKey_CredentialsAndTLSSet(t *testing.T) { clientOpts, key, auth, err := cp.fetchClientUsingAPIKeySecret(opts) require.NoError(t, err) - assert.Equal(t, AuthModeAPIKey, key.AuthMode) - assert.Equal(t, AuthModeAPIKey, auth.mode) + assert.Equal(t, temporaliov1alpha1.AuthModeAPIKey, key.AuthMode) + assert.Equal(t, temporaliov1alpha1.AuthModeAPIKey, auth.mode) assert.Nil(t, auth.mTLS) assert.NotNil(t, clientOpts.Credentials, "API key credentials must be set") assert.NotNil(t, clientOpts.ConnectionOptions.TLS, "TLS config must be non-nil for gRPC API key transport") @@ -401,8 +401,8 @@ func TestParseClientSecret_OpaqueSecretType(t *testing.T) { _, key, auth, err := cp.fetchClientUsingMTLSSecret(opaqueSecret, makeOpts("localhost:7233")) require.NoError(t, err, "Opaque secret with tls.crt and tls.key should be accepted for mTLS auth") - assert.Equal(t, AuthModeTLS, key.AuthMode) - assert.Equal(t, AuthModeTLS, auth.mode) + assert.Equal(t, temporaliov1alpha1.AuthModeTLS, key.AuthMode) + assert.Equal(t, temporaliov1alpha1.AuthModeTLS, auth.mode) assert.NotNil(t, auth.mTLS) } @@ -414,15 +414,15 @@ func TestParseClientSecret_OpaqueSecretType(t *testing.T) { // Cloud, namespace-scoped API keys do not have permission to call the system-scoped // CheckHealth RPC, so every connection attempt failed. // -// After the fix, CheckHealth is skipped for AuthModeAPIKey because client.Dial already +// After the fix, CheckHealth is skipped for temporaliov1alpha1.AuthModeAPIKey because client.Dial already // calls GetSystemInfo internally (a superset of CheckHealth). func TestDialAndUpsert_APIKeySkipsCheckHealth(t *testing.T) { mock := &mockSDKClient{} cp := newTestPool() cp.dialFn = func(_ sdkclient.Options) (sdkclient.Client, error) { return mock, nil } - key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: AuthModeAPIKey} - auth := ClientAuth{mode: AuthModeAPIKey} + key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: temporaliov1alpha1.AuthModeAPIKey} + auth := ClientAuth{mode: temporaliov1alpha1.AuthModeAPIKey} c, err := cp.DialAndUpsertClient(sdkclient.Options{}, key, auth) @@ -439,9 +439,9 @@ func TestDialAndUpsert_TLSCallsCheckHealth(t *testing.T) { cp := newTestPool() cp.dialFn = func(_ sdkclient.Options) (sdkclient.Client, error) { return mock, nil } - key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: AuthModeTLS} + key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: temporaliov1alpha1.AuthModeTLS} auth := ClientAuth{ - mode: AuthModeTLS, + mode: temporaliov1alpha1.AuthModeTLS, mTLS: &MTLSAuth{tlsConfig: &tls.Config{}, expiryTime: time.Now().Add(time.Hour)}, } @@ -458,8 +458,8 @@ func TestDialAndUpsert_NoCredsCallsCheckHealth(t *testing.T) { cp := newTestPool() cp.dialFn = func(_ sdkclient.Options) (sdkclient.Client, error) { return mock, nil } - key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: AuthModeNoCredentials} - auth := ClientAuth{mode: AuthModeNoCredentials} + key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: temporaliov1alpha1.AuthModeNoCredentials} + auth := ClientAuth{mode: temporaliov1alpha1.AuthModeNoCredentials} c, err := cp.DialAndUpsertClient(sdkclient.Options{}, key, auth) @@ -476,7 +476,7 @@ func TestEvictClient_RemovesAndClosesClient(t *testing.T) { HostPort: "localhost:7233", Namespace: "default", SecretName: "my-secret", - AuthMode: AuthModeAPIKey, + AuthMode: temporaliov1alpha1.AuthModeAPIKey, } mock := &mockSDKClient{} cp.SetClientForTesting(key, mock) @@ -493,7 +493,7 @@ func TestEvictClient_RemovesAndClosesClient(t *testing.T) { func TestEvictClient_NoopWhenKeyAbsent(t *testing.T) { cp := newTestPool() - key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: AuthModeNoCredentials} + key := ClientPoolKey{HostPort: "localhost:7233", Namespace: "default", AuthMode: temporaliov1alpha1.AuthModeNoCredentials} // Should not panic when key is not in the pool cp.EvictClient(key) } diff --git a/internal/controller/reconciler_events_test.go b/internal/controller/reconciler_events_test.go index 92b374cd..dc7fd60c 100644 --- a/internal/controller/reconciler_events_test.go +++ b/internal/controller/reconciler_events_test.go @@ -283,7 +283,7 @@ func noCredsPoolKey(hostPort, temporalNamespace string) clientpool.ClientPoolKey HostPort: hostPort, Namespace: temporalNamespace, SecretName: "", - AuthMode: clientpool.AuthModeNoCredentials, + AuthMode: temporaliov1alpha1.AuthModeNoCredentials, } } diff --git a/internal/controller/worker_controller.go b/internal/controller/worker_controller.go index c0acb5cc..a6232550 100644 --- a/internal/controller/worker_controller.go +++ b/internal/controller/worker_controller.go @@ -55,34 +55,6 @@ const ( finalizerName = "temporal.io/delete-protection" ) -// getAPIKeySecretName extracts the secret name from a SecretKeySelector -func getAPIKeySecretName(secretRef *corev1.SecretKeySelector) (string, error) { - if secretRef != nil && secretRef.Name != "" { - return secretRef.Name, nil - } - - return "", errors.New("API key secret name is not set") -} - -func getTLSSecretName(secretRef *temporaliov1alpha1.SecretReference) (string, error) { - if secretRef != nil && secretRef.Name != "" { - return secretRef.Name, nil - } - - return "", errors.New("TLS secret name is not set") -} - -func resolveAuthSecretName(tc *temporaliov1alpha1.Connection) (clientpool.AuthMode, string, error) { - if tc.Spec.MutualTLSSecretRef != nil { - name, err := getTLSSecretName(tc.Spec.MutualTLSSecretRef) - return clientpool.AuthModeTLS, name, err - } else if tc.Spec.APIKeySecretRef != nil { - name, err := getAPIKeySecretName(tc.Spec.APIKeySecretRef) - return clientpool.AuthModeAPIKey, name, err - } - return clientpool.AuthModeNoCredentials, "", nil -} - // WorkerDeploymentReconciler reconciles a WorkerDeployment object type WorkerDeploymentReconciler struct { client.Client @@ -261,7 +233,8 @@ func (r *WorkerDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req } // Get the Auth Mode and Secret Name - authMode, secretName, err := resolveAuthSecretName(&connection) + authMode := connection.Spec.AuthMode() + secretName, err := connection.Spec.SecretName() if err != nil { l.Error(err, "unable to resolve auth secret name") r.recordWarningAndSetBlocked(ctx, &workerDeploy, @@ -563,22 +536,23 @@ func (r *WorkerDeploymentReconciler) handleDeletion( // Resolve Connection. // The Connection is guaranteed to exist because we hold a finalizer on it // that prevents deletion while any WD references it. - var temporalConnection temporaliov1alpha1.Connection + var connection temporaliov1alpha1.Connection if err := r.Get(ctx, types.NamespacedName{ Name: workerDeploy.Spec.WorkerOptions.ConnectionRef.Name, Namespace: workerDeploy.Namespace, - }, &temporalConnection); err != nil { + }, &connection); err != nil { return fmt.Errorf("unable to fetch Connection: %w", err) } - authMode, secretName, err := resolveAuthSecretName(&temporalConnection) + authMode := connection.Spec.AuthMode() + secretName, err := connection.Spec.SecretName() if err != nil { return fmt.Errorf("unable to resolve auth secret name: %w", err) } temporalClient, ok := r.TemporalClientPool.GetSDKClient(clientpool.ClientPoolKey{ - HostPort: temporalConnection.Spec.HostPort, - TLSServerName: temporalConnection.Spec.TLSServerName(), + HostPort: connection.Spec.HostPort, + TLSServerName: connection.Spec.TLSServerName(), Namespace: workerDeploy.Spec.WorkerOptions.TemporalNamespace, SecretName: secretName, AuthMode: authMode, @@ -587,7 +561,7 @@ func (r *WorkerDeploymentReconciler) handleDeletion( clientOpts, key, clientAuth, err := r.TemporalClientPool.ParseClientSecret(ctx, secretName, authMode, clientpool.NewClientOptions{ K8sNamespace: workerDeploy.Namespace, TemporalNamespace: workerDeploy.Spec.WorkerOptions.TemporalNamespace, - Spec: temporalConnection.Spec, + Spec: connection.Spec, Identity: getControllerIdentity(), }) if err != nil {