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

Member

Signed-off-by: Aleksey Savaitan a.savaitan@yadro.com

Signed-off-by: Aleksey Savaitan <a.savaitan@yadro.com>
AlekseySVTN added 1 commit 2024-09-04 11:43:57 +00:00
[#13] support tls over grpc for otlp_grpc exporter type
All checks were successful
DCO action / DCO (pull_request) Successful in 45s
Tests and linters / Tests (1.21) (pull_request) Successful in 1m38s
Tests and linters / Tests (1.22) (pull_request) Successful in 1m38s
Tests and linters / Tests with -race (pull_request) Successful in 1m42s
Tests and linters / Staticcheck (pull_request) Successful in 1m50s
Tests and linters / Lint (pull_request) Successful in 2m0s
db6cf1ea16
Signed-off-by: Aleksey Savaitan <a.savaitan@yadro.com>
AlekseySVTN reviewed 2024-09-04 11:46:01 +00:00
@ -0,0 +1,121 @@
package tracing_test
Author
Member

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.
AlekseySVTN reviewed 2024-09-04 11:47:51 +00:00
@ -0,0 +1,25 @@
package testdata
Author
Member

It is just helper file, it is not very necessary, but helpful.

It is just helper file, it is not very necessary, but helpful.
fyrchik marked this conversation as resolved
fyrchik requested changes 2024-09-05 12:27:47 +00:00
Dismissed
tracing/setup.go Outdated
@ -138,1 +143,3 @@
return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), otlptracegrpc.WithInsecure())
securityOption := otlptracegrpc.WithInsecure()
if cfg.ServerCaCertPath != "" {
ca, err := os.ReadFile(cfg.ServerCaCertPath)
Owner

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

Maybe even accept certpool.

Maybe even accept certpool.
Owner

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.
fyrchik marked this conversation as resolved
fyrchik requested review from storage-core-committers 2024-09-05 12:37:29 +00:00
fyrchik requested review from storage-core-developers 2024-09-05 12:37:30 +00:00
fyrchik requested review from storage-services-developers 2024-09-05 12:37:31 +00:00
fyrchik requested review from storage-services-committers 2024-09-05 12:37:31 +00:00
Owner

I am OK with the naming from this PR.
On the other hand, we could introduce otlp_grpc/tls or otlp_grpc/mtls or otlp_grpc as the exporter type. Makes code a bit more complicated, though.
WDYT? @dstepanov-yadro @alexvanin

I am OK with the naming from this PR. On the other hand, we could introduce `otlp_grpc/tls` or `otlp_grpc/mtls` or `otlp_grpc` as the exporter type. Makes code a bit more complicated, though. WDYT? @dstepanov-yadro @alexvanin

I am OK with the naming from this PR.
On the other hand, we could introduce otlp_grpc/tls or otlp_grpc/mtls or otlp_grpc as the exporter type. Makes code a bit more complicated, though.
WDYT? @dstepanov-yadro @alexvanin

Now we use only otlp_grpc exporter. Jaeger supports also others: https://www.jaegertracing.io/docs/1.60/apis/

So I think it is more convenient to not to store mtls, tls as exporter type.

> I am OK with the naming from this PR. > On the other hand, we could introduce `otlp_grpc/tls` or `otlp_grpc/mtls` or `otlp_grpc` as the exporter type. Makes code a bit more complicated, though. > WDYT? @dstepanov-yadro @alexvanin Now we use only `otlp_grpc` exporter. Jaeger supports also others: https://www.jaegertracing.io/docs/1.60/apis/ So I think it is more convenient to not to store `mtls`, `tls` as exporter type.
AlekseySVTN force-pushed support-tls-over-grpc-for-otlp-grpc-exporter-type from db6cf1ea16 to cb173523f4 2024-09-09 10:02:55 +00:00 Compare
AlekseySVTN force-pushed support-tls-over-grpc-for-otlp-grpc-exporter-type from cb173523f4 to c0c5a9b81d 2024-09-09 10:05:17 +00:00 Compare
Author
Member

@fyrchik @dstepanov-yadro @alexvanin
I have applied some suggestions. Please, check it again.

@fyrchik @dstepanov-yadro @alexvanin I have applied some suggestions. Please, check it again.
fyrchik requested changes 2024-09-09 10:41:10 +00:00
Dismissed
@ -0,0 +16,4 @@
// Path returns the absolute path the given relative file or directory path,
// relative to the frostfs-observability/testdata directory in the user's GOPATH.
// If rel is already absolute, it is returned unmodified.
func Path(rel string) string {
Owner

Why not just use relative path? You provide it to ReadFile, we should have no problems here.
This file seems redundant.

Why not just use relative path? You provide it to `ReadFile`, we should have no problems here. This file seems redundant.
Author
Member

Fixed.

Fixed.
fyrchik marked this conversation as resolved
@ -64,6 +69,10 @@ func (c *Config) hasChange(other *Config) bool {
return !c.serviceInfoEqual(other)
}
if other.Exporter == OTLPgRPCExporter && !c.ServerCaCertPool.Equal(other.ServerCaCertPool) {
Owner

Does Equal check for nil?

Does `Equal` check for `nil`?
Author
Member

Yes.
image

Yes. ![image](/attachments/d4a57498-bead-4641-a43f-26a5a10f7209)
fyrchik marked this conversation as resolved
tracing/setup.go Outdated
@ -18,2 +19,4 @@
"google.golang.org/grpc/credentials"
)
var ErrEmptyServerRootCaPool = fmt.Errorf("empty server root ca cert pool")
Owner

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

Fixed.

Fixed.
fyrchik marked this conversation as resolved
tracing/setup.go Outdated
@ -138,1 +142,3 @@
return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), otlptracegrpc.WithInsecure())
securityOption := otlptracegrpc.WithInsecure()
if cfg.ServerCaCertPool != nil {
if cfg.ServerCaCertPool.Equal(x509.NewCertPool()) {
Owner

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?
Author
Member

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

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

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

Seems not worth the effort, the proposed solution looks ok.

Seems not worth the effort, the proposed solution looks ok.
fyrchik marked this conversation as resolved
AlekseySVTN force-pushed support-tls-over-grpc-for-otlp-grpc-exporter-type from c0c5a9b81d to 666d326cc5 2024-09-09 11:43:25 +00:00 Compare
fyrchik approved these changes 2024-09-09 11:55:45 +00:00
AlekseySVTN merged commit 666d326cc5 into master 2024-09-09 12:12:33 +00:00
AlekseySVTN deleted branch support-tls-over-grpc-for-otlp-grpc-exporter-type 2024-09-09 12:12:34 +00:00
acid-ant approved these changes 2024-09-09 12:13:12 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-developers
TrueCloudLab/storage-services-committers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-observability#14
No description provided.