debugger: refactor httpserver and debugger (openservicemesh#2308)

- Debugger specific code, logic and tests that was living in
`httpserver` has been entirely moved to `debugger` package.
- Some specific code related to metrics, probes and versioning that
was also living in `httpserver` has been moved out in order to make `httpserver`
reusable in both osm-controller and debugger HTTP server instances.
- `Debugger` package now manages its own HTTP server, specified and opened
on `constants.DebugPort`.
- Updated E2E to repeat some iterations of the DebugServer on/off scenario.

Signed-off-by: Eduard Serra <eduser25@gmail.com>

cmd/osm-controller/osm-controller.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+	"net/http"
 	"os"
 	"path"
 	"strings"
@@ -245,22 +246,34 @@ func main() {
 		events.GenericEventRecorder().FatalEvent(err, events.InitializationError, "Error initializing ADS server")
 	}
 
-	// Initialize function and HTTP health probes
-	funcProbes := []health.Probes{xdsServer}
-	httpProbes := getHTTPHealthProbes()
-
-	// Initialize the http server and start it
-	httpServer := httpserver.NewHTTPServer(funcProbes, httpProbes, metricsstore.DefaultMetricsStore, constants.MetricsServerPort)
-	httpServer.Start()
-
 	if err := createControllerManagerForOSMResources(certManager); err != nil {
 		events.GenericEventRecorder().FatalEvent(err, events.InitializationError, "Error creating controller manager to reconcile OSM resources")
 	}
 
-	// Expose /debug endpoints and data only if the enableDebugServer flag is enabled
+	// Initialize OSM's http service server
+	httpServer := httpserver.NewHTTPServer(constants.OSMServicePort)
+
+	// Health/Liveness probes
+	funcProbes := []health.Probes{xdsServer}
+	httpServer.AddHandlers(map[string]http.Handler{
+		"/health/ready": health.ReadinessHandler(funcProbes, getHTTPHealthProbes()),
+		"/health/alive": health.LivenessHandler(funcProbes, getHTTPHealthProbes()),
+	})
+	// Metrics
+	httpServer.AddHandler("/metrics", metricsstore.DefaultMetricsStore.Handler())
+	// Version
+	httpServer.AddHandler("/version", version.GetVersionHandler())
+
+	// Start HTTP server
+	err = httpServer.Start()
+	if err != nil {
+		log.Fatal().Err(err).Msgf("Failed to start OSM metrics/probes HTTP server")
+	}
+
+	// Create DebugServer and start its config event listener.
+	// Listener takes care to start and stop the debug server as appropriate
 	debugConfig := debugger.NewDebugConfig(certDebugger, xdsServer, meshCatalog, kubeConfig, kubeClient, cfg, kubernetesClient)
-	debugServerInterface := httpserver.NewDebugHTTPServer(debugConfig, constants.DebugPort)
-	httpserver.RegisterDebugServer(debugServerInterface, cfg)
+	debugConfig.StartDebugServerConfigListener()
 
 	<-stop
 	log.Info().Msg("Goodbye!")

cmd/osm-controller/osm-controller_test.go

@@ -2,82 +2,17 @@ package main
 
 import (
 	"context"
-	"strconv"
 	"testing"
-	"time"
-
-	"github.com/pkg/errors"
 
 	tassert "github.com/stretchr/testify/assert"
 
-	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	testclient "k8s.io/client-go/kubernetes/fake"
 
 	"github.com/openservicemesh/osm/pkg/certificate/providers/tresor"
-	"github.com/openservicemesh/osm/pkg/configurator"
 	"github.com/openservicemesh/osm/pkg/constants"
-	"github.com/openservicemesh/osm/pkg/httpserver"
-)
-
-const (
-	testOSMNamespace     = "-test-osm-namespace-"
-	testOSMConfigMapName = "-test-osm-config-map-"
 )
 
-func toggleDebugServer(enable bool, kubeClient *testclient.Clientset) error {
-	updatedConfigMap := v1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: testOSMNamespace,
-			Name:      testOSMConfigMapName,
-		},
-		Data: map[string]string{
-			"enable_debug_server": strconv.FormatBool(enable),
-		},
-	}
-	_, err := kubeClient.CoreV1().ConfigMaps(testOSMNamespace).Update(context.TODO(), &updatedConfigMap, metav1.UpdateOptions{})
-	return err
-}
-
-func TestDebugServer(t *testing.T) {
-	assert := tassert.New(t)
-
-	// set up a controller
-	stop := make(chan struct{})
-	kubeClient, _, cfg, err := setupComponents(testOSMNamespace, testOSMConfigMapName, false, stop)
-	assert.NoError(err)
-
-	fakeDebugServer := FakeDebugServer{0, 0, nil, false}
-	httpserver.RegisterDebugServer(&fakeDebugServer, cfg)
-
-	assert.Equal(0, fakeDebugServer.startCount)
-	assert.Equal(0, fakeDebugServer.stopCount)
-
-	// Start server
-	err = toggleDebugServer(true, kubeClient)
-	assert.NoError(err)
-
-	assert.Eventually(func() bool {
-		return fakeDebugServer.running == true
-	}, 2*time.Second, 100*time.Millisecond)
-	assert.Eventually(func() bool {
-		return fakeDebugServer.startCount == 1
-	}, 2*time.Second, 100*time.Millisecond)
-	assert.Equal(0, fakeDebugServer.stopCount) // No eventually for an non-expected-to-change value
-
-	// Stop it
-	err = toggleDebugServer(false, kubeClient)
-	assert.NoError(err)
-
-	assert.Eventually(func() bool {
-		return fakeDebugServer.running == false
-	}, 2*time.Second, 100*time.Millisecond)
-	assert.Eventually(func() bool {
-		return fakeDebugServer.stopCount == 1
-	}, 2*time.Second, 100*time.Millisecond)
-	assert.Equal(1, fakeDebugServer.startCount) // No eventually for an non-expected-to-change value
-}
-
 func TestCreateCABundleKubernetesSecret(t *testing.T) {
 	assert := tassert.New(t)
 
@@ -122,45 +57,3 @@ func TestJoinURL(t *testing.T) {
 		assert.Equal(result, ju.expectedOutput)
 	}
 }
-
-type FakeDebugServer struct {
-	stopCount  int
-	startCount int
-	stopErr    error
-	running    bool
-}
-
-func (f *FakeDebugServer) Stop() error {
-	f.stopCount++
-	if f.stopErr != nil {
-		return errors.Errorf("Debug server error")
-	}
-	f.running = false
-	return nil
-}
-
-func (f *FakeDebugServer) Start() {
-	f.startCount++
-	f.running = true
-}
-
-func setupComponents(namespace, configMapName string, initialDebugServerEnabled bool, stop chan struct{}) (*testclient.Clientset, v1.ConfigMap, configurator.Configurator, error) {
-	kubeClient := testclient.NewSimpleClientset()
-
-	defaultConfigMap := map[string]string{
-		"enable_debug_server": strconv.FormatBool(initialDebugServerEnabled),
-	}
-	configMap := v1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: namespace,
-			Name:      configMapName,
-		},
-		Data: defaultConfigMap,
-	}
-	_, err := kubeClient.CoreV1().ConfigMaps(namespace).Create(context.TODO(), &configMap, metav1.CreateOptions{})
-	if err != nil {
-		return kubeClient, configMap, nil, err
-	}
-	cfg := configurator.NewConfigurator(kubeClient, stop, namespace, configMapName)
-	return kubeClient, configMap, cfg, nil
-}

pkg/constants/constants.go

