Repository: matrixorigin/matrixone
Pull request #4318 (2022-08-02 05:49:06)

logservice: always validate client config first
**What type of PR is this?**

- [ ] API-change
- [ ] BUG
- [x] Improvement
- [ ] Documentation
- [ ] Feature
- [ ] Test and CI
- [ ] Code Refactoring

**Which issue(s) this PR fixes:**

issue #

**What this PR does / why we need it:**

always validate client config first

**Special notes for your reviewer:**

Not Available

**Additional documentation (e.g. design docs, usage docs, etc.):**

Not Available



Patch:
diff --git a/pkg/logservice/client.go b/pkg/logservice/client.go
index d40906719..ea9f85d3f 100644
--- a/pkg/logservice/client.go
+++ b/pkg/logservice/client.go
@@ -49,21 +49,6 @@ func IsTempError(err error) bool {
 	return isTempError(err)
 }
 
-// ClientConfig is the configuration for log service clients.
-type ClientConfig struct {
-	// ReadOnly indicates whether this is a read-only client.
-	ReadOnly bool
-	// LogShardID is the shard ID of the log service shard to be used.
-	LogShardID uint64
-	// DNReplicaID is the replica ID of the DN that owns the created client.
-	DNReplicaID uint64
-	// DiscoveryAddress is the Log Service discovery address provided by k8s.
-	DiscoveryAddress string
-	// LogService nodes service addresses. This field is provided for testing
-	// purposes only.
-	ServiceAddresses []string
-}
-
 // Client is the Log Service Client interface exposed to the DN.
 type Client interface {
 	// Close closes the client.
@@ -115,6 +100,9 @@ var _ Client = (*managedClient)(nil)
 // to the Log Service in parallel, multiple clients should be created and used
 // to do so.
 func NewClient(ctx context.Context, cfg ClientConfig) (Client, error) {
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
 	client, err := newClient(ctx, cfg)
 	if err != nil {
 		return nil, err
diff --git a/pkg/logservice/config.go b/pkg/logservice/config.go
index d3a17365f..d6f18b39e 100644
--- a/pkg/logservice/config.go
+++ b/pkg/logservice/config.go
@@ -45,14 +45,6 @@ const (
 	defaultHeartbeatInterval = time.Second
 )
 
-// HAKeeperClientConfig is the config for HAKeeper clients.
-type HAKeeperClientConfig struct {
-	// DiscoveryAddress is the Log Service discovery address provided by k8s.
-	DiscoveryAddress string `toml:"discovery-address"`
-	// ServiceAddresses is a list of well known Log Services' service addresses.
-	ServiceAddresses []string `toml:"service-addresses"`
-}
-
 // Config defines the Configurations supported by the Log Service.
 type Config struct {
 	// FS is the underlying virtual FS used by the log service. Leave it as empty
@@ -328,6 +320,51 @@ func (c *Config) Fill() {
 
 }
 
+// HAKeeperClientConfig is the config for HAKeeper clients.
+type HAKeeperClientConfig struct {
+	// DiscoveryAddress is the Log Service discovery address provided by k8s.
+	DiscoveryAddress string `toml:"discovery-address"`
+	// ServiceAddresses is a list of well known Log Services' service addresses.
+	ServiceAddresses []string `toml:"service-addresses"`
+}
+
+// Validate validates the HAKeeperClientConfig.
+func (c *HAKeeperClientConfig) Validate() error {
+	if len(c.DiscoveryAddress) == 0 && len(c.ServiceAddresses) == 0 {
+		return errors.Wrapf(ErrInvalidConfig, "empty HAKeeperClientConfig")
+	}
+	return nil
+}
+
+// ClientConfig is the configuration for log service clients.
+type ClientConfig struct {
+	// ReadOnly indicates whether this is a read-only client.
+	ReadOnly bool
+	// LogShardID is the shard ID of the log service shard to be used.
+	LogShardID uint64
+	// DNReplicaID is the replica ID of the DN that owns the created client.
+	DNReplicaID uint64
+	// DiscoveryAddress is the Log Service discovery address provided by k8s.
+	DiscoveryAddress string
+	// LogService nodes service addresses. This field is provided for testing
+	// purposes only.
+	ServiceAddresses []string
+}
+
+// Validate validates the ClientConfig.
+func (c *ClientConfig) Validate() error {
+	if c.LogShardID == 0 {
+		return errors.Wrapf(ErrInvalidConfig, "invalid LogShardID")
+	}
+	if c.DNReplicaID == 0 {
+		return errors.Wrapf(ErrInvalidConfig, "invalid DNReplicaID")
+	}
+	if len(c.DiscoveryAddress) == 0 && len(c.ServiceAddresses) == 0 {
+		return errors.Wrapf(ErrInvalidConfig, "log service address info is empty")
+	}
+	return nil
+}
+
 func splitAddresses(v string) []string {
 	results := make([]string, 0)
 	parts := strings.Split(v, ";")
diff --git a/pkg/logservice/hakeeper_client.go b/pkg/logservice/hakeeper_client.go
index ab05cbfa2..fec04816e 100644
--- a/pkg/logservice/hakeeper_client.go
+++ b/pkg/logservice/hakeeper_client.go
@@ -71,18 +71,27 @@ var _ LogHAKeeperClient = (*managedHAKeeperClient)(nil)
 // NewCNHAKeeperClient creates a HAKeeper client to be used by a CN node.
 func NewCNHAKeeperClient(ctx context.Context,
 	cfg HAKeeperClientConfig) (CNHAKeeperClient, error) {
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
 	return newManagedHAKeeperClient(ctx, cfg)
 }
 
 // NewDNHAKeeperClient creates a HAKeeper client to be used by a DN node.
 func NewDNHAKeeperClient(ctx context.Context,
 	cfg HAKeeperClientConfig) (DNHAKeeperClient, error) {
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
 	return newManagedHAKeeperClient(ctx, cfg)
 }
 
 // NewLogHAKeeperClient creates a HAKeeper client to be used by a Log Service node.
 func NewLogHAKeeperClient(ctx context.Context,
 	cfg HAKeeperClientConfig) (LogHAKeeperClient, error) {
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
 	return newManagedHAKeeperClient(ctx, cfg)
 }
 


Test patch:
diff --git a/pkg/logservice/client_test.go b/pkg/logservice/client_test.go
index 25aab7449..5a4910e0c 100644
--- a/pkg/logservice/client_test.go
+++ b/pkg/logservice/client_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cockroachdb/errors"
 	"github.com/lni/dragonboat/v4"
 	"github.com/lni/goutils/leaktest"
 	"github.com/lni/vfs"
@@ -63,6 +64,13 @@ func runClientTest(t *testing.T,
 	fn(t, service, scfg, c)
 }
 
+func TestClientConfigIsValidated(t *testing.T) {
+	cfg := ClientConfig{}
+	cc, err := NewClient(context.TODO(), cfg)
+	assert.Nil(t, cc)
+	assert.True(t, errors.Is(err, ErrInvalidConfig))
+}
+
 func TestClientCanBeReset(t *testing.T) {
 	fn := func(t *testing.T, s *Service, cfg ClientConfig, c Client) {
 		client := c.(*managedClient)
diff --git a/pkg/logservice/config_test.go b/pkg/logservice/config_test.go
index 03009e52a..3d86635b6 100644
--- a/pkg/logservice/config_test.go
+++ b/pkg/logservice/config_test.go
@@ -188,3 +188,73 @@ func TestListenAddressCanBeFilled(t *testing.T) {
 	assert.Equal(t, cfg.RaftAddress, cfg.RaftListenAddress)
 	assert.Equal(t, cfg.GossipAddress, cfg.GossipListenAddress)
 }
+
+func TestClientConfigValidate(t *testing.T) {
+	tests := []struct {
+		cfg ClientConfig
+		ok  bool
+	}{
+		{
+			ClientConfig{}, false,
+		},
+		{
+			ClientConfig{LogShardID: 1, DiscoveryAddress: "localhost:9090"}, false,
+		},
+		{
+			ClientConfig{LogShardID: 1, DiscoveryAddress: "localhost:9090"}, false,
+		},
+		{
+			ClientConfig{LogShardID: 1, DNReplicaID: 100, DiscoveryAddress: "localhost:9090"}, true,
+		},
+		{
+			ClientConfig{
+				LogShardID:       1,
+				DNReplicaID:      100,
+				DiscoveryAddress: "localhost:9090",
+				ServiceAddresses: []string{"localhost:9091"},
+			},
+			true,
+		},
+	}
+
+	for _, tt := range tests {
+		err := tt.cfg.Validate()
+		if tt.ok {
+			assert.NoError(t, err)
+		} else {
+			assert.True(t, errors.Is(err, ErrInvalidConfig))
+		}
+	}
+}
+
+func TestHAKeeperClientConfigValidate(t *testing.T) {
+	tests := []struct {
+		cfg HAKeeperClientConfig
+		ok  bool
+	}{
+		{
+			HAKeeperClientConfig{}, false,
+		},
+		{
+			HAKeeperClientConfig{DiscoveryAddress: "localhost:9090"}, true,
+		},
+		{
+			HAKeeperClientConfig{ServiceAddresses: []string{"localhost:9090"}}, true,
+		},
+		{
+			HAKeeperClientConfig{
+				DiscoveryAddress: "localhost:9091",
+				ServiceAddresses: []string{"localhost:9090"},
+			}, true,
+		},
+	}
+
+	for _, tt := range tests {
+		err := tt.cfg.Validate()
+		if tt.ok {
+			assert.NoError(t, err)
+		} else {
+			assert.True(t, errors.Is(err, ErrInvalidConfig))
+		}
+	}
+}
diff --git a/pkg/logservice/hakeeper_client_test.go b/pkg/logservice/hakeeper_client_test.go
index 9c253f36e..fd0b626ef 100644
--- a/pkg/logservice/hakeeper_client_test.go
+++ b/pkg/logservice/hakeeper_client_test.go
@@ -20,6 +20,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cockroachdb/errors"
+
 	"github.com/google/uuid"
 	"github.com/lni/dragonboat/v4"
 	"github.com/lni/goutils/leaktest"
@@ -30,6 +32,19 @@ import (
 	pb "github.com/matrixorigin/matrixone/pkg/pb/logservice"
 )
 
+func TestHAKeeperClientConfigIsValidated(t *testing.T) {
+	cfg := HAKeeperClientConfig{}
+	cc1, err := NewCNHAKeeperClient(context.TODO(), cfg)
+	assert.Nil(t, cc1)
+	assert.True(t, errors.Is(err, ErrInvalidConfig))
+	cc2, err := NewDNHAKeeperClient(context.TODO(), cfg)
+	assert.Nil(t, cc2)
+	assert.True(t, errors.Is(err, ErrInvalidConfig))
+	cc3, err := NewLogHAKeeperClient(context.TODO(), cfg)
+	assert.Nil(t, cc3)
+	assert.True(t, errors.Is(err, ErrInvalidConfig))
+}
+
 func TestHAKeeperClientsCanBeCreated(t *testing.T) {
 	fn := func(t *testing.T, s *Service) {
 		cfg := HAKeeperClientConfig{