generated from TrueCloudLab/basic
[#13] support tls over grpc for otlp_grpc exporter type #14
No reviewers
Labels
No labels
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-observability#14
Loading…
Reference in a new issue
No description provided.
Delete branch "AlekseySVTN/frostfs-observability:support-tls-over-grpc-for-otlp-grpc-exporter-type"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Aleksey Savaitan a.savaitan@yadro.com
@ -0,0 +1,121 @@
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.
@ -0,0 +1,25 @@
package testdata
It is just helper file, it is not very necessary, but helpful.
@ -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)
I'd prefer all file reading to be done outside this library,
Config
should accept raw certificate instead of filepath.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.I am OK with the naming from this PR.
On the other hand, we could introduce
otlp_grpc/tls
orotlp_grpc/mtls
orotlp_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.db6cf1ea16
tocb173523f4
cb173523f4
toc0c5a9b81d
@fyrchik @dstepanov-yadro @alexvanin
I have applied some suggestions. Please, check it again.
@ -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 {
Why not just use relative path? You provide it to
ReadFile
, we should have no problems here.This file seems redundant.
Fixed.
@ -64,6 +69,10 @@ func (c *Config) hasChange(other *Config) bool {
return !c.serviceInfoEqual(other)
}
if other.Exporter == OTLPgRPCExporter && !c.ServerCaCertPool.Equal(other.ServerCaCertPool) {
Does
Equal
check fornil
?Yes.
@ -18,2 +19,4 @@
"google.golang.org/grpc/credentials"
)
var ErrEmptyServerRootCaPool = fmt.Errorf("empty server root ca cert pool")
Please, add some doc comment to public identifier and use
errors.New
if there are no%
directives.Fixed.
@ -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()) {
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.
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:
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.
c0c5a9b81d
to666d326cc5