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

Allow current host in peers #25

Merged
merged 1 commit into from
Dec 13, 2021
Merged

Conversation

sporksmith
Copy link
Contributor

There are legitimate cases where one might want to use the current host
in "peers". In particular our tests do this by using "localhost"; this
usually works because the hostname returned by gethostname is the
~unique name of the host, so doesn't match "localhost".

In some environments, though, gethostname can return "localhost",
which causes the peer to get filtered out. Notably this includes nix
build environments, which intentionally avoid letting global unique
state into the environment, since it would make the build
non-reproducible.


I'm not sure what problem this check was originally trying to solve. If we want to keep the safeguard some alternative options are:

  • Whitelist "localhost" in this check, since it's pretty clear in that case the intent is to connect to the current host. That'd fix the immediate issue under nix, though could leave this as potentially surprising behavior for future users.
  • Add a command-line option to enable or disable this filtering behavior. The main place I could imagine wanting it is setting up some p2p configuration where several hosts share a list of hosts that they want to connect to, minus the current host. If that's the intent, maybe it'd be better for the command-line option to specify a hostname to filter, since the name returned by gethostname could a different name for the host than used in the config.

There are legitimate cases where one might want to use the current host
in "peers". In particular our tests do this by using "localhost"; this
*usually* works because the hostname returned by `gethostname` is the
~unique name of the host, so doesn't match "localhost".

In some environments, though, `gethostname` can return "localhost",
which causes the peer to get filtered out. Notably this includes nix
build environments, which intentionally avoid letting global unique
state into the environment, since it would make the build
non-reproducible.
@sporksmith sporksmith marked this pull request as ready for review December 11, 2021 19:58
@sporksmith
Copy link
Contributor Author

sporksmith commented Dec 11, 2021

I see that setting TGENHOSTNAME will cause that to be used for the current host name instead of calling gethostname. I could work around my immediate problem in nix by setting this environment variable.

OTOH I still think the current behavior is surprising, and doesn't appear to be documented.

Copy link
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

It looks like that filter was added 7 years ago, near the initial efforts to write tgen. IIRC, the idea was that we just want to put the full peer list on every node, but we only wanted remote downloads. This was back when every tgen acted as both a server and a client.

These days, in tornettools, servers and clients are independent

I'm happy to remove it as long as we're sure it does not upset our new(er) tornettools workflow (I don't think it does) :)

sporksmith added a commit to sporksmith/tornettools that referenced this pull request Dec 13, 2021
@sporksmith
Copy link
Contributor Author

Tested using shadow/tornettools#52

@sporksmith sporksmith merged commit 8cec86f into shadow:main Dec 13, 2021
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