NSAPI DNS query IP version check for non LWIP stacks.

[tymoteuszblochmobica]

Description

NetworkStack::gethostbyname(name, address, version) should return the same IP version as network interface.
There was solution #10794 for case when LWIP is in use.
This is fix for AT modems where DNS_API does the selection of server address.

Pull request type

[x ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AriParkkila
@kjbracey-arm
@SeppoTakalo
@kivaisan
@mikaleppanen

Release Notes

[ciarmcom]

@tymoteuszblochmobica, thank you for your changes.
@kivaisan @mikaleppanen @kjbracey-arm @AriParkkila @SeppoTakalo @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

[AriParkkila]

Accept also NSAPI_UNSPEC as cellular stack does not always get IP address from a modem.

If gethostbyname(,,version) was given it should probably be accepted in any case.

If we don't have IPv4 then should not accept A records, and if we don't have IPv6 then should not accept AAAA records.

If we have both IPv4 and IPv6 then get_ip_address prefers IPv6, but DNS should also query IPv4 servers as that might be the network's primary DNS server.

[0xc0170]

Please also fix astyle failures

[tymoteuszblochmobica]

Function gethostbyname remains unchanged. This fix refers only nsapi_dns_query_multiple ().
It uses the loop to find proper DNS server.

First nsapi_dns_get_server_addr(stack, &index, &total_attempts, &send_success, &dns_addr, interface_name)
tries to get it from stack calling:
stack->get_dns_server(*index, dns_addr, interface_name);

If it fails then dns_addr->set_addr(dns_servers[*index - DNS_STACK_SERVERS_NUM]);

Inside nsapi_dns.cpp there is secondary storage for DNS servers

static nsapi_addr_t dns_servers[DNS_SERVERS_SIZE] = {
{NSAPI_IPv4, {8, 8, 8, 8}}, // Google
{NSAPI_IPv6, {0x20,0x01, 0x48,0x60, 0x48,0x60, 0,0, // Google
0,0, 0,0, 0,0, 0x88,0x88}},
{NSAPI_IPv4, {209, 244, 0, 3}}, // Level 3
{NSAPI_IPv4, {84, 200, 69, 80}}, // DNS.WATCH
{NSAPI_IPv6, {0x20,0x01, 0x16,0x08, 0,0x10, 0,0x25, // DNS.WATCH
0,0, 0,0, 0x1c,0x04, 0xb1,0x2f}},
};

and it is used in this approach.

Previously it was a reason of improper behavoiur due to dns_addr->set_addr sets first adress from nsapi_dns dns_servers table (8.8.8.8) even if required version was IP6.
Therefore i added version checking

if(dns_addr.get_ip_version() != version){
retries = MBED_CONF_NSAPI_DNS_RETRIES;
index++;
continue;
}

to find next adresses from stack->get_dns_server or nsapi_dns dns_servers table.

I assume stack->get_dns_server is implemented only for LWIP so in case of cellular stack nsapi_dns dns_servers will be used.

Im confused with :
" Accept also NSAPI_UNSPEC as cellular stack does not always get IP address from a modem."

If nsapi_dns_query_multiple (,version==NSAPI_UNSPEC) and stack->get_dns_server is not implemented for cellular, how can we get proper DNS IP version?

[AriParkkila]

@tymoteuszblochmobica I think that version can be also NSAPI_UNSPEC?

[tymoteuszblochmobica]

Do you mean
if(version != NSAPI_UNSPEC && (dns_addr.get_ip_version() != version)){

[AriParkkila]

@tymoteuszblochmobica If dual-stack is not required then A/AAAA version could be devised from the versions of the DNS servers' addresses?

[tymoteuszblochmobica]

Current implementation:
// send the question
int len = dns_append_question(packet, 1, host, version);
err = socket.sendto(dns_addr, packet, len);

dns_append_question selects which record we want
// fill out question footer
if (version != NSAPI_IPv6) {
dns_append_word(p, RR_A); // qtype = ipv4
} else {
dns_append_word(p, RR_AAAA); // qtype = ipv6
}
dns_append_word(p, CLASS_IN);

[AriParkkila]

@tymoteuszblochmobica Probably dns_addr.get_ip_version() could be given to dns_append_question if version is NSAPI_UNSPEC, assuming we do not request both IPv4 and IPv6?

[0xc0170]

@tymoteuszblochmobica @AriParkkila How can we progress this PR? Is it still relevant?

[michalpasztamobica]

@AriParkkila . I talked to @tymoteuszblochmobica and it seems that the version should always be set before reaching the nsapi_dns_query() or nsapi_dns_query_async(). If it is unspecified then gethostbyname() will determine in based on stack's settings.

Do you agree?

[AriParkkila]

the version should always be set before reaching the nsapi_dns_query()

It doesn't seem so with cellular modems/networks as we don't always get IP address at all.

[AriParkkila]

Probably this should also have version != NSAPI_UNSPEC &&

[AriParkkila]

It could be better to give dns_addr.get_ip_version() in dns_append_question()?

[michalpasztamobica]

@AriParkkila , please help me make sure I understand the background correctly... :)

I assume that our entry point to the DNS resolution is gethostbyname function. and that cellular modems, which inherit from NetworkStack have two options:

1. rely on the gethostbyname provided by NetworkStack (implemented in nsapi_dns.cpp)
2. implement their own gethostbyname, like RiotMicro does for example.

This PR will only affect the cellular modems which rely on our implementation of gethostbyname or it's async version. They both now try to guess what kind of IP connectivity is in place (4 or 6) based on the stack's IP address.

Now, your point is: what happens if DNS resolution takes place when the device doesn't have an IP address - do I understand correctly?
I wonder how would a DNS resolution work in this situation? Can the response be sent back to the device in any way?

[AriParkkila]

DNS resolution takes place when the device doesn't have an IP address - do I understand correctly

Yes, the modem has IP address but it does not tell it to the device via AT commands. That is, we don't know the IP address type on the device, but since the modem has IP address DNS query is working fine.

[michalpasztamobica]

@AriParkkila , ok, we see your point now, thanks a lot for being patient with us 😃 But does this device actually use our nsapi_dns implementation for DNS resolution? Which device is it exactly? I wonder if it makes a difference if we provide it with IPv4 or IPv6 - we could just as well guess or assign a default value, as to my understanding there is no relation between DNS server's IP address and the type of addresses it will resolve?

[0xc0170]

Reviewers please review

[michalpasztamobica]

I guess the most recent push from Tymoteusz includes @AriParkkila 's request: in case the ip version is unspecified the request will be send based upon whatever was specified in the dns_addr.
I think it's best to wait for Ari's approval, though.

[0xc0170]

CI started

[0xc0170]

I also restarted travis, internal fault

[mbed-ci]

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts