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

docs: clarify ExternalServer DNS resolution behavior #2563

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isker
Copy link
Contributor

@isker isker commented Aug 20, 2024

From reading the source code of workerd and KJ, I can see that ExternalServer does DNS resolution only once ever. This was certainly surprising to me, and I suspect it would be a surprise to many, as ExternalServer is the seemingly most natural way to get a binding to a particular networked service, but it is in fact only suitable to look up domain names that are guaranteed to not change over the lifetime of the workerd service on which it is configured, like container sidecars.

So, be more explicit about the behavior in the documentation.

From reading the source code of workerd and KJ, I can see that ExternalServer
does DNS resolution only once ever. This was certainly surprising to me, and I
suspect it would be a surprise to many, as ExternalServer is the seemingly most
natural way to get a binding to a particular networked service, but it is in fact
only suitable to look up domain names that are guaranteed to not change over the
lifetime of the workerd service on which it is configured, like container
sidecars.

So, be more explicit about the behavior in the documentation.
@isker isker requested review from a team as code owners August 20, 2024 16:06
@isker isker requested review from dom96 and mar-cf August 20, 2024 16:06
@isker
Copy link
Contributor Author

isker commented Aug 20, 2024

Here's a TODO explaining just this:

// In fact, this version should be designed to redo the DNS lookup periodically to see if it
// changed, which would be nice for workerd when the remote address may change over time.

@jasnell jasnell requested a review from kentonv August 20, 2024 16:34
@kentonv
Copy link
Member

kentonv commented Aug 21, 2024

I think this is a bug. If we document it, we should say that it's a bug that may be fixed later. But maybe we should just fix it?

@isker
Copy link
Contributor Author

isker commented Aug 21, 2024

You added the TODO two years ago in dc27ddc. So it doesn’t seem to be a bug as much as consciously acknowledged missing functionality. And I don’t think that commit regressed DNS resolution in any way, I think ExternalServer has always been this way.

Either way, I have got what it takes to clarify the documentation but not, I think, to add this functionality. So if you’d prefer the latter to the exclusion of the former, go ahead and close this PR.

@mar-cf mar-cf removed their request for review September 12, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants