-
Notifications
You must be signed in to change notification settings - Fork 616
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
Use custom gRPC dialer to override default proxy dialer #2802
Conversation
cc: @anshulpundir @dperny |
LGTM. Pretty sure the test failure is known so I'm rebuilding. |
Should we have an integration test for this that set an invalid domain ( |
Looks like this is related to moby/moby#36951 and moby/moby#35395 (comment) |
We still waiting on CI here? Current failure doesn't look related. |
Not a maintainer here, but ideally;
I prepared backports, but those will have to be updated if this PR is updated before it's merged I also triggered CI, but it's now failing on
which should be fixed by #2811 after rebasing this PR |
Agreed an integration test would be great :) |
+1 for #2802 (comment) |
ping @dani-docker can you address the review comments? |
Please sign your commits following these rules: $ git clone -b "esc-1003" git@github.com:dani-docker/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357316928
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztah |
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.
LGTM
Discussing on slack; the tricky bit with writing a test for this, is that Golang won't use a proxy for local IP-addresses, so there's some work needed to write a test for this.
The change has been manually tests, and works, so I'm ok with going ahead for now and do a test as follow-up
Codecov Report
@@ Coverage Diff @@
## master #2802 +/- ##
==========================================
+ Coverage 62.12% 62.16% +0.03%
==========================================
Files 139 139
Lines 22294 22297 +3
==========================================
+ Hits 13851 13861 +10
+ Misses 6973 6964 -9
- Partials 1470 1472 +2 |
Signed-off-by: Dani Louca dani.louca@docker.com
- What I did
Followed the same logic as in #2419 to use a custom grpc dialer and prevent the use of default dialer which picks up the proxy env variable when a manager is joining the cluster.
The affected method
dial
is only used when manager is joining the cluster and part ofcheckHealth
which is also called on join.- How to test it
node 1
,docker swarm init
node 2
as managerActual Result:
Expected Result (with this PR):
Node joins successfully
- Description for the changelog
Fix manager node join when HTTP_PROXY, HTTPS_PROXY, or NO_PROXY are used