Skip to content
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

Add support for multiple address binds to dns and http servers #2646

Closed
wants to merge 1 commit into from

Conversation

rhyas
Copy link

@rhyas rhyas commented Jan 10, 2017

First pass at implementing DNS/HTTP servers that allow multiple binds. This works on every manual test, and I put in some tests for the HTTP and DNS listeners. On my mac, the test harness was doing weird things with the dns bind, and I couldn't figure out if it was something on my mac and it's vpn/firewall stuff, or something else. All tests pass in travis-ci though, so I'm hoping it's just my dev environment. This would possibly resolve, or at least help, both #2217 and #473.

@rhyas rhyas force-pushed the multiple-http-dns-bind branch 2 times, most recently from 5a425a7 to 8f5711c Compare February 2, 2017 03:55
@rhyas rhyas force-pushed the multiple-http-dns-bind branch from c8345b9 to c1477d5 Compare February 2, 2017 04:11
@rhyas
Copy link
Author

rhyas commented Feb 2, 2017

So I just updated this PR to fix a merge conflict, but the test failed. I get successes on the local testing I'm running. When I run it in Travis, I get a different failure. They all seem to be related to tests where the TestServer seems to get confused on it's port. (i.e. the failure is a failure to connect to a port that's not listed in any of the test server startup logs) It seemed odd to me, so I started looking at other failed tests, and see that this isn't all that uncommon, as several other builds/branches/PR's have failed in the same way.

Is this a known issue? or just a random thing that's ignored if a build eventually passes?? or something else entirely?

Near as I can tell, this PR is good. I've actually used it in ECS to bind loopback and the Docker0 interface to restrict agent access to docker containers. But I'd love to hear some feedback on it and if there's anything else I can/should address or think of.

@rhyas
Copy link
Author

rhyas commented Mar 3, 2017

Just curious, but is it worth fixing this conflict again for this patch? It doesn't seem like there's a lot of interest in it...

@slackpad
Copy link
Contributor

Hi @rhyas thanks for the PR and sorry it took so long to get feedback to you. @magiconair is attempting a deeper refactor to accomplish this under #3037 (we are also working on test stability). I'll close this against #3037 for now.

@slackpad slackpad closed this May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants