search

ghettovoice / gosip

SIP in Go
BSD 2-Clause "Simplified" License
425 stars 91 forks source link

Outbound proxy #5

Open negbie opened 4 years ago

negbie commented 4 years ago

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 commented 4 years ago

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

negbie commented 4 years ago

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 commented 4 years ago

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

ghettovoice commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@ghettovoice will do it asap.

sreeram-narayanan commented 2 years ago

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 commented 1 year ago

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