[#13] support tls over grpc for otlp_grpc exporter type #14
0
testdata/tracing/invalid_empty_root_ca.pem
vendored
Normal file
1
testdata/tracing/invalid_root_ca.pem
vendored
Normal file
|
@ -0,0 +1 @@
|
|||
invalid content
|
12
testdata/tracing/valid_google_globalsign_r4_rsa_root_ca.pem
vendored
Normal file
|
@ -0,0 +1,12 @@
|
|||
-----BEGIN CERTIFICATE-----
|
||||
MIIB3DCCAYOgAwIBAgINAgPlfvU/k/2lCSGypjAKBggqhkjOPQQDAjBQMSQwIgYD
|
||||
VQQLExtHbG9iYWxTaWduIEVDQyBSb290IENBIC0gUjQxEzARBgNVBAoTCkdsb2Jh
|
||||
bFNpZ24xEzARBgNVBAMTCkdsb2JhbFNpZ24wHhcNMTIxMTEzMDAwMDAwWhcNMzgw
|
||||
MTE5MDMxNDA3WjBQMSQwIgYDVQQLExtHbG9iYWxTaWduIEVDQyBSb290IENBIC0g
|
||||
UjQxEzARBgNVBAoTCkdsb2JhbFNpZ24xEzARBgNVBAMTCkdsb2JhbFNpZ24wWTAT
|
||||
BgcqhkjOPQIBBggqhkjOPQMBBwNCAAS4xnnTj2wlDp8uORkcA6SumuU5BwkWymOx
|
||||
uYb4ilfBV85C+nOh92VC/x7BALJucw7/xyHlGKSq2XE/qNS5zowdo0IwQDAOBgNV
|
||||
HQ8BAf8EBAMCAYYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUVLB7rUW44kB/
|
||||
+wpu+74zyTyjhNUwCgYIKoZIzj0EAwIDRwAwRAIgIk90crlgr/HmnKAWBVBfw147
|
||||
bmF0774BxL4YSFlhgjICICadVGNA3jdgUM/I2O2dgq43mLyjj0xMqTQrbO/7lZsm
|
||||
-----END CERTIFICATE-----
|
13
testdata/tracing/valid_google_gts_r4_ecdsa_root_ca.pem
vendored
Normal file
|
@ -0,0 +1,13 @@
|
|||
-----BEGIN CERTIFICATE-----
|
||||
MIICCTCCAY6gAwIBAgINAgPlwGjvYxqccpBQUjAKBggqhkjOPQQDAzBHMQswCQYD
|
||||
VQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZpY2VzIExMQzEUMBIG
|
||||
A1UEAxMLR1RTIFJvb3QgUjQwHhcNMTYwNjIyMDAwMDAwWhcNMzYwNjIyMDAwMDAw
|
||||
WjBHMQswCQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZpY2Vz
|
||||
IExMQzEUMBIGA1UEAxMLR1RTIFJvb3QgUjQwdjAQBgcqhkjOPQIBBgUrgQQAIgNi
|
||||
AATzdHOnaItgrkO4NcWBMHtLSZ37wWHO5t5GvWvVYRg1rkDdc/eJkTBa6zzuhXyi
|
||||
QHY7qca4R9gq55KRanPpsXI5nymfopjTX15YhmUPoYRlBtHci8nHc8iMai/lxKvR
|
||||
HYqjQjBAMA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQW
|
||||
BBSATNbrdP9JNqPV2Py1PsVq8JQdjDAKBggqhkjOPQQDAwNpADBmAjEA6ED/g94D
|
||||
9J+uHXqnLrmvT/aDHQ4thQEd0dlq7A/Cr8deVl5c1RxYIigL9zC2L7F8AjEA8GE8
|
||||
p/SgguMh1YQdc4acLa/KNJvxn7kjNuK8YAOdgLOaVsjh4rsUecrNIdSUtUlD
|
||||
-----END CERTIFICATE-----
|
|
@ -1,6 +1,9 @@
|
|||
package tracing
|
||||
|
||||
import "fmt"
|
||||
import (
|
||||
"crypto/x509"
|
||||
"fmt"
|
||||
)
|
||||
|
||||
// Exporter is type of tracing target.
|
||||
type Exporter string
|
||||
|
@ -18,6 +21,8 @@ type Config struct {
|
|||
Exporter Exporter
|
||||
// Endpoint is collector endpoint for OTLP exporters.
|
||||
Endpoint string
|
||||
// ServerCaCertPool is cert pool of the remote server CA certificate. Use for TLS setup.
|
||||
ServerCaCertPool *x509.CertPool
|
||||
|
||||
// Service is service name that will be used in tracing.
|
||||
// Mandatory.
|
||||
|
@ -64,6 +69,10 @@ func (c *Config) hasChange(other *Config) bool {
|
|||
return !c.serviceInfoEqual(other)
|
||||
}
|
||||
|
||||
if other.Exporter == OTLPgRPCExporter && !c.ServerCaCertPool.Equal(other.ServerCaCertPool) {
|
||||
fyrchik marked this conversation as resolved
Outdated
|
||||
return true
|
||||
}
|
||||
|
||||
return c.Exporter != other.Exporter ||
|
||||
c.Endpoint != other.Endpoint ||
|
||||
!c.serviceInfoEqual(other)
|
||||
|
|
|
@ -1,7 +1,11 @@
|
|||
package tracing
|
||||
|
||||
import (
|
||||
"crypto/x509"
|
||||
"os"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestConfig_validate(t *testing.T) {
|
||||
|
@ -232,6 +236,28 @@ func TestConfig_hasChange(t *testing.T) {
|
|||
Version: "v1.0.0",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "use tls root ca certificate for grpc",
|
||||
want: true,
|
||||
config: Config{
|
||||
Enabled: true,
|
||||
Exporter: OTLPgRPCExporter,
|
||||
Endpoint: "localhost:4717",
|
||||
Service: "test",
|
||||
InstanceID: "s01",
|
||||
Version: "v1.0.0",
|
||||
ServerCaCertPool: readCertPoolByPath(t, "../testdata/tracing/valid_google_globalsign_r4_rsa_root_ca.pem"),
|
||||
},
|
||||
other: Config{
|
||||
Enabled: true,
|
||||
Exporter: OTLPgRPCExporter,
|
||||
Endpoint: "localhost:4717",
|
||||
Service: "test",
|
||||
InstanceID: "s01",
|
||||
Version: "v1.0.0",
|
||||
ServerCaCertPool: readCertPoolByPath(t, "../testdata/tracing/valid_google_gts_r4_ecdsa_root_ca.pem"),
|
||||
},
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
|
@ -241,3 +267,12 @@ func TestConfig_hasChange(t *testing.T) {
|
|||
})
|
||||
}
|
||||
}
|
||||
|
||||
func readCertPoolByPath(t *testing.T, path string) *x509.CertPool {
|
||||
ca, err := os.ReadFile(path)
|
||||
require.NoError(t, err)
|
||||
roots := x509.NewCertPool()
|
||||
ok := roots.AppendCertsFromPEM(ca)
|
||||
require.True(t, ok)
|
||||
return roots
|
||||
}
|
||||
|
|
|
@ -2,6 +2,8 @@ package tracing
|
|||
|
||||
import (
|
||||
"context"
|
||||
"crypto/x509"
|
||||
"errors"
|
||||
"fmt"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
|
@ -15,8 +17,12 @@ import (
|
|||
semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
|
||||
"go.opentelemetry.io/otel/trace"
|
||||
"go.opentelemetry.io/otel/trace/noop"
|
||||
"google.golang.org/grpc/credentials"
|
||||
)
|
||||
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
Please, add some doc comment to public identifier and use Please, add some doc comment to public identifier and use `errors.New` if there are no `%` directives.
AlekseySVTN
commented
Fixed. Fixed.
|
||||
// ErrEmptyServerRootCaPool indicates that cert pool is empty and does not have any certificates inside.
|
||||
var ErrEmptyServerRootCaPool = errors.New("empty server root ca cert pool")
|
||||
|
||||
var (
|
||||
// tracingLock protects provider, done, config and tracer from concurrent update.
|
||||
// These fields change when the config is updated or the application is shutdown.
|
||||
|
@ -135,7 +141,14 @@ func getExporter(ctx context.Context, cfg *Config) (sdktrace.SpanExporter, error
|
|||
case NoOpExporter:
|
||||
return tracetest.NewNoopExporter(), nil
|
||||
case OTLPgRPCExporter:
|
||||
return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), otlptracegrpc.WithInsecure())
|
||||
securityOption := otlptracegrpc.WithInsecure()
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
Why do we have this specific check here, what will happen without it? Why do we have this specific check here, what will happen without it?
AlekseySVTN
commented
NewCertPool returns a new, empty CertPool. When we would like to fill CertPoll we can write: ok := roots.AppendCertsFromPEM(ca) So, here we check that our serverCaCertPool is not empty by comparing our cert pool with empty cert pool. Why we shoud check this? Bassicaly - I do not know, what can happend, but it looks like not very good idea to pass empty certPool for grpc connection. NewCertPool returns a new, empty CertPool.
When we would like to fill CertPoll we can write:
ok := roots.AppendCertsFromPEM(ca)
ok - can be false, it means that our certPool still has not any certificates inside.
__
So, here we check that our serverCaCertPool is not empty by comparing our cert pool with empty cert pool.
I have not found any other options to do this. But may be you can give me another variants.
Why we shoud check this? Bassicaly - I do not know, what can happend, but it looks like not very good idea to pass empty certPool for grpc connection.
fyrchik
commented
Yes. My point was that probably we already get an error from some other place, so this line seem redundant. >not very good idea to pass empty certPool for grpc connection.
Yes. My point was that probably we already get an error from some other place, so this line seem redundant.
I don't know, really, see no problem with leaving it as it is.
AlekseySVTN
commented
I have one knowledge, that:
didn't provide an error with an empty certificate pool. So, I can check it more dive, if it seems for you like good idea. I have one knowledge, that:
```
otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), securityOption)
```
didn't provide an error with an empty certificate pool.
It's possible that we might get an error in the following parts of the code, but it seems unlikely. It seems that this is normal behavior (like insecure connection) for dealing with an empty certificate pool.
So, I can check it more dive, if it seems for you like good idea.
fyrchik
commented
Seems not worth the effort, the proposed solution looks ok. Seems not worth the effort, the proposed solution looks ok.
|
||||
if cfg.ServerCaCertPool != nil {
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
I'd prefer all file reading to be done outside this library, I'd prefer all file reading to be done outside this library, `Config` should accept raw certificate instead of filepath.
fyrchik
commented
Maybe even accept certpool. Maybe even accept certpool.
fyrchik
commented
Also, this will make Also, this will make `hasChange` more correct.
Even if the path hasn't change, the certificate under it _could_.
With certpool we have `Equals` method.
|
||||
if cfg.ServerCaCertPool.Equal(x509.NewCertPool()) {
|
||||
return nil, fmt.Errorf("failed to setup tracing: %w", ErrEmptyServerRootCaPool)
|
||||
}
|
||||
securityOption = otlptracegrpc.WithTLSCredentials(credentials.NewClientTLSFromCert(cfg.ServerCaCertPool, ""))
|
||||
}
|
||||
return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), securityOption)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
118
tracing/setup_test.go
Normal file
|
@ -0,0 +1,118 @@
|
|||
package tracing_test
|
||||
AlekseySVTN
commented
It can be only "tracing" (not tracing_test) package test, if so it can test only unexported "getExporter" function. It can be only "tracing" (not tracing_test) package test, if so it can test only unexported "getExporter" function.
Please note me, if it seems to be better.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/x509"
|
||||
"os"
|
||||
"testing"
|
||||
|
||||
"git.frostfs.info/TrueCloudLab/frostfs-observability/tracing"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestSetup(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
config tracing.Config
|
||||
want bool
|
||||
expErr error
|
||||
}{
|
||||
{
|
||||
name: "setup stdout exporter",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.StdoutExporter,
|
||||
Service: "service-name",
|
||||
},
|
||||
want: true,
|
||||
expErr: nil,
|
||||
},
|
||||
{
|
||||
name: "setup noop exporter",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.NoOpExporter,
|
||||
Service: "service-name",
|
||||
},
|
||||
|
||||
want: true,
|
||||
expErr: nil,
|
||||
},
|
||||
{
|
||||
name: "setup otlp_grpc insecure exporter",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.OTLPgRPCExporter,
|
||||
Service: "service-name",
|
||||
Endpoint: "test-endpoint.com:4317",
|
||||
},
|
||||
want: true,
|
||||
expErr: nil,
|
||||
},
|
||||
{
|
||||
name: "setup otlp_grpc secure exporter with valid rsa root ca certificate",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.OTLPgRPCExporter,
|
||||
Service: "service-name",
|
||||
Endpoint: "test-endpoint.com:4317",
|
||||
ServerCaCertPool: readCertPoolByPath(t, "../testdata/tracing/valid_google_globalsign_r4_rsa_root_ca.pem"),
|
||||
},
|
||||
want: true,
|
||||
expErr: nil,
|
||||
},
|
||||
{
|
||||
name: "setup otlp_grpc secure exporter with valid ecdsa root ca certificate",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.OTLPgRPCExporter,
|
||||
Service: "service-name",
|
||||
Endpoint: "test-endpoint.com:4317",
|
||||
ServerCaCertPool: readCertPoolByPath(t, "../testdata/tracing/valid_google_gts_r4_ecdsa_root_ca.pem"),
|
||||
},
|
||||
want: true,
|
||||
expErr: nil,
|
||||
},
|
||||
{
|
||||
name: "setup otlp_grpc secure exporter with invalid root ca certificate",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.OTLPgRPCExporter,
|
||||
Service: "service-name",
|
||||
Endpoint: "test-endpoint.com:4317",
|
||||
ServerCaCertPool: readCertPoolByPath(t, "../testdata/tracing/invalid_root_ca.pem"),
|
||||
},
|
||||
want: false,
|
||||
expErr: tracing.ErrEmptyServerRootCaPool,
|
||||
},
|
||||
{
|
||||
name: "setup otlp_grpc secure exporter with empty root ca certificate",
|
||||
config: tracing.Config{
|
||||
Enabled: true,
|
||||
Exporter: tracing.OTLPgRPCExporter,
|
||||
Service: "service-name",
|
||||
Endpoint: "test-endpoint.com:4317",
|
||||
ServerCaCertPool: readCertPoolByPath(t, "../testdata/tracing/invalid_empty_root_ca.pem"),
|
||||
},
|
||||
want: false,
|
||||
expErr: tracing.ErrEmptyServerRootCaPool,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
got, err := tracing.Setup(context.Background(), tt.config)
|
||||
require.ErrorIs(t, err, tt.expErr)
|
||||
if got != tt.want {
|
||||
t.Errorf("Setup config = %v, want %v", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func readCertPoolByPath(t *testing.T, path string) *x509.CertPool {
|
||||
ca, err := os.ReadFile(path)
|
||||
require.NoError(t, err)
|
||||
roots := x509.NewCertPool()
|
||||
_ = roots.AppendCertsFromPEM(ca)
|
||||
return roots
|
||||
}
|
Does
Equal
check fornil
?Yes.