Decouple certificate common name from proxy registry (#4763)

* Decouple certificate common name from proxy registry

Signed-off-by: Sean Teeling <[email protected]>

* address comment 1

Signed-off-by: Sean Teeling <[email protected]>

Author: steeling

pkg/debugger/proxy.go

@@ -6,71 +6,91 @@ import (
 	"sort"
 	"time"
 
-	"github.com/openservicemesh/osm/pkg/certificate"
 	"github.com/openservicemesh/osm/pkg/envoy"
 )
 
 const (
-	specificProxyQueryKey = "proxy"
-	proxyConfigQueryKey   = "cfg"
+	uuidQueryKey        = "uuid"
+	proxyConfigQueryKey = "cfg"
 )
 
 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 {
-		proxies := make(map[certificate.CommonName]time.Time)
-		for _, proxy := range ds.proxyRegistry.ListConnectedProxies() {
-			proxies[proxy.GetCertificateCommonName()] = (*proxy).GetConnectedAt()
-		}
-		return proxies
-	}
-
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "text/html")
-		if proxyConfigDump, ok := r.URL.Query()[proxyConfigQueryKey]; ok {
-			ds.getConfigDump(certificate.CommonName(proxyConfigDump[0]), w)
-		} else if specificProxy, ok := r.URL.Query()[specificProxyQueryKey]; ok {
-			ds.getProxy(certificate.CommonName(specificProxy[0]), w)
-		} else {
-			printProxies(w, listConnected(), "Connected")
+		proxyConfigDump := r.URL.Query()[proxyConfigQueryKey]
+		uuid := r.URL.Query()[uuidQueryKey]
+
+		switch {
+		case len(uuid) == 0:
+			ds.printProxies(w)
+		case len(proxyConfigDump) > 0:
+			ds.getConfigDump(uuid[0], w)
+		default:
+			ds.getProxy(uuid[0], w)
 		}
 	})
 }
 
-func printProxies(w http.ResponseWriter, proxies map[certificate.CommonName]time.Time, category string) {
-	var commonNames []string
-	for cn := range proxies {
-		commonNames = append(commonNames, cn.String())
+func (ds DebugConfig) printProxies(w http.ResponseWriter) {
+	// This function is needed to convert the list of connected proxies to
+	// the type (map) required by the printProxies function.
+	proxyMap := ds.proxyRegistry.ListConnectedProxies()
+	proxies := make([]*envoy.Proxy, 0, len(proxyMap))
+	for _, proxy := range proxyMap {
+		proxies = append(proxies, proxy)
 	}
 
-	sort.Strings(commonNames)
+	sort.Slice(proxies, func(i, j int) bool {
+		return proxies[i].Identity.String() < proxies[j].Identity.String()
+	})
 
-	_, _ = fmt.Fprintf(w, "<h1>%s Proxies (%d):</h1>", category, len(proxies))
+	_, _ = fmt.Fprintf(w, "<h1>Connected Proxies (%d):</h1>", len(proxies))
 	_, _ = fmt.Fprint(w, `<table>`)
-	_, _ = fmt.Fprint(w, "<tr><td>#</td><td>Envoy's certificate CN</td><td>Connected At</td><td>How long ago</td><td>tools</td></tr>")
-	for idx, cn := range commonNames {
-		ts := proxies[certificate.CommonName(cn)]
-		_, _ = fmt.Fprintf(w, `<tr><td>%d:</td><td>%s</td><td>%+v</td><td>(%+v ago)</td><td><a href="/debug/proxy?%s=%s">certs</a></td><td><a href="/debug/proxy?%s=%s">cfg</a></td></tr>`,
-			idx, cn, ts, time.Since(ts), specificProxyQueryKey, cn, proxyConfigQueryKey, cn)
+	_, _ = fmt.Fprint(w, "<tr><td>#</td><td>Envoy's Service Identity</td><td>Envoy's UUID</td><td>Connected At</td><td>How long ago</td><td>tools</td></tr>")
+	for idx, proxy := range proxies {
+		ts := proxy.GetConnectedAt()
+		proxyURL := fmt.Sprintf("/debug/proxy?%s=%s", uuidQueryKey, proxy.UUID)
+		configDumpURL := fmt.Sprintf("%s&%s=%t", proxyURL, proxyConfigQueryKey, true)
+		_, _ = fmt.Fprintf(w, `<tr><td>%d:</td><td>%s</td><td>%+v</td><td>(%+v ago)</td><td><a href="%s">certs</a></td><td><a href="%s">cfg</a></td></tr>`,
+			idx+1, proxy.Identity, ts, time.Since(ts), proxyURL, configDumpURL)
 	}
 	_, _ = fmt.Fprint(w, `</table>`)
 }
 
-func (ds DebugConfig) getConfigDump(cn certificate.CommonName, w http.ResponseWriter) {
-	pod, err := envoy.GetPodFromCertificate(cn, ds.kubeController)
+func (ds DebugConfig) getConfigDump(uuid string, w http.ResponseWriter) {
+	proxy, ok := ds.proxyRegistry.GetConnectedProxy(uuid)
+	if !ok {
+		msg := fmt.Sprintf("Proxy for UUID %s not found, may have been disconnected", uuid)
+		log.Error().Msg(msg)
+		http.Error(w, msg, http.StatusNotFound)
+		return
+	}
+	pod, err := envoy.GetPodFromCertificate(proxy.GetCertificateCommonName(), ds.kubeController)
 	if err != nil {
-		log.Error().Err(err).Msgf("Error getting Pod from certificate with CN=%s", cn)
+		msg := fmt.Sprintf("Error getting Pod from certificate with CN=%s", proxy.GetCertificateCommonName())
+		log.Error().Err(err).Msg(msg)
+		http.Error(w, msg, http.StatusNotFound)
+		return
 	}
 	w.Header().Set("Content-Type", "application/json")
 	envoyConfig := ds.getEnvoyConfig(pod, "config_dump")
 	_, _ = fmt.Fprintf(w, "%s", envoyConfig)
 }
 
-func (ds DebugConfig) getProxy(cn certificate.CommonName, w http.ResponseWriter) {
-	pod, err := envoy.GetPodFromCertificate(cn, ds.kubeController)
+func (ds DebugConfig) getProxy(uuid string, w http.ResponseWriter) {
+	proxy, ok := ds.proxyRegistry.GetConnectedProxy(uuid)
+	if !ok {
+		msg := fmt.Sprintf("Proxy for UUID %s not found, may have been disconnected", uuid)
+		log.Error().Msg(msg)
+		http.Error(w, msg, http.StatusNotFound)
+		return
+	}
+	pod, err := envoy.GetPodFromCertificate(proxy.GetCertificateCommonName(), ds.kubeController)
 	if err != nil {
-		log.Error().Err(err).Msgf("Error getting Pod from certificate with CN=%s", cn)
+		msg := fmt.Sprintf("Error getting Pod from certificate with CN=%s", proxy.GetCertificateCommonName())
+		log.Error().Err(err).Msg(msg)
+		http.Error(w, msg, http.StatusNotFound)
+		return
 	}
 	w.Header().Set("Content-Type", "application/json")
 	envoyConfig := ds.getEnvoyConfig(pod, "certs")

pkg/envoy/ads/stream.go

@@ -33,7 +33,7 @@ func (s *Server) StreamAggregatedResources(server xds_discovery.AggregatedDiscov
 	}
 
 	// If maxDataPlaneConnections is enabled i.e. not 0, then check that the number of Envoy connections is less than maxDataPlaneConnections
-	if s.cfg.GetMaxDataPlaneConnections() != 0 && s.proxyRegistry.GetConnectedProxyCount() >= s.cfg.GetMaxDataPlaneConnections() {
+	if s.cfg.GetMaxDataPlaneConnections() > 0 && s.proxyRegistry.GetConnectedProxyCount() >= s.cfg.GetMaxDataPlaneConnections() {
 		metricsstore.DefaultMetricsStore.ProxyMaxConnectionsRejected.Inc()
 		return errTooManyConnections
 	}

pkg/envoy/registry/announcement_handlers.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/openservicemesh/osm/pkg/announcements"
 	"github.com/openservicemesh/osm/pkg/constants"
+	"github.com/openservicemesh/osm/pkg/envoy"
 	"github.com/openservicemesh/osm/pkg/k8s/events"
 )
 
@@ -35,8 +36,8 @@ func (pr *ProxyRegistry) ReleaseCertificateHandler(certManager certificateReleas
 
 			proxyUUID := deletedPodObj.Labels[constants.EnvoyUniqueIDLabelName]
 			if proxyIface, ok := pr.connectedProxies.Load(proxyUUID); ok {
-				connectedProxy := proxyIface.(connectedProxy)
-				cn := connectedProxy.proxy.GetCertificateCommonName()
+				proxy := proxyIface.(*envoy.Proxy)
+				cn := proxy.GetCertificateCommonName()
 				log.Warn().Msgf("Pod with label %s: %s found in proxy registry; releasing certificate %s", constants.EnvoyUniqueIDLabelName, proxyUUID, cn)
 				certManager.ReleaseCertificate(cn)
 			} else {

pkg/envoy/registry/debugger.go

@@ -1,8 +1,6 @@
 package registry
 
 import (
-	"time"
-
 	"github.com/openservicemesh/osm/pkg/envoy"
 	"github.com/openservicemesh/osm/pkg/messaging"
 )
@@ -15,22 +13,40 @@ func NewProxyRegistry(mapper ProxyServiceMapper, msgBroker *messaging.Broker) *P
 	}
 }
 
-// RegisterProxy implements MeshCatalog and registers a newly connected proxy.
+// RegisterProxy registers a newly connected proxy.
 func (pr *ProxyRegistry) RegisterProxy(proxy *envoy.Proxy) {
-	pr.connectedProxies.Store(proxy.UUID.String(), connectedProxy{
-		proxy:       proxy,
-		connectedAt: time.Now(),
-	})
+	pr.connectedProxies.Store(proxy.UUID.String(), proxy)
 	log.Debug().Str("proxy", proxy.String()).Msg("Registered new proxy")
 }
 
+// GetConnectedProxy loads a connected proxy from the registry.
+func (pr *ProxyRegistry) GetConnectedProxy(uuid string) (*envoy.Proxy, bool) {
+	p, ok := pr.connectedProxies.Load(uuid)
+	if !ok {
+		return nil, false
+	}
+	return p.(*envoy.Proxy), true
+}
+
 // UnregisterProxy unregisters the given proxy from the catalog.
 func (pr *ProxyRegistry) UnregisterProxy(p *envoy.Proxy) {
 	pr.connectedProxies.Delete(p.UUID.String())
 	log.Debug().Msgf("Unregistered proxy %s", p.String())
 }
 
 // GetConnectedProxyCount counts the number of connected proxies
+// TODO(steeling): switch to a regular map with mutex so we can get the count without iterating the entire list.
 func (pr *ProxyRegistry) GetConnectedProxyCount() int {
 	return len(pr.ListConnectedProxies())
 }
+
+// ListConnectedProxies lists the Envoy proxies already connected and the time they first connected.
+func (pr *ProxyRegistry) ListConnectedProxies() map[string]*envoy.Proxy {
+	proxies := make(map[string]*envoy.Proxy)
+	pr.connectedProxies.Range(func(keyIface, propsIface interface{}) bool {
+		uuid := keyIface.(string)
+		proxies[uuid] = propsIface.(*envoy.Proxy)
+		return true // continue the iteration
+	})
+	return proxies
+}

pkg/envoy/registry/registry.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"github.com/google/uuid"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
@@ -13,7 +14,8 @@ import (
 
 var _ = Describe("Test catalog proxy register/unregister", func() {
 	proxyRegistry := NewProxyRegistry(nil, nil)
-	certCommonName := certificate.CommonName(fmt.Sprintf("%s.sidecar.foo.bar", uuid.New()))
+	uuid := uuid.New()
+	certCommonName := certificate.CommonName(fmt.Sprintf("%s.sidecar.foo.bar", uuid))
 	certSerialNumber := certificate.SerialNumber("123456")
 	proxy, err := envoy.NewProxy(certCommonName, certSerialNumber, nil)
 

pkg/envoy/registry/debugger_test.go pkg/envoy/registry/registry_test.go

@@ -2,10 +2,8 @@ package registry
 
 import (
 	"sync"
-	"time"
 
 	"github.com/openservicemesh/osm/pkg/certificate"
-	"github.com/openservicemesh/osm/pkg/envoy"
 	"github.com/openservicemesh/osm/pkg/logger"
 	"github.com/openservicemesh/osm/pkg/messaging"
 )
@@ -22,14 +20,6 @@ type ProxyRegistry struct {
 	msgBroker *messaging.Broker
 }
 
-type connectedProxy struct {
-	// Proxy which connected to the XDS control plane
-	proxy *envoy.Proxy
-
-	// When the proxy connected to the XDS control plane
-	connectedAt time.Time
-}
-
 // A simple interface to release certificates. Created to abstract the certificate.Manager struct for testing purposes.
 type certificateReleaser interface {
 	ReleaseCertificate(cn certificate.CommonName)