[#13] support tls over grpc for otlp_grpc exporter type #14

8 changed files with 203 additions and 2 deletions
Showing only changes of commit 666d326cc5 - Show all commits

View file

1
testdata/tracing/invalid_root_ca.pem vendored Normal file
View file

@ -0,0 +1 @@
invalid content

View 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-----

View 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-----

View file

@ -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

Does Equal check for nil?

Does `Equal` check for `nil`?

Yes.
image

Yes. ![image](/attachments/d4a57498-bead-4641-a43f-26a5a10f7209)
return true
}
return c.Exporter != other.Exporter ||
c.Endpoint != other.Endpoint ||
!c.serviceInfoEqual(other)

View file

@ -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
}

View file

@ -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

Please, add some doc comment to public identifier and use errors.New if there are no % directives.

Please, add some doc comment to public identifier and use `errors.New` if there are no `%` directives.

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

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?

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.

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.

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.

>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.

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.

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.

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

I'd prefer all file reading to be done outside this library, Config should accept raw certificate instead of filepath.

I'd prefer all file reading to be done outside this library, `Config` should accept raw certificate instead of filepath.

Maybe even accept certpool.

Maybe even accept certpool.

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.

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
View file

@ -0,0 +1,118 @@
package tracing_test

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.

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
}