RFC: Implement support for connecting to consul via unix socket #217

asteven · 2018-06-29T23:21:15Z

Basically I need python-consul to work with this:

# env | grep CONSUL_HTTP_ADDR
CONSUL_HTTP_ADDR=unix:///var/run/consul/http.sock

Just like the consul cli does.

Unfortunately I had to change a lot of code to make this work :-(
Basically the uri (unix:///var/run/consul/http.sock) has to be passed to all the different implementations like std, aio, tornado as they all handle this type of connection differently.
I implemented the feature for all the backends I'm familiar with std and aio but not tornado and twisted.

I also removed the default environment variables support in base.py for 2 reasons:

The existing implementation interfered with the feature I wanted to implement.
I strongly believe a environment variable should never override an explicitly defined argument.

To fully support configuring a client through environment variables I added a explicit helper function similar to how the docker-py project implements it.

c = consul.Consul.from_env()

cablehead · 2018-07-05T17:26:24Z

It looks like those test failures are just flake8 errors with python3.

This looks great. Do you have time to research adding tornado + twisted support? It'd be good to keep things consistent. If not we could try and recruit someone to help out.

If you can fix the test suite, and add Tornado + Twisted, I'd love to merge and push this to PyPI.

asteven · 2018-07-05T21:46:24Z

FWIW this pull request also fixes #192. IMHO more elegantly then the impl in #193.

asteven · 2018-07-05T21:48:25Z

@cablehead re Twisted I can have a look. I used that like 10 years ago and the implementation will probably be pretty similar to aio. I have never used Tornado in any way, so if you know someone who does a contribution would be great.

asteven · 2018-07-05T22:22:15Z

Status update on this PR. I plan to add some tests that actually start a consul agent bound to a unix socket and a client connecting to that socket. Something simple like connect && call agent.self(). I want to add these tests for all the different client implementations. Initially this would mean that tests for std and aio would succeed, but tests for twisted and tornado would fail until someone has implemented unix socket support.

@cablehead what do you think? Anything else that could or should be tested?

asteven · 2018-07-05T22:26:06Z

@cablehead: The introduction of the consul.Consul.from_env() function changes python-consul's current behaviour. Environment variables no longer take precedence over explicitly given parameters. This is related to #192.

IMHO the current behaviour is broken, so by changing this I actually fix a problem.
Off course this depends on your point of view.
How would you handle this in regards to release and backcompat management?

cablehead · 2018-07-07T20:32:32Z

@asteven I think that test plan is great, and sufficient.

cablehead · 2018-07-07T20:38:53Z

@asteven I think our two choices are:

just go for it, and hope no-one is effected
do one release that keeps the current behavior, but logs a deprecation warning when an environment variable overrides an explicit param. let that sit for a month or two. then push this PR.

I can't think of another option? If you're ok with delaying this PR for a short bit, the latter would be safer for the community...

\cc @matusvalo @abn for input..

matusvalo · 2018-07-10T13:27:22Z

consul/base.py

+        kwargs = {
+            'consistency': consistency,
+            'dc': dc,
+            'addr': oe.get('CONSUL_HTTP_ADDR', None),


In consul docs [1] CONSUL_HTTP_ADDR specified as follows:

CONSUL_HTTP_ADDR=127.0.0.1:8500

But proposed environment variable requires also protocol embedded in CONSUL_HTTP_ADDR:

CONSUL_HTTP_ADDR=http://127.0.0.1:8500

Moreover in [1] CONSUL_HTTP_SSL is present which is not in current PR. I don't think that the form of env. variables of consul is the best but I think that we should copy it as much as possible and mainly when we are reffering in docs to it.

[1] https://www.consul.io/docs/commands/index.html#environment-variables

Nice catch!
Updated PR to handle CONSUL_HTTP_ADDR with and without scheme as well as taking CONSUL_HTTP_SSL as a hint to prefix with either https or just http.

matusvalo · 2018-07-10T13:33:44Z

I prefer option with one additional release with warnings.

I have quickly reviewed the proposed PR and see my comments. Regarding tornado implementation, here should be code snippet for support of unix sockets:

https://gist.github.com/bdarnell/8641880

I will have a look on it when I will have more time.

asteven · 2018-07-10T20:51:08Z

Latest checks have failed for reasons that seem unrelated to my changes.
Can someone please re-run the tests? If I can do this myself please teach me how.

matusvalo · 2018-07-12T08:23:15Z

I have rerun tests and now are green. We have an issue in test suite....

I have created new PR #220 which should fix the issues with unittests. I have already merged this PR to master. @asteven try to update your pull request with newest version of master and we will see if it is better.

asteven · 2018-07-13T20:42:28Z

@matusvalo thanks for #220. Have rebased this PR on current master.

matusvalo · 2018-07-18T12:57:42Z

Thank you @asteven. Now the PR has green unittests. Currently the PR is missing implementation of unix sockets in Twisted and Tornado backends. Could you finish Twisted backend? I hope I will soon have a look in Tornado.

asteven · 2018-07-20T20:10:27Z

Will do asap, ETA ~2 weeks.

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

Replaced most of the boilerplate code that creates a consul client to run tests with fixtures. Prepare for testing with consul clients that can not work with the consul_port fixture, e.g. when using a unix socket. Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

…P_SSL Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

… CONSUL_HTTP_SSL is set Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

asteven mentioned this pull request Jul 5, 2018

Project and maintainer status. #210

Closed

matusvalo reviewed Jul 10, 2018

View reviewed changes

asteven force-pushed the unix-socket branch from f3f3e53 to a7b66ff Compare July 13, 2018 20:41

matusvalo added the not ready label Jul 18, 2018

nzlosh added a commit to nzlosh/python-consul that referenced this pull request May 28, 2019

Merge patch from asteven python-consul#217

52ee9d7

asteven added 12 commits September 11, 2020 21:10

implement support for unix:// address

7c91dee

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

add example for from_env()

fba532a

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

add link to consul env vars

11558b1

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

properly format link

9965d4a

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

remove initial legacy env handling of verify

88b1ced

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

qualified name for exception

e843148

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

python 2.6 support

c1f0bf0

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

fix flake8 tests for python 3.5

59f5fc0

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

fix flake8 E712 test failures for boolean comparison

9ae3b20

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

fix flake8 E265: remove commented/unused code

b8beff2

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

fix flake8 E128 and E501

9287fe7

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

asteven added 3 commits September 11, 2020 21:10

add tests for CONSUL_HTTP_ADDR with and without scheme and CONSUL_HTT…

905b598

…P_SSL Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

ensure CONSUL_HTTP_ADDR starts with a scheme, either http or https if…

84d73ca

… CONSUL_HTTP_SSL is set Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

fix flake8 E225

a23c4d8

Signed-off-by: Steven Armstrong <steven.armstrong@id.ethz.ch>

asteven force-pushed the unix-socket branch from a7b66ff to a23c4d8 Compare September 11, 2020 19:10

cablehead force-pushed the master branch 2 times, most recently from 636f367 to a91daae Compare April 15, 2024 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Implement support for connecting to consul via unix socket #217

RFC: Implement support for connecting to consul via unix socket #217

asteven commented Jun 29, 2018 •

edited

cablehead commented Jul 5, 2018 •

edited

asteven commented Jul 5, 2018

asteven commented Jul 5, 2018

asteven commented Jul 5, 2018

asteven commented Jul 5, 2018 •

edited

cablehead commented Jul 7, 2018

cablehead commented Jul 7, 2018

matusvalo Jul 10, 2018 •

edited

asteven Jul 10, 2018

matusvalo commented Jul 10, 2018 •

edited

asteven commented Jul 10, 2018

matusvalo commented Jul 12, 2018 •

edited

asteven commented Jul 13, 2018

matusvalo commented Jul 18, 2018 •

edited

asteven commented Jul 20, 2018

RFC: Implement support for connecting to consul via unix socket #217

Are you sure you want to change the base?

RFC: Implement support for connecting to consul via unix socket #217

Conversation

asteven commented Jun 29, 2018 • edited

cablehead commented Jul 5, 2018 • edited

asteven commented Jul 5, 2018

asteven commented Jul 5, 2018

asteven commented Jul 5, 2018

asteven commented Jul 5, 2018 • edited

cablehead commented Jul 7, 2018

cablehead commented Jul 7, 2018

matusvalo Jul 10, 2018 • edited

Choose a reason for hiding this comment

asteven Jul 10, 2018

Choose a reason for hiding this comment

matusvalo commented Jul 10, 2018 • edited

asteven commented Jul 10, 2018

matusvalo commented Jul 12, 2018 • edited

asteven commented Jul 13, 2018

matusvalo commented Jul 18, 2018 • edited

asteven commented Jul 20, 2018

asteven commented Jun 29, 2018 •

edited

cablehead commented Jul 5, 2018 •

edited

asteven commented Jul 5, 2018 •

edited

matusvalo Jul 10, 2018 •

edited

matusvalo commented Jul 10, 2018 •

edited

matusvalo commented Jul 12, 2018 •

edited

matusvalo commented Jul 18, 2018 •

edited