@@ -60,8 +60,8 @@ const (
 	// InjectorWebhookPort is the port on which the sidecar injection webhook listens
 	InjectorWebhookPort = 9090
 
-	// MetricsServerPort is the port on which OSM exposes its own metrics server
-	MetricsServerPort = 9091
+	// OSMServicePort is the port on which OSM exposes HTTP paths for metrics and probes
+	OSMServicePort = 9091
 
 	//DebugPort is the port on which OSM exposes its debug server
 	DebugPort = 9092

pkg/debugger/certificate.go

@@ -10,7 +10,7 @@ import (
 	"github.com/openservicemesh/osm/pkg/certificate"
 )
 
-func (ds debugConfig) getCertHandler() http.Handler {
+func (ds DebugConfig) getCertHandler() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		certs := ds.certDebugger.ListIssuedCertificates()
 

pkg/debugger/certificate_test.go

@@ -18,7 +18,7 @@ func TestGetCertHandler(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	mock := NewMockCertificateManagerDebugger(mockCtrl)
 
-	ds := debugConfig{
+	ds := DebugConfig{
 		certDebugger: mock,
 	}
 

pkg/debugger/debugger_config_listener.go

@@ -0,0 +1,54 @@
+package debugger
+
+import (
+	"github.com/openservicemesh/osm/pkg/announcements"
+	"github.com/openservicemesh/osm/pkg/constants"
+	"github.com/openservicemesh/osm/pkg/httpserver"
+	"github.com/openservicemesh/osm/pkg/kubernetes/events"
+)
+
+// StartDebugServerConfigListener registers a go routine to listen to configuration and configure debug server as needed
+func (d *DebugConfig) StartDebugServerConfigListener() {
+	// Subscribe to configuration updates
+	ch := events.GetPubSubInstance().Subscribe(
+		announcements.ConfigMapAdded,
+		announcements.ConfigMapDeleted,
+		announcements.ConfigMapUpdated)
+
+	// This is the Debug server
+	httpDebugServer := httpserver.NewHTTPServer(constants.DebugPort)
+	httpDebugServer.AddHandlers(d.GetHandlers())
+
+	// Run config listener
+	go func(cfgSubChannel chan interface{}, dConf *DebugConfig, httpServ *httpserver.HTTPServer) {
+		// Bootstrap after subscribing
+		started := false
+
+		if d.configurator.IsDebugServerEnabled() {
+			if err := httpDebugServer.Start(); err != nil {
+				log.Error().Err(err).Msgf("error starting debug server")
+			}
+			started = true
+		}
+
+		for {
+			<-cfgSubChannel
+			isDbgSrvEnabled := d.configurator.IsDebugServerEnabled()
+
+			if isDbgSrvEnabled && !started {
+				if err := httpDebugServer.Start(); err != nil {
+					log.Error().Err(err).Msgf("error starting debug server")
+				} else {
+					started = true
+				}
+			}
+			if !isDbgSrvEnabled && started {
+				if err := httpDebugServer.Stop(); err != nil {
+					log.Error().Err(err).Msgf("error stoping debug server")
+				} else {
+					started = false
+				}
+			}
+		}
+	}(ch, d, httpDebugServer)
+}

pkg/debugger/envoy.go

@@ -11,7 +11,7 @@ import (
 	"github.com/openservicemesh/osm/pkg/certificate"
 )
 
-func (ds debugConfig) getEnvoyConfig(pod *v1.Pod, cn certificate.CommonName, url string) string {
+func (ds DebugConfig) getEnvoyConfig(pod *v1.Pod, cn certificate.CommonName, url string) string {
 	log.Info().Msgf("Getting Envoy config for CN=%s, podIP=%s", cn, pod.Status.PodIP)
 
 	minPort := 16000

pkg/debugger/feature_flags.go

@@ -8,7 +8,7 @@ import (
 	"github.com/openservicemesh/osm/pkg/featureflags"
 )
 
-func (ds debugConfig) getFeatureFlags() http.Handler {
+func (ds DebugConfig) getFeatureFlags() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if featureFlagsJSON, err := json.Marshal(featureflags.Features); err != nil {
 			log.Error().Err(err).Msgf("Error marshaling feature flags struct: %+v", featureflags.Features)

pkg/debugger/index.go

@@ -5,7 +5,7 @@ import (
 	"net/http"
 )
 
-func (ds debugConfig) getDebugIndex(handlers map[string]http.Handler) http.Handler {
+func (ds DebugConfig) getDebugIndex(handlers map[string]http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "text/html")
 		_, _ = fmt.Fprint(w, `<ul>`)

pkg/debugger/namespace.go

@@ -10,7 +10,7 @@ type namespaces struct {
 	Namespaces []string `json:"namespaces"`
 }
 
-func (ds debugConfig) getMonitoredNamespacesHandler() http.Handler {
+func (ds DebugConfig) getMonitoredNamespacesHandler() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		var n namespaces
 		n.Namespaces = ds.meshCatalogDebugger.ListMonitoredNamespaces()

pkg/debugger/namespace_test.go

@@ -16,7 +16,7 @@ func TestMonitoredNamespaceHandler(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	mock := NewMockMeshCatalogDebugger(mockCtrl)
 
-	ds := debugConfig{
+	ds := DebugConfig{
 		meshCatalogDebugger: mock,
 	}
 	monitoredNamespacesHandler := ds.getMonitoredNamespacesHandler()

pkg/debugger/policy.go

@@ -20,7 +20,7 @@ type policies struct {
 	TrafficTargets   []*target.TrafficTarget     `json:"traffic_targets"`
 }
 
-func (ds debugConfig) getOSMConfigHandler() http.Handler {
+func (ds DebugConfig) getOSMConfigHandler() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		confJSON, err := ds.configurator.GetConfigMap()
 		if err != nil {
@@ -31,7 +31,7 @@ func (ds debugConfig) getOSMConfigHandler() http.Handler {
 	})
 }
 
-func (ds debugConfig) getSMIPoliciesHandler() http.Handler {
+func (ds DebugConfig) getSMIPoliciesHandler() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		var p policies
 		p.TrafficSplits, p.WeightedServices, p.ServiceAccounts, p.RouteGroups, p.TrafficTargets = ds.meshCatalogDebugger.ListSMIPolicies()

pkg/debugger/policy_test.go

@@ -22,7 +22,7 @@ func TestGetSMIPolicies(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	mock := NewMockMeshCatalogDebugger(mockCtrl)
 
-	ds := debugConfig{
+	ds := DebugConfig{
 		meshCatalogDebugger: mock,
 	}
 

pkg/debugger/port_forward.go

@@ -30,7 +30,7 @@ type portForward struct {
 	Ready chan struct{}
 }
 
-func (ds debugConfig) forwardPort(req portForward) {
+func (ds DebugConfig) forwardPort(req portForward) {
 	log.Info().Msgf("Start port forward to podIP=%s on PodPort=%d to LocalPort=%d", req.Pod.Status.PodIP, req.PodPort, req.LocalPort)
 	path := fmt.Sprintf("/api/v1/namespaces/%s/pods/%s/portforward", req.Pod.Namespace, req.Pod.Name)
 	hostIP := strings.TrimLeft(ds.kubeConfig.Host, "htps:/")

pkg/debugger/proxy.go

@@ -15,7 +15,7 @@ const (
 	proxyConfigQueryKey   = "cfg"
 )
 
-func (ds debugConfig) getProxies() http.Handler {
+func (ds DebugConfig) getProxies() http.Handler {
 	// This function is needed to convert the list of connected proxies to
 	// the type (map) required by the printProxies function.
 	listConnected := func() map[certificate.CommonName]time.Time {
@@ -59,7 +59,7 @@ func printProxies(w http.ResponseWriter, proxies map[certificate.CommonName]time
 	_, _ = fmt.Fprint(w, `</table>`)
 }
 
-func (ds debugConfig) getConfigDump(cn certificate.CommonName, w http.ResponseWriter) {
+func (ds DebugConfig) getConfigDump(cn certificate.CommonName, w http.ResponseWriter) {
 	pod, err := catalog.GetPodFromCertificate(cn, ds.kubeController)
 	if err != nil {
 		log.Error().Err(err).Msgf("Error getting Pod from certificate with CN=%s", cn)
@@ -69,7 +69,7 @@ func (ds debugConfig) getConfigDump(cn certificate.CommonName, w http.ResponseWr
 	_, _ = fmt.Fprintf(w, "%s", envoyConfig)
 }
 
-func (ds debugConfig) getProxy(cn certificate.CommonName, w http.ResponseWriter) {
+func (ds DebugConfig) getProxy(cn certificate.CommonName, w http.ResponseWriter) {
 	pod, err := catalog.GetPodFromCertificate(cn, ds.kubeController)
 	if err != nil {
 		log.Error().Err(err).Msgf("Error getting Pod from certificate with CN=%s", cn)

pkg/debugger/server.go

@@ -12,7 +12,7 @@ import (
 )
 
 // GetHandlers implements DebugConfig interface and returns the rest of URLs and the handling functions.
-func (ds debugConfig) GetHandlers() map[string]http.Handler {
+func (ds DebugConfig) GetHandlers() map[string]http.Handler {
 	handlers := map[string]http.Handler{
 		"/debug/certs":         ds.getCertHandler(),
 		"/debug/xds":           ds.getXDSHandler(),
@@ -38,7 +38,7 @@ func (ds debugConfig) GetHandlers() map[string]http.Handler {
 
 // NewDebugConfig returns an implementation of DebugConfig interface.
 func NewDebugConfig(certDebugger CertificateManagerDebugger, xdsDebugger XDSDebugger, meshCatalogDebugger MeshCatalogDebugger, kubeConfig *rest.Config, kubeClient kubernetes.Interface, cfg configurator.Configurator, kubeController k8s.Controller) DebugConfig {
-	return debugConfig{
+	return DebugConfig{
 		certDebugger:        certDebugger,
 		xdsDebugger:         xdsDebugger,
 		meshCatalogDebugger: meshCatalogDebugger,