Skip to content

Commit

Permalink
Do not embed ProtocolSettings in gRPC (open-telemetry#1210)
Browse files Browse the repository at this point in the history 
Adding `ProtocolServerSettings` was a bit of a rush. We determined that `endpoint` has different meaning based on protocol, also not all the protocols support TLS.

In this PR we revert embedding `ProtocolServerSettings` in the GRPCServerSettings and make it consistent with HttpServerSettings.

Work left: Consistent config name for TLS settings `tls_settings` or `tls_credentials`.
  • Loading branch information
@bogdandrutu @wyTrivail
bogdandrutu authored and wyTrivail committed Jul 13, 2020
1 parent 91fbc29 commit 912ecb6
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 118 deletions.
14 changes: 9 additions & 5 deletions config/configgrpc/configgrpc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@ import (
"strings"
"time"

"github.com/pkg/errors"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/encoding/gzip"
"google.golang.org/grpc/keepalive"

"go.opentelemetry.io/collector/config/configprotocol"
"go.opentelemetry.io/collector/config/configtls"
)

Expand Down Expand Up @@ -103,8 +101,14 @@ type KeepaliveEnforcementPolicy struct {
}

type GRPCServerSettings struct {
// Configures the generic server protocol.
configprotocol.ProtocolServerSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct
// The target to which the exporter is going to send traces or metrics,
// using the gRPC protocol. The valid syntax is described at
// https://github.com/grpc/grpc/blob/master/doc/naming.md.
Endpoint string `mapstructure:"endpoint"`

// Configures the protocol to use TLS.
// The default value is nil, which will cause the protocol to not use TLS.
TLSCredentials *configtls.TLSServerSetting `mapstructure:"tls_credentials, omitempty"`

// MaxRecvMsgSizeMiB sets the maximum size (in MiB) of messages accepted by the server.
MaxRecvMsgSizeMiB uint64 `mapstructure:"max_recv_msg_size_mib,omitempty"`
Expand Down Expand Up @@ -158,7 +162,7 @@ func (gss *GRPCServerSettings) ToServerOption() ([]grpc.ServerOption, error) {
if gss.TLSCredentials != nil {
tlsCfg, err := gss.TLSCredentials.LoadTLSConfig()
if err != nil {
return nil, errors.Wrap(err, "error initializing TLS Credentials")
return nil, err
}
opts = append(opts, grpc.Creds(credentials.NewTLS(tlsCfg)))
}
Expand Down
49 changes: 48 additions & 1 deletion config/configgrpc/configgrpc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestBasicGrpcSettings(t *testing.T) {
assert.NoError(t, err)
}

func TestInvalidPemFile(t *testing.T) {
func TestGRPCClientSettingsError(t *testing.T) {
tests := []struct {
settings GRPCClientSettings
err string
Expand Down Expand Up @@ -93,6 +93,53 @@ func TestUseSecure(t *testing.T) {
assert.Equal(t, len(dialOpts), 1)
}

func TestGRPCServerSettingsError(t *testing.T) {
tests := []struct {
settings GRPCServerSettings
err string
}{
{
err: "^failed to load TLS config: failed to load CA CertPool: failed to load CA /doesnt/exist:",
settings: GRPCServerSettings{
Endpoint: "",
TLSCredentials: &configtls.TLSServerSetting{
TLSSetting: configtls.TLSSetting{
CAFile: "/doesnt/exist",
},
},
Keepalive: nil,
},
},
{
err: "^failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither",
settings: GRPCServerSettings{
Endpoint: "",
TLSCredentials: &configtls.TLSServerSetting{
TLSSetting: configtls.TLSSetting{
CertFile: "/doesnt/exist",
},
},
Keepalive: nil,
},
},
{
err: "^failed to load TLS config: failed to load client CA CertPool: failed to load CA /doesnt/exist:",
settings: GRPCServerSettings{
Endpoint: "",
TLSCredentials: &configtls.TLSServerSetting{
ClientCAFile: "/doesnt/exist",
},
},
},
}
for _, test := range tests {
t.Run(test.err, func(t *testing.T) {
_, err := test.settings.ToServerOption()
assert.Regexp(t, test.err, err)
})
}
}

func TestGetGRPCCompressionKey(t *testing.T) {
if GetGRPCCompressionKey("gzip") != CompressionGzip {
t.Error("gzip is marked as supported but returned unsupported")
Expand Down
46 changes: 14 additions & 32 deletions receiver/opencensusreceiver/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/configgrpc"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/config/configprotocol"
"go.opentelemetry.io/collector/config/configtls"
)

Expand Down Expand Up @@ -53,10 +52,7 @@ func TestLoadConfig(t *testing.T) {
NameVal: "opencensus/customname",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "0.0.0.0:9090",
TLSCredentials: nil,
},
Endpoint: "0.0.0.0:9090",
},
Transport: "tcp",
})
Expand All @@ -69,10 +65,7 @@ func TestLoadConfig(t *testing.T) {
NameVal: "opencensus/keepalive",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
TLSCredentials: nil,
Endpoint: "0.0.0.0:55678",
},
Endpoint: "0.0.0.0:55678",
Keepalive: &configgrpc.KeepaliveServerConfig{
ServerParameters: &configgrpc.KeepaliveServerParameters{
MaxConnectionIdle: 11 * time.Second,
Expand All @@ -98,10 +91,7 @@ func TestLoadConfig(t *testing.T) {
NameVal: "opencensus/msg-size-conc-connect-max-idle",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "0.0.0.0:55678",
TLSCredentials: nil,
},
Endpoint: "0.0.0.0:55678",
MaxRecvMsgSizeMiB: 32,
MaxConcurrentStreams: 16,
Keepalive: &configgrpc.KeepaliveServerConfig{
Expand All @@ -123,13 +113,11 @@ func TestLoadConfig(t *testing.T) {
NameVal: "opencensus/tlscredentials",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "0.0.0.0:55678",
TLSCredentials: &configtls.TLSServerSetting{
TLSSetting: configtls.TLSSetting{
CertFile: "test.crt",
KeyFile: "test.key",
},
Endpoint: "0.0.0.0:55678",
TLSCredentials: &configtls.TLSServerSetting{
TLSSetting: configtls.TLSSetting{
CertFile: "test.crt",
KeyFile: "test.key",
},
},
},
Expand All @@ -144,9 +132,7 @@ func TestLoadConfig(t *testing.T) {
NameVal: "opencensus/cors",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "0.0.0.0:55678",
},
Endpoint: "0.0.0.0:55678",
},
Transport: "tcp",
CorsOrigins: []string{"https://*.test.com", "https://test.com"},
Expand All @@ -160,9 +146,7 @@ func TestLoadConfig(t *testing.T) {
NameVal: "opencensus/uds",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "/tmp/opencensus.sock",
},
Endpoint: "/tmp/opencensus.sock",
},
Transport: "unix",
})
Expand All @@ -174,17 +158,15 @@ func TestBuildOptions_TLSCredentials(t *testing.T) {
NameVal: "IncorrectTLS",
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
TLSCredentials: &configtls.TLSServerSetting{
TLSSetting: configtls.TLSSetting{
CertFile: "willfail",
},
TLSCredentials: &configtls.TLSServerSetting{
TLSSetting: configtls.TLSSetting{
CertFile: "willfail",
},
},
},
}
_, err := cfg.buildOptions()
assert.EqualError(t, err, `error initializing TLS Credentials: failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither`)
assert.EqualError(t, err, `failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither`)

cfg.TLSCredentials = &configtls.TLSServerSetting{}
opt, err := cfg.buildOptions()
Expand Down
5 changes: 1 addition & 4 deletions receiver/opencensusreceiver/factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configgrpc"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/config/configprotocol"
"go.opentelemetry.io/collector/consumer"
)

Expand Down Expand Up @@ -53,9 +52,7 @@ func (f *Factory) CreateDefaultConfig() configmodels.Receiver {
NameVal: typeStr,
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "0.0.0.0:55678",
},
Endpoint: "0.0.0.0:55678",
},
Transport: "tcp",
}
Expand Down
25 changes: 6 additions & 19 deletions receiver/opencensusreceiver/factory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"go.opentelemetry.io/collector/config/configcheck"
"go.opentelemetry.io/collector/config/configgrpc"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/config/configprotocol"
"go.opentelemetry.io/collector/exporter/exportertest"
"go.opentelemetry.io/collector/testutil"
)
Expand Down Expand Up @@ -63,9 +62,7 @@ func TestCreateTraceReceiver(t *testing.T) {
NameVal: typeStr,
}
defaultGRPCSettings := configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: endpoint,
},
Endpoint: endpoint,
}
tests := []struct {
name string
Expand All @@ -88,9 +85,7 @@ func TestCreateTraceReceiver(t *testing.T) {
NameVal: typeStr,
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "localhost:112233",
},
Endpoint: "localhost:112233",
},
Transport: "tcp",
},
Expand All @@ -101,9 +96,7 @@ func TestCreateTraceReceiver(t *testing.T) {
cfg: &Config{
ReceiverSettings: defaultReceiverSettings,
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: endpoint,
},
Endpoint: endpoint,
MaxRecvMsgSizeMiB: 32,
MaxConcurrentStreams: 16,
},
Expand Down Expand Up @@ -138,9 +131,7 @@ func TestCreateMetricReceiver(t *testing.T) {
NameVal: typeStr,
}
defaultGRPCSettings := configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: endpoint,
},
Endpoint: endpoint,
}

tests := []struct {
Expand All @@ -164,9 +155,7 @@ func TestCreateMetricReceiver(t *testing.T) {
NameVal: typeStr,
},
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: "327.0.0.1:1122",
},
Endpoint: "327.0.0.1:1122",
},
Transport: "tcp",
},
Expand All @@ -177,9 +166,7 @@ func TestCreateMetricReceiver(t *testing.T) {
cfg: &Config{
ReceiverSettings: defaultReceiverSettings,
GRPCServerSettings: configgrpc.GRPCServerSettings{
ProtocolServerSettings: configprotocol.ProtocolServerSettings{
Endpoint: endpoint,
},
Endpoint: endpoint,
Keepalive: &configgrpc.KeepaliveServerConfig{
ServerParameters: &configgrpc.KeepaliveServerParameters{
MaxConnectionAge: 60 * time.Second,
Expand Down
Loading

0 comments on commit 912ecb6

Please sign in to comment.