pyrra-dev / pyrra

Making SLOs with Prometheus manageable, accessible, and easy to use for everyone!
https://demo.pyrra.dev
Apache License 2.0
1.26k stars 113 forks source link

Webhook Panic on Unexpected Parser Expression #1181

Closed squat closed 5 months ago

squat commented 6 months ago

If you're like me and tried mistakenly wrote a weird SLO like

spec:
  description: ""
  indicator:
    latency:
      success:
        metric: foo{le="1.0"} or vector(0)
                            # ^^^^^^^^^^^^
      total:
        metric: foo{le="1.0"}

The Kubernetes webhook API handler will panic with:

2024/05/22 11:25:56 http: panic serving 10.52.5.26:37498: interface conversion: parser.Expr is *parser.BinaryExpr, not *parser.VectorSelector
goroutine 576365 [running]:
net/http.(*conn).serve.func1()
    net/http/server.go:1854 +0xbf
panic({0x19859a0, 0xc0009bde00})
    runtime/panic.go:890 +0x263
github.com/pyrra-dev/pyrra/kubernetes/api/v1alpha1.(*ServiceLevelObjective).validate(0xc000896b60)
    github.com/pyrra-dev/pyrra/kubernetes/api/v1alpha1/servicelevelobjective_types.go:283 +0xfab
github.com/pyrra-dev/pyrra/kubernetes/api/v1alpha1.(*ServiceLevelObjective).ValidateUpdate(0xc00036af40?, {0xc0004e2000?, 0x781?})
    github.com/pyrra-dev/pyrra/kubernetes/api/v1alpha1/servicelevelobjective_types.go:191 +0x19
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*validatingHandler).Handle(_, {_, _}, {{{0xc000c6a210, 0x24}, {{0xc000951150, 0x9}, {0xc000951128, 0x8}, {0xc00012c168, ...}}, ...}})
    sigs.k8s.io/controller-runtime@v0.16.1/pkg/webhook/admission/validator.go:103 +0x6a9
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).Handle(_, {_, _}, {{{0xc000c6a210, 0x24}, {{0xc000951150, 0x9}, {0xc000951128, 0x8}, {0xc00012c168, ...}}, ...}})
    sigs.k8s.io/controller-runtime@v0.16.1/pkg/webhook/admission/webhook.go:169 +0x20a
sigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).ServeHTTP(0xc00003ceb0, {0x7fd32b84c0a0?, 0xc000466550}, 0xc0005c8300)
    sigs.k8s.io/controller-runtime@v0.16.1/pkg/webhook/admission/http.go:98 +0xc94
github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerInFlight.func1({0x7fd32b84c0a0, 0xc000466550}, 0x222c900?)
    github.com/prometheus/client_golang@v1.16.0/prometheus/promhttp/instrument_server.go:60 +0xd4
net/http.HandlerFunc.ServeHTTP(0x222c900?, {0x7fd32b84c0a0?, 0xc000466550?}, 0xc000355828?)
    net/http/server.go:2122 +0x2f
github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1({0x222c900?, 0xc00032c2a0?}, 0xc0005c8300)
    github.com/prometheus/client_golang@v1.16.0/prometheus/promhttp/instrument_server.go:147 +0xc5
net/http.HandlerFunc.ServeHTTP(0x6fbce5?, {0x222c900?, 0xc00032c2a0?}, 0x40dc2a?)
    net/http/server.go:2122 +0x2f
github.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerDuration.func2({0x222c900, 0xc00032c2a0}, 0xc0005c8300)
    github.com/prometheus/client_golang@v1.16.0/prometheus/promhttp/instrument_server.go:109 +0xc7
net/http.HandlerFunc.ServeHTTP(0xc00032c2a0?, {0x222c900?, 0xc00032c2a0?}, 0x1bbea7e?)
    net/http/server.go:2122 +0x2f
net/http.(*ServeMux).ServeHTTP(0xc0008937a8?, {0x222c900, 0xc00032c2a0}, 0xc0005c8300)
    net/http/server.go:2500 +0x149
net/http.serverHandler.ServeHTTP({0x221d810?}, {0x222c900, 0xc00032c2a0}, 0xc0005c8300)
    net/http/server.go:2936 +0x316
net/http.(*conn).serve(0xc000622240, {0x222d7b0, 0xc000663bf0})
    net/http/server.go:1995 +0x612
created by net/http.(*Server).Serve
    net/http/server.go:3089 +0x5ed
metalmatze commented 6 months ago

Opening again until we have a patch release.

metalmatze commented 6 months ago

By the way, Pyrra already adds the or vector(0) for the error matches in some rules and alert expressions. Therefore there shouldn't be a need to do this manually.

https://github.com/pyrra-dev/pyrra/blob/15624d30915a2dba766909fbebaa07b7b4c3bebb/slo/promql.go#L246 https://github.com/pyrra-dev/pyrra/blob/15624d30915a2dba766909fbebaa07b7b4c3bebb/slo/rules.go#L1173