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

client: add a WithNoProxy dialoption #3411

Merged
merged 3 commits into from Mar 26, 2020

Conversation

pdbogen
Copy link
Contributor

@pdbogen pdbogen commented Feb 28, 2020

Hullo! I'm working with a system that talks to some things via grpc and some things not via grpc. (actually, talking over a UNIX domain socket via grpc 😬).

The non-grpc things are mostly HTTP requests that we do need to proxy. The grpc things, as noted, as via local sockets, which can't and shouldn't be proxied.

Providing our own dialer is an option, but this seems a little cleaner and like other folks might also find it useful.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 28, 2020

CLA Check
The committers are authorized under a signed CLA.

@easwars easwars requested a review from menghanl March 5, 2020 21:32
@menghanl menghanl changed the title add a WithoutProxy dialoption client: add a WithoutProxy dialoption Mar 5, 2020
dialoptions.go Outdated Show resolved Hide resolved
dialoptions.go Outdated Show resolved Hide resolved
dialoptions.go Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Mar 19, 2020

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@stale stale bot added the stale label Mar 19, 2020
@easwars
Copy link
Contributor

easwars commented Mar 26, 2020

ping @pdbogen

@stale stale bot removed the stale label Mar 26, 2020
@pdbogen
Copy link
Contributor Author

pdbogen commented Mar 26, 2020

Thanks for the ping, and for your patience! The original requests for change came in while I was on vacation, so I'd forgotten about them.

@pdbogen pdbogen changed the title client: add a WithoutProxy dialoption client: add a WithNoProxy dialoption Mar 26, 2020
@pdbogen pdbogen requested a review from menghanl March 26, 2020 21:15
@menghanl menghanl added Type: Feature New features or improvements in behavior and removed Status: Requires Reporter Clarification labels Mar 26, 2020
@menghanl menghanl added this to the 1.29 Release milestone Mar 26, 2020
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. LGTM!

@thaJeztah
Copy link
Contributor

#3411 (comment)

Can you make this withProxy, and set it to true in defaultDialOptions()? So we don't have "if not without proxy".

@menghanl Isn't that a breaking change? I saw this feature in the changelog (knowing that we did something similar in moby/swarmkit#2802), but if consumers of this package setup their own grpc.ClientConn (not using the defaultDialOptions()), they'll end up with a change in behavior?

I think withoutProxy was the better choice, so that the default for the bool (false) is also the default to use.

@thaJeztah
Copy link
Contributor

Ehm, ignore my comment, had my wires crossed; blame it on "commenting before enough coffee"

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants