Please add support for pushgateways protected with basic authentication #60
Comments
|
Just noticed that the URL is hardcoded to begin with "http://", so ideally HTTPS should be supported as well. |
|
I'll need to think on this a little as there's lots of different options a user may want to specify in this area. How would a function that returned the content and path work, that you could then adjust and send on work for you? |
|
Yes, I think this solution would be quite OK for me. |
|
We're hitting this as well. I setup a HTTPS nginx instance to receive metrics and forward them to the pushgateway. Specifying https doesn't work since the http is hardcoded. A redirect doesn't work (and according to the RFC shouldn't work for PUT requests anyway): Basically for us it would be handy to be able to specify the full URL including HTTPS. |
|
Is this still desired, and would somebody who needs the functionality be able/willing to contribute it? |
|
Rough draft for comment. Tested on python 2.7.9, 3.4.2, 3.5.2. n.b. older pythons won't enforce https certificate checking (PEP 476 etc.). Also re previous comments, https urls already worked (but https://user:pwd@host:port form didn't, and still don't with this patch applied either). python_prometheus_client_http_basicauth_support.patch.txt Minimally tested with: push_to_gateway('https://pushgw.example.com', job='batchA', registry=registry, username='foo', password='bar') ... with haproxy doing basic auth and https reverse proxying on the other end. Cheers, Tim. |
|
That's cover us for basic auth, but I expect someone will want bearer token and client cert at some point. |
|
True @brian-brazil , but on the other hand, even though other features are needed in the future, this patch does seem to fix the specific issue reported in this, well, issue. Using basic auth with https for internal services does seem like a reasonable solution for internal systems and a real blocker for adoption according to the original reporter. Just my €.02 |
|
This patch sets a pattern where we'll end up with 30-40 arguments to this function as all the various client auth options are added. I appreciate that users need this, but that's not a sane API direction to set. |
|
Just trying to help a bit with forward momentum, even with an imperfect API design as a stepping stone. I'll leave the preferred design to you. |
|
I'll see if I can knock up something so allow the option of passing in a handler... |
|
OK, I've got something which seems to work, and allow both https client certificates and basic auth to work by passing in a handler to push_to_gateway() etc.. I don't write much Python, so excuse me if I've done something daft... python3 POC: import urllib
import http
from prometheus_client import CollectorRegistry, Gauge, push_to_gateway
registry = CollectorRegistry()
g = Gauge('some_metric_too', 'Last time a batch job successfully finished', registry=registry)
g.set_to_current_time()
class HTTPSClientAuthHandler(urllib.request.HTTPSHandler):
def __init__(self, key_file, cert_file):
urllib.request.HTTPSHandler.__init__(self)
self.key_file = key_file
self.cert_file= cert_file
def https_open(self, req):
return self.do_open(self.getConnection, req)
def getConnection(self, host, timeout):
return http.client.HTTPSConnection(host, key_file=self.key_file,
cert_file=self.cert_file)
url='https://pushgw.example.org'
p = urllib.request.HTTPPasswordMgrWithDefaultRealm()
p.add_password(realm=None,
uri=url,
user='admin',
passwd='shh-dont-tell-anyone')
handler = urllib.request.HTTPBasicAuthHandler(p)
#handler = HTTPSClientAuthHandler(key_file=r'x509-client-key.pem', cert_file=r'x509-client.pem')
push_to_gateway(url, job='batchA', registry=registry, handler=handler)Whilst I realise you don't want to turn the API into a swiss army knife, using basic auth seems like a pretty common use case (I'd guess at least an order of magnitude more common than client certs, bearer tokens etc.), I think I'd argue for adding username, password, anyway, as well as a handler argument, but I don't mind too much either way. Which additional argument(s) would you like me to implement? . username, password and would you prefer it as a diff or a PR? Cheers, Tim. |
|
Let's not tie this to I'm thinking more a handler that takes the content, content-type, path and host and goes from there. That should be enough flexibility. |
|
How about adding support for passing in a function to push_to_getway (and related functions) which takes the following args: url, request_method, timeout, content_type, content and should raise an IOError on failure? |
|
That's along the lines I was thinking. Probably want headers rather than just content_type, there might be more in future. |
|
Just got back on to this. Quick Q: The "the url for your push gateway. Either of the form 'http://pushgateway.local', or 'pushgateway.local'. Scheme defaults to 'http' if none is provided" If a handler is also passed to Don't mind either way, but in either case, it seems like there is some tension between this behaviour, and giving the option to pass your own handler in. Thoughts? |
|
Offhand I'd always add in the default scheme, as depending on which libraries the user is using there's a good chance they'd have to do it themselves anyway. That still works with other schemes. |
|
This should be resolved now in latest release. |
Currently there doesn't seem to be any way to provide basic authentication credentials for talking to pushgateways (urllib2 doesn't allow putting them in the URL). Would be great if this could be added.
The text was updated successfully, but these errors were encountered: