Add TLS support to HTTP/GRPC clients #2502

annanay25 · 2020-04-22T11:48:58Z

What this PR does:

go.mod updated to vendor latest commit of weaveworks/common which adds TLS support to the server.
Adds TLS support to all HTTP and GRPC clients in cortex.
- GRPC:
  - pkg/util/grpcclient updated to include TLS info.
  - All GRPC clients updated to use grpcclient.DialOptions() which provides TLS options as well.
- HTTP:
  - Util package pkg/util/httpclient created with a Config struct that stores necessary endpoint, timeout and tls config parameters.
  - All HTTP clients updated to use this config struct.

Which issue(s) this PR fixes:
Fixes #2350

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Annanay <[email protected]>

cmd/query-tee/main.go

cmd/query-tee/proxy_backend.go

Signed-off-by: Annanay <[email protected]>

Signed-off-by: Marco Pracucci <[email protected]>

pracucci

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.

pkg/querier/querier.go

pkg/querier/store_gateway_client.go

pkg/ruler/ruler.go

docs/configuration/config-file-reference.md

pkg/util/httpclient/httpclient.go

Signed-off-by: Marco Pracucci <[email protected]>

… into add-tls-support Signed-off-by: Annanay <[email protected]>

Signed-off-by: Annanay <[email protected]>

jtlisi

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.

pkg/chunk/gcp/bigtable_index_client.go

Signed-off-by: Annanay <[email protected]>

jtlisi

You need to update the clients used by the integration tests to support TLS as well.

jtlisi

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.

jtlisi · 2020-05-12T15:59:07Z

integration/query_frontend_test.go

+	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))
+


Suggested change

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.

I would actually suggest to commit certs in the integration/certs and then copy all of them to the shared dir.

sounds good, better than generating them for every run.

jtlisi · 2020-05-12T16:01:09Z

integration/query_frontend_test.go

-	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()), "")


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.

pracucci

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.

pkg/util/tls/tls.go

pkg/util/grpcclient/grpcclient.go

pracucci · 2020-05-13T08:25:01Z

docs/configuration/single-process-config-blocks-tls.yaml

@@ -0,0 +1,100 @@
+


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).

docs/production/tls.md

pracucci · 2020-05-13T08:29:09Z

integration/query_frontend_test.go

+	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))
+


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 <[email protected]>

pracucci

Could you also add a CHANGELOG entry, please?

pkg/util/tls/tls.go

pkg/util/grpcclient/grpcclient.go

pkg/ruler/ruler.go

pkg/querier/store_gateway_client.go

docs/production/tls.md

integration/query_frontend_test.go

Signed-off-by: Annanay <[email protected]>

pracucci

Thanks @annanay25 for addressing my feedback! LGTM 🎉 (I left one very last nit)

integration/e2ecortex/services.go

Signed-off-by: Annanay <[email protected]>

jtlisi

LGTM

* Checkpoint Signed-off-by: Annanay <[email protected]> * Add tls options to grpc client Signed-off-by: Annanay <[email protected]> * Add new httpclient util package for use in all client configs Signed-off-by: Annanay <[email protected]> * Change all grpc clients to use grpcclient Signed-off-by: Annanay <[email protected]> * Fix build, add docs Signed-off-by: Annanay <[email protected]> * Fix tests Signed-off-by: Annanay <[email protected]> * Fix lint, add tls to store-gw-client Signed-off-by: Annanay <[email protected]> * Rename config parameters Signed-off-by: Annanay <[email protected]> * Lint Signed-off-by: Annanay <[email protected]> * Nit fix Signed-off-by: Annanay <[email protected]> * Checkpoint Signed-off-by: Annanay <[email protected]> * Checkpoint Signed-off-by: Annanay <[email protected]> * Checkpoint Signed-off-by: Annanay <[email protected]> * Add integration tests for TLS Signed-off-by: Annanay <[email protected]> * Correct package names, fix config file reference Signed-off-by: Annanay <[email protected]> * Fix cert paths Signed-off-by: Annanay <[email protected]> * Fix lint, add sample tls config file Signed-off-by: Annanay <[email protected]> * Crash quickly if certs are bad Signed-off-by: Annanay <[email protected]> * Fixed linter and doc generation Signed-off-by: Marco Pracucci <[email protected]> * Cleaned white noise Signed-off-by: Marco Pracucci <[email protected]> * Address review comments Signed-off-by: Annanay <[email protected]> * Fix docs, flags Signed-off-by: Annanay <[email protected]> * Fix test Signed-off-by: Annanay <[email protected]> * Fix lint, docs Signed-off-by: Annanay <[email protected]> * Do not use TLS options with GCP clients Signed-off-by: Annanay <[email protected]> * Add client auth type, go mod tidy/vendor Signed-off-by: Annanay <[email protected]> * Address comments Signed-off-by: Annanay <[email protected]> * Fix lint, add new integration test Signed-off-by: Annanay <[email protected]> * Revert logging level to warn, add CHANGELOG entry Signed-off-by: Annanay <[email protected]> Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Jacob Lisi <[email protected]>

annanay25 added 7 commits April 9, 2020 22:30

Checkpoint

67ac979

Signed-off-by: Annanay <[email protected]>

Add tls options to grpc client

828e968

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

e85437b

Signed-off-by: Annanay <[email protected]>

Add new httpclient util package for use in all client configs

1ee9ddb

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

ef58c41

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

745299d

Signed-off-by: Annanay <[email protected]>

Change all grpc clients to use grpcclient

b93af11

Signed-off-by: Annanay <[email protected]>

pull-request-size bot added the size/XXL label Apr 22, 2020

annanay25 added 7 commits April 23, 2020 14:16

Fix build, add docs

8a71ce9

Signed-off-by: Annanay <[email protected]>

