-
Notifications
You must be signed in to change notification settings - Fork 10
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
Pass CA file location as an arg to the worker #255
Changes from 2 commits
5632804
7b2c374
eeaa899
f042d81
31ff0f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
11.1.0 | ||
11.1.1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package worker | |
import ( | ||
"context" | ||
"crypto/tls" | ||
"crypto/x509" | ||
"os" | ||
"time" | ||
|
||
|
@@ -28,11 +29,24 @@ var redisBytesRead = prometheus.NewCounter(prometheus.CounterOpts{ | |
Name: "redis_bytes_read_total", | ||
}) | ||
|
||
func getTLSConfig(caFile string) *tls.Config { | ||
caCert, err := os.ReadFile(caFile) | ||
if err != nil { | ||
log.Warning("Failed to collect CA cert from file %s: '%s'. Redis connection will not work", caFile, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should probably return an error here, and die if it fails. If someone passes a CA I'd expect it not to carry on if it can't load it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean let the whole worker die? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well writing and connecting to Redis is best-effort, but I wouldn't expect loading the CA cert to fail? |
||
return &tls.Config{} | ||
} | ||
caCertPool := x509.NewCertPool() | ||
caCertPool.AppendCertsFromPEM(caCert) | ||
return &tls.Config{ | ||
RootCAs: caCertPool, | ||
} | ||
} | ||
|
||
// newRedisClient augments an existing elan.Client with a Redis connection. | ||
// All usage of Redis is best-effort only. | ||
// If readURL is set, all reads will happen on this URL. If not, everything | ||
// will go to url. | ||
func newRedisClient(client elan.Client, url, readURL, password string, useTLS bool) elan.Client { | ||
func newRedisClient(client elan.Client, url, readURL, password, caFile string, useTLS bool) elan.Client { | ||
primaryOpts := &redis.Options{ | ||
Addr: url, | ||
Password: password, | ||
|
@@ -42,8 +56,8 @@ func newRedisClient(client elan.Client, url, readURL, password string, useTLS bo | |
Password: password, | ||
} | ||
if useTLS { | ||
primaryOpts.TLSConfig = &tls.Config{} | ||
readOpts.TLSConfig = &tls.Config{} | ||
primaryOpts.TLSConfig = getTLSConfig(caFile) | ||
readOpts.TLSConfig = getTLSConfig(caFile) | ||
} | ||
primaryClient := redis.NewClient(primaryOpts) | ||
readClient := primaryClient | ||
|
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.
If we're strictly doing semver this is 11.2.0 because it's a feature :) Not that we're that strict in this repo.