feat!(core): enable tls for grpc connection #3922
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3922 +/- ##
==========================================
+ Coverage 44.83% 45.28% +0.44%
==========================================
Files 265 309 +44
Lines 14620 22026 +7406
==========================================
+ Hits 6555 9974 +3419
- Misses 7313 10974 +3661
- Partials 752 1078 +326 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
vgonkivs
changed the title
vgonkivs
added
kind:break!
vgonkivs
requested review from
renaynay,
Wondertan,
walldiss and
cristaloleg
as code owners
nodebuilder/core/config.go
Outdated
| // It should not include file names('cert.pem' and 'key.pem'). | ||
| // If left empty, the client will be configured for an insecure (non-TLS) connection. | ||
| TLSPath string | ||
| // XTokenPath specifies the file path to the JSON file containing the X-Token for gRPC authentication. |
There was a problem hiding this comment.
comments say this is path to the JSON file, but due to the use of the xtoken const, needs to be the path to the directory containing a file named xtoken.json
| if !exist { | ||
| return nil, os.ErrNotExist | ||
| } |
There was a problem hiding this comment.
Should just return empty tls config? Seems os.ErrNotExist is not needed
There was a problem hiding this comment.
Yes. I added this behavior initially but then realized that we may have more usecases in the future so the caller will decide what to do with this error.
| xtoken, err := core.XToken(corecfg.XTokenPath) | ||
| if err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| return nil, nil, nil, err | ||
| } | ||
| opts = append(opts, state.WithTLSConfig(tlsCfg), state.WithXToken(xtoken)) |
There was a problem hiding this comment.
If token is not present, perhaps we should not invoke state.WithXToken options
There was a problem hiding this comment.
in this case xToken is "" so, I don't think it will break any logic but from the other - less code fewer problems 🙂 . I can add a comment explaining the usage of state.WithXToken
| authInterceptor := func(ctx context.Context, | ||
| method string, | ||
| req, reply interface{}, | ||
| cc *grpc.ClientConn, | ||
| invoker grpc.UnaryInvoker, | ||
| opts ...grpc.CallOption, | ||
| ) error { | ||
| ctx = metadata.AppendToOutgoingContext(ctx, "x-token", ca.xtoken) | ||
| return invoker(ctx, method, req, reply, cc, opts...) | ||
| } |
There was a problem hiding this comment.
nit, but would be easier to read if interceptor is extracted
There was a problem hiding this comment.
Yeah, I was planning to move it out. Thanks 🙏
This PR includes all changes from #3917.
The PR introduces 2 additional fields in the core config and 2 cli flags that allow to configure these fields.