Fix tests

bab4c82

Signed-off-by: Annanay <[email protected]>

Fix lint, add tls to store-gw-client

8fee619

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

2ff0a73

Signed-off-by: Annanay <[email protected]>

Rename config parameters

d24a44d

Signed-off-by: Annanay <[email protected]>

Lint

485f8ba

Signed-off-by: Annanay <[email protected]>

Nit fix

5561a0f

Signed-off-by: Annanay <[email protected]>

annanay25 mentioned this pull request Apr 28, 2020

Update weaveworks/common dependency version #2535

Merged

2 tasks

annanay25 added 7 commits April 28, 2020 17:44

Merge branch 'master' into add-tls-support

598b00f

Signed-off-by: Annanay <[email protected]>

Checkpoint

548ddfa

Signed-off-by: Annanay <[email protected]>

Checkpoint

1a185b5

Signed-off-by: Annanay <[email protected]>

Checkpoint

1ccefbf

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

5fd056c

Signed-off-by: Annanay <[email protected]>

Add integration tests for TLS

0a8c4eb

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

b6aafa9

Signed-off-by: Annanay <[email protected]>

pull-request-size bot added size/XL and removed size/XXL labels May 4, 2020

annanay25 added 2 commits May 4, 2020 16:50

Correct package names, fix config file reference

d28a3d6

Signed-off-by: Annanay <[email protected]>

Fix cert paths

480308e

Signed-off-by: Annanay <[email protected]>

annanay25 commented May 6, 2020

View reviewed changes

cmd/query-tee/main.go Outdated Show resolved Hide resolved

annanay25 commented May 6, 2020

View reviewed changes

cmd/query-tee/proxy_backend.go Outdated Show resolved Hide resolved

Fix lint, add sample tls config file

ca7a6d9

Signed-off-by: Annanay <[email protected]>

annanay25 changed the title ~~WIP - Add TLS support to HTTP/GRPC clients~~ Add TLS support to HTTP/GRPC clients May 7, 2020

annanay25 and others added 2 commits May 7, 2020 18:03

Crash quickly if certs are bad

db64cd4

Signed-off-by: Annanay <[email protected]>

Fixed linter and doc generation

ee48ed3

Signed-off-by: Marco Pracucci <[email protected]>

pull-request-size bot added size/XXL and removed size/XL labels May 8, 2020

pracucci reviewed May 8, 2020

View reviewed changes

pracucci and others added 3 commits May 8, 2020 17:43

Cleaned white noise

b9325bd

Signed-off-by: Marco Pracucci <[email protected]>

Merge commit 'refs/pull/2502/head' of github.com:cortexproject/cortex…

721fed1

… into add-tls-support Signed-off-by: Annanay <[email protected]>

Address review comments

af4935d

Signed-off-by: Annanay <[email protected]>

pull-request-size bot added size/XL and removed size/XXL labels May 11, 2020

annanay25 added 3 commits May 11, 2020 18:28

Fix docs, flags

8f9f2e7

Signed-off-by: Annanay <[email protected]>

Fix test

668e988

Signed-off-by: Annanay <[email protected]>

Fix lint, docs

ef761f5

Signed-off-by: Annanay <[email protected]>

jtlisi suggested changes May 11, 2020

View reviewed changes

pkg/chunk/gcp/bigtable_index_client.go Outdated Show resolved Hide resolved

annanay25 added 2 commits May 12, 2020 02:13

Do not use TLS options with GCP clients

24919ee

Signed-off-by: Annanay <[email protected]>

Add client auth type, go mod tidy/vendor

9939303

Signed-off-by: Annanay <[email protected]>

jtlisi suggested changes May 12, 2020

View reviewed changes

pracucci reviewed May 13, 2020

View reviewed changes

Address comments

6eb9331

Signed-off-by: Annanay <[email protected]>

pracucci reviewed May 13, 2020

View reviewed changes

Fix lint, add new integration test

5aa65d1

Signed-off-by: Annanay <[email protected]>

pracucci approved these changes May 13, 2020

View reviewed changes

integration/e2ecortex/services.go Outdated Show resolved Hide resolved

annanay25 added 2 commits May 13, 2020 23:21

Revert logging level to warn, add CHANGELOG entry

ae5f6f7

Signed-off-by: Annanay <[email protected]>

Merge branch 'master' into add-tls-support

d645572

Signed-off-by: Annanay <[email protected]>

jtlisi approved these changes May 13, 2020

View reviewed changes

pracucci merged commit bca34cb into cortexproject:master May 14, 2020

annanay25 mentioned this pull request May 14, 2020

Add metric to monitor cert expiry time #2592

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS support to HTTP/GRPC clients #2502

Add TLS support to HTTP/GRPC clients #2502

annanay25 commented Apr 22, 2020 •

edited

Loading

pracucci left a comment

jtlisi left a comment

jtlisi left a comment

jtlisi left a comment

jtlisi May 12, 2020

pracucci May 13, 2020

jtlisi May 13, 2020

jtlisi May 12, 2020

pracucci left a comment

pracucci May 13, 2020

pracucci May 13, 2020

pracucci left a comment

pracucci left a comment

jtlisi left a comment

Add TLS support to HTTP/GRPC clients #2502

Add TLS support to HTTP/GRPC clients #2502

Conversation

annanay25 commented Apr 22, 2020 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

jtlisi May 12, 2020

Choose a reason for hiding this comment

pracucci May 13, 2020

Choose a reason for hiding this comment

jtlisi May 13, 2020

Choose a reason for hiding this comment

jtlisi May 12, 2020

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pracucci May 13, 2020

Choose a reason for hiding this comment

pracucci May 13, 2020

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

annanay25 commented Apr 22, 2020 •

edited

Loading