Skip to content

Commit

Permalink
[fix][query] Fix misconfiguration in TLS settings from using OTEL HTT…
Browse files Browse the repository at this point in the history
…P helper (jaegertracing#6239)

## Which problem is this PR solving?
- Fixes jaegertracing#6230 

## Description of the changes
- This PR fixes an oversight that was made when migrating to OTEL's
helpers for creating the HTTP server. OTEL's helper was adding TLS to
the listener whereas we left the code to add TLS to the server. This was
leading to the connection being in a weird state causing the bug in
jaegertracing#6230. This PR simplifies the flow by doing the following:
- We do not need to add TLS to the server because its added when the
listener is initialized using `.ToListener()`
- We do not need to call `ServeTLS` and can simply call `Serve` and TLS
will be used when the connection is configured to do so (which it will
be if the TLSSetting is set on the server).

## How was this change tested?
Start the server by doing the following
```
go run ./cmd/all-in-one \
    --query.http.tls.enabled=true \
    --query.http.tls.cert=./pkg/config/tlscfg/testdata/example-server-cert.pem \
    --query.http.tls.key=./pkg/config/tlscfg/testdata/example-server-key.pem
```
Send request with HTTP1.1
```
curl -k --http1.1 https://localhost:16686/ > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4454    0  4454    0     0   393k      0 --:--:-- --:--:-- --:--:--  869k
```

Send request with HTTP2 (default)
```
curl -k --http2 https://localhost:16686/ > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4454    0  4454    0     0   140k      0 --:--:-- --:--:-- --:--:--  173k
```

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Mahad Zaryab <[email protected]>
  • Loading branch information
mahadzaryab1 authored Nov 22, 2024
1 parent adbdf26 commit edb896e
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 18 deletions.
16 changes: 1 addition & 15 deletions cmd/query/app/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,15 +228,6 @@ func createHTTPServer(
staticHandlerCloser: staticHandlerCloser,
}

// TODO why doesn't OTEL helper do that already?
if queryOpts.HTTP.TLSSetting != nil {
tlsCfg, err := queryOpts.HTTP.TLSSetting.LoadTLSConfig(ctx) // This checks if the certificates are correctly provided
if err != nil {
return nil, errors.Join(err, staticHandlerCloser.Close())
}
server.TLSConfig = tlsCfg
}

return server, nil
}

Expand Down Expand Up @@ -327,12 +318,7 @@ func (s *Server) Start(ctx context.Context) error {
go func() {
defer s.bgFinished.Done()
s.Logger.Info("Starting HTTP server", zap.Int("port", httpPort), zap.String("addr", s.queryOptions.HTTP.Endpoint))
var err error
if s.queryOptions.HTTP.TLSSetting != nil {
err = s.httpServer.ServeTLS(s.httpConn, "", "")
} else {
err = s.httpServer.Serve(s.httpConn)
}
err := s.httpServer.Serve(s.httpConn)
if err != nil && !errors.Is(err, http.ErrServerClosed) && !errors.Is(err, cmux.ErrListenerClosed) && !errors.Is(err, cmux.ErrServerClosed) {
s.Logger.Error("Could not start HTTP server", zap.Error(err))
s.ReportStatus(componentstatus.NewFatalErrorEvent(err))
Expand Down
10 changes: 7 additions & 3 deletions cmd/query/app/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func TestCreateTLSGrpcServerError(t *testing.T) {
require.Error(t, err)
}

func TestCreateTLSHttpServerError(t *testing.T) {
func TestStartTLSHttpServerError(t *testing.T) {
tlsCfg := configtls.ServerConfig{
ClientCAFile: "invalid/path",
Config: configtls.Config{
Expand All @@ -117,12 +117,16 @@ func TestCreateTLSHttpServerError(t *testing.T) {
},
}
telset := initTelSet(zaptest.NewLogger(t), jtracer.NoOp(), healthcheck.New())
_, err := NewServer(context.Background(), &querysvc.QueryService{}, nil,
s, err := NewServer(context.Background(), &querysvc.QueryService{}, nil,
&QueryOptions{
HTTP: confighttp.ServerConfig{Endpoint: ":8080", TLSSetting: &tlsCfg},
GRPC: configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{Endpoint: ":8081", Transport: confignet.TransportTypeTCP}},
}, tenancy.NewManager(&tenancy.Options{}), telset)
require.Error(t, err)
require.NoError(t, err)
require.Error(t, s.Start(context.Background()))
t.Cleanup(func() {
require.NoError(t, s.Close())
})
}

var testCases = []struct {
Expand Down

0 comments on commit edb896e

Please sign in to comment.