search

go-spatial / tegola

Tegola is a Mapbox Vector Tile server written in Go
http://tegola.io/
MIT License
1.26k stars 193 forks source link

feat: redis ssl in cache options #816

Closed iwpnd closed 2 years ago

iwpnd commented 2 years ago

As discussed in #815 I create a new option for redis. I follow your implementation to the point where I check if ssl is true. If it's the case I split the addr into host and port and create a tls.Config. I follow ParseURL implementation here and add a TLSConfig to the redis.Options.

iwpnd commented 2 years ago

Not quite unexpected that I cannot test an ssl connection with a non-ssl redis in the pipeline. I'm pretty sure we can setup another redis instance in the pipeline that is properly configured to test SSL. Your call @ARolek :)

edit: Was checking how node-redis and redis-go are testing with SSL and they don't. go-redis for example, separately tests methods that create the redis.Options and validate its output, rather than testing whether the connection is secured or not.

edit2: tested a forked image with these changes and was able to deploy tegola in kubernetes and using AWS elasticache as cache. 🥳

edit3: Got changes lined up that would test a new createOptions method separately. This way the configuration can safely be tested without running into connection issues with redis when ssl is set to true. Let me know if you want this to be changed and I will add it to the PR.

iwpnd commented 2 years ago

@ARolek if you have a minute to spare, I would highly value your feedback on this. Have a great start of the week! ✌️

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1d9feac04-PR-816

Changes Missing Coverage Covered Lines Changed/Added Lines %
cache/redis/errors.go 0 3 0.0%
<!-- Total: 25 28 89.29% -->
Totals Coverage Status
Change from base Build 8e12766c4: 0.1%
Covered Lines: 5460
Relevant Lines: 12093
💛 - Coveralls
iwpnd commented 2 years ago

Alright, all set @ARolek @gdey - thank you for considering it and thank you for the review. 🙏

ARolek commented 2 years ago

@iwpnd I realized we missed one minor detail, adding the new parameter to the README! Can you please do that when you have a min:

https://github.com/go-spatial/tegola/tree/v0.15.x/cache/redis#properties