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

gcoap_dns: implement Max-Age-based TTL calculation #18443

Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Aug 11, 2022

Contribution description

This implements the TTL calculation algorithm that is described in Section 4.3.2 of the current draft.

Testing procedure

tests/gcoap_dns should still compile. DNS caching itself is currently not implemented in tests, but the code should speak for itself.

Issues/PRs references

None

@miri64 miri64 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Aug 11, 2022
@miri64 miri64 requested a review from benpicco August 11, 2022 12:41
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Aug 11, 2022
@benpicco benpicco requested a review from chrysn August 11, 2022 12:51
@benpicco
Copy link
Contributor

benpicco commented Aug 11, 2022

Since this is still a draft, I might as well give my opinion on it.
I think this is really error-prone. I suppose the original idea was to just carry the DNS message verbatim over CoAP, but now this requires a modification of it.

I further assume that the MAX_AGE option is to allow for CoAP caching - but the way this is specified now requires to modify the DNS message in a weird way - why calculate the sum and not mandate that MAX_AGE and DNS TTL must always match?

Now you have some weird cross-layer dependency where neither of the two values are valid on their own.

@miri64 miri64 force-pushed the gcoap_dns/enh/implement-max_age-ttl-algorithm branch from 42617a6 to 6f18132 Compare August 11, 2022 13:00
@chrysn
Copy link
Member

chrysn commented Aug 11, 2022 via email

@benpicco
Copy link
Contributor

Thank you for the clarification.
Still I don't see how this doesn't extend the lifetime beyond what the upstream DNS server advertised.

The RECOMMENDED algorithm to assure the requirement for the DoC is to set the Max-Age option of a response to the minimum TTL of a DNS response and to subtract this value from all TTLs of that DNS response.

Sounds like the DNS message should be modified.

@miri64
Copy link
Member Author

miri64 commented Aug 11, 2022

I think this is really error-prone. I suppose the original idea was to just carry the DNS message verbatim over CoAP, but now this requires a modification.

The server will not get around manipulating the message. For DNS subscriptions e.g. a complete different message format is used, which is not intended to be used over other transports than DNS over TLS. However, since CoAP supports Observe, we actually want subscriptions.

Additionally, the performance benefits of the solution implemented here are non-negligible:

See also:

@miri64
Copy link
Member Author

miri64 commented Aug 11, 2022

The server will not get around manipulating the message.

+ in the end we want to use the quite verbose wire format only as a fallback anyway.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's weird, but apparently this is in the standard now.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 19, 2022
@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

but apparently this is in the standard now.

There is no standard yet, just a draft. If that changes, we can always change it here too.

@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

There is no standard yet, just a draft.

i.e. you are also welcome to discuss this either on the core mailing list or in the dedicated GitHub repo

@miri64 miri64 merged commit ff64898 into RIOT-OS:master Sep 19, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants