Skip to content

Commit 61e6992

Browse files
committed
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.
1 parent 5b142ed commit 61e6992

File tree

2 files changed

+48
-0
lines changed

2 files changed

+48
-0
lines changed

internal/ingress/metric/collectors/socket.go

+17
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"io"
2222
"net"
2323
"os"
24+
"slices"
2425
"strings"
2526
"syscall"
2627

@@ -98,6 +99,19 @@ var requestTags = []string{
9899
"canary",
99100
}
100101

102+
var validHTTPMethods = []string{
103+
// Unless otherwise noted, these are defined in RFC 7231 section 4.3.
104+
"GET",
105+
"HEAD",
106+
"POST",
107+
"PUT",
108+
"PATCH", // RFC 5789
109+
"DELETE",
110+
"CONNECT",
111+
"OPTIONS",
112+
"TRACE",
113+
}
114+
101115
// NewSocketCollector creates a new SocketCollector instance using
102116
// the ingress watch namespace and class used by the controller
103117
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) {
316330
if sc.reportStatusClasses && stats.Status != "" {
317331
stats.Status = fmt.Sprintf("%cxx", stats.Status[0])
318332
}
333+
if !slices.Contains(validHTTPMethods, stats.Method) {
334+
stats.Method = "invalid_method"
335+
}
319336

320337
// Note these must match the order in requestTags at the top
321338
requestLabels := prometheus.Labels{

internal/ingress/metric/collectors/socket_test.go

+31
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,37 @@ func TestCollector(t *testing.T) {
648648
metrics: []string{"nginx_ingress_controller_requests"},
649649
useStatusClasses: true,
650650
},
651+
{
652+
name: "invalid http methods should not be set as label values",
653+
data: []string{`[{
654+
"host":"testshop.com",
655+
"status":"200",
656+
"bytesSent":150.0,
657+
"method":"XYZGET",
658+
"path":"/admin",
659+
"requestLength":300.0,
660+
"requestTime":60.0,
661+
"upstreamLatency":1.0,
662+
"upstreamHeaderTime":5.0,
663+
"upstreamName":"test-upstream",
664+
"upstreamIP":"1.1.1.1:8080",
665+
"upstreamResponseTime":200,
666+
"upstreamStatus":"220",
667+
"namespace":"test-app-production",
668+
"ingress":"web-yml",
669+
"service":"test-app",
670+
"canary":""
671+
}]`},
672+
metrics: []string{"nginx_ingress_controller_requests"},
673+
wantBefore: `
674+
# HELP nginx_ingress_controller_requests The total number of client requests
675+
# TYPE nginx_ingress_controller_requests counter
676+
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
677+
`,
678+
removeIngresses: []string{"test-app-production/web-yml"},
679+
wantAfter: `
680+
`,
681+
},
651682
}
652683

653684
for _, c := range cases {

0 commit comments

Comments
 (0)