fix(dns): preserve header shape and ensure Host

[FelixVaughan]

This relates to...

#4444

Rationale

Preserve header shape and correct Host after DNS resolution to avoid routing issues/header corruption

Changes

Added addHostHeader to dns interceptor and parameterized tests for different header types.

Bug Fixes

Fixed DNS interceptor turning [string, string][]/iterables into numeric-key headers

Status

• [x] I have read and agreed to the Developer's Certificate of Origin
• [x] Tested
• [ ] Benchmarked (optional)
• [ ] Documented
• [x] Review ready
• [ ] In review
• [ ] Merge ready

[codecov-commenter]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 93.56%. Comparing base (2a72563) to head (daf988c). :warning: Report is 671 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4498      +/-   ##
==========================================
- Coverage   94.17%   93.56%   -0.61%     
==========================================
  Files          90      103      +13     
  Lines       24559    32366    +7807     
==========================================
+ Hits        23128    30283    +7155     
- Misses       1431     2083     +652     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[metcoder95]

-1 on the algorithm or the feature itself?

[FelixVaughan]

@Uzlopak the normalization function was moved out of lib/util/cache.js to lib/core/util.js, with some minor refactors https://github.com/nodejs/undici/blob/95d835cd9db685e4f7baaea8e10aee8ce77e7989/lib/util/cache.js#L36

Many suggestions are referring to things that were present in the initial implementation which were left unchanged to avoid introducing reversions.

[metcoder95]

cc @Uzlopak

[Uzlopak]

I know it is maybe a little bit frustrating situation right now. I am still thinking if this is the right thing to do. Also I want to evaluate it further and see what the rest of the code does after passing further down the line. Didnt have the muse to do it till now.

It creates alot of new intermediary objects and makes alot of toLowerCase() calls. And that just for the sake of normalizing the header.

What if it is an array? why not push key and value? Dont we have a logic, which overwrites the value if a key is already set? So why do we need to transform it to an object in that moment?

What if it is an Object? Afaik an Object.keys does not return the keys alphabetically but in the order of being assigned to the object. Should we rely on this behavior?

Should we not use wellknown header logic (see /lib/core/constants.js) to avoid potential toLowerCase() calls. Maybe it was already a bad design, when cache did not use that weelknownheader logic. Doesnt mean that we should ignore this perf regression

Or should we not just actually create a Header Instance?

[FelixVaughan]

All great questions, some of which I don’t have immediate answers for either, but in general I think we should be utilizing the lowercase wellKnownHeader functionality already in place wherever possible.

For the arrays and objects, we normalize to avoid the case where the passed in header is neither an object in the traditional key/value sense or an array, but an iterable like a set; where a traditional push would lead to undefined behavior. We might be able to shortcircuit this by a straight append in the case of an array as you pointed out but objects and iterators are slightly more complex.

Also, though I think it makes sense to potentially tackle it in this pr, due to the pre-existing nature of the problem, should this perf concern be handled as a separate issue?

[Uzlopak]

We should not transplant one bad implementation to other places and widely use it.

Right now only one person or few people complained regarding passing arrays into the interceptor.

Who is gonna fix that bad implementation later?

"A permanent solution is a temporary solutation that works." or other variation: "Temporary solutions often become permanent problems."

Also: We have multiple implementations of normalizeHeaders. One in snapshot. Also it is dangerous, because normalizeHeader is actually just normalizeRequestHeader.

[FelixVaughan]

I understand your concern. I'd work on it if we move it to a separate issue. My main worry is potential scope creep; case in point the duplicate normalization in snapshot.

I've made the changes to address the above, but if we decide to tackle the rest here, we should lay out what all we want to change and I can handle it.

Cheers!

[FelixVaughan]

@metcoder95 can we progress or close this?

[metcoder95]

@Uzlopak can you take a look?

Codewise, lgtm, but I understand the concerns about performance implications; I'd like suggest fix the issue at hand a find a midterm of optimization/bug-fixing.