temporal: update sdk to v1.14.0 with lazy client

api/config/service/temporal/v1/temporal.proto

@@ -17,10 +17,4 @@ message Config {
 
 message ConnectionOptions {
   bool use_system_ca_bundle = 1;
-
-  // If set to true, the Temporal client will attempt to connect to the Temporal server when GetNamespaceClient is
-  // called. In general the health check should not be enabled unless the only purpose of the configured Clutch instance
-  // is to execute Temporal worker code, as it may block Clutch from running at all in the event Temporal is down. This
-  // can create a circular dependency if Clutch also hosts tools that could be used to mitigate a Temporal outage.
-  bool enable_health_check = 2;
 }

backend/go.mod

@@ -44,7 +44,7 @@ require (
 	github.com/slack-go/slack v0.10.2
 	github.com/stretchr/testify v1.7.1
 	github.com/uber-go/tally/v4 v4.1.1
-	go.temporal.io/sdk v1.13.1
+	go.temporal.io/sdk v1.14.0
 	go.temporal.io/sdk/contrib/tally v0.1.0
 	go.uber.org/zap v1.21.0
 	golang.org/x/net v0.0.0-20220225172249-27dd8689420f
@@ -130,7 +130,7 @@ require (
 	github.com/twmb/murmur3 v1.1.5 // indirect
 	github.com/xanzy/ssh-agent v0.3.0 // indirect
 	go.opencensus.io v0.23.0 // indirect
-	go.temporal.io/api v1.6.1-0.20211110205628-60c98e9cbfe2 // indirect
+	go.temporal.io/api v1.7.1-0.20220223032354-6e6fe738916a // indirect
 	go.uber.org/atomic v1.9.0 // indirect
 	go.uber.org/multierr v1.7.0 // indirect
 	golang.org/x/crypto v0.0.0-20211108221036-ceb1ce70b4fa // indirect

backend/service/temporal/temporal.go

@@ -8,9 +8,10 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"fmt"
+	"sync"
 
 	"github.com/uber-go/tally/v4"
-	"go.temporal.io/sdk/client"
+	temporalclient "go.temporal.io/sdk/client"
 	temporaltally "go.temporal.io/sdk/contrib/tally"
 	"go.temporal.io/sdk/log"
 	"go.uber.org/zap"
@@ -31,21 +32,27 @@ func New(cfg *anypb.Any, logger *zap.Logger, scope tally.Scope) (service.Service
 }
 
 type ClientManager interface {
-	GetNamespaceClient(namespace string) (client.Client, error)
+	GetNamespaceClient(namespace string) (Client, error)
+}
+
+// Client exists to protect users from creating a connection during instantiation of a component,
+// since Temporal's NewClient function has the side effect of connecting to the server. See
+// https://github.com/temporalio/sdk-go/issues/753 for more details.
+type Client interface {
+	// GetConnection will connect to the server in order to check its capabilities on the first call.
+	// Subsequent calls to GetConnection will return a cached client.
+	GetConnection() (temporalclient.Client, error)
 }
 
 func newClient(cfg *temporalv1.Config, logger *zap.Logger, scope tally.Scope) (ClientManager, error) {
-	ret := &clientImpl{
+	ret := &clientManagerImpl{
 		hostPort:       fmt.Sprintf("%s:%d", cfg.Host, cfg.Port),
 		metricsHandler: temporaltally.NewMetricsHandler(scope),
 		logger:         newTemporalLogger(logger),
-
-		// Disable the healthcheck by default (i.e. connect lazily) as it's not normally preferable (see config proto documentation).
-		copts: client.ConnectionOptions{DisableHealthCheck: true},
+		copts:          temporalclient.ConnectionOptions{},
 	}
 
 	if cfg.ConnectionOptions != nil {
-		ret.copts.DisableHealthCheck = !cfg.ConnectionOptions.EnableHealthCheck
 		if cfg.ConnectionOptions.UseSystemCaBundle {
 			certs, err := x509.SystemCertPool()
 			if err != nil {
@@ -60,26 +67,43 @@ func newClient(cfg *temporalv1.Config, logger *zap.Logger, scope tally.Scope) (C
 	return ret, nil
 }
 
-type clientImpl struct {
+type clientManagerImpl struct {
 	hostPort       string
 	logger         log.Logger
-	metricsHandler client.MetricsHandler
-	copts          client.ConnectionOptions
+	metricsHandler temporalclient.MetricsHandler
+	copts          temporalclient.ConnectionOptions
 }
 
-func (c *clientImpl) GetNamespaceClient(namespace string) (client.Client, error) {
-	tc, err := client.NewClient(client.Options{
-		HostPort:          c.hostPort,
-		Logger:            c.logger,
-		MetricsHandler:    c.metricsHandler,
-		Namespace:         namespace,
-		ConnectionOptions: c.copts,
-	})
-	if err != nil {
-		return nil, err
-	}
+func (c *clientManagerImpl) GetNamespaceClient(namespace string) (Client, error) {
+	return &lazyClientImpl{
+		opts: &temporalclient.Options{
+			HostPort:          c.hostPort,
+			Logger:            c.logger,
+			MetricsHandler:    c.metricsHandler,
+			Namespace:         namespace,
+			ConnectionOptions: c.copts,
+		},
+	}, nil
+}
 
-	// TODO: cache clients? is there any benefit?
+type lazyClientImpl struct {
+	mu           sync.Mutex
+	cachedClient temporalclient.Client
+
+	opts *temporalclient.Options
+}
+
+func (l *lazyClientImpl) GetConnection() (temporalclient.Client, error) {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	if l.cachedClient == nil {
+		c, err := temporalclient.NewClient(*l.opts)
+		if err != nil {
+			return nil, err
+		}
+		l.cachedClient = c
+	}
 
-	return tc, err
+	return l.cachedClient, nil
 }

backend/service/temporal/temporal_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/uber-go/tally/v4"
+	temporalclient "go.temporal.io/sdk/client"
 	"go.uber.org/zap"
 	"google.golang.org/protobuf/types/known/anypb"
 
@@ -20,26 +21,28 @@ func TestNew(t *testing.T) {
 
 	c, err := New(anycfg, zap.NewNop(), tally.NoopScope)
 	assert.NoError(t, err)
-	impl := c.(*clientImpl)
+	impl := c.(*clientManagerImpl)
 	assert.NotNil(t, impl.logger)
 	assert.NotNil(t, impl.metricsHandler)
 	assert.Nil(t, impl.copts.TLS)
-	assert.True(t, impl.copts.DisableHealthCheck)
 	assert.Equal(t, impl.hostPort, "dns:///example.com:9233")
 }
 
 func TestNewClientWithConnectionOptions(t *testing.T) {
 	cfg := &temporalv1.Config{
 		Host:              "dns:///example.com",
 		Port:              9233,
-		ConnectionOptions: &temporalv1.ConnectionOptions{UseSystemCaBundle: true, EnableHealthCheck: true},
+		ConnectionOptions: &temporalv1.ConnectionOptions{UseSystemCaBundle: true},
 	}
 	c, err := newClient(cfg, zap.NewNop(), tally.NoopScope)
 	assert.NoError(t, err)
 
-	impl := c.(*clientImpl)
+	impl := c.(*clientManagerImpl)
 	assert.NotNil(t, impl.copts.TLS.RootCAs)
-	assert.False(t, impl.copts.DisableHealthCheck)
+}
+
+type mockClient struct {
+	temporalclient.Client
 }
 
 func TestGetNamespaceClient(t *testing.T) {
@@ -48,4 +51,14 @@ func TestGetNamespaceClient(t *testing.T) {
 	client, err := c.GetNamespaceClient("foo-namespace")
 	assert.NoError(t, err)
 	assert.NotNil(t, client)
+
+	impl := client.(*lazyClientImpl)
+	assert.Nil(t, impl.cachedClient)
+
+	mc := &mockClient{}
+
+	impl.cachedClient = mc
+	ret, err := client.GetConnection()
+	assert.NoError(t, err)
+	assert.Equal(t, mc, ret)
 }