Skip to content

Commit

Permalink
[chore] Move grpc ClientConfig validation to Validate (#11233)
Browse files Browse the repository at this point in the history 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
  • Loading branch information
@bogdandrutu
bogdandrutu authored Sep 25, 2024
1 parent 7589901 commit 0ed97aa
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 59 deletions.
18 changes: 10 additions & 8 deletions config/configgrpc/configgrpc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,16 @@ func NewDefaultServerConfig() *ServerConfig {
}
}

func (gcs *ClientConfig) Validate() error {
if gcs.BalancerName != "" {
if balancer.Get(gcs.BalancerName) == nil {
return fmt.Errorf("invalid balancer_name: %s", gcs.BalancerName)
}
}

return nil
}

// sanitizedEndpoint strips the prefix of either http:// or https:// from configgrpc.ClientConfig.Endpoint.
func (gcs *ClientConfig) sanitizedEndpoint() string {
switch {
Expand Down Expand Up @@ -334,10 +344,6 @@ func (gcs *ClientConfig) getGrpcDialOptions(
}

if gcs.BalancerName != "" {
valid := validateBalancerName(gcs.BalancerName)
if !valid {
return nil, fmt.Errorf("invalid balancer_name: %s", gcs.BalancerName)
}
opts = append(opts, grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingPolicy":"%s"}`, gcs.BalancerName)))
}

Expand All @@ -363,10 +369,6 @@ func (gcs *ClientConfig) getGrpcDialOptions(
return opts, nil
}

func validateBalancerName(balancerName string) bool {
return balancer.Get(balancerName) != nil
}

func (gss *ServerConfig) Validate() error {
if gss.MaxRecvMsgSizeMiB*1024*1024 < 0 {
return fmt.Errorf("invalid max_recv_msg_size_mib value, must be between 1 and %d: %d", math.MaxInt/1024/1024, gss.MaxRecvMsgSizeMiB)
Expand Down
84 changes: 33 additions & 51 deletions config/configgrpc/configgrpc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"
"google.golang.org/grpc"
"google.golang.org/grpc/balancer"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/peer"
Expand Down Expand Up @@ -92,21 +91,6 @@ func TestNewDefaultServerConfig(t *testing.T) {
assert.Equal(t, expected, result)
}

// testBalancerBuilder facilitates testing validateBalancerName().
type testBalancerBuilder struct{}

func (testBalancerBuilder) Build(balancer.ClientConn, balancer.BuildOptions) balancer.Balancer {
return nil
}

func (testBalancerBuilder) Name() string {
return "configgrpc_balancer_test"
}

func init() {
balancer.Register(testBalancerBuilder{})
}

var (
componentID = component.MustNewID("component")
testAuthID = component.MustNewID("testauth")
Expand Down Expand Up @@ -333,7 +317,7 @@ func TestGrpcServerValidate(t *testing.T) {
t.Run(tt.err, func(t *testing.T) {
err := tt.gss.Validate()
require.Error(t, err)
assert.Regexp(t, tt.err, err)
assert.ErrorContains(t, err, tt.err)
})
}
}
Expand Down Expand Up @@ -390,18 +374,37 @@ func TestGrpcServerAuthSettings(t *testing.T) {
assert.NotNil(t, srv)
}

func TestGRPCClientSettingsError(t *testing.T) {
tt, err := componenttest.SetupTelemetry(componentID)
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, tt.Shutdown(context.Background())) })
func TestGrpcClientConfigInvalidBalancer(t *testing.T) {
settings := ClientConfig{
Headers: map[string]configopaque.String{
"test": "test",
},
Endpoint: "localhost:1234",
Compression: "gzip",
TLSSetting: configtls.ClientConfig{
Insecure: false,
},
Keepalive: &KeepaliveClientConfig{
Time: time.Second,
Timeout: time.Second,
PermitWithoutStream: true,
},
ReadBufferSize: 1024,
WriteBufferSize: 1024,
WaitForReady: true,
BalancerName: "test",
}
assert.ErrorContains(t, settings.Validate(), "invalid balancer_name: test")
}

func TestGRPCClientSettingsError(t *testing.T) {
tests := []struct {
settings ClientConfig
err string
host component.Host
}{
{
err: "^failed to load TLS config: failed to load CA CertPool File: failed to load cert /doesnt/exist:",
err: "failed to load TLS config: failed to load CA CertPool File: failed to load cert /doesnt/exist:",
settings: ClientConfig{
Headers: nil,
Endpoint: "",
Expand All @@ -417,7 +420,7 @@ func TestGRPCClientSettingsError(t *testing.T) {
},
},
{
err: "^failed to load TLS config: failed to load TLS cert and key: for auth via TLS, provide both certificate and key, or neither",
err: "failed to load TLS config: failed to load TLS cert and key: for auth via TLS, provide both certificate and key, or neither",
settings: ClientConfig{
Headers: nil,
Endpoint: "",
Expand All @@ -432,28 +435,6 @@ func TestGRPCClientSettingsError(t *testing.T) {
Keepalive: nil,
},
},
{
err: "invalid balancer_name: test",
settings: ClientConfig{
Headers: map[string]configopaque.String{
"test": "test",
},
Endpoint: "localhost:1234",
Compression: "gzip",
TLSSetting: configtls.ClientConfig{
Insecure: false,
},
Keepalive: &KeepaliveClientConfig{
Time: time.Second,
Timeout: time.Second,
PermitWithoutStream: true,
},
ReadBufferSize: 1024,
WriteBufferSize: 1024,
WaitForReady: true,
BalancerName: "test",
},
},
{
err: "failed to resolve authenticator \"doesntexist\": authenticator not found",
settings: ClientConfig{
Expand Down Expand Up @@ -506,9 +487,10 @@ func TestGRPCClientSettingsError(t *testing.T) {
}
for _, test := range tests {
t.Run(test.err, func(t *testing.T) {
_, err := test.settings.ToClientConn(context.Background(), test.host, tt.TelemetrySettings())
require.NoError(t, test.settings.Validate())
_, err := test.settings.ToClientConn(context.Background(), test.host, componenttest.NewNopTelemetrySettings())
require.Error(t, err)
assert.Regexp(t, test.err, err)
assert.ErrorContains(t, err, test.err)
})
}
}
Expand Down Expand Up @@ -587,7 +569,7 @@ func TestGRPCServerSettingsError(t *testing.T) {
err string
}{
{
err: "^failed to load TLS config: failed to load CA CertPool File: failed to load cert /doesnt/exist:",
err: "failed to load TLS config: failed to load CA CertPool File: failed to load cert /doesnt/exist:",
settings: ServerConfig{
NetAddr: confignet.AddrConfig{
Endpoint: "127.0.0.1:1234",
Expand All @@ -601,7 +583,7 @@ func TestGRPCServerSettingsError(t *testing.T) {
},
},
{
err: "^failed to load TLS config: failed to load TLS cert and key: for auth via TLS, provide both certificate and key, or neither",
err: "failed to load TLS config: failed to load TLS cert and key: for auth via TLS, provide both certificate and key, or neither",
settings: ServerConfig{
NetAddr: confignet.AddrConfig{
Endpoint: "127.0.0.1:1234",
Expand All @@ -615,7 +597,7 @@ func TestGRPCServerSettingsError(t *testing.T) {
},
},
{
err: "^failed to load client CA CertPool: failed to load CA /doesnt/exist:",
err: "failed to load client CA CertPool: failed to load CA /doesnt/exist:",
settings: ServerConfig{
NetAddr: confignet.AddrConfig{
Endpoint: "127.0.0.1:1234",
Expand All @@ -630,7 +612,7 @@ func TestGRPCServerSettingsError(t *testing.T) {
for _, test := range tests {
t.Run(test.err, func(t *testing.T) {
_, err := test.settings.ToServer(context.Background(), componenttest.NewNopHost(), componenttest.NewNopTelemetrySettings())
assert.Regexp(t, test.err, err)
assert.ErrorContains(t, err, test.err)
})
}
}
Expand Down

0 comments on commit 0ed97aa

Please sign in to comment.