-
Notifications
You must be signed in to change notification settings - Fork 530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix GRPC/HTTP/TLS #3300
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is only in the e2e, but are we restricted to ecdsa
in the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be true for the CA cert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to validate the server cert in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is fine. TLS is still in use and functional. we're just not validating the cert against the CA's
@@ -15,7 +15,7 @@ type Config struct { | |||
|
|||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With an empty string here, does the underlying implementation listen on everything or local or fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I agree listen on everything unless you know better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netstat -luntp
or apk add iproute2 && ss -ltn
are friends of mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not the networking wizard you are :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. One comment about the behavior of an empty listen address for the internal server.
thanks for the review! |
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.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]