Tell "the host didn't answer" apart from "we refuse that address" - #1
Merged
Conversation
Both arrived as the same nil, empty list or false, which reads fine until a caller treats a refusal as permanent. A webhook that deactivates a customer's endpoint when the guard refuses it would retire that endpoint on one bad DNS minute, because a lookup returning nothing was indistinguishable from a lookup returning the metadata address. They are not the same answer. A refusal is a statement about the address; an empty lookup is a statement about the moment. So a host that resolves to nothing now raises Unresolvable, deliberately not a subclass of Violation -- callers that don't care can rescue both, and the ones that do can finally choose. This is also where a guard built on Resolv.getaddress used to raise Resolv::ResolvError, so callers migrating from one keep the distinction they already had. resolvable_public_ip? still answers false either way. A predicate answers the question it was asked and doesn't raise. Two things fell out of writing the tests: Resolv.getaddresses sat inside the `rescue IPAddr::InvalidAddressError` clause that detects an IP literal, so the `rescue Resolv::ResolvError, ResolvTimeout` below it could never fire -- sibling rescue clauses don't catch each other. The literal check moves into its own method and the DNS call moves back into the body, where the rescue reaches it. It was only ever reachable through a resolver configured to raise timeouts, but it read as protection it wasn't providing. URI#host is "" for "http://", not nil, so a blank host slipped past host_of and came back as a lookup that resolved to nothing. There was never a name to look up: that's malformed, and it now says so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
"The host didn't answer" and "we refuse that address" arrived as the same
nil, empty list, orfalse. That reads fine until a caller treats a refusal as permanent — a webhook that deactivates a customer's endpoint when the guard refuses it would retire that endpoint on one bad DNS minute, because a lookup returning nothing was indistinguishable from a lookup returning the metadata address.A refusal is a statement about the address. An empty lookup is a statement about the moment. So a host that resolves to nothing now raises
Unresolvable, deliberately not a subclass ofViolation— callers that don't care can rescue both, and the ones that do can finally choose.resolve_public_ips(takes a host)[]Unresolvableresolve_public_ipnilUnresolvablenilenforce_public_ipViolationUnresolvableViolationresolvable_public_ip?truefalsefalsefalseresolvable_public_ip?still answersfalseeither way — a predicate answers the question it was asked and doesn't raise.This is also where a guard built on
Resolv.getaddressused to raiseResolv::ResolvError, so callers migrating from one keep a distinction they already had rather than silently losing it.Two bugs the tests turned up
The resolver's
ResolvErrorrescue was dead code.Resolv.getaddressessat inside therescue IPAddr::InvalidAddressErrorclause that detects an IP literal, and sibling rescue clauses don't catch each other — so therescue Resolv::ResolvError, Resolv::ResolvTimeoutbelow it could never fire. Only reachable through a resolver configured to raise timeout errors, so nothing was breaking today, but it read as protection it wasn't providing. The literal check moves into its own method and the DNS call moves back into the body where the rescue reaches it.A blank host resolved to nothing instead of being refused.
URI#hostis""for"http://", notnil, so it slipped pasthost_ofand came back as a failed lookup. There was never a name to look up — that's malformed, and it now says so.Compatibility
Breaking for anyone relying on
resolve_public_ip/resolve_public_ipsreturningnil/[]for an unresolvable host, or onenforce_public_ipraisingViolationfor one. RescuingUnresolvablealongsideViolationrestores the old behavior exactly.63 tests green on the existing suite plus the new cases.