Redis TLS and Auth support #96

repl-david-winiarski · 2019-08-23T16:09:52Z

This adds support for TLS connections to redis as well as support for authentication.
Somewhat related to issue #61

mattklein123 · 2019-08-23T16:18:04Z

stale · 2019-08-30T16:48:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

junr03

Thanks. Some changes needed. Additionally:

Add tests for this functionality
Document this functionality.

src/settings/settings.go

src/redis/driver_impl.go

repl-david-winiarski · 2019-09-04T16:25:00Z

@junr03 I've updated the README and added a set of integration tests, Mind having another look? Thanks.

junr03 · 2019-09-09T17:46:07Z

Thanks for the update @repl-david-winiarski. If you are ok, I would like to wait for a moment while @anpryl updates #95 as both PRs are adding related features, and I would like to build consistency here.

repl-david-winiarski · 2019-09-09T20:05:32Z

No rush @junr03. Ill keep an eye on this PR in case anything else needs to be done.

stale · 2019-09-16T20:34:15Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repl-david-winiarski · 2019-09-17T12:49:55Z

bump to remove stale label

junr03 · 2019-09-18T16:36:15Z

@HenryYYang for a second pair of eyes.

stale · 2019-09-25T17:11:04Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repl-david-winiarski · 2019-09-25T18:03:16Z

bump to remove stale label

stale · 2019-10-02T18:31:58Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repl-david-winiarski · 2019-10-02T18:38:54Z

bump to remove stale label

stale · 2019-10-09T18:42:02Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repl-david-winiarski · 2019-10-09T19:08:57Z

bump

HenryYYang · 2019-10-09T20:17:04Z

LGTM

repl-david-winiarski · 2019-10-10T12:53:04Z

Thanks @HenryYYang

junr03 · 2019-10-16T23:17:46Z

Thanks @HenryYYang for the second pair of eyes. I will review this in the morning. Thanks for bearing with me @repl-david-winiarski

junr03

A couple comments, thanks for working on this!

junr03 · 2019-10-16T23:52:17Z

test/integration/integration_test.go


 		go func() {
 			runner.Run()
 		}()

 		// HACK: Wait for the server to come up. Make a hook that we can wait on.
-		time.Sleep(100 * time.Millisecond)
+		time.Sleep(100 * time.Second)


why is the waiting time longer?

I needed to increase this because of the additional redis servers, and stunnel that are spun up in the travis build. I've made this a more sane value at 1s instead of 100s.

junr03 · 2019-10-16T23:56:15Z

src/service_cmd/runner/runner.go


 	}
-
+	var otherPool redis.Pool


Two thoughts here:

Should we realistically support enabling TLS for one of the pools but not the other?

For the auth case, right now we need the user to set up both the tls envvar and the auth one, should we make it so that if auth is set, tls is set by default?

This was done this way, since it seemed that the two types of pools where meant to be uniquely configurable. I don't mind having a single tls envvar to enable tls on both pools though.

I think that makes sense, since you'll almost always want tls on when using auth, so having it as the default makes sense with the option to override.

I guess it is not that much complexity, and it leaves it open for configurability.

lgtm, thanks!

junr03

Thanks for sticking with this!

* Add Docker Compose File (#27) * docs: match envoy docs for remote_address ratelimiting (#29) * docs: document dependency on gostats (#30) * test and document whitelist behavior (#31) Signed-off-by: Daniel Hochman <danielhochman@users.noreply.github.com> * update dependencies (#35) * update dependencies * proto: use the proto defined in data-plane-api (#39) * proto: check in protos to allow importing ratelimit as a library (#40) * docs: update contact info (#42) * redis: add the option to use a separate redis pool for per second limits (#41) * fix duplicate mv (#43) * docker: upgrade docker-compose setup (#46) * Add gRPC health check (#47) * logging: set log level (#50) * go version: update to 1.11 (#53) * docker compose: expose gRPC port on docker compose setup (#55) * Configuration to ignore dotfiles. (#52) This allows ratelimit to run on Kubernetes with configuration from a configmap. * Add Dockerfile to enable builds (#58) Signed-off-by: Steve Sloka <steves@heptio.com> * ci: fix build (#73) Fixes envoyproxy#71 Signed-off-by: Matt Klein <mklein@lyft.com> * Run unit and integration tests with race detector enabled (#65) * deps: update several of ratelimit's dependencies (#76) * add stalebot (#78) Signed-off-by: Matt Klein <mklein@lyft.com> * docs: fix example 4 sample config (#79) * fix redis-server binary name (envoyproxy#88) * Fix build Dockerfile (envoyproxy#98) Fix problem: src/service_cmd/runner/runner.go:10:2: cannot find package "github.com/lyft/ratelimit/proto/ratelimit" in any of: /usr/local/go/src/github.com/lyft/ratelimit/vendor/github.com/lyft/ratelimit/proto/ratelimit (vendor tree) /usr/local/go/src/vendor/github.com/lyft/ratelimit/proto/ratelimit /usr/local/go/src/github.com/lyft/ratelimit/proto/ratelimit (from $GOROOT) /go/src/github.com/lyft/ratelimit/proto/ratelimit (from $GOPATH) The command '/bin/sh -c go build -o /usr/local/bin/ratelimit src/service_cmd/main.go' returned a non-zero code: 1 * Redis TLS and Auth support (envoyproxy#96) This adds support for TLS connections to redis as well as support for authentication. Somewhat related to issue #61 * healthcheck: allow customizable healthcheck name (envoyproxy#102) Description: this patch allows a consumer of the server package to customize the name of the healthchecker. Signed-off-by: Jose Nino <jnino@lyft.com> * health: make a few more types public (envoyproxy#104) Description: envoyproxy#102 allowed for some customization. This PR makes the types public so that other servers can use this implementation. Signed-off-by: Jose Nino <jnino@lyft.com> * Add local cache to store whether it is over the limit (envoyproxy#111) * Plugin statstore into runner (envoyproxy#115) * fix: support auth without tls (envoyproxy#116) Signed-off-by: tangxinfa <tangxinfa@gmail.com> * add local cache stats (envoyproxy#114) * Move license to templated Apache-2.0 (envoyproxy#123) Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com> * Enable go modules (envoyproxy#124) Signed-off-by: Steve Sloka <slokas@vmware.com> * CI: Github Actions (envoyproxy#127) Signed-off-by: Steve Sloka <slokas@vmware.com> * community: update contributing guide (envoyproxy#139) Fixes envoyproxy#138 Signed-off-by: Matt Klein <mklein@lyft.com> * add http 1 `/json` endpoint (envoyproxy#136) Signed-off-by: David Black <david.black@autodesk.com> * Use mockgen version from go.mod instead of from "make bootstrap" (envoyproxy#143) Even though the Makefile wants to encourage using mockgen@1.4.1, it seems like the mocks have been generated using a pre-1.0 version of mockgen. Using "go run github.com/golang/mock/mockgen" as a go:generate command instead of just "mockgen" avoids the need to pre-install into the developer's $PATH and uses the go.mod-specified version Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Upgrade gostats dependency from 0.2.6 to 0.4.0 (envoyproxy#141) My interest is the UDP protocol support which appeared in gotstats 0.3.10 There's a breaking change as of https://github.com/lyft/gostats/releases/tag/v0.3.0 which is that gostats no longer publishes stats as expvars. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Upgrade radix (envoyproxy#137) Signed-off-by: Tong Cai <caitong93@gmail.com> * cache_impl_test.go: fix failing test with ipv6 (envoyproxy#144) A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: The testify assert package doesn't seem to support inexact matching on error messages, so the code gets a bit uglier than before. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Split redis-specific logic from generic key-value store logic (envoyproxy#142) This is a pure refactoring with no behavior changes. It's a step toward being able to add memcache as a backend (see envoyproxy#140). This PR moves RateLimitCache from the redis package to a new "limiter" package, along with code for time/jitter, local cache stats, and constructing cache keys. All that can be reused with memcache. After this PR, the redis package is imported in exactly two places: - in service_cmd/runner/runner.go to call redis.NewRateLimiterCacheImplFromSettings() - in service/ratelimit.go in ShouldRateLimit to identify if a recovered panic is a redis.RedisError. If so, a stat is incremented and the panic() propagation is ended and in favor of returning the error as a the function result. The PR also includes changes by goimports to test/service/ratelimit_test.go so that the difference between package name vs file path name is explicit instead of implicit. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * json handler: return full ratelimit service response as json (envoyproxy#148) Previously an HTTP POST to /json would only return an HTTP status code, not all the other details supported by grpc ratelimit responses. With this change an HTTP POST to /json receives the full proto3 response encoded as json by jsonpb. It seems unlikely that anyone would be parsing the text "over limit" from the HTTP body instead of just reading the 429 response code, but for anyone doing that this would be a breaking change. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Update goruntime to latest, 0.2.5. Add new config for watching changes in runtime config folder directly instead of the runtime root dir. (envoyproxy#151) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Drop support for legacy ratelimit.proto and upgrade to v3 rls.proto (envoyproxy#153) Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> * Followups to v3 upgrade (envoyproxy#155) - Regenerate mocks based on new default protocol - Manually transform v2 messages to v3 messages - some of the fields were renamed thus json Marshal/Unmarshal does not work anymore - Added tests that verify conversion v2<->v3 works for headers fields - Update tests to use proto.Equal - simple assert.Equals might not work correctly for protobuf messages. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> * Introduce a Dockerfile for running integration tests (envoyproxy#156) This diff creates Dockerfile.integration for running integration tests with clearly-defined dependencies. Previously the dependencies of the integration tests were defined within the github actions config. The new "make docker_tests" target should work for any developer with Docker installed. Previously there was no single command that would run integration tests across platforms, which makes development and onboarding harder. Even copying the command from github actions wouldn't have worked before, since that command quietly assumed that redis was already running on port 6379. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Add support for rate limit overrides. (envoyproxy#158) Fixes envoyproxy#154 Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> * redis client: default to use explicit pipelining (envoyproxy#163) Signed-off-by: Tong Cai <caitong93@gmail.com> * Clean go.mod file and update logrus to latest (envoyproxy#166) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Add full test environment example. Fix bug in existing docker-compose. (envoyproxy#170) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Implement LOG_FORMAT=json (envoyproxy#173) Centralized log collection system works better with logs in json format. E.g. DataDog strongly encourage setting up your logging library to produce your logs in JSON format to avoid the need for custom parsing rules. So, the next small fix is all we need to get json logs. Signed-off-by: Sergey Belyaev <sbelyaev@setronica.com> * ci: Update github action to push docker image tagged with sha for each merge to master branch (envoyproxy#176) Updates the github action to also push a tagged image based upon the git sha. The tag also includes the current version of the release. Example tag: envoyproxy/ratelimit:f1758150b6dfed3e5c0ae13fb7bb6b8f6ae00b0e Fixes envoyproxy#174 Signed-off-by: Steve Sloka <slokas@vmware.com> * Update README.md to refer to existing files (envoyproxy#178) Signed-off-by: Margaret Gorguissian <margaret.gorguissian@tufts.edu> * Add redis cluster and sentinel support (envoyproxy#179) Signed-off-by: Diego Erdody <diego@medallia.com> * Add support for x-ratelimit-reset header (envoyproxy#182) Signed-off-by: Clara Andrew-Wani <candrewwani@gmail.com> * Create repokitteh.star (envoyproxy#187) Signed-off-by: Itay Donanhirsh <itay@bazoo.org> * refactor NearLimitRatio to environment variable (envoyproxy#186) Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com> * Fix flakey tests with DurationUntilReset. Update docker example to V3 config. (envoyproxy#192) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Separate Redis cache and driver implementation (envoyproxy#194) Signed-off-by: William Albertus Dembo <w.albertusd@gmail.com> * Set ratelimit filter to v3 api (envoyproxy#196) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Add debug logging to indicate descriptor and limit (envoyproxy#197) Signed-off-by: Sasha Kulbii <okulbii@wayfair.com> * Implement BACKEND_TYPE=memcache as an alternative k/v store to redis (envoyproxy#172) MEMCACHE_HOST_PORT=host:port must be set with BACKEND_TYPE=memcache To minimize roundtrips when getting multiple keys, the memcache implementation does a GetMulti to fetch the existing rate limit usage and does increments asynchronously in background goroutines, since the memcache API doesn't offer multi-increment. Resolves envoyproxy#140 Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Refactoring of duplicated code across backend types (envoyproxy#202) Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com> * Small typo fix in README (envoyproxy#204) Signed-off-by: cpaika <paika.christopher@gmail.com> * Circle CI (#1) * Add Circle Config * Touch to build * Move to expected path * Use Docker Command * Do it all here then * Env * Add Readme * Actual README * Add Docker Compose (#2) * VAULT-893 Fix docker-compose.yml * VAULT-893 Fix docker-compose.yml Co-authored-by: Marshall Jones <marshall@offby3.com> Co-authored-by: Jose Ulises Nino Rivera <junr03@users.noreply.github.com> Co-authored-by: Daniel Hochman <danielhochman@users.noreply.github.com> Co-authored-by: Martien Verbruggen <martien.verbruggen@gmail.com> Co-authored-by: Ben Pope <BenPope@users.noreply.github.com> Co-authored-by: Steve Sloka <steve@stevesloka.com> Co-authored-by: Matt Klein <mattklein123@gmail.com> Co-authored-by: Charlie Vieth <charlie.vieth@gmail.com> Co-authored-by: Adil Hafeez <ahafeez@lyft.com> Co-authored-by: Kartograf <kartogrof@gmail.com> Co-authored-by: repl-david-winiarski <33431229+repl-david-winiarski@users.noreply.github.com> Co-authored-by: Junchao Lyu <6963707+freedomljc@users.noreply.github.com> Co-authored-by: tangxinfa <tangxinfa@gmail.com> Co-authored-by: Steve Sloka <slokas@vmware.com> Co-authored-by: Matt Klein <mklein@lyft.com> Co-authored-by: dblackdblack <github@dhb.is> Co-authored-by: David Weitzman <dweitzman@pinterest.com> Co-authored-by: Tong Cai <caitong@caicloud.io> Co-authored-by: Yuki Sawa <yukisawa@gmail.com> Co-authored-by: Petr Pchelko <petrpchelko@gmail.com> Co-authored-by: Petr Pchelko <ppchelko@wikimedia.org> Co-authored-by: Sergey Belyaev <svdba@users.noreply.github.com> Co-authored-by: Margaret G <Margaret.Gorguissian@tufts.edu> Co-authored-by: Diego Erdody <erdody@gmail.com> Co-authored-by: Clara <candrewwani@gmail.com> Co-authored-by: Itay Donanhirsh <itay@bazoo.org> Co-authored-by: Zufar Dhiyaulhaq <zufardhiyaulhaq@gmail.com> Co-authored-by: William Albertus Dembo <w.albertusd@gmail.com> Co-authored-by: Alex Kulbii <jncneo@gmail.com> Co-authored-by: Kateryna Nezdolii <nezdolik@spotify.com> Co-authored-by: Christopher <paika.christopher@gmail.com>

repl-david-winiarski added 2 commits August 22, 2019 08:28

basic redis tls and auth client

15176ba

toggle tls and auth using env var

52c438a

mattklein123 assigned junr03 Aug 23, 2019

stale bot added the stale label Aug 30, 2019

junr03 suggested changes Aug 30, 2019

View reviewed changes

src/settings/settings.go Outdated Show resolved Hide resolved

src/settings/settings.go Outdated Show resolved Hide resolved

src/redis/driver_impl.go Outdated Show resolved Hide resolved

stale bot removed stale labels Aug 30, 2019

add integration tests

2ae07fd

repl-david-winiarski force-pushed the Redis-TLS-Auth branch 2 times, most recently from 6d2db54 to a6a37bb Compare September 4, 2019 13:46

repl-david-winiarski added 3 commits September 4, 2019 10:22

add sudo in travis.yml

0563d2e

update README

34a745d

fix env name in settings

c93d0c5

repl-david-winiarski force-pushed the Redis-TLS-Auth branch from 780e0ab to c93d0c5 Compare September 4, 2019 16:22

remove format changes to cache test

13719c9

junr03 mentioned this pull request Sep 9, 2019

Add Redis database number to settings #95

Closed

stale bot added the stale label Sep 16, 2019

stale bot removed the stale label Sep 17, 2019

stale bot added the stale label Sep 25, 2019

stale bot removed the stale label Sep 25, 2019

stale bot added the stale label Oct 2, 2019

stale bot removed the stale label Oct 2, 2019

stale bot added the stale label Oct 9, 2019

stale bot removed the stale label Oct 9, 2019

junr03 reviewed Oct 16, 2019

View reviewed changes

repl-david-winiarski added 2 commits October 17, 2019 07:14

enable tls by default if auth is set

8fe2508

increase wait time for redis servers to be ready

318e99f

junr03 approved these changes Oct 17, 2019

View reviewed changes

junr03 merged commit 80a2ab3 into envoyproxy:master Oct 17, 2019

repl-david-winiarski deleted the Redis-TLS-Auth branch October 17, 2019 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redis TLS and Auth support #96

Redis TLS and Auth support #96

repl-david-winiarski commented Aug 23, 2019 •

edited

Loading

mattklein123 commented Aug 23, 2019

stale bot commented Aug 30, 2019

junr03 left a comment

repl-david-winiarski commented Sep 4, 2019

junr03 commented Sep 9, 2019

repl-david-winiarski commented Sep 9, 2019

stale bot commented Sep 16, 2019

repl-david-winiarski commented Sep 17, 2019

junr03 commented Sep 18, 2019

stale bot commented Sep 25, 2019

repl-david-winiarski commented Sep 25, 2019

stale bot commented Oct 2, 2019

repl-david-winiarski commented Oct 2, 2019

stale bot commented Oct 9, 2019

repl-david-winiarski commented Oct 9, 2019

HenryYYang commented Oct 9, 2019

repl-david-winiarski commented Oct 10, 2019

junr03 commented Oct 16, 2019

junr03 left a comment

junr03 Oct 16, 2019

repl-david-winiarski Oct 17, 2019

junr03 Oct 16, 2019

repl-david-winiarski Oct 17, 2019

junr03 Oct 17, 2019

junr03 left a comment

Redis TLS and Auth support #96

Redis TLS and Auth support #96

Conversation

repl-david-winiarski commented Aug 23, 2019 • edited Loading

mattklein123 commented Aug 23, 2019

stale bot commented Aug 30, 2019

junr03 left a comment

Choose a reason for hiding this comment

repl-david-winiarski commented Sep 4, 2019

junr03 commented Sep 9, 2019

repl-david-winiarski commented Sep 9, 2019

stale bot commented Sep 16, 2019

repl-david-winiarski commented Sep 17, 2019

junr03 commented Sep 18, 2019

stale bot commented Sep 25, 2019

repl-david-winiarski commented Sep 25, 2019

stale bot commented Oct 2, 2019

repl-david-winiarski commented Oct 2, 2019

stale bot commented Oct 9, 2019

repl-david-winiarski commented Oct 9, 2019

HenryYYang commented Oct 9, 2019

repl-david-winiarski commented Oct 10, 2019

junr03 commented Oct 16, 2019

junr03 left a comment

Choose a reason for hiding this comment

junr03 Oct 16, 2019

Choose a reason for hiding this comment

repl-david-winiarski Oct 17, 2019

Choose a reason for hiding this comment

junr03 Oct 16, 2019

Choose a reason for hiding this comment

repl-david-winiarski Oct 17, 2019

Choose a reason for hiding this comment

junr03 Oct 17, 2019

Choose a reason for hiding this comment

junr03 left a comment

Choose a reason for hiding this comment

repl-david-winiarski commented Aug 23, 2019 •

edited

Loading