From 61e699267c32d6913e1e4a022d0e7b5609a11a94 Mon Sep 17 00:00:00 2001
From: Jacek Nykis <jacek@stellar.org>
Date: Mon, 20 Jan 2025 22:45:22 +0000
Subject: [PATCH] Improve HTTP method label handling in prometheus metrics

## What this PR does / why we need it:

This PR addresses #10208 by checking whether the request method is valid.
For invalid methods we set `method="invalid_method"` label so that operators
can stil see traffic in the metrics but without unbound label value.

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which addresses an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] CVE Report (Scanner found CVE and adding report)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation only

## How Has This Been Tested?

I tested by building the image locally using `make build && make image`
and running the image in minikube.
The change has a fairly narrow scope so should be low risk.

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/main/CONTRIBUTING.md) guide
- [x] I have added unit and/or e2e tests to cover my changes.
- [x] All new and existing tests passed.
---
 internal/ingress/metric/collectors/socket.go  | 17 ++++++++++
 .../ingress/metric/collectors/socket_test.go  | 31 +++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/internal/ingress/metric/collectors/socket.go b/internal/ingress/metric/collectors/socket.go
index 0bdd816ae1..1feb785af7 100644
--- a/internal/ingress/metric/collectors/socket.go
+++ b/internal/ingress/metric/collectors/socket.go
@@ -21,6 +21,7 @@ import (
 	"io"
 	"net"
 	"os"
+	"slices"
 	"strings"
 	"syscall"
 
@@ -98,6 +99,19 @@ var requestTags = []string{
 	"canary",
 }
 
+var validHTTPMethods = []string{
+	// Unless otherwise noted, these are defined in RFC 7231 section 4.3.
+	"GET",
+	"HEAD",
+	"POST",
+	"PUT",
+	"PATCH", // RFC 5789
+	"DELETE",
+	"CONNECT",
+	"OPTIONS",
+	"TRACE",
+}
+
 // NewSocketCollector creates a new SocketCollector instance using
 // the ingress watch namespace and class used by the controller
 func NewSocketCollector(pod, namespace, class string, metricsPerHost, metricsPerUndefinedHost, reportStatusClasses bool, buckets HistogramBuckets, bucketFactor float64, maxBuckets uint32, excludeMetrics []string) (*SocketCollector, error) {
@@ -316,6 +330,9 @@ func (sc *SocketCollector) handleMessage(msg []byte) {
 		if sc.reportStatusClasses && stats.Status != "" {
 			stats.Status = fmt.Sprintf("%cxx", stats.Status[0])
 		}
+		if !slices.Contains(validHTTPMethods, stats.Method) {
+			stats.Method = "invalid_method"
+		}
 
 		// Note these must match the order in requestTags at the top
 		requestLabels := prometheus.Labels{
diff --git a/internal/ingress/metric/collectors/socket_test.go b/internal/ingress/metric/collectors/socket_test.go
index 3a1f29f35d..0d3e62ed26 100644
--- a/internal/ingress/metric/collectors/socket_test.go
+++ b/internal/ingress/metric/collectors/socket_test.go
@@ -648,6 +648,37 @@ func TestCollector(t *testing.T) {
 			metrics:          []string{"nginx_ingress_controller_requests"},
 			useStatusClasses: true,
 		},
+		{
+			name: "invalid http methods should not be set as label values",
+			data: []string{`[{
+				"host":"testshop.com",
+				"status":"200",
+				"bytesSent":150.0,
+				"method":"XYZGET",
+				"path":"/admin",
+				"requestLength":300.0,
+				"requestTime":60.0,
+				"upstreamLatency":1.0,
+				"upstreamHeaderTime":5.0,
+				"upstreamName":"test-upstream",
+				"upstreamIP":"1.1.1.1:8080",
+				"upstreamResponseTime":200,
+				"upstreamStatus":"220",
+				"namespace":"test-app-production",
+				"ingress":"web-yml",
+				"service":"test-app",
+				"canary":""
+			}]`},
+			metrics: []string{"nginx_ingress_controller_requests"},
+			wantBefore: `
+				# HELP nginx_ingress_controller_requests The total number of client requests
+				# TYPE nginx_ingress_controller_requests counter
+				nginx_ingress_controller_requests{canary="",controller_class="ingress",controller_namespace="default",controller_pod="pod",host="testshop.com",ingress="web-yml",method="invalid_method",namespace="test-app-production",path="/admin",service="test-app",status="200"} 1
+			`,
+			removeIngresses: []string{"test-app-production/web-yml"},
+			wantAfter: `
+			`,
+		},
 	}
 
 	for _, c := range cases {