-
Notifications
You must be signed in to change notification settings - Fork 188
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
*: watch tls certificate changes #47
Conversation
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.
Just two small comments otherwise overall looks good!
pkg/tls/reloader.go
Outdated
|
||
var err error | ||
|
||
crt, err := ioutil.ReadFile(r.certPath) |
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.
Unlikely but I think we should check both errors here
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.
oh, doh, i wanted to achieve a running err check, silly me, i'll add an err check here too obviously, good catch 👍
pkg/tls/reloader.go
Outdated
} | ||
|
||
h := fnv.New64a() | ||
_, err = h.Write(crt) |
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.
Is hashing really necessary? Seems like we should just be able to compare the bytes of the certs themselves.
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 👍
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.
small complication, I rechecked, and the parsing function stores DEM bytes as per https://golang.org/src/crypto/tls/tls.go?s=6337:6408#L193, so comparing the raw file content is not applicable.
Does comparing the https://golang.org/pkg/crypto/tls/#Certificate.Certificate slice after parsing sound good to you?
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.
Can’t we save the raw content bytes in the same fashion as we do the hash right now?
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.
we definitely can, i'll add that 👍
c1502bb
to
cad3efb
Compare
@brancz addressed comments, PTAL |
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.
We’re very close, just a small nit around the lock granularity.
pkg/tls/reloader.go
Outdated
} | ||
|
||
func (r *CertReloader) reload() error { | ||
r.mu.Lock() |
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 lock seems a little excessive, let’s just lock around the actual replacement, once we’ve ensured we really do need to replace and just read lock around reading the “current” cert bytes (as in the if statement doing bytes.Equal)
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.
ack, i've been thinking the same, but reasoned that this locks once per minute, on average. agreed on the finer granularity 👍
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 this was just for exporters I'd be fine with that, but we're guarding Prometheus with this, which gets frequent query requests, so I'd prefer to do this at this point already
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.
that's perfectly reasonable, addressed, ptal
cad3efb
to
ef4527d
Compare
lgtm 👍 |
…rator.yaml-`build_root_image`-from-openshift/release Updating .ci-operator.yaml `build_root_image` from openshift/release
loosen up username and identity name validation
TODOs:
This is an implementation as discussed in #45 (comment), ready for an initial pass.
I am keeping the other PR open to scavenge the code for the prometheus config reloader implementation.