From 8d4db65908334a663c0f14ce7bad74902d9fb50b Mon Sep 17 00:00:00 2001 From: Vincent Mallet Date: Mon, 18 Dec 2023 16:31:47 -0800 Subject: [PATCH] Fix profiler port Two endpoints should be on the profiler port rather than the main port: `gc_stats` and `live_profiler`. This is done by installing them on the default mux used by the profiler, rather than a custom mux passed on to the main server. Once `live_profiler` is on the internal profiler port, we can remove the `127.0.0.1` remote-ip restriction to make it easier to use, given that the default base image is a no-login image. --- cmd/main.go | 6 +-- server/monitoring/metrics_server.go | 5 +++ server/monitoring/profile_server.go | 6 --- server/streaming/server.go | 3 +- server/streaming/server_test.go | 3 +- test/integration/integration_test.go | 5 --- test/integration/profiler_test.go | 59 ++++++++++++++++++++++++++++ test/integration/server_test.go | 1 + 8 files changed, 69 insertions(+), 19 deletions(-) create mode 100644 test/integration/profiler_test.go diff --git a/cmd/main.go b/cmd/main.go index a423fa3..ba09c8f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -2,8 +2,6 @@ package main import ( "fmt" - "net/http" - _ "net/http/pprof" "os" "github.com/sirupsen/logrus" @@ -40,10 +38,8 @@ func main() { func startServer(config *config.Config, logger *logrus.Logger) (err error) { logger.Infoln("starting") - mux := http.NewServeMux() registry := streaming.NewSocketRegistry() - monitoring.StartProfilerServer(config, mux, logger) if config.StatusPort > 0 { monitoring.StartStatusServer(config, logger) } @@ -56,7 +52,7 @@ func startServer(config *config.Config, logger *logrus.Logger) (err error) { return err } - server, _, err := streaming.InitServer(config, mux, producerRules, logger, registry) + server, _, err := streaming.InitServer(config, producerRules, logger, registry) if err != nil { return err } diff --git a/server/monitoring/metrics_server.go b/server/monitoring/metrics_server.go index 525ce5b..efe5fb4 100644 --- a/server/monitoring/metrics_server.go +++ b/server/monitoring/metrics_server.go @@ -6,6 +6,9 @@ import ( "sync" "time" + // This registers the profiler on the default mux which we will use for monitoring port. + _ "net/http/pprof" + "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/sirupsen/logrus" @@ -42,6 +45,8 @@ func StartServerMetrics(config *config.Config, logger *logrus.Logger, registry * if config.Monitoring.ProfilerPort > 0 { go func() { + StartProfilerServer(config, http.DefaultServeMux, logger) + if err := http.ListenAndServe(fmt.Sprintf(":%d", config.Monitoring.ProfilerPort), nil); err != nil { logger.Errorf("profiler_listen_error: %v", err) } diff --git a/server/monitoring/profile_server.go b/server/monitoring/profile_server.go index 5c72c37..df00c66 100644 --- a/server/monitoring/profile_server.go +++ b/server/monitoring/profile_server.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "runtime/debug" - "strings" "github.com/sirupsen/logrus" @@ -19,11 +18,6 @@ type profileServer struct { // liveProfiler profiles https requests func (p *profileServer) liveProfiler(config *config.Config) func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - if !strings.Contains(r.RemoteAddr, "127.0.0.1") { // enable this only locally - w.WriteHeader(404) - return - } - if config.Monitoring == nil || config.Monitoring.ProfilingPath == "" { // disabled by default w.WriteHeader(405) _, _ = w.Write([]byte(`{"error":"profiler not configured"}`)) diff --git a/server/streaming/server.go b/server/streaming/server.go index 49b2690..2f94400 100644 --- a/server/streaming/server.go +++ b/server/streaming/server.go @@ -39,7 +39,7 @@ type Server struct { } // InitServer initializes the main server -func InitServer(c *config.Config, mux *http.ServeMux, producerRules map[string][]telemetry.Producer, logger *logrus.Logger, registry *SocketRegistry) (*http.Server, *Server, error) { +func InitServer(c *config.Config, producerRules map[string][]telemetry.Producer, logger *logrus.Logger, registry *SocketRegistry) (*http.Server, *Server, error) { reliableAck := false if c.Kafka != nil { reliableAck = c.ReliableAck @@ -52,6 +52,7 @@ func InitServer(c *config.Config, mux *http.ServeMux, producerRules map[string][ logger: logger, } + mux := http.NewServeMux() mux.HandleFunc("/", socketServer.ServeBinaryWs(c, registry)) mux.HandleFunc("/status", socketServer.Status()) diff --git a/server/streaming/server_test.go b/server/streaming/server_test.go index c836a38..d27fedf 100644 --- a/server/streaming/server_test.go +++ b/server/streaming/server_test.go @@ -49,8 +49,7 @@ var _ = Describe("Socket handler test", func() { req := httptest.NewRequest("GET", "http://tel.vn.tesla.com", nil) producerRules := make(map[string][]telemetry.Producer) - mux := http.NewServeMux() - _, s, err := streaming.InitServer(conf, mux, producerRules, logger, registry) + _, s, err := streaming.InitServer(conf, producerRules, logger, registry) Expect(err).NotTo(HaveOccurred()) srv := httptest.NewServer(http.HandlerFunc(s.ServeBinaryWs(conf, registry))) diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index f85f9fb..caa2a91 100644 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -125,11 +125,6 @@ var _ = Describe("Test messages", Ordered, func() { Expect(string(body)).To(Equal("ok")) }) - It("returns 200 for gc stats", func() { - _, err := VerifyHTTPSRequest(serviceURL, "gc_stats", tlsConfig) - Expect(err).NotTo(HaveOccurred()) - }) - It("returns 200 for prom metrics", func() { _, err := VerifyHTTPRequest(prometheusURL, "metrics") Expect(err).NotTo(HaveOccurred()) diff --git a/test/integration/profiler_test.go b/test/integration/profiler_test.go new file mode 100644 index 0000000..329ae4e --- /dev/null +++ b/test/integration/profiler_test.go @@ -0,0 +1,59 @@ +package integration_test + +import ( + "fmt" + "net/http" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var ( + gcStatsPath = "/gc_stats" + liveProfilerPath = "/live_profiler?mode=off" + pprofPath = "/debug/pprof/" +) + +var _ = Describe("Profiler Endpoints", func() { + var ( + client *http.Client + ) + + BeforeEach(func() { + cfg, err := GetTLSConfig() + Expect(err).NotTo(HaveOccurred()) + + client = &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: cfg, + }, + } + }) + + Context("Service port", func() { + DescribeTable("returns 400", + func(path string) { + r, err := client.Get(fmt.Sprintf("https://%s%s", serviceURL, path)) + Expect(err).NotTo(HaveOccurred()) + Expect(r.StatusCode).To(Equal(400)) + }, + + Entry("for "+gcStatsPath, gcStatsPath), + Entry("for "+liveProfilerPath, liveProfilerPath), + Entry("for "+pprofPath, pprofPath)) + }) + + Context("Profiler port", func() { + DescribeTable("succeeds", + func(path string, expectedCode int) { + r, err := client.Get(fmt.Sprintf("http://%s%s", monitoringURL, path)) + Expect(err).NotTo(HaveOccurred()) + Expect(r.StatusCode).To(Equal(expectedCode)) + }, + + Entry("for "+gcStatsPath, gcStatsPath, 200), + Entry("for "+liveProfilerPath, liveProfilerPath, 304), // We get 304 when the profiler is enabled, and we ask it to switch to the mode it is already in + Entry("for "+pprofPath, pprofPath, 200), + ) + }) +}) diff --git a/test/integration/server_test.go b/test/integration/server_test.go index 40f3ca4..cbf0dab 100644 --- a/test/integration/server_test.go +++ b/test/integration/server_test.go @@ -28,6 +28,7 @@ const ( serviceURL = "app:4443" statusURL = "app:8080" + monitoringURL = "app:4269" prometheusURL = "app:9090" clientCert = "./test-certs/vehicle_device.device-1.cert"