Provide an API to use specific ip addresses for discovery

[mh-cbon]

Hi !

I need to test upnp connectivity over virtual "any addresses" IE 0.0.0.0.

The current API auto detects and auto selects the IPs based on the output of net.Interface which does not include those.

For consistency this line might be an issue https://github.com/huin/goupnp/blob/8c07ff7bf4e9ac7d7c6f121dc17d5a2f6cc6ab43/httpu/httpu.go#L147

Indeed, when listening upon v4AnyAddr, here at least, it returns AnyAddrV6, ie [::].... I am not looking forward for ipv6 support.

[mh-cbon]

I have sketched some changes here, to see.

• gupnp.localIPv4MCastAddrs is made public
• ssdp.SSDPRawSearch is modified to de duplicate answers using the client local address as additional uniq identifier parameter. It receives an additional parameter client httpu.ClientInterface.
• httpu.HTTPUClient is modified to add a new Field Addr string
• goupnp.DiscoverDevicesCtx and others, are modified to take in a client httpu.ClientInterface
• httpu.HTTPUClient.Do is modified to set the response header key httpu.LocalAddressHeader to the related httpuClient.Addr instead of httpuClient.conn.LocalAddr

Overall, in this form it is breaking the API. At some point i though that it probably should exist a(nother) Client API onto which functions defined within goupnp.go are defined with more flexibility. Than some default client is defined and those package functions redirect their call to that default client.

Edit: looking further,

• Discovery services functions defined within goupnp/service.go should also exist within that aforementioned client.
• the auto generated code would have to make public those definitions that maps a generic client to a dedicated service client. https://github.com/huin/goupnp/blob/main/dcps/internetgateway1/internetgateway1.go#L109
• Alternatively, maybe, all those dedicated clients like WANEthernetLinkConfig1, LANHostConfigManagement1 etc could share a small API like interface { setGenericClient(goupnp.ServiceClient} and he package goupnp provides a MultiServiceClient []GenericClientSetter API to distribute calls to individual underlying setGenericClient instances, whichever their upnp implementations is. (After further reading, i figured we could define func (client *ServiceClient) setGenericClient(n *ServiceClient){*client=*n}, thus, all dedicated upnp implementations would benefit from it. I don't usually write this kind of method, but that should do it, I guess)
• that MultiServiceClient Could also provide some filtering/mapping methods to group services by LocationURL/USN/Device ID/Local Address. That would be useful to work when dealing with multi homed setup.
• So that "client" is something about the discovery process, guessing.

I dont share the sketch because it contains lots of useless dup code that would not exist if i had used a fork directly, i worked around. Although, here is the resulting equivalent invocation of the API, to figure how different that would be

    addrs := []string{"0.0.0.0"}
    addrs = append(addrs, upnp.SomeLocalIPv4MCastAddrs()...)
    log.Println(addrs)
    client, err := upnp.NewHTTPUMultiClient(addrs...)
    if err != nil {
        return err
    }
    rootDevices, err := upnp.DiscoverDevicesCtx(context.Background(), client, ssdp.UPNPRootDevice)
    if err != nil {
        return err
    }

    for _, rtDevice := range rootDevices {
        log.Println(rtDevice.LocalAddr)
        log.Println(rtDevice.Location)
        log.Println(rtDevice.Root.SpecVersion)
        log.Println(rtDevice.Root.Device.String())
    }

[mh-cbon]

I ended up crafting many things.

https://github.com/mh-cbon/goupnp/commit/f058335caa4db07f6550936cf6403718900430bf

there is a new discovery client like

client := goupnp.Discovery{
        Client: goupnp.AutoDiscover("0.0.0.0"),
    }

A new kind to lookup services and provide concrete implementations such as lookup.ServiceTable map[string]func(goupnp.MultiServiceClient) ServiceImplementations

svcTable := lookup.ServiceTable{
        internetgateway2.URN_WANIPConnection_1: func(services goupnp.MultiServiceClient) (out lookup.ServiceImplementations) {
            for _, v := range internetgateway2.NewWANIPConnection1MultiClients(services) {
                out = append(out, v)
            }
            return
        },

Then it is possible to resolve services to implementation with a call like

    searchTargets := svcTable.Targets() // internetgateway2.URN_WANIPConnection_1, internetgateway2.URN_WANIPConnection_2 etc....
    services, _, err := client.NewServiceClientsCtx(context.Background(), searchTargets...)
    if err != nil {
        return err
    }

    // do some filterings...
    // hostwhatever := services.Filter(goupnp.ServiceHost("192..."))

        impls := svcTable.Implement(services)
        // impls is interface []lookup.ServiceImplementation, which in turn is one of ` internetgateway2.NewWANIPConnection1MultiClients` , ` internetgateway2.NewWANIPConnection2MultiClients` etc... 

    for _, impl := range impls {
        fmt.Printf("%T\n", impl)
        // need type assert
        // impl.(portmapping.PortMapper).AddPortMapping()
    }

The Template is modified to instantiate DCP client for a MultiServiceClient, such as

func NewDeviceProtection1MultiClients(mutiClients goupnp.MultiServiceClient) []*DeviceProtection1 {
    services := mutiClients.Filter(goupnp.ServiceType(URN_DeviceProtection_1)) // The service clients are filtered to keep only those that corresponds to the related service definition
    clients := make([]*DeviceProtection1, len(services))
    for i := range services {
        clients[i] = &DeviceProtection1{services[i]}
    }
    return clients
}

The MultiServiceClient is a list filter that works on the flat list of all services for every local addr. It defines, func (m MultiServiceClient) Filter(f ServiceClientFilter, andAlso ...ServiceClientFilter) MultiServiceClient and type ServiceClientFilter func(ServiceClient) bool. With some dedicated helpers such as ServiceType(ts ...string) ServiceClientFilter.

Doing so, i was able to define an appropriate IGW service table

var IGW = lookup.ServiceTable{
    internetgateway1.URN_WANIPConnection_1: func(services goupnp.MultiServiceClient) (out lookup.ServiceImplementations) {
        for _, v := range internetgateway1.NewWANIPConnection1MultiClients(services) {
            out = append(out, v)
        }
        return
    },
    internetgateway2.URN_WANIPConnection_2: func(services goupnp.MultiServiceClient) (out lookup.ServiceImplementations) {
        for _, v := range internetgateway2.NewWANIPConnection2MultiClients(services) {
            out = append(out, v)
        }
        return
    },
}

And its accompanying Lookup function to deal with virtual any addresses.

    var out []PortMapperClient

    searchTargets := IGW.Targets()

    services, _, err := client.NewServiceClientsCtx(context.Background(), searchTargets...)
    if err != nil {
        return out, err
    }

    impls := IGW.ExpandAnyAddr(services)
    for _, impl := range impls {
        switch x := impl.(type) {
        case lookup.AnyAddrServiceImplementation:
            out = append(out, PortMapperClient{
                PortMapper:            x.ServiceImplementation.(PortMapper),
                ServiceImplementation: impl,
            })
        case PortMapper:
            out = append(out, PortMapperClient{
                PortMapper:            x,
                ServiceImplementation: impl,
            })
        default:
            log.Printf("not a port mapper %T", x)
        }
    }

in the end it can fetch all clients, resolve the virtual address thing and get the right local address to use for the call to AddPortMapping when it encounters anyAddr.

    for _, impl := range impls {
        switch x := impl.(type) {
        case lookup.AnyAddrServiceImplementation:
            fmt.Printf("%v should port map using %v\n", x.ServiceImplementation.LocalAddr(), impl.LocalAddr())
        }
        _, ok := impl.(portmapping.PortMapper)
        fmt.Printf("%T PortMapper=%v\n", impl, ok)
        printService(*impl.GetServiceClient())
    }

anyway.. Much.... much changes !

[huin]

Sorry for the slow reply, I tend to only deal with this repo on weekends.

A few questions/comments:

• MultiServiceClient - is this something that benefits from being in this library, or might be better placed in a more specific library or application codebase? That is: is its utility general and not special purpose?

• httpu.HTTPUClient:

  • having a new address field (presumably affecting which address+interface it talks on) - or perhaps receiving the local address as a parameter - this seems possible without breaking compatibility, by either adding a new New* function, or better yet: using functional options. This pattern could be applied elsewhere.

  • httpu.HTTPUClient.Do is modified to set the response header key httpu.LocalAddressHeader to the related httpuClient.Addr instead of httpuClient.conn.LocalAddr

  What if instead the HTTPClient.conn field was created based on the given local address? And then set the header based on that (which should be effectively the same, for specific addresses?)

• ssdp.SSDPRawSearch:

  • taking a client parameter - it already receives an HTTPU client, perhaps we simply need to broaden it to accepting ClientInterface instead of a concrete type?
  • Changing the dedupe behaviour - also possible to change without breaking compatibility with functional options. Perhaps provide a function that maps from result to deduplication key? (with the default being the current behaviour).

  All that said - isn't this achievable by users of the library calling SSDPRawSearch multiple times (optionally concurrently)?

• goupnp.DiscoverDevicesCtx and others, are modified to take in a client httpu.ClientInterface

  Seems reasonable - functional options for compatibility?

[mh-cbon]

hi, no worries for the late reply, thanks for the inputs.

About functional options, it surely could do few things, although, this is still breaking the API signature. I dont think this is right for someone that built up some APIs that relies on interfaces, or functions type def.

About httpuClient, the reason I did that is that if you do net.Listen("0.0.0.0").Addr() you dont exactly get back 0.0.0.0, but its ipv6 equivalent notation. It is anything but obvious. So that seem just right to re use the strict input.

About ssdpRawSearch, bad comment on my part, sorry, indeed it already receives one, it just happens I had to rename the variable because it was conflicting with the newly imported httpu package, and misread that. Although, again, such change, take n an hypothetical ClientInterface would break signature APIs.

About MultiServiceClient, I have introduced that new type in order to manipulate found clients before they are instantiated, filtering and select against the multiple upnp gateways. Otherwise, such API leaks into all generated types (https://github.com/huin/goupnp/blob/9278656124e7cc35d35c35bf6234f7ad8d751ffc/dcps/internetgateway1/internetgateway1.go#L109 for example), or it needed an extra step to unbox the clients from their instance implementation, then rebox into some slice type to perform filtering, then later, re instantiate the specific services client, it did not look right at the time of writing. Although, this is definitely an intentional major shift in regard to the current API design.

Overall, besides the specific implementation details change, like the dedup mechanism, which could be made configurable, one way or another, it is a lot of changes, despite my attempts at being conservative (i tried = ). I feel mixed about it regarding the shift in the design api. Maybe i kept that away and for an eventual V2 this is considered.