[haproxy] unix socket stats urls #3005
Conversation
sj26
force-pushed
the
haproxy-unix-stats-urls
branch
from
e0b2874 to
bed8d80
Compare
sj26
force-pushed
the
haproxy-unix-stats-urls
branch
from
bed8d80 to
0a08eac
Compare
|
Hi @sj26 and thanks for your code, we now that running the tests locally is not as easy as it should be, sorry for that! Aside from the details that are making the CI fail, I'm a little bit concerned about the overall approach - if I understood correctly, by I agree moving to unix socket would improve security but we cannot break the compatibility with existing configurations. I'd make the check compatible with both approaches, defaulting to the old behaviour and making available the new one through the config file. What do you think? |
|
The monkey patch shouldn't prevent requests from functioning as normal, or change any existing integrations, it just adds an extra adapter for the "http+unix" scheme. :-) |
|
Nice, that was my impression looking at the CI but I discovered the existence of |
| pid = spawn("#{haproxy_rootdir}/haproxy", '-d', '-f', | ||
| "#{ENV['TRAVIS_BUILD_DIR']}/ci/resources/haproxy/#{name}.cfg", | ||
| "#{ENV["VOLATILE_DIR"]}/#{name}.cfg", |
There was a problem hiding this comment.
change these to single quotes to make rubocop stop complaining
|
|
||
| r = requests.get(url, auth=auth, headers=headers(self.agentConfig), verify=verify, timeout=self.default_integration_http_timeout) | ||
| r.raise_for_status() | ||
| sock.close() |
There was a problem hiding this comment.
Most of this was borrowed from https://github.com/neurogeek/haproxyctl/blob/master/haproxy/conn.py#L51-L83
|
@masci sorry for the delays. Turns out I was completely wrong and the admin/stats sockets don't run HTTP at all but uses their own line-based protocol. I've updated the implementation to accept I also managed to get the tests working (kind of) on my local environment, so can actually verify it works. Mind taking another look? |
| import time | ||
| try: | ||
| import urlparse | ||
| except LoadError: |
There was a problem hiding this comment.
We don't actually need this since the agent ships with python 2.7 (the exception name should be ImportError anyways, tests are failing on this).
simply import urlparse is ok.
There was a problem hiding this comment.
(Too much Ruby on the brain.)
| sock.send("show stat\r\n") | ||
|
|
||
| response = "" | ||
| output = sock.recv(8192) |
There was a problem hiding this comment.
Can you define this number as a constant on the top of the module? Like:
BUFSIZE=8192
| @@ -5,6 +5,10 @@ instances: | |||
| # username: username | |||
| # password: password | |||
| # | |||
| # or, with a unix stats or admin socket: | |||
| # | |||
| # url: unix:///var/run/haproxy.sock | |||
There was a problem hiding this comment.
Nitpick: could you add a leading - to the example so it's clear that should be another instance and not an attribute of the first one?
instances:
- url: http://localhost/admin?stats
# username: username
# password: password
# or, with a unix stats or admin socket:
# - url: unix:///var/run/haproxy.sock
There was a problem hiding this comment.
Hmm, that looks odd to me, but updated.
There was a problem hiding this comment.
Yeah at first I thought it'd be clearer but doesn't seem so...
Feel free to adjust it as you think it's better from the user perspective, or leave it as it is and we'll fix that later.
|
Thanks! |
What does this PR do?
Support unix sockets for haproxy stats collection.
Motivation
Running a separate stats socket with tight permissions instead of over a tcp port provides greater security.
Testing Guidelines
Sorry, I have no idea how to get the tests actually working in a local environment. It has been a world of pain. I'm hoping travis does a better job at running them and providing some useful feedback.
I'm on macOS 10.12, using homebrew, ruby 2.3.1, python 2.7.12.
rakeaborts on rubocop errors. So doesrake ci:run.rake ci:run[haproxy]tries to compile haproxy which can't find openssl, even if I supply correct CFLAGS and LDFLAGS to my homebrew'd openssl install as env. Subsequentrake ci:run[haproxy]runs don't try to recompile haproxy, they fail withErrno::ENOENT No such file or directory - /Users/sj26/Projects/dd-agent/embedded/haproxy_1.5.11/haproxy. And cleanup fails because it can't find an haproxy pid.I haven't attempted to figure out mocks yet.
Additional Notes
I realise this introduces a new dependency which requires updating your omnibus, but I'd like to get the code working first before jumping on that dragon.