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

Change default APM server_url to 'http://127.0.0.1:8200' to avoid IPv6 ambiguity #727

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

trentm
Copy link
Member

@trentm trentm commented Nov 28, 2022

The current default APM server URL -- 'http://localhost:8200' -- is ambiguous. "localhost" can resolve to a '127.0.0.1' (IPv4) or '[::1]' (IPv6).

At least in Node v17 the default dns.lookup() ordering of results was changed to no longer explicitly sort IPv4 addresses first. That means that on systems where the default resolver returns '[::1]' first for "localhost" will result in a broken attempt to talk to a default-configured local APM server:
elastic/apm-agent-nodejs#3045
This is because APM server only binds to the IPv4 port:
elastic/apm-server#1405

Checklist

  • May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
    • Yes
      • Add a section to the spec how agents should apply sanitization (such as sanitize_field_names)
    • No
      • Why?
    • n/a
  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
    To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents) -> Change default APM server_url to 'http://127.0.0.1:8200' to avoid IPv6 ambiguity #735
  • If this spec adds a new dynamic config option, add it to central config.

/schedule 2022-12-13

…6 ambiguity

The current default APM server URL -- 'http://localhost:8200' -- is
ambiguous. "localhost" can resolve to a '127.0.0.1' (IPv4) or '[::1]'
(IPv6).

At least in Node v17 the default `dns.lookup()` ordering of results was
changed to no longer explicitly sort IPv4 addresses first.  That means
that on systems where the default resolver returns '[::1]' first for
"localhost" will result in a broken attempt to talk to a
default-configured local APM server:
  elastic/apm-agent-nodejs#3045
This is because APM server only binds to the IPv4 port:
  elastic/apm-server#1405
@apmmachine
Copy link

apmmachine commented Nov 28, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-12-12T04:33:00.395+0000

  • Duration: 3 min 16 sec

@SergeyKleyman
Copy link
Contributor

Is it possible for the system to be IPv6 only or less drastic case when list of addresses to which localhost resolves does not include 127.0.0.1?

@trentm
Copy link
Member Author

trentm commented Nov 29, 2022

(Sorry for not having opened this as draft to start.)

@trentm
Copy link
Member Author

trentm commented Nov 29, 2022

Is it possible for the system to be IPv6 only or less drastic case when list of addresses to which localhost resolves does not include 127.0.0.1?

IPv6-only is likely possible yes. I’ve no encountered it. I think it would be rare, but am not sure.

I can try to look at the Go lang docs to see if we think APM server would even listen on the IPv6 localhost by default in that case.

Re: ‘localhost’ not resolving to 127.0.0.1. Oof, I would think that would be very unlikely but possible. I am not sure if some RFC has a “MUST” statement about “localhost” resolving so. If some user had this case, I am not sure we would feel the need to have the default agent and server configs successfully reach each other. I may be missing a legitimate use case though?

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. I agree with Trent that while there may be edge cases, I think a user with a host file that redirects localhost or an ipv6-only system will be used to defaults not working.

@z1c0
Copy link
Contributor

z1c0 commented Nov 30, 2022

The change makes sense 👍 .

(On a side note, I'm just wondering though:

  • how many applications will be impacted by this change of behavior in Node v17?
  • why APM Server binds only to IPv4?)

@gregkalapos gregkalapos self-requested a review November 30, 2022 20:05
@trentm trentm removed the request for review from a team December 5, 2022 15:56
@trentm
Copy link
Member Author

trentm commented Dec 5, 2022

  • how many applications will be impacted by this change of behavior in Node v17?

@z1c0 Possibly a lot. Here is the core node issue, which shows a number of incoming links from impacted projects. Also the links in that issue's description point to a number of back issues related to this.

  • why APM Server binds only to IPv4?

There is some discussion in elastic/apm-server#1405
I suspect it is mainly just because that's what Go lang's native net library works and just binding to IPv4 has always mostly been fine.

Interestingly https://pkg.go.dev/net#Listen says:

The address can use a host name, but this is not recommended, because it will create a listener for at most one of the host's IP addresses.

So it might be worth asking APM Server devs. I'll ask separately.

@trentm
Copy link
Member Author

trentm commented Dec 6, 2022

APM server will be making the same change in elastic/apm-server#9749

trentm added a commit that referenced this pull request Dec 6, 2022
E.g. Before this change the "add it to central config" link on the
existing #727 goes to
https://github.com/elastic/apm/specs/agents/configuration.md#adding-a-new-configuration-option
which is a 404.
trentm added a commit to elastic/apm-agent-nodejs that referenced this pull request Dec 8, 2022
…7.0.0.1' rather than 'localhost' (#3049)

Starting in node v17 the defaults for DNS resolution order was
changed (nodejs/node#39987) such that
`dns.lookup()` no longer sorted IPv4 addresses first. This impacts
usage of the *default* APM Server URL (the `serverUrl` config var),
'http://localhost:8200', when using node >=17 because the APM server
only binds to the IPv4 address by default
(elastic/apm-server#1405).

Fixes: #3045
Refs: elastic/apm#727
@github-actions github-actions bot merged commit 43e5a03 into main Dec 13, 2022
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
…7.0.0.1' rather than 'localhost' (elastic#3049)

Starting in node v17 the defaults for DNS resolution order was
changed (nodejs/node#39987) such that
`dns.lookup()` no longer sorted IPv4 addresses first. This impacts
usage of the *default* APM Server URL (the `serverUrl` config var),
'http://localhost:8200', when using node >=17 because the APM server
only binds to the IPv4 address by default
(elastic/apm-server#1405).

Fixes: elastic#3045
Refs: elastic/apm#727
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.

8 participants