Skip to content

dns: not sending null updates#19461

Closed
alyssawilk wants to merge 1 commit intoenvoyproxy:mainfrom
alyssawilk:dns
Closed

dns: not sending null updates#19461
alyssawilk wants to merge 1 commit intoenvoyproxy:mainfrom
alyssawilk:dns

Conversation

@alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Jan 10, 2022

Fixing unexpected behavior, where null updates are sent on first resolution.

With the new timeouts, the major functional change is that every DNS lookup for a given host will wait for the configured timeout before expiring, rather than the initial request(s) waiting until a one time timeout expiring, and all subsequent requests fast-failing.

I'm honestly not sure which is a preferred behavior given the DNS resolver will continue to attempt to re-resolve, so sending for discussion before I go fix up unit tests. Matt had been inclined to not send null updates but if we've got re-resolves going and there has been no response after the initial timeout I'm inclined to think continuing is the correct choice, but that the DNS filters should fast-fail if there's no resolution result.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19461 was opened by alyssawilk.

see: more, trace.

@alyssawilk alyssawilk assigned snowp, junr03 and RyanTheOptimist and unassigned lizan Jan 10, 2022
@alyssawilk
Copy link
Contributor Author

I guess my question is do we ever always want to wait? Assume we have 5s timeout. I'd think all requests should wait 5s from "initial resolution attempt" but if request 1 was at T=0 and request 2 was T=1s I'd assume once we'd spent 5s trying to resolve we'd want to fail request 2 at T=5s not T=6s

So basically I think we should ditch this approach, allow null updates for initial update, and fast-fail instead of continue if the resolved address is nullptr.

@RyanTheOptimist
Copy link
Contributor

I guess my question is do we ever always want to wait? Assume we have 5s timeout. I'd think all requests should wait 5s from "initial resolution attempt" but if request 1 was at T=0 and request 2 was T=1s I'd assume once we'd spent 5s trying to resolve we'd want to fail request 2 at T=5s not T=6s

So basically I think we should ditch this approach, allow null updates for initial update, and fast-fail instead of continue if the resolved address is nullptr.

Yes, I think I agree with this summary.

@alyssawilk alyssawilk closed this Jan 13, 2022
alyssawilk added a commit that referenced this pull request Jan 24, 2022
…ons (#19588)

in #17645 there was a bunch of discussion around the DNS cache returning null addresses and how to handle it. After discussion on #19461 we agreed to keep sending null updates, but to fast-fail if no address was resolved.

Risk Level: Medium (data plane change)
Testing: updated integration tests, unit tests
Docs Changes: n/a
Release Notes: inline

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
…ons (envoyproxy#19588)

in envoyproxy#17645 there was a bunch of discussion around the DNS cache returning null addresses and how to handle it. After discussion on envoyproxy#19461 we agreed to keep sending null updates, but to fast-fail if no address was resolved.

Risk Level: Medium (data plane change)
Testing: updated integration tests, unit tests
Docs Changes: n/a
Release Notes: inline

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Josh Perry <josh.perry@mx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants