multi: introduce status server and sub-server manager #442

ellemouton · 2022-10-28T13:32:16Z

This PR does a few things:

Adds a new subServerMgr that handles the connections to Loop, Pool & Faraday. This will also make it easy for us to add another sub server in future. This is also nice because it makes it easier in the next steps to treat these sub-servers ,& any errors we may get from them, differently to how we would treat errors from LND or LiTd.
Adds a new Status server that is "always-on". This will allow the UI to query info about the status of any of LiTs sub-servers (including LND, Loop, Pool, Faraday and LiT itself).
Does a big refactor so that Litd does not ever shutdown unless requested to via a shutdown signal. This ensures that the Status server can always be reached.

$ litcli status 

{
        "sub_servers": {
                "faraday": {
                        "running": true,
                        "error": ""
                },
                "lit": {
                        "running": true,
                        "error": ""
                },
                "lnd": {
                        "running": true,
                        "error": ""
                },
                "loop": {
                        "running": true,
                        "error": ""
                },
                "pool": {
                        "running": false,
                        "error": "could not start pool: <something something>"
                }
        }
}

TODO:

documentation explaining the difference (and reasoning) between the lnd 10009 (gRPC only) and the litd 8443
More itests to cover the startup process

Fixes #421
Fixes #420
Fixes #290

ellemouton · 2022-11-02T11:03:02Z

@guggero - as discussed offline, I have requested your review here but am mainly looking for approach comments to begin with. And also, no rush on this as I will be afk for the next 2 weeks

guggero

Very nice work 💯 This is an important step towards making it possible to unlock lnd from the UI and also to make everything more user friendly.
Will need to do a second pass with a focus on security but looks pretty good for its WIP status!

guggero · 2022-11-17T16:02:34Z

config.go

@@ -150,6 +150,9 @@ type Config struct {
 	LetsEncryptDir    string `long:"letsencryptdir" description:"The directory where the Let's Encrypt library will store its key and certificate."`
 	LetsEncryptListen string `long:"letsencryptlisten" description:"The IP:port on which LiT will listen for Let's Encrypt challenges. Let's Encrypt will always try to contact on port 80. Often non-root processes are not allowed to bind to ports lower than 1024. This configuration option allows a different port to be used, but must be used in combination with port forwarding from port 80. This configuration can also be used to specify another IP address to listen on, for example an IPv6 address."`

+	TLSCertPath string `long:"tlscertpath" description:"Path to write the self signed TLS certificate for LiT's RPC and REST proxy service (if Let's Encrypt is not used)."`


Do we need to explicitly mention this only applies to the HTTP(S)Listen port (default 8443)? Just to distinguish it more from the 10009 lnd RPC port.

guggero · 2022-11-17T16:04:31Z

config.go

@@ -280,9 +280,9 @@ func (c *Config) lndConnectParams() (string, lndclient.Network, string,
 func defaultConfig() *Config {
 	return &Config{
 		HTTPSListen: defaultHTTPSListen,
+		TLSCertPath: DefaultTLSCertPath,


We should also update doc/config-lnd-integrated.md and perhaps explain the difference (and reasoning) between the lnd 10009 (gRPC only) and the litd 8443 (grpc-web, REST, gRPC proxy, UI) a bit more explicitly?
Also this change should be made very explicit in the release notes (not sure where to put the reminder for us).

Yeah good call here, as it would also be a breaking change (CLI/config wise).

guggero · 2022-11-17T16:08:32Z

rpc_proxy.go

 // isHandling checks if the specified request is something to be handled by lnd
 // or any of the attached sub daemons. If true is returned, the call was handled
 // by the RPC proxy and the caller MUST NOT handle it again. If false is
 // returned, the request was not handled and the caller MUST handle it.
 func (p *rpcProxy) isHandling(resp http.ResponseWriter,
 	req *http.Request) bool {

+	if !p.hasStarted() {
+		return false


I'm not sure if returning false here is a good idea, as the request will just be passed to the next handler. Instead I think we should check whether it's a request we would handle (the same conditions as we check below) and return an error page (perhaps 503 service unavailable) if we did and only false if we didn't?

good point!

guggero · 2022-11-17T16:13:58Z

terminal.go

@@ -1437,6 +1450,20 @@ func (g *LightningTerminal) showStartupInfo() error {
 	return nil
 }

+// connectLND sets up LiT's LND connection.
+func (g *LightningTerminal) connectLND(bufListener *bufconn.Listener) error {


Could turn this into a function by taking the cfg as a parameter and returning the conn. Then we could place this function and the dialBackend() and dialBufConnBackend() into a different file to reduce the size of this one a bit. Maybe lnd_connection.go or something like that?

sounds gooood

guggero · 2022-11-17T16:14:12Z

rpc_proxy.go

@@ -168,21 +166,9 @@ type rpcProxy struct {
 }

 // Start creates initial connection to lnd.
-func (p *rpcProxy) Start() error {
+func (p *rpcProxy) Start(lndConn *grpc.ClientConn) error {


guggero · 2022-11-17T16:29:28Z

terminal.go

+	// Start the main web server that dispatches requests either to the
+	// static UI file server or the RPC proxy. This makes it possible to
+	// unlock lnd through the UI.
+	if err := g.startMainWebServer(); err != nil {


Since we start the main web server this early, we have to guard the g.restHandler from being called if it's nil (. Otherwise a REST request coming in too early would panic. Or we move the call to createRESTProxy() to before this call even.

Hmm, no, creating the REST proxy before the main web server is started will probably not work since the REST proxy will attempt to connect to the main web server on creation.

That makes me wonder if we could extract the main web server into its own struct that owns the REST proxy and RPC proxy (and probably also the status server)? Then the start/stop would be a bit more abstracted as well. But not sure how large of a diff that is.

good catch!
Ok so looks like, for now, moving the createRESTProxy() call to before the startMainWebServer call does actually do the trick. Makes sense cause it has always been called in that order.

guggero · 2022-11-17T16:40:32Z

state_server.go

+		},
+	}
+
+	options := []grpcweb.Option{


Do we really need to spin up our own grpc-web and gRPC server? Can't we instead have this as a sub component of the rpcProxy that already does all of this for us?

yeah good point

Just realised why I did it this way initially - the reason is that if we fail to start LND, then we never get to the registration and serving of the status server. So I think we need to re-add this actually

guggero · 2022-11-17T16:42:02Z

state_server.go

+
+	// gRPC web requests are easy to identify. Send them to the gRPC
+	// web proxy.
+	if s.grpcWebProxy.IsGrpcWebRequest(req) ||


Again, it feels like this should just be hooked up in the right order to allow the existing rpcProxy to do this.

subserver_mgr.go

guggero · 2022-11-17T16:49:11Z

terminal.go

-				"requests, lnd possibly still starting or " +
-				"syncing")
-		}
+	handled, err := g.subServerMgr.ValidateMacaroon(


We'll need to make sure we don't create any authentication by-passes by accident, for example when starting up the individual daemons. Maybe adding a few extra itests around the whole startup process would be a good idea.

ellemouton

thanks for the initial review @guggero!

I have addressed most of your comments.

I just still need to add the extra documentation& also add more itests around the startup process. Will re-request once I have done that.

ellemouton · 2022-12-01T19:59:39Z

rpc_proxy.go

 // isHandling checks if the specified request is something to be handled by lnd
 // or any of the attached sub daemons. If true is returned, the call was handled
 // by the RPC proxy and the caller MUST NOT handle it again. If false is
 // returned, the request was not handled and the caller MUST handle it.
 func (p *rpcProxy) isHandling(resp http.ResponseWriter,
 	req *http.Request) bool {

+	if !p.hasStarted() {
+		return false


good point!

ellemouton · 2022-12-01T20:05:53Z

terminal.go

@@ -1437,6 +1450,20 @@ func (g *LightningTerminal) showStartupInfo() error {
 	return nil
 }

+// connectLND sets up LiT's LND connection.
+func (g *LightningTerminal) connectLND(bufListener *bufconn.Listener) error {


sounds gooood

ellemouton · 2022-12-01T20:07:24Z

terminal.go

 	if err != nil {
-		log.Errorf("Could not start subservers: %v", err)
+		log.Errorf("Could not set up LND clients: %v", err)


even for logs? thought it only mattered for error variables

ellemouton · 2022-12-03T09:41:29Z

terminal.go

+	// Start the main web server that dispatches requests either to the
+	// static UI file server or the RPC proxy. This makes it possible to
+	// unlock lnd through the UI.
+	if err := g.startMainWebServer(); err != nil {


good catch!
Ok so looks like, for now, moving the createRESTProxy() call to before the startMainWebServer call does actually do the trick. Makes sense cause it has always been called in that order.

ellemouton · 2022-12-03T10:18:49Z

state_server.go

+		},
+	}
+
+	options := []grpcweb.Option{


yeah good point

lightninglabs-deploy · 2023-01-23T03:45:36Z

@ellemouton, remember to re-request review from reviewers when ready

ellemouton · 2023-01-25T06:02:19Z

!lightninglabs-deploy mute

ellemouton · 2023-01-25T06:02:55Z

muting for the time being. Will pick up again one I have more room on my plate.

Although would be cool to get back to this soon as I think it could make the addition of taro easier

Roasbeef · 2023-02-10T00:50:57Z

config.go

@@ -280,9 +280,9 @@ func (c *Config) lndConnectParams() (string, lndclient.Network, string,
 func defaultConfig() *Config {
 	return &Config{
 		HTTPSListen: defaultHTTPSListen,
+		TLSCertPath: DefaultTLSCertPath,


Yeah good call here, as it would also be a breaking change (CLI/config wise).

Roasbeef · 2023-02-10T01:07:37Z

rpc_proxy.go

@@ -165,6 +166,9 @@ type rpcProxy struct {

 	grpcServer   *grpc.Server
 	grpcWebProxy *grpcweb.WrappedGrpcServer
+
+	started bool


Could use an atomic bool alternatively.

Still getting thru the diff, but could also use an event type to allow callers to get notifications for when things are fully up vs polling.

The hasStarted thing is more for the sake of external callers who are making calls to Litd before it has fully started up.

Roasbeef · 2023-02-10T01:09:05Z

rpc_proxy.go

-		log.Infof("Handling gRPC web request: %s", req.URL.Path)
-		p.grpcWebProxy.ServeHTTP(resp, req)
+		if !p.hasStarted() {
+			resp.WriteHeader(http.StatusServiceUnavailable)


Why this vs a gRPC status/error like we do on the lnd side?

Roasbeef · 2023-02-10T01:11:26Z

rpc_proxy.go

-		p.grpcServer.ServeHTTP(resp, req)
+		if !p.hasStarted() {
+			resp.WriteHeader(http.StatusServiceUnavailable)
+			_, _ = resp.Write(make([]byte, 0))


Comment above may moreso apply here. Another route would be an http middleware chain (gRPC package) that checked this field and returned a response if things weren't fully active yet.

good idea! will give it a spin

Roasbeef · 2023-02-10T01:15:25Z

rpc_proxy.go

@@ -61,7 +56,7 @@ func (e *proxyErr) Unwrap() error {
 // component.
 func newRpcProxy(cfg *Config, validator macaroons.MacaroonValidator,
 	superMacValidator session.SuperMacaroonValidator,
-	permsMgr *perms.Manager, bufListener *bufconn.Listener) *rpcProxy {
+	permsMgr *perms.Manager) *rpcProxy {


Arg passed but not used anywhere (in this commit at least).

this diff removes an arg

Roasbeef · 2023-02-10T01:31:25Z

subserver_mgr.go

+		s.cfg.onStartError(err)
+		return
+	}
+	s.integratedStarted = true


Doesn't need a mutex to protect access?

Roasbeef · 2023-02-10T01:31:34Z

subserver_mgr.go

+		return
+	}
+
+	go func() {


Should be tracked by a wait group.

subserver_mgr.go

Roasbeef · 2023-02-10T01:38:19Z

rpc_proxy.go

-	faradayConn *grpc.ClientConn
-	loopConn    *grpc.ClientConn
-	poolConn    *grpc.ClientConn
+	lndConn *grpc.ClientConn


Any reason lnd isn't also managed as a sub-server? I guess since it's sort of the "root" sub-server that all the rest depend on?

I guess since it's sort of the "root" sub-server that all the rest depend on?

Yes exactly. It is handled as a special case (see next commit) since Litd cant really continue operation if the start or connection to LND fails

Roasbeef · 2023-02-10T01:42:46Z

terminal.go

+			lndGrpc *lndclient.GrpcLndServices,
+			withMacaroonService bool) error {
+
+			return poolServer.StartAsSubserver(


Re that comment re converting many of the closures to be an interface, given this is just a pass thru call (same args and return value), a wrapper struct that embeds all the main daemon configs would take care of this nicely.

changed to interfaces - turned out nicely I think

positiveblue

Thank you for these changes @ellemouton , they'll make my life easier for integrating the taro daemon 💯

positiveblue · 2023-02-13T11:02:04Z

rpc_proxy.go

@@ -279,17 +298,27 @@ func (p *rpcProxy) isHandling(resp http.ResponseWriter,
 	if p.grpcWebProxy.IsGrpcWebRequest(req) ||
 		p.grpcWebProxy.IsGrpcWebSocketRequest(req) {

-		log.Infof("Handling gRPC web request: %s", req.URL.Path)
-		p.grpcWebProxy.ServeHTTP(resp, req)
+		if !p.hasStarted() {


Should this function return false if p.hasStarted() is false?

See previous discussion on this: #442 (comment)

We dont want to return false here cause then Litd will pass it on to other services. We instead want to exit early

lnd_connection.go

positiveblue · 2023-02-13T14:12:01Z

rpc_proxy.go

+
+	handled, subServerName := p.subServerMgr.HandledBy(requestURI)
+
+	switch {


I wonder if we can simplify this logic if it looked like:

// old checkSubSystemStarted now called func (p *rpcProxy) hasRegisteredMatchingMethod(uri string) error { // HandledBy checks for status server, lnd, lit and all the subservers system, err := p.subServerMgr.HandledBy(uri) if err != nil { fmt.Errorf("no matching method ...", err) } // getSubServerState returns alwasy true for status + state for the other subsystems started, startErr := p.statusServer.getSubServerState(system) if !started { return fmt.Errorf("%s is not running: %s", system, startErr) } return nil }

but we do want to check lit's other parts (LND and Lit) if the sub-server manager does not handle the request URI. So not sure I follow?

positiveblue · 2023-02-13T14:31:53Z

subserver_mgr.go

+	// onStartError will be called if there is a startup error (in
+	// integrated mode) or if the remote connection could not be made in
+	// remote mode.
+	onStartError func(err error)


Do we plan onStartError, onStartSuccess to do more things in the future beyond setting the status for this subserver? If not we can directly set the status using subServerCfg.name

great point - removed

positiveblue · 2023-02-13T14:34:19Z

subserver_mgr.go

+
+	// remote must be set if the sub-server is running remotely and so must
+	// not be started in integrated mode.
+	remote bool


here remote == (remoteCfg != nil)?

no - remoteCfg is always set by default.

positiveblue · 2023-02-13T14:35:36Z

subserver_mgr.go

+
+// StartRemoteSubServers creates connections to all the manager's sub-servers
+// that are running remotely.
+func (s *subServerMgr) StartRemoteSubServers() {


The func name can be more accurate if we use something like ConnectRemoteSubServers given that we are not starting anything.

good point! done!

In this commit, we remove Lit's dependency on LND's tls cert in integrated mode. With this commit, even if Lit is started in integrated mode, it will create its own tls cert to use for serving the webserver.

In this commit, a new LitService proto is added and the StopDaemon method is added to the service. This is added due to the fact that in an upcoming commit, we want Litd to continue running even if LND cannot be connected to and so this means that Lit will no longer shutdown if LNDs StopDaemon method is called.

Add a `started` variable to the rpcProxy that is used to indicate if the proxy is ready to handle requests. This is because currently the webserver is dependent on the rpcProxy to start and we want to be able to start the webserver without being dependent on the rpcProxy so that it can be used to handle status requests in a future commit. So with this commit, we can now saftely start the webserver earlier on and then if requests come through for the rpcProxy, an error will be displayed to the user.

Remove the responsibility of creating an LND connection from the rpcProxy and instead let the main LightningTerminal struct handle it. All the lnd-connection specific functions are also moved into their own file.

A pure refactor commit. It just splits various parts of the main Run function out into helper functions in order to more clearly show the steps taken to set up LiT.

Since all the relevant dependancies have been accouted for, we know move the starting of the main webserver to earlier on in the Run function. This is in preparation for later when the webserver will be "always-on" and will be used to server status information about the rest of the LiT subservers.

Add the protos for a new Status server that can be used to query the status of a number of LiT's subservers.

In this commit, the various IsURI methods of the permissions manager are removed and replaced with a single IsSubServerURI method that gets passed both the URI and the subserver that should be checked. This will greatly simplifiy implementing a subserver manager in an upcoming commit.

Define a SubServer interface and a subServer manager

With this commit, we now also track the start-up state of LiT and LiT's connection to LND. These differ from the other sub-servers (loop, pool & faraday) because a failure to start LiT or LND is fatal and so should stop the rest of the start-process, however, we still want the webserver to continue serving the new Status server so that the UI can query the start-up status of LND and LiT. So: if any errors occur while starting/connecting to LND or any other errors occur while starting any of LiTs other processes, then we throw an error but we dont kill the main LiT process. The main process is only killed upon receiving a shutdown signal.

ellemouton · 2023-02-16T16:21:13Z

I need to switch away from this for a bit now so just making a note of some issues that the current version still has:

THe itests seem to not exit
we currently construct the autopilot server and session server before getting to the registration of rpc servers. This means that if the setup of those fail, then we cant access the status server to see this. And the reason we ordered it this way is so that LND can register non-nil servers. so a bit of a catch 22 here...
I think to solve the above, we might need to add a new grpc server that just servers the status server. So that it is not dependent on the start up of LND. see here

ellemouton · 2023-05-03T10:40:00Z

replaced by #516 & #541

ellemouton marked this pull request as draft October 28, 2022 20:37

ellemouton force-pushed the statusServer branch 4 times, most recently from 5d1ca50 to 940e413 Compare October 31, 2022 17:29

ellemouton changed the title ~~[WIP] multi: introduce status server and sub-server manager~~ multi: introduce status server and sub-server manager Oct 31, 2022

ellemouton marked this pull request as ready for review October 31, 2022 17:29

ellemouton force-pushed the statusServer branch from 940e413 to ab91ede Compare October 31, 2022 19:04

ellemouton changed the title ~~multi: introduce status server and sub-server manager~~ [WIP] multi: introduce status server and sub-server manager Nov 2, 2022

ellemouton force-pushed the statusServer branch from ab91ede to 809bdf9 Compare November 2, 2022 11:01

ellemouton requested a review from guggero November 2, 2022 11:02

guggero reviewed Nov 17, 2022

View reviewed changes

ellemouton force-pushed the statusServer branch from 809bdf9 to fc97568 Compare December 3, 2022 11:08

ellemouton commented Dec 3, 2022

View reviewed changes

ellemouton changed the title ~~[WIP] multi: introduce status server and sub-server manager~~ multi: introduce status server and sub-server manager Dec 3, 2022

ellemouton force-pushed the statusServer branch 4 times, most recently from cb7f9ab to ec46889 Compare December 10, 2022 07:12

Roasbeef mentioned this pull request Feb 10, 2023

subserver: add tarod sub-server as both integrated and remote operating mode #485

Closed

Roasbeef requested changes Feb 10, 2023

View reviewed changes

positiveblue reviewed Feb 13, 2023

View reviewed changes

ellemouton added 4 commits February 16, 2023 13:56

multi: always use Lit's TLS cert

a6e0e3c

In this commit, we remove Lit's dependency on LND's tls cert in integrated mode. With this commit, even if Lit is started in integrated mode, it will create its own tls cert to use for serving the webserver.

multi: split out LND connection from rpcProxy

f06d735

Remove the responsibility of creating an LND connection from the rpcProxy and instead let the main LightningTerminal struct handle it. All the lnd-connection specific functions are also moved into their own file.

ellemouton added 11 commits February 16, 2023 13:56

terminal: modularise the Run function

5b1e5fd

A pure refactor commit. It just splits various parts of the main Run function out into helper functions in order to more clearly show the steps taken to set up LiT.

litrpc: add Status server protos

82230fd

Add the protos for a new Status server that can be used to query the status of a number of LiT's subservers.

multi: implement the status server

00fda36

cmd/litcli: commands for status server

e998ad4

multi: define a SubServer and add a subServer manager

77a9db7

Define a SubServer interface and a subServer manager

multi: plug in subserverMgr into LightningTerminal

26415d4

lit: add a faraday SubServer

fbf009a

lit: add a loop SubServer

b9863f1

lit: add a pool SubServer

ae563c6

ellemouton force-pushed the statusServer branch from ec46889 to 7edcc68 Compare February 16, 2023 15:14

ellemouton added 2 commits February 16, 2023 17:36

rpc_proxy: allow Litd's StopDaemon call at any time

486fae3

ellemouton force-pushed the statusServer branch from 7edcc68 to 486fae3 Compare February 16, 2023 15:36

positiveblue mentioned this pull request Feb 21, 2023

multi: always use Lit's TLS cert #499

Merged

positiveblue mentioned this pull request Mar 7, 2023

Add new subservers package #516

Merged

ellemouton mentioned this pull request May 3, 2023

multi: add a Status server #541

Merged

ellemouton closed this May 3, 2023


		handled, subServerName := p.subServerMgr.HandledBy(requestURI)

		switch {

multi: introduce status server and sub-server manager #442

multi: introduce status server and sub-server manager #442

Uh oh!

Conversation

ellemouton commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellemouton commented Nov 2, 2022

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Jan 23, 2023

Uh oh!

ellemouton commented Jan 25, 2023

Uh oh!

ellemouton commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ellemouton commented Oct 28, 2022 •

edited

Loading

ellemouton commented Jan 25, 2023 •

edited

Loading

ellemouton Feb 16, 2023 •

edited

Loading