-
Notifications
You must be signed in to change notification settings - Fork 791
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
Add TLS support to HTTP/GRPC clients #2502
Conversation
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Marco Pracucci <marco@pracucci.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.
Thanks @annanay25 for working on it! Adding TLS support between Cortex components/services is very neat.
I've some concerns about the generic httpclient.Config
struct because this causes you some troubles to keep backward compatibility and also add extra options to clients where such options are not necessarily required. Pretty much the same with gprcclient.Config
.
I will reach you offline to further discuss a sligthly different design.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
… into add-tls-support Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@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 PR looks close to ready. Can you take a look at the GCP clients you configured. I don't think we want to instrument those with support for TLS. It will only confuse the Auth support Google already has.
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@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.
You need to update the clients used by the integration tests to support TLS as well.
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.
You need to refactor the integration tests to support TLS. Consider limiting the scope of this PR to add a single TLS integration test that ensures the server and clients work.
Later, other integration tests can be updated to check for TLS.
integration/query_frontend_test.go
Outdated
cmd := exec.Command("bash", "certs/genCerts.sh", "certs", "1") | ||
require.NoError(t, cmd.Run()) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+clientCertFile, clientCertFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+clientKeyFile, clientKeyFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+rootCertFile, rootCertFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+serverCertFile, serverCertFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+serverKeyFile, serverKeyFile)) | ||
|
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.
cmd := exec.Command("bash", "certs/genCerts.sh", "certs", "1") | |
require.NoError(t, cmd.Run()) | |
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+clientCertFile, clientCertFile)) | |
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+clientKeyFile, clientKeyFile)) | |
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+rootCertFile, rootCertFile)) | |
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+serverCertFile, serverCertFile)) | |
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+serverKeyFile, serverKeyFile)) | |
cmd := exec.Command("bash", "certs/genCerts.sh", s.SharedDir()+"/certs", "1") | |
require.NoError(t, cmd.Run()) |
You don't need to mount each file into the env, you can generate the certs directly into the e2e shared directory.
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 would actually suggest to commit certs in the integration/certs
and then copy all of them to the shared dir.
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.
sounds good, better than generating them for every run.
integration/query_frontend_test.go
Outdated
queryFrontend := e2ecortex.NewQueryFrontendWithConfigFile("query-frontend", configFile, flags, "") | ||
ingester := e2ecortex.NewIngesterWithConfigFile("ingester", consul.NetworkHTTPEndpoint(), configFile, flags, "") | ||
distributor := e2ecortex.NewDistributorWithConfigFile("distributor", consul.NetworkHTTPEndpoint(), configFile, flags, "") | ||
queryFrontend := e2ecortex.NewQueryFrontendWithConfigFile("query-frontend", configFile, mergeFlags(flags, GetServerTLSFlags()), "") |
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.
When you create the Service you need to ensure that the built in ReadinessCheck supports TLS. Same with any external client that accesses the service.
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.
Thanks @annanay25 for addressing my initial comments. Much appreciated! This PR looks in a better shape now and I think we're quite close. I left some comments to further polish the config and nits here and there.
@@ -0,0 +1,100 @@ | |||
|
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.
For each pre-cooked config we provide, we have an integration test in integration/getting_started_single_process_config_test.go
. Would be great if you could add a test there for this file (a new test, which can be an existing one you copy, paste and modify as needed).
integration/query_frontend_test.go
Outdated
cmd := exec.Command("bash", "certs/genCerts.sh", "certs", "1") | ||
require.NoError(t, cmd.Run()) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+clientCertFile, clientCertFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+clientKeyFile, clientKeyFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+rootCertFile, rootCertFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+serverCertFile, serverCertFile)) | ||
require.NoError(t, copyFileToSharedDir(s, integrationHomeFolder+serverKeyFile, serverKeyFile)) | ||
|
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 would actually suggest to commit certs in the integration/certs
and then copy all of them to the shared dir.
Signed-off-by: Annanay <annanayagarwal@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.
Could you also add a CHANGELOG entry, please?
Signed-off-by: Annanay <annanayagarwal@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.
Thanks @annanay25 for addressing my feedback! LGTM 🎉 (I left one very last nit)
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@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.
LGTM
* Checkpoint Signed-off-by: Annanay <annanayagarwal@gmail.com> * Add tls options to grpc client Signed-off-by: Annanay <annanayagarwal@gmail.com> * Add new httpclient util package for use in all client configs Signed-off-by: Annanay <annanayagarwal@gmail.com> * Change all grpc clients to use grpcclient Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix build, add docs Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix tests Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix lint, add tls to store-gw-client Signed-off-by: Annanay <annanayagarwal@gmail.com> * Rename config parameters Signed-off-by: Annanay <annanayagarwal@gmail.com> * Lint Signed-off-by: Annanay <annanayagarwal@gmail.com> * Nit fix Signed-off-by: Annanay <annanayagarwal@gmail.com> * Checkpoint Signed-off-by: Annanay <annanayagarwal@gmail.com> * Checkpoint Signed-off-by: Annanay <annanayagarwal@gmail.com> * Checkpoint Signed-off-by: Annanay <annanayagarwal@gmail.com> * Add integration tests for TLS Signed-off-by: Annanay <annanayagarwal@gmail.com> * Correct package names, fix config file reference Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix cert paths Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix lint, add sample tls config file Signed-off-by: Annanay <annanayagarwal@gmail.com> * Crash quickly if certs are bad Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fixed linter and doc generation Signed-off-by: Marco Pracucci <marco@pracucci.com> * Cleaned white noise Signed-off-by: Marco Pracucci <marco@pracucci.com> * Address review comments Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix docs, flags Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix test Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix lint, docs Signed-off-by: Annanay <annanayagarwal@gmail.com> * Do not use TLS options with GCP clients Signed-off-by: Annanay <annanayagarwal@gmail.com> * Add client auth type, go mod tidy/vendor Signed-off-by: Annanay <annanayagarwal@gmail.com> * Address comments Signed-off-by: Annanay <annanayagarwal@gmail.com> * Fix lint, add new integration test Signed-off-by: Annanay <annanayagarwal@gmail.com> * Revert logging level to warn, add CHANGELOG entry Signed-off-by: Annanay <annanayagarwal@gmail.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
What this PR does:
go.mod
updated to vendor latest commit ofweaveworks/common
which adds TLS support to the server.pkg/util/grpcclient
updated to include TLS info.grpcclient.DialOptions()
which provides TLS options as well.pkg/util/httpclient
created with aConfig
struct that stores necessary endpoint, timeout and tls config parameters.Which issue(s) this PR fixes:
Fixes #2350
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]