nodejs
dns: add TLSA record query support
This PR adds resolveTlsa so that the resolver can query TLSA records.
c-ares added the parser in https://github.com/c-ares/c-ares/pull/600 and @bradh352 (thanks!) provided some code to get started with: https://github.com/nodejs/node/issues/39569#issuecomment-1812437718
Related to #39569
P.S. I'm new to both node core as well as C++ so the code may be unideal, am open to any changes to be made.
Also, I'm not sure about the YAML markup in docs, what should the "Added in" say?
And is this considered a Notable Change?
I've updated to fix linting and formatting, but not sure about the LeakSanitizer: detected memory leaks one.
Tried reproducing it locally with tools/test.py but could not, all tests always run without issues. And it seems to be complaining about dnsrec but I think it's handled properly? Would love some advice here.
might be useful to define the known values for the TLSA records like we do in c-ares with ares_tlsa_match_t, ares_tlsa_selector_t, and ares_tlsa_usage_t
Fixed the mem leak and confirmed the asan test passes (on a different branch, not in this PR: https://github.com/rithvikvibhu/node/actions/runs/9137751681/job/25128086198)
I think it's ready for review (assuming all checks pass).
@bradh352 I searched but couldn't find constants defined in dns for any other type, they are all stored and returned as regular objects. For ex, ParseMxReply: https://github.com/nodejs/node/blob/559212e64c21af74e1aedf9fe43a3bf844111bd8/src/cares_wrap.cc#L301-L311
@lpinca can you approve the workflow again? I believe all check errors are fixed now. Also, any idea if someone or a group must be tagged for reviewing? (I don't mind if it takes time, just making sure I understand the process)
The only failing test is benchmark/test-benchmark-crypto, which this PR does not touch. There is an issue with the same error and a PR to mark it as flaky:
- https://github.com/nodejs/node/issues/52690
- https://github.com/nodejs/node/pull/52955
I think we just wait till #52955 is merged, then I rebase?
This needs someone to review it. I already pinged @nodejs/dns 2 weeks ago, so now I'm going to try current collaborators who show up a lot in git blame src/cares_wrap.cc: @jasnell @addaleax @XadillaX @bnoordhuis @ShogunPanda @joyeecheung
as I am really looking forward to these changes I wanted to kindly ask on what is missing until this PR can be finally included in a release.
Thank you!
as I am really looking forward to these changes I wanted to kindly ask on what is missing until this PR can be finally included in a release.
As of right now, there's a merge conflict in src/env_properties.h that needs to be resolved. Once that's done, tests will need to be re-run.
Codecov Report
Attention: Patch coverage is 25.75758% with 49 lines in your changes missing coverage. Please review.
Project coverage is 89.09%. Comparing base (
79f96b6) to head (660774a). Report is 32 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/cares_wrap.cc | 23.43% | 46 Missing and 3 partials :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #52983 +/- ##
==========================================
- Coverage 89.10% 89.09% -0.02%
==========================================
Files 665 665
Lines 193203 193269 +66
Branches 37220 37228 +8
==========================================
+ Hits 172158 172193 +35
- Misses 13771 13814 +43
+ Partials 7274 7262 -12
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/dns/utils.js | 99.44% <100.00%> (+<0.01%) |
:arrow_up: |
| src/cares_wrap.h | 79.89% <ø> (ø) |
|
| src/cares_wrap.cc | 63.05% <23.43%> (-1.99%) |
:arrow_down: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Lint cpp issues are probably fixable via rebase against current main, so I've done that and force-pushed.
Thanks @Trott! I had the formatted commits ready to force push but was waiting to see if any other checks fail :smile:
The internet test failing (internet/test-net-autoselectfamily-timeout-close) doesn't seem related to any change this PR makes.
I'll push one last commit for the open review comment once it gets a reply.
@bradh352 @Trott review comments addressed and all checks are passing. Lemme know if anything else is required.
@nodejs/collaborators This is ready to land but needs one more approval since there was a change since the last approval. I could rubber-stamp it, but a careful review by someone knowledgable about DNS and/or C++ would be better. I've tried pinging the relevant smaller groups (@nodejs/dns, @nodejs/cpp-reviewers) and had a little success getting reviews, but I'd prefer more eyes on something like this, so pinging more widely now.
(If no one comes by in the next 48 hours or so with a review/approval, I'll rubber-stamp it and land it.)
Commit Queue failed
- Loading data for nodejs/node/pull/52983 ✔ Done loading data for nodejs/node/pull/52983 ----------------------------------- PR info ------------------------------------ Title dns: add TLSA record query support (#52983) Author Rithvik Vibhu <[email protected]> (@rithvikvibhu, first-time contributor) Branch rithvikvibhu:dns-tlsa -> nodejs:main Labels c++, dns, semver-minor, lib / src, needs-ci, commit-queue-squash Commits 2 - dns: add TLSA record query and parsing - Update src/cares_wrap.cc Committers 2 - Rithvik Vibhu <[email protected]> - GitHub <[email protected]> PR-URL: https://github.com/nodejs/node/pull/52983 Refs: https://github.com/nodejs/node/issues/39569 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52983 Refs: https://github.com/nodejs/node/issues/39569 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 14 May 2024 18:28:18 GMT ✔ Approvals: 3 ✔ - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/52983#pullrequestreview-2122736173 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52983#pullrequestreview-2619429908 ✔ - Rich Trott (@Trott): https://github.com/nodejs/node/pull/52983#pullrequestreview-2624522158 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-02-16T05:12:15Z: https://ci.nodejs.org/job/node-test-pull-request/65245/ - Querying data for job/node-test-pull-request/65245/ ✔ Last Jenkins CI successful ⚠ PR author is a new contributor: @rithvikvibhu([email protected]) ⚠ - commit 660774a9846c is authored by [email protected] -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/13397499730
Aw, I did the force push after pushing to main, so the PR is marked as Closed rather than Merged. Sorry about that!
@rithvikvibhu Thanks for your patience and the contribution!
If anyone wants to improve the docs (per @Ethan-Arrowood's comment), please open a new PR to do it! Thanks in advance!
Apologies for all the exclamation points!!!!!!!!
Haha np just glad it's finally done and merged!
Thanks to all the reviewers, especially @Trott and @bradh352 for all the help and follow ups
