kubeflow / kubeflow/katib

metrics-collector: file-metricscollector panics (nil regexp) when a user-provided filter is an invalid regular expression

Open
#2,707 1 comment 0 reactions 1 assignee Claimed by @AdeshDeshmukh View on GitHub
kind/bug lifecycle/needs-triage
Dominant language
Python
Stars
1.7k
Forks
542
PR merge metrics
No merged PRs in 30d

Description

### What happened?

/kind bug

`GetFilterRegexpList` in [`pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go`](https://github.com/kubeflow/katib/blob/master/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go) discards the error from `regexp.Compile`:

```go
func GetFilterRegexpList(filters []string) []*regexp.Regexp {
regexpList := make([]*regexp.Regexp, 0, len(filters))
if len(filters) == 0 {
filters = append(filters, common.DefaultFilter)
}
for _, filter := range filters {
reg, _ := regexp.Compile(filter) // error discarded; reg == nil for invalid patterns
regexpList = append(regexpList, reg)
}
return regexpList
}
```

If a user provides an invalid regex via `filter` in the `metricsCollectorSpec` (passed to the `file-metricscollector` sidecar's `-f` flag and split on `;` in `main.go`), every invalid pattern produces a `nil` `*regexp.Regexp` entry in the returned list.

There are two crash sites that dereference the `nil` regexp:
1. `parseLogsInTextFormat` ([`pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go`](https://github.com/kubeflow/katib/blob/master/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go)) — `metricReg.FindAllStringSubmatch(logline, -1)` on a `nil` regexp.
2. `watchMetricsFile` in [`cmd/metricscollector/v1beta1/file-metricscollector/main.go`](https://github.com/kubeflow/katib/blob/master/cmd/metricscollector/v1beta1/file-metricscollector/main.go) — same call inside the log-tail loop (where the filter list is also recompiled for every log line).

Either path panics the `metrics-collector` sidecar container, so no metrics are ever reported and the Trial ends up with unavailable objective metric values / gets stuck — with no hint that the user's filter was the problem, since the compile error was silently discarded.

**Proposed fix:**
Handle the compile error and fail fast with a descriptive message:
```go
reg, err := regexp.Compile(filter)
if err != nil {
return nil, fmt.Errorf("invalid metric filter %q: %v", filter, err)
}
regexpList = append(regexpList, reg)
```

This changes `GetFilterRegexpList` to also return an error, propagated through `parseLogsInTextFormat` (which already returns error) and handled in `main.go` with `klog.Fatalf` at startup (consistent with its existing error handling), compiling the filters once before the tail loop instead of per line. Happy to submit a PR.

### What did you expect to happen?

An invalid user-provided filter should fail fast at sidecar startup with a clear error identifying the bad pattern — not silently produce nil regexps that crash the collector and leave the Trial without metrics.

### Environment

Kubernetes version:
Any (bug is version-independent; code path exists on master)

Katib controller version:
master / latest (`pkg/metricscollector/v1beta1/file-metricscollector`)

Katib Python SDK version:
Not applicable — bug is in the Go file-metricscollector sidecar (`pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go`, `cmd/metricscollector/v1beta1/file-metricscollector/main.go`)

### Impacted by this bug?

Give it a 👍 We prioritize the issues with most 👍

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.