From d9ae7d670e4352ff5246711e30accea4fa5090c2 Mon Sep 17 00:00:00 2001
From: Andrew Dunham <andrew@du.nham.ca>
Date: Tue, 12 Sep 2023 19:16:51 -0400
Subject: [PATCH] net/portmapper: add clientmetric for UPnP error codes

This should allow us to gather a bit more information about errors that
we encounter when creating UPnP mappings. Since we don't have a
"LabelMap" construction for clientmetrics, do what sockstats does and
lazily register a new metric when we see a new code.

Updates #9343

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: Ibb5aadd6138beb58721f98123debcc7273b611ba
---
 net/portmapper/portmapper.go | 20 ++++++++++++++++++++
 net/portmapper/upnp.go       | 24 ++++++++++++++----------
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go
index ee16bfe61..880d9d486 100644
--- a/net/portmapper/portmapper.go
+++ b/net/portmapper/portmapper.go
@@ -28,6 +28,7 @@ import (
 	"tailscale.com/types/logger"
 	"tailscale.com/types/nettype"
 	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/mak"
 )
 
 // DebugKnobs contains debug configuration that can be provided when creating a
@@ -1024,3 +1025,22 @@ var (
 	// we received a UPnP response with a new meta.
 	metricUPnPUpdatedMeta = clientmetric.NewCounter("portmap_upnp_updated_meta")
 )
+
+// UPnP error metric that's keyed by code; lazily registered on first read
+var (
+	metricUPnPErrorsByCodeMu sync.Mutex
+	metricUPnPErrorsByCode   map[int]*clientmetric.Metric
+)
+
+func getUPnPErrorsMetric(code int) *clientmetric.Metric {
+	metricUPnPErrorsByCodeMu.Lock()
+	defer metricUPnPErrorsByCodeMu.Unlock()
+	mm := metricUPnPErrorsByCode[code]
+	if mm != nil {
+		return mm
+	}
+
+	mm = clientmetric.NewCounter(fmt.Sprintf("portmap_upnp_errors_with_code_%d", code))
+	mak.Set(&metricUPnPErrorsByCode, code, mm)
+	return mm
+}
diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go
index 7350becae..31650a516 100644
--- a/net/portmapper/upnp.go
+++ b/net/portmapper/upnp.go
@@ -337,9 +337,14 @@ func (c *Client) getUPnPPortMapping(
 	// duration; see the following issue for details:
 	//    https://github.com/tailscale/tailscale/issues/9343
 	if err != nil {
+		code, ok := getUPnPErrorCode(err)
+		if ok {
+			getUPnPErrorsMetric(code).Add(1)
+		}
+
 		// From the UPnP spec: http://upnp.org/specs/gw/UPnP-gw-WANIPConnection-v2-Service.pdf
 		//     725: OnlyPermanentLeasesSupported
-		if isUPnPError(err, 725) {
+		if ok && code == 725 {
 			newPort, err = addAnyPortMapping(
 				ctx,
 				client,
@@ -387,13 +392,13 @@ func (c *Client) getUPnPPortMapping(
 	return upnp.external, true
 }
 
-// isUPnPError returns whether the provided error is a UPnP error response with
-// the given error code. It returns false if the error is not a SOAP error, or
-// the inner error details are not a UPnP error.
-func isUPnPError(err error, errCode int) bool {
+// getUPnPErrorCode returns the UPnP error code from the given response, if the
+// error is a SOAP error in the proper format, and a boolean indicating whether
+// the provided error was actually a UPnP error.
+func getUPnPErrorCode(err error) (int, bool) {
 	soapErr, ok := err.(*soap.SOAPFaultError)
 	if !ok {
-		return false
+		return 0, false
 	}
 
 	var upnpErr struct {
@@ -402,13 +407,12 @@ func isUPnPError(err error, errCode int) bool {
 		Description string `xml:"errorDescription"`
 	}
 	if err := xml.Unmarshal([]byte(soapErr.Detail.Raw), &upnpErr); err != nil {
-		return false
+		return 0, false
 	}
 	if upnpErr.XMLName.Local != "UPnPError" {
-		return false
+		return 0, false
 	}
-
-	return upnpErr.Code == errCode
+	return upnpErr.Code, true
 }
 
 type uPnPDiscoResponse struct {