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.
This commit is contained in:
parent
5b142ed7c4
commit
61e699267c
2 changed files with 48 additions and 0 deletions
|
@ -21,6 +21,7 @@ import (
|
||||||
"io"
|
"io"
|
||||||
"net"
|
"net"
|
||||||
"os"
|
"os"
|
||||||
|
"slices"
|
||||||
"strings"
|
"strings"
|
||||||
"syscall"
|
"syscall"
|
||||||
|
|
||||||
|
@ -98,6 +99,19 @@ var requestTags = []string{
|
||||||
"canary",
|
"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
|
// NewSocketCollector creates a new SocketCollector instance using
|
||||||
// the ingress watch namespace and class used by the controller
|
// 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) {
|
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 != "" {
|
if sc.reportStatusClasses && stats.Status != "" {
|
||||||
stats.Status = fmt.Sprintf("%cxx", stats.Status[0])
|
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
|
// Note these must match the order in requestTags at the top
|
||||||
requestLabels := prometheus.Labels{
|
requestLabels := prometheus.Labels{
|
||||||
|
|
|
@ -648,6 +648,37 @@ func TestCollector(t *testing.T) {
|
||||||
metrics: []string{"nginx_ingress_controller_requests"},
|
metrics: []string{"nginx_ingress_controller_requests"},
|
||||||
useStatusClasses: true,
|
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 {
|
for _, c := range cases {
|
||||||
|
|
Loading…
Reference in a new issue