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

debug api basic and full routing #1358

Merged
merged 3 commits into from
Mar 2, 2021
Merged

debug api basic and full routing #1358

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
feab565
debug api basic and full routing
janos Mar 1, 2021
70f37db
expose /addresses debug api endpoint asap
janos Mar 2, 2021
a2c4894
debug api test TestServer_Configure with [] underlay addresses
janos Mar 2, 2021
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
107 changes: 64 additions & 43 deletions pkg/debugapi/debugapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ package debugapi
import (
"crypto/ecdsa"
"net/http"
"sync"
"unicode/utf8"

"github.com/ethereum/go-ethereum/common"
Expand All @@ -30,33 +31,32 @@ import (

type Service interface {
http.Handler
Configure(overlay swarm.Address, publicKey, pssPublicKey ecdsa.PublicKey, ethereumAddress common.Address, p2p p2p.DebugService, pingpong pingpong.Interface, topologyDriver topology.Driver, storer storage.Storer, tags *tags.Tags, accounting accounting.Interface, settlement settlement.Interface, chequebookEnabled bool, swap swap.ApiInterface, chequebook chequebook.Service)
Copy link
Member Author

Choose a reason for hiding this comment

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

As explained in PR description, this interface should be removed for simplification and to avoid to have this method here.

MustRegisterMetrics(cs ...prometheus.Collector)
}

type server struct {
Overlay swarm.Address
PublicKey ecdsa.PublicKey
PSSPublicKey ecdsa.PublicKey
EthereumAddress common.Address
P2P p2p.DebugService
Pingpong pingpong.Interface
TopologyDriver topology.Driver
Storer storage.Storer
Logger logging.Logger
Tracer *tracing.Tracer
Tags *tags.Tags
Accounting accounting.Interface
Settlement settlement.Interface
ChequebookEnabled bool
Chequebook chequebook.Service
Swap swap.ApiInterface
metricsRegistry *prometheus.Registry
Options
http.Handler
}

type Options struct {
Overlay swarm.Address
PublicKey ecdsa.PublicKey
PSSPublicKey ecdsa.PublicKey
EthereumAddress common.Address
P2P p2p.DebugService
Pingpong pingpong.Interface
TopologyDriver topology.Driver
Storer storage.Storer
Logger logging.Logger
Tracer *tracing.Tracer
Tags *tags.Tags
Accounting accounting.Interface
Settlement settlement.Interface
ChequebookEnabled bool
Chequebook chequebook.Service
Swap swap.ApiInterface
CORSAllowedOrigins []string
metricsRegistry *prometheus.Registry
// handler is changed in the Configure method
handler http.Handler
handlerMu sync.RWMutex
}

// checkOrigin returns true if the origin is not set or is equal to the request host.
Expand Down Expand Up @@ -103,28 +103,49 @@ func equalASCIIFold(s, t string) bool {
return s == t
}

func New(overlay swarm.Address, publicKey, pssPublicKey ecdsa.PublicKey, ethereumAddress common.Address, p2p p2p.DebugService, pingpong pingpong.Interface, topologyDriver topology.Driver, storer storage.Storer, logger logging.Logger, tracer *tracing.Tracer, tags *tags.Tags, accounting accounting.Interface, settlement settlement.Interface, chequebookEnabled bool, swap swap.ApiInterface, chequebook chequebook.Service, o Options) Service {
s := &server{
Overlay: overlay,
PublicKey: publicKey,
PSSPublicKey: pssPublicKey,
EthereumAddress: ethereumAddress,
P2P: p2p,
Pingpong: pingpong,
TopologyDriver: topologyDriver,
Storer: storer,
Logger: logger,
Tracer: tracer,
Tags: tags,
Accounting: accounting,
Settlement: settlement,
metricsRegistry: newMetricsRegistry(),
ChequebookEnabled: chequebookEnabled,
Chequebook: chequebook,
Swap: swap,
}
// New creates a new Debug API Service with only basic routers enabled in order
// to expose /health endpoint, Go metrics and pprof. It is useful to expose
// these endpoints before all dependencies are configured and injected to have
// access to basic debugging tools and /health endpoint.
func New(logger logging.Logger, tracer *tracing.Tracer, corsAllowedOrigins []string) Service {
Copy link
Member

Choose a reason for hiding this comment

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

Minor suggestion: I think that overlay swarm.Address, publicKey, pssPublicKey ecdsa.PublicKey, ethereumAddress common.Address can already be set in this constructor as they are static and not expected to change during runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a very good suggestion. The only problem is ethereumAddress common.Address which needs to wait for InitChain function to complete. I am suggesting to have explicit SetEthereumAddress method and move other address arguments to constructor as you suggested. The EthereumAddress would require lock, but that is not so drastic.

This way there would be a debug api available as soon as possible for debugging and health and also with /addresses endpoint exposed, but ethereum address would be set only if and after InitChain is done. This would address some of the points in #1295 that @Eknir mentioned. WTYT @acud @Eknir?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't think that you need InitChain for this. You can have the ethereum address from the signer which is passed to NewBee, exactly as it is used in InitChain

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct. Great. :)

s := new(server)
s.Logger = logger
s.Tracer = tracer
s.CORSAllowedOrigins = corsAllowedOrigins
s.metricsRegistry = newMetricsRegistry()

s.setupRouting()
s.setRouter(s.newBasicRouter())

return s
}

// Configure injects required dependencies and configuration parameters and
// constructs HTTP routes that depend on them. It is intended and safe to call
// this method only once.
func (s *server) Configure(overlay swarm.Address, publicKey, pssPublicKey ecdsa.PublicKey, ethereumAddress common.Address, p2p p2p.DebugService, pingpong pingpong.Interface, topologyDriver topology.Driver, storer storage.Storer, tags *tags.Tags, accounting accounting.Interface, settlement settlement.Interface, chequebookEnabled bool, swap swap.ApiInterface, chequebook chequebook.Service) {
s.Overlay = overlay
s.PublicKey = publicKey
s.PSSPublicKey = pssPublicKey
s.EthereumAddress = ethereumAddress
s.P2P = p2p
s.Pingpong = pingpong
s.TopologyDriver = topologyDriver
s.Storer = storer
s.Tags = tags
s.Accounting = accounting
s.Settlement = settlement
s.ChequebookEnabled = chequebookEnabled
s.Chequebook = chequebook
s.Swap = swap

s.setRouter(s.newRouter())
}

func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// protect handler as it is changed by the Configure method
s.handlerMu.RLock()
h := s.handler
s.handlerMu.RUnlock()

h.ServeHTTP(w, r)
}
73 changes: 72 additions & 1 deletion pkg/debugapi/debugapi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ import (
"testing"

"github.com/ethereum/go-ethereum/common"
"github.com/ethersphere/bee"
accountingmock "github.com/ethersphere/bee/pkg/accounting/mock"
"github.com/ethersphere/bee/pkg/debugapi"
"github.com/ethersphere/bee/pkg/jsonhttp"
"github.com/ethersphere/bee/pkg/jsonhttp/jsonhttptest"
"github.com/ethersphere/bee/pkg/logging"
p2pmock "github.com/ethersphere/bee/pkg/p2p/mock"
"github.com/ethersphere/bee/pkg/pingpong"
Expand Down Expand Up @@ -57,7 +60,8 @@ func newTestServer(t *testing.T, o testServerOptions) *testServer {
settlement := swapmock.New(o.SettlementOpts...)
chequebook := chequebookmock.NewChequebook(o.ChequebookOpts...)
swapserv := swapmock.NewApiInterface(o.SwapOpts...)
s := debugapi.New(o.Overlay, o.PublicKey, o.PSSPublicKey, o.EthereumAddress, o.P2P, o.Pingpong, topologyDriver, o.Storer, logging.New(ioutil.Discard, 0), nil, o.Tags, acc, settlement, true, swapserv, chequebook, debugapi.Options{})
s := debugapi.New(logging.New(ioutil.Discard, 0), nil, nil)
s.Configure(o.Overlay, o.PublicKey, o.PSSPublicKey, o.EthereumAddress, o.P2P, o.Pingpong, topologyDriver, o.Storer, o.Tags, acc, settlement, true, swapserv, chequebook)
ts := httptest.NewServer(s)
t.Cleanup(ts.Close)

Expand Down Expand Up @@ -86,3 +90,70 @@ func mustMultiaddr(t *testing.T, s string) multiaddr.Multiaddr {
}
return a
}

// TestServer_Configure validates that http routes are correct when server is
// constructed with only basic routes and after it is configured with
// dependencies.
func TestServer_Configure(t *testing.T) {
var o testServerOptions
topologyDriver := topologymock.NewTopologyDriver(o.TopologyOpts...)
acc := accountingmock.NewAccounting(o.AccountingOpts...)
settlement := swapmock.New(o.SettlementOpts...)
chequebook := chequebookmock.NewChequebook(o.ChequebookOpts...)
swapserv := swapmock.NewApiInterface(o.SwapOpts...)
s := debugapi.New(logging.New(ioutil.Discard, 0), nil, nil)
ts := httptest.NewServer(s)
t.Cleanup(ts.Close)

client := &http.Client{
Transport: web.RoundTripperFunc(func(r *http.Request) (*http.Response, error) {
u, err := url.Parse(ts.URL + r.URL.String())
if err != nil {
return nil, err
}
r.URL = u
return ts.Client().Transport.RoundTrip(r)
}),
}

testBasicRouter(t, client)
jsonhttptest.Request(t, client, http.MethodGet, "/readiness", http.StatusNotFound,
jsonhttptest.WithExpectedJSONResponse(jsonhttp.StatusResponse{
Message: http.StatusText(http.StatusNotFound),
Code: http.StatusNotFound,
}),
)

s.Configure(o.Overlay, o.PublicKey, o.PSSPublicKey, o.EthereumAddress, o.P2P, o.Pingpong, topologyDriver, o.Storer, o.Tags, acc, settlement, true, swapserv, chequebook)

testBasicRouter(t, client)
jsonhttptest.Request(t, client, http.MethodGet, "/readiness", http.StatusOK,
jsonhttptest.WithExpectedJSONResponse(debugapi.StatusResponse{
Status: "ok",
Version: bee.Version,
}),
)
}

func testBasicRouter(t *testing.T, client *http.Client) {
t.Helper()

jsonhttptest.Request(t, client, http.MethodGet, "/health", http.StatusOK,
jsonhttptest.WithExpectedJSONResponse(debugapi.StatusResponse{
Status: "ok",
Version: bee.Version,
}),
)

for _, path := range []string{
"/metrics",
"/debug/pprof",
"/debug/pprof/cmdline",
"/debug/pprof/profile?seconds=1", // profile for only 1 second to check only the status code
"/debug/pprof/symbol",
"/debug/pprof/trace",
"/debug/vars",
} {
jsonhttptest.Request(t, client, http.MethodGet, path, http.StatusOK)
}
}
43 changes: 32 additions & 11 deletions pkg/debugapi/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,23 @@ import (
"github.com/ethersphere/bee/pkg/logging/httpaccess"
)

func (s *server) setupRouting() {
baseRouter := http.NewServeMux()
// newBasicRouter constructs only the routes that do not depend on the injected dependencies:
// - /health
// - pprof
// - vars
// - metrics
func (s *server) newBasicRouter() *mux.Router {
router := mux.NewRouter()
router.NotFoundHandler = http.HandlerFunc(jsonhttp.NotFoundHandler)

baseRouter.Handle("/metrics", web.ChainHandlers(
router.Path("/metrics").Handler(web.ChainHandlers(
httpaccess.SetAccessLogLevelHandler(0), // suppress access log messages
web.FinalHandler(promhttp.InstrumentMetricHandler(
s.metricsRegistry,
promhttp.HandlerFor(s.metricsRegistry, promhttp.HandlerOpts{}),
)),
))

router := mux.NewRouter()
router.NotFoundHandler = http.HandlerFunc(jsonhttp.NotFoundHandler)

router.Handle("/debug/pprof", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
u := r.URL
u.Path += "/"
Expand All @@ -48,11 +51,21 @@ func (s *server) setupRouting() {

router.Handle("/health", web.ChainHandlers(
httpaccess.SetAccessLogLevelHandler(0), // suppress access log messages
web.FinalHandlerFunc(s.statusHandler),
web.FinalHandlerFunc(statusHandler),
))

return router
}

// newRouter construct the complete set of routes after all of the dependencies
// are injected and exposes /readiness endpoint to provide information that
// Debug API is fully active.
func (s *server) newRouter() *mux.Router {
router := s.newBasicRouter()

router.Handle("/readiness", web.ChainHandlers(
httpaccess.SetAccessLogLevelHandler(0), // suppress access log messages
web.FinalHandlerFunc(s.statusHandler),
web.FinalHandlerFunc(statusHandler),
))

router.Handle("/pingpong/{peer-id}", jsonhttp.MethodHandler{
Expand Down Expand Up @@ -149,7 +162,13 @@ func (s *server) setupRouting() {
"GET": http.HandlerFunc(s.getTagHandler),
})

baseRouter.Handle("/", web.ChainHandlers(
return router
}

// setRouter sets the base Debug API handler with common middlewares.
func (s *server) setRouter(router http.Handler) {
h := http.NewServeMux()
h.Handle("/", web.ChainHandlers(
httpaccess.NewHTTPAccessLogHandler(s.Logger, logrus.InfoLevel, s.Tracer, "debug api access"),
handlers.CompressHandler,
func(h http.Handler) http.Handler {
Expand All @@ -164,10 +183,12 @@ func (s *server) setupRouting() {
h.ServeHTTP(w, r)
})
},
// todo: add recovery handler
web.NoCacheHeadersHandler,
web.FinalHandler(router),
))

s.Handler = baseRouter
s.handlerMu.Lock()
defer s.handlerMu.Unlock()

s.handler = h
}
2 changes: 1 addition & 1 deletion pkg/debugapi/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type statusResponse struct {
Version string `json:"version"`
}

func (s *server) statusHandler(w http.ResponseWriter, r *http.Request) {
func statusHandler(w http.ResponseWriter, r *http.Request) {
jsonhttp.OK(w, statusResponse{
Status: "ok",
Version: bee.Version,
Expand Down
Loading