Skip to content

Commit

Permalink
Improve HTTP method label handling in prometheus metrics
Browse files Browse the repository at this point in the history
## What this PR does / why we need it:

This PR addresses kubernetes#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.
  • Loading branch information
jacekn committed Jan 20, 2025
1 parent 5b142ed commit 61e6992
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 0 deletions.
17 changes: 17 additions & 0 deletions internal/ingress/metric/collectors/socket.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"io"
"net"
"os"
"slices"
"strings"
"syscall"

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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{
Expand Down
31 changes: 31 additions & 0 deletions internal/ingress/metric/collectors/socket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 61e6992

Please sign in to comment.