Fix GRPC/HTTP/TLS - #3300
Conversation
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
zalegrala
left a comment
zalegrala
left a comment
There was a problem hiding this comment.
This looks pretty good. Just a couple comments.
| util_log "github.com/grafana/tempo/pkg/util/log" | ||
| ) | ||
|
|
||
| type TempoServer interface { |
There was a problem hiding this comment.
Do we need the interface? It looks like we would be able to use the struct directly. Are you anticipating adding another implementation?
There was a problem hiding this comment.
the primary reason i used an interface was to limit the ways in which the rest of the code could interact with the t.server var. Another option would have been to move this into its own package but i suppose that didn't feel necessary.
There was a problem hiding this comment.
I suppose my feeling is that interfaces should have more than one implementation, but I see your perspective to limit the interactions.
| } | ||
|
|
||
| type ca struct { | ||
| key *ecdsa.PrivateKey |
There was a problem hiding this comment.
I see this is only in the e2e, but are we restricted to ecdsa in the implementation?
There was a problem hiding this comment.
we are restricted to whatever go supports i suppose. we're using vanilla HTTP everything.
| return err | ||
| } | ||
|
|
||
| template.IsCA = false |
There was a problem hiding this comment.
Should this be true for the CA cert?
There was a problem hiding this comment.
the ca cert is handled in writeCACertificate() which uses the ca.cert struct which sets IsCA to true.
to be transparent, i lifted this file directly from dskit and am not familiar with all choices made.
There was a problem hiding this comment.
I read this a little quick. Now I see that this is the cert signed by the ca.
| util.SearchAndAssertTrace(t, apiClient, info) | ||
| util.SearchTraceQLAndAssertTrace(t, apiClient, info) | ||
|
|
||
| creds := credentials.NewTLS(&tls.Config{InsecureSkipVerify: true}) |
There was a problem hiding this comment.
Do we want to validate the server cert in this case?
There was a problem hiding this comment.
i think this is fine. TLS is still in use and functional. we're just not validating the cert against the CA's
|
|
||
| // RegisterFlags add internal server flags to flagset | ||
| func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | ||
| f.StringVar(&cfg.Config.HTTPListenAddress, "internal-server.http-listen-address", "localhost", "HTTP internal server listen address.") |
There was a problem hiding this comment.
With an empty string here, does the underlying implementation listen on everything or local or fail?
There was a problem hiding this comment.
it listens on everything which is the default behavior for our main server as well. it took me awhile to figure out why the internal server wasn't responding and this was the reason.
imo, if you turn on/configure internal server you know you are exposing a new port and at that time you will make configuration choices to secure it in whatever way makes sense for your org.
right now if you do the minimal config to start the internal server it refuses requests and it's difficult to determine why.
There was a problem hiding this comment.
Nice, I agree listen on everything unless you know better.
There was a problem hiding this comment.
netstat -luntp or apk add iproute2 && ss -ltn are friends of mine.
There was a problem hiding this comment.
i am not the networking wizard you are :)
zalegrala
left a comment
zalegrala
left a comment
There was a problem hiding this comment.
Looks good to me. One comment about the behavior of an empty listen address for the internal server.
|
thanks for the review! |
* first pass - working Signed-off-by: Joe Elliott <number101010@gmail.com> * refactor Signed-off-by: Joe Elliott <number101010@gmail.com> * working? Signed-off-by: Joe Elliott <number101010@gmail.com> * vendor Signed-off-by: Joe Elliott <number101010@gmail.com> * integration test Signed-off-by: Joe Elliott <number101010@gmail.com> * updated go.mod to latest Signed-off-by: Joe Elliott <number101010@gmail.com> * cleanup Signed-off-by: Joe Elliott <number101010@gmail.com> * changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> * update servless mods Signed-off-by: Joe Elliott <number101010@gmail.com> * satisfy interface updates Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> --------- Signed-off-by: Joe Elliott <number101010@gmail.com>
2 participants
What this PR does:
Tempo currently has the ability to support both HTTP and GRPC requests on its primary HTTP port. It accomplishes it by using cmux through a feature in dskit. Unfortunately cmux breaks TLS on this port. This PR reworks our implementation of the shared GRPC/HTTP port to allow TLS to work.
Also:
Which issue(s) this PR fixes:
Fixes #3278
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]