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.

Sign up for GitHub

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

relay: turn autorelay into a service and always filter out relay addresses #578

Merged
merged 8 commits into from
Apr 6, 2019
Merged

relay: turn autorelay into a service and always filter out relay addresses #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
80ada8a
relay: no more wrapping
Stebalien Apr 5, 2019
5b66564
relay: only stop advertising relay addresses if we fully disconnect
Stebalien Apr 6, 2019
45df977
bump relay record limit to 50
vyzo Apr 6, 2019
1d8efc3
doc: document relay options
Stebalien Apr 6, 2019
c580255
relay: drop design comments
Stebalien Apr 6, 2019
fee7b01
add missing word
vyzo Apr 6, 2019
7093262
relay: update documentation
Stebalien Apr 6, 2019
87d57b5
relay: allow users to explicitly add relay addresses
Stebalien Apr 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,29 @@ func (cfg *Config) NewNode(ctx context.Context) (host.Host, error) {
swrm.Filters = cfg.Filters
}

var h host.Host
h, err = bhost.NewHost(ctx, swrm, &bhost.HostOpts{
h, err := bhost.NewHost(ctx, swrm, &bhost.HostOpts{
ConnManager: cfg.ConnManager,
AddrsFactory: cfg.AddrsFactory,
NATManager: cfg.NATManager,
EnablePing: !cfg.DisablePing,
})

if err != nil {
swrm.Close()
return nil, err
}

if cfg.Relay {
// If we've enabled the relay, we should filter out relay
// addresses by default.
//
// TODO: We shouldn't be doing this here.
oldFactory := h.AddrsFactory
h.AddrsFactory = func(addrs []ma.Multiaddr) []ma.Multiaddr {
return oldFactory(relay.Filter(addrs))
}
}
vyzo marked this conversation as resolved.
Show resolved Hide resolved

upgrader := new(tptu.Upgrader)
upgrader.Protector = cfg.Protector
upgrader.Filters = swrm.Filters
Expand Down Expand Up @@ -206,18 +217,16 @@ func (cfg *Config) NewNode(ctx context.Context) (host.Host, error) {
}

if hop {
h = relay.NewRelayHost(swrm.Context(), h.(*bhost.BasicHost), discovery)
// advertise ourselves
relay.Advertise(ctx, discovery)
} else {
h = relay.NewAutoRelayHost(swrm.Context(), h.(*bhost.BasicHost), discovery, router)
_ = relay.NewAutoRelay(swrm.Context(), h, discovery, router)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to keep the autorelay host, otherwise we will wrap/return the basic host and all the address manipulation will be voided.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually it's probably ok, we are overriding the AddressFactory at the end of the day.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it's not really an abuse of context. Contexts are the only construct go has for denoting dynamic scopes, so it's ok to use it for long-lived ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Context in go are rather... overloaded. They are the only construct for dynamic scopes so, in that sense, it's correct to pass them into constructors. However, they're more commonly (e.g., here) used for cancellation and that's where we get into trouble: contexts don't allow waiting for something to finish.

They're useful for requests where either:

  1. You pass the context and then wait on a returned channel. This channel allows you to actually wait for the request.
  2. You pass the context to a synchronous request.
  3. You just don't care about waiting.

However, services should really just provide a Stop() or Close() function.

I bring this up because it prevents orderly shutdown in go-ipfs. For example, I'd like to be able to say:

  1. Shut down all services using the datastore.
  2. Shut down the datastore.

However, when we use contexts to manage services, we can't wait for 1 to finish before we move on to 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

And how are we going to implement Stop or Close when there are background goroutines doing work? Why, with contextx of course!
So we can't really get rid of them and I think it's better to have one global context for the entire host and tree of services rather than have each service make its own background one with cancel.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not arguing against Close methods; all I am saying is that contexts will inevitably be used for this and it's convenient to have a global context with user cancellation.
When we go services, we might have GlobalContext service dependency that provides this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I'm explicitly arguing against using contexts for canceling services at all because:

  1. It causes all services to shut down all at once. This has been causing real and really annoying problems in go-ipfs where we'll, e.g., close the blockservice before we stop all services trying to write blocks.
  2. It makes it really easy to incorrectly implement services. Before the transport refactor, we had quite a few services that wouldn't actually stop unless you both canceled the context and called Close.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a Close method for services is undeniably a useful abstraction, as it could handle orderly dependency shutdown as well (but it won't be easy).
The global context could be used by services that need a context to manager their internal goroutines, there is nothing stopping them from wrapping this context in a WithCancel.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that canceling this context will cause the service to start shutting down before close is called, throwing any orderly shutdown out the window. Now, of course, we could just say "don't cancel this context till you've finished the orderly shutdown" but that's not exactly user-friendly.

Copy link
Member

Choose a reason for hiding this comment

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

Let's put together a requirements/design document outlining what we actually need from the refactor. That will help us drive alignment and consensus.

I sense we all have slightly different views, understandings and wishes, and it'll cause frustration if we jump into technical solutions before we decide what problems we want to solve.

Things we have discussed:

  • dependency injection
  • service lifecycles
  • hot replacement of runtime services
  • service tree (lifecycle dependencies)
  • eventbus
  • etc.

}
}

if router != nil {
h = routed.Wrap(h, router)
return routed.Wrap(h, router), nil
}

// TODO: Bootstrapping.

return h, nil
}

Expand Down
23 changes: 19 additions & 4 deletions options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,12 @@ func AddrsFactory(factory config.AddrsFactory) Option {
}
}

// EnableRelay configures libp2p to enable the relay transport with configuration options.
// EnableRelay configures libp2p to enable the relay transport with
// configuration options. By default, this option only configures libp2p to
// accept inbound connections from relays and make outbound connections
// _through_ relays when requested by the remote peer. (default: enabled)
//
// To _act_ as a relay, pass the circuit.OptHop option.
func EnableRelay(options ...circuit.RelayOpt) Option {
return func(cfg *Config) error {
cfg.RelayCustom = true
Expand All @@ -211,7 +216,7 @@ func EnableRelay(options ...circuit.RelayOpt) Option {
}
}

// DisableRelay configures libp2p to disable the relay transport
// DisableRelay configures libp2p to disable the relay transport.
func DisableRelay() Option {
return func(cfg *Config) error {
cfg.RelayCustom = true
Expand All @@ -220,8 +225,18 @@ func DisableRelay() Option {
}
}

// EnableAutoRelay configures libp2p to enable autorelay advertising; requires relay to
// be enabled and the Routing option to provide an instance of ContentRouting.
// EnableAutoRelay configures libp2p to enable the AutoRelay subsystem. It is an
// error to enable AutoRelay without enabling relay (enabled by default) and
// routing (not enabled by default).
//
// This subsystem performs two functions:
//
// 1. When this libp2p node is configured to act as a relay "hop"
// (circuit.OptHop is passed to EnableRelay), this node will advertise itself
// as a public relay using the provided routing system.
// 2. When this libp2p node is _not_ configured as a relay "hop", it will
// automatically detect if it is unreachable (e.g., behind a NAT). If so, it will
// find, configure, and announce a set of public relays.
Copy link
Member Author

Choose a reason for hiding this comment

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

@vyzo, this is why I don't like this option design. I now have to explain to the user that the "AutoRelay" option can do two very different things depending on how they've configured their relay.

Copy link
Contributor

Choose a reason for hiding this comment

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

AutoRelay is a system service.
If you are a hop relay, then the functionality is to advertise relay services. If you are a client, then the functionality is to use automatically use the services advertised by hop relays.

Copy link
Member Author

Choose a reason for hiding this comment

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

What service? There are, and always have been, two completely separate and orthogonal services: one for "hop" relays and one for clients. That's why this is so damn confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to think of autorelay as a single service. But yes, at some level you are right that these are two seperate services and that could potentially be confusing.

Copy link
Member

@raulk raulk Apr 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There are, and always have been, two completely separate and orthogonal services: one for "hop" relays and one for clients.

Actually, it's even more confusing than that. A client can be a consumer of a relay service, or a target of a relayed connection. Currently, all of these flow over the same thick protocol. I really have a hard time wrapping my head around this, and so do our users.

I'd like to segregate this protocol if possible into at least two:

  • /libp2p/circuit/relay/hop/v0.0.0: exposed only by relay nodes. Used inside the initiator -> relay connection.
  • /libp2p/circuit/relay/target/v0.0.0: exposed by clients accepting relayed connections. Used in the relay -> target connection.

This would allow for better implicit observability too. Would've helped a lot in the incident we're analysing.

func EnableAutoRelay() Option {
return func(cfg *Config) error {
cfg.EnableAutoRelay = true
Expand Down
Loading