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

Outbound proxy #5

Open
negbie opened this issue May 7, 2020 · 9 comments
Open

Outbound proxy #5

negbie opened this issue May 7, 2020 · 9 comments
Labels
enhancement pinned Issues don't touched by stalebot

Comments

@negbie
Copy link

negbie commented May 7, 2020

Hi @ghettovoice would you be interested in a outbound proxy setting? Maybe by changing the NewLayer function signature? But this would be a breaking change. If you are interested I can create a pr but first I want to discuss it with you.

@ghettovoice
Copy link
Owner

Hello @negbie ,
what type of changes do you propose? Is it to allow gosip correctly work with otubound proxy?

@negbie
Copy link
Author

negbie commented May 8, 2020

Currently you use msg.Destination() as the socket address here:
https://github.com/ghettovoice/gosip/blob/master/transport/layer.go#L248

This makes it hard to use an outbound address which is different than the domain address.

Maybe

func NewLayer(
ip net.IP,
dnsResolver *net.Resolver,
msgMapper sip.MessageMapper,
logger log.Logger,
)

can be changed to take a new proxy argument?

@negbie
Copy link
Author

negbie commented May 8, 2020

With that the send method can be extended with some logic to check if proxy != "" and so on...

@ghettovoice
Copy link
Owner

You are right, with current implementation it's hard to override.
I'm just thinking maybe replace first arg ip net.IP with something like cfg LayerConfig where LayerConfig:

type LayerConfig {
  HostIP net.IP
  ProxyAddr net.Addr
 ...
}

@negbie
Copy link
Author

negbie commented May 11, 2020

Yeah that would be a way. Another way would be to use functional options https://sagikazarmark.hu/blog/functional-options-on-steroids/

Add ProxyAddr net.Addr to the layer struct and add a Proxy function which will return func(*layer). Now you can change the layer constructor to something like

func NewLayer(
ip net.IP,
dnsResolver *net.Resolver,
msgMapper sip.MessageMapper,
logger log.Logger,
opts ...func(*layer),
)

This has the benefit that it wouldn't be a breaking change.

@ghettovoice
Copy link
Owner

Functional options looks very promising, especially non breaking api.
Let stay with this solution: leave ip net.IP as now because this is required option for transport layer, and implement proxy (any other future options) as functional options.
Do you make pull request?

@negbie
Copy link
Author

negbie commented May 12, 2020

@ghettovoice will do it asap.

@ghettovoice ghettovoice added the pinned Issues don't touched by stalebot label Jun 11, 2020
@sreeram-narayanan
Copy link

Hello @negbie, I'm checking in to see if you had a chance to work on this feature.

In case you were not able to, could you please share any progress you made on this? I will be happy to take this forward.

@yunginnanet
Copy link
Contributor

+1, interested in this
may have to jump in and try to help with this implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pinned Issues don't touched by stalebot
Projects
None yet
Development

No branches or pull requests

4 participants