OT_ERROR_NOT_FOUND when canonical names received from otDnsClientResolveIp4Address() about ot-nrf528xx HOT 7 CLOSED

@abtink I have confirmed that your PR #9339 solves my issue. Thank you again for the quick assistance!

from ot-nrf528xx.

@tom-brown-kr I have a guess/theory of why it is not working.

The CNAME RR are used to provide a different name for a host name.
In the response there seems to two levels CNAME name changes happening:

• The name you queried is mapped to -support.private-... and then the -support.private... name is again mapped to .azuremicro... name and A record is provided for this name

In the Dns::Clinet implementation we do check for one level of indirection (we check if there is CNAME and we use the new name instead in Client::Response::CheckForHostNameAlias) but not if there are multiple CNAME records and multiple name changes. We can add this in the code to support this.

from ot-nrf528xx.

@abtink Thank you for the quick investigation - support for multiple CNAMEs would be greatly appreciated. I will be sure to read through dns_client.cpp in more detail for my own education.

Do you know approximately how long it might take to commit an update?

from ot-nrf528xx.

@abtink As it turns out, CheckForHostNameAlias() is written in a way that is very easy to iterate! The // <- comments below indicate my changes and additions.

Error Client::Response::CheckForHostNameAlias(Section aSection, Name &aHostName) const
{
    // If the response includes a CNAME record mapping the query host
    // name to a canonical name, we update `aHostName` to the new alias
    // name. Otherwise `aHostName` remains as before.

    Error       findRecError, error; // <- added findRecError
    uint16_t    offset;
    uint16_t    numRecords;
    CnameRecord cnameRecord;
    int i=0;
    const int i_maxcounter = 20; // <- added i_maxcounter 

    VerifyOrExit(mMessage != nullptr, error = kErrorNotFound);

    do { // <- added do-while loop to iterate through one or more CNAME record(s). i_maxcounter prevents infinite while loop
        SelectSection(aSection, offset, numRecords);
        error = ResourceRecord::FindRecord(*mMessage, offset, numRecords, /* aIndex */ 0, aHostName, cnameRecord);

        findRecError = error; // <- use findRecError to avoid interfering with current returning of "error"

        switch (error)
        {
        case kErrorNone:
            // A CNAME record was found. `offset` now points to after the
            // last read byte within the `mMessage` into the `cnameRecord`
            // (which is the start of the new canonical name).
            aHostName.SetFromMessage(*mMessage, offset);
            error = Name::ParseName(*mMessage, offset);
            break;

        case kErrorNotFound:
            error = kErrorNone;
            break;

        default:
            break;
        }

        i+=1;
    } while(findRecError==kErrorNone && i<=i_maxcounter);   
exit:
    return error;
}

If I modify the comments and double-check your guidelines/conventions, could I submit a pull request for this? Is there anything you would recommend changing?

from ot-nrf528xx.

Just saw this. I am actually adding support for this in the code now. Will submit a PR soon.

from ot-nrf528xx.

Submitted the PR below which should help address this:

• openthread/openthread#9339

@tom-brown-kr it would be great if you can cherry-pick this and test it with your setup, so we can validate it :)

from ot-nrf528xx.

@tom-brown-kr, awesome. Thanks helping validate the PR quickly. :)

from ot-nrf528xx.