Skip to content

✨Expose metrics http server for extra endpoints #824

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

Merged
merged 1 commit into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 37 additions & 3 deletions pkg/manager/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const (

defaultReadinessEndpoint = "/readyz"
defaultLivenessEndpoint = "/healthz"
defaultMetricsEndpoint = "/metrics"
)

var log = logf.RuntimeLog.WithName("manager")
Expand Down Expand Up @@ -95,6 +96,9 @@ type controllerManager struct {
// metricsListener is used to serve prometheus metrics
metricsListener net.Listener

// metricsExtraHandlers contains extra handlers to register on http server that serves metrics.
metricsExtraHandlers map[string]http.Handler

// healthProbeListener is used to serve liveness probe
healthProbeListener net.Listener

Expand Down Expand Up @@ -260,6 +264,25 @@ func (cm *controllerManager) SetFields(i interface{}) error {
return nil
}

// AddMetricsExtraHandler adds extra handler served on path to the http server that serves metrics.
func (cm *controllerManager) AddMetricsExtraHandler(path string, handler http.Handler) error {
if path == defaultMetricsEndpoint {
return fmt.Errorf("overriding builtin %s endpoint is not allowed", defaultMetricsEndpoint)
}

cm.mu.Lock()
defer cm.mu.Unlock()

_, found := cm.metricsExtraHandlers[path]
if found {
return fmt.Errorf("can't register extra handler by duplicate path %q on metrics http server", path)
}

cm.metricsExtraHandlers[path] = handler
log.V(2).Info("Registering metrics http server extra handler", "path", path)
return nil
}

// AddHealthzCheck allows you to add Healthz checker
func (cm *controllerManager) AddHealthzCheck(name string, check healthz.Checker) error {
cm.mu.Lock()
Expand Down Expand Up @@ -341,19 +364,28 @@ func (cm *controllerManager) GetWebhookServer() *webhook.Server {
}

func (cm *controllerManager) serveMetrics(stop <-chan struct{}) {
var metricsPath = "/metrics"
handler := promhttp.HandlerFor(metrics.Registry, promhttp.HandlerOpts{
ErrorHandling: promhttp.HTTPErrorOnError,
})
// TODO(JoelSpeed): Use existing Kubernetes machinery for serving metrics
mux := http.NewServeMux()
mux.Handle(metricsPath, handler)
mux.Handle(defaultMetricsEndpoint, handler)

func() {
cm.mu.Lock()
defer cm.mu.Unlock()

for path, extraHandler := range cm.metricsExtraHandlers {
mux.Handle(path, extraHandler)
}
}()

server := http.Server{
Handler: mux,
}
// Run the server
go func() {
log.Info("starting metrics server", "path", metricsPath)
log.Info("starting metrics server", "path", defaultMetricsEndpoint)
if err := server.Serve(cm.metricsListener); err != nil && err != http.ErrServerClosed {
cm.errSignal.SignalError(err)
}
Expand All @@ -367,6 +399,8 @@ func (cm *controllerManager) serveMetrics(stop <-chan struct{}) {
}

func (cm *controllerManager) serveHealthProbes(stop <-chan struct{}) {
// TODO(hypnoglow): refactor locking to use anonymous func in the similar way
// it's done in serveMetrics.
cm.mu.Lock()
mux := http.NewServeMux()

Expand Down
12 changes: 12 additions & 0 deletions pkg/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package manager
import (
"fmt"
"net"
"net/http"
"time"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -54,6 +55,13 @@ type Manager interface {
// interface - e.g. inject.Client.
SetFields(interface{}) error

// AddMetricsExtraHandler adds an extra handler served on path to the http server that serves metrics.
// Might be useful to register some diagnostic endpoints e.g. pprof. Note that these endpoints meant to be
// sensitive and shouldn't be exposed publicly.
// If the simple path -> handler mapping offered here is not enough, a new http server/listener should be added as
// Runnable to the manager via Add method.
AddMetricsExtraHandler(path string, handler http.Handler) error

// AddHealthzCheck allows you to add Healthz checker
AddHealthzCheck(name string, check healthz.Checker) error

Expand Down Expand Up @@ -282,6 +290,9 @@ func New(config *rest.Config, options Options) (Manager, error) {
return nil, err
}

// By default we have no extra endpoints to expose on metrics http server.
metricsExtraHandlers := make(map[string]http.Handler)

// Create health probes listener. This will throw an error if the bind
// address is invalid or already in use.
healthProbeListener, err := options.newHealthProbeListener(options.HealthProbeBindAddress)
Expand All @@ -302,6 +313,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
resourceLock: resourceLock,
mapper: mapper,
metricsListener: metricsListener,
metricsExtraHandlers: metricsExtraHandlers,
internalStop: stop,
internalStopper: stop,
port: options.Port,
Expand Down
36 changes: 35 additions & 1 deletion pkg/manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ var _ = Describe("manger.Manager", func() {
Expect(resp.StatusCode).To(Equal(200))
})

It("should not serve anything other than metrics endpoint", func(done Done) {
It("should not serve anything other than metrics endpoint by default", func(done Done) {
opts.MetricsBindAddress = ":0"
m, err := New(cfg, opts)
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -469,6 +469,40 @@ var _ = Describe("manger.Manager", func() {
ok := metrics.Registry.Unregister(one)
Expect(ok).To(BeTrue())
})

It("should serve extra endpoints", func(done Done) {
opts.MetricsBindAddress = ":0"
m, err := New(cfg, opts)
Expect(err).NotTo(HaveOccurred())

err = m.AddMetricsExtraHandler("/debug", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
_, _ = w.Write([]byte("Some debug info"))
}))
Expect(err).NotTo(HaveOccurred())

// Should error when we add another extra endpoint on the already registered path.
err = m.AddMetricsExtraHandler("/debug", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
_, _ = w.Write([]byte("Another debug info"))
}))
Expect(err).To(HaveOccurred())

s := make(chan struct{})
defer close(s)
go func() {
defer GinkgoRecover()
Expect(m.Start(s)).NotTo(HaveOccurred())
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the start race with the the Get request we do later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this in a similar way as in other tests where HTTP requests are involved. It seems to work, but I see no strong argument why this could not race potentially, maybe I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

hm okay, interesting. Well, we can still look at it if we actually see it flake.

close(done)
}()

endpoint := fmt.Sprintf("http://%s/debug", listener.Addr().String())
resp, err := http.Get(endpoint)
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode).To(Equal(http.StatusOK))

body, err := ioutil.ReadAll(resp.Body)
Expect(err).NotTo(HaveOccurred())
Expect(string(body)).To(Equal("Some debug info"))
})
})
})

Expand Down