-
Notifications
You must be signed in to change notification settings - Fork 651
Use custom gRPC dialer to override default proxy dialer #2802
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
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" [email protected]: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 -fAmending updates the existing PR. You DO NOT need to open a new one. |
Signed-off-by: Dani Louca <[email protected]>
|
@thaJeztah |
thaJeztah
left a comment
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 [email protected]
- 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
dialis only used when manager is joining the cluster and part ofcheckHealthwhich is also called on join.- How to test it
node 1,docker swarm initnode 2as 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