Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions pkg/systemlogmonitor/log_buffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package systemlogmonitor

import (
"fmt"
"regexp"
"regexp/syntax"
"slices"
Expand Down Expand Up @@ -74,23 +75,18 @@ func CompilePattern(expr string) (*Pattern, error) {
if _, err := regexp.Compile(expr); err != nil {
return nil, err

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything LGTM. Only one comment - I think with this change we should wrap the error:

fmt.Errorf("invalid pattern %q: %w", expr, err)

This is based on running a before/after with a string with \Q in it:

old  regexp.Compile(`error\Q` + `\z`)     → no err
new  regexp.Compile(`(?:error\Q)` + `\z`) → missing closing ): `(?:error\Q)\z`

The error message shows our internal (?:...)\z pattern which the user did not write.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will update. thanks for checking this.

}
anchored := expr + `\z`
// Group the expression so the end anchor applies to every top-level branch.
anchored := `(?:` + expr + `)\z`
reg, err := regexp.Compile(anchored)
if err != nil {
return nil, err
return nil, fmt.Errorf("invalid pattern %q: %w", expr, err)
}
p := &Pattern{regexp: reg}
tree, err := syntax.Parse(anchored, syntax.Perl)
if err != nil {
return p, nil
}
// A top-level alternation binds the appended anchor to its last branch only.
// Equal trees prove that the anchor covers every branch.
grouped, err := syntax.Parse(`(?:`+expr+`)\z`, syntax.Perl)
if err != nil {
return p, nil
}
p.lastLineOnly = tree.Equal(grouped) && isLastLineOnly(tree)
p.lastLineOnly = isLastLineOnly(tree)
return p, nil
}

Expand Down
29 changes: 15 additions & 14 deletions pkg/systemlogmonitor/log_buffer_equivalence_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ var equivalencePatterns = []string{
`alpha[\s\S]*beta`,
`alpha[\x00-\x7f]+beta`,
`\Aalpha`,
// Top-level alternations that the appended anchor does not bind.
// Top-level alternations must also stay on the last line.
`alpha|beta`,
`abort|abandon`,
`alpha|`,
Expand Down Expand Up @@ -111,7 +111,7 @@ func TestMatchEquivalence(t *testing.T) {
t.Fatalf("failed to compile %q: %v", expr, err)
}
patterns = append(patterns, p)
refRegexps = append(refRegexps, regexp.MustCompile(expr+`\z`))
refRegexps = append(refRegexps, regexp.MustCompile(`(?:`+expr+`)\z`))
}
for _, maxLines := range []int{1, 2, 3, 5, 10} {
for trial := range 200 {
Expand Down Expand Up @@ -167,12 +167,10 @@ func TestLastLineOnlyClassification(t *testing.T) {
`alpha[\n]beta`: false,
`alpha(beta|\n)`: false,
`alpha{1,3}[\t-\r]beta`: false,
// The appended anchor reaches the last branch of a top-level alternation only.
`alpha|beta`: false,
// The parser factors the shared prefix out, so the root stays a concatenation.
`abort|abandon`: false,
`a|`: false,
`(alpha|beta) gamma`: true,
`alpha|beta`: true,
`abort|abandon`: true,
`a|`: true,
`(alpha|beta) gamma`: true,
} {
p, err := CompilePattern(expr)
if err != nil {
Expand All @@ -194,9 +192,9 @@ func TestLastLineOnlyClassification(t *testing.T) {
}
}

// TestMatchAlternationSpansBuffer pins the reported repro for a top-level alternation.
// The first branch matches an older line, so the last line shortcut must not apply.
func TestMatchAlternationSpansBuffer(t *testing.T) {
// TestMatchAlternationAnchorsEveryBranch verifies that an earlier branch cannot
// match a stale buffered line.
func TestMatchAlternationAnchorsEveryBranch(t *testing.T) {
b := NewLogBuffer(2)
b.Push(&types.Log{Message: "kernel: oom-kill:constraint=CONSTRAINT_NONE"})
b.Push(&types.Log{Message: "kubelet: node ready"})
Expand All @@ -205,11 +203,14 @@ func TestMatchAlternationSpansBuffer(t *testing.T) {
if err != nil {
t.Fatalf("failed to compile %q: %v", expr, err)
}
want := referenceMatch(b, regexp.MustCompile(expr+`\z`))
if len(want) == 0 {
t.Fatalf("pattern %q: the reference matcher found nothing", expr)
if got := b.Match(p); len(got) != 0 {
t.Fatalf("pattern %q matched stale logs: %v", expr, messages(got))
}

last := &types.Log{Message: "kernel: Out of memory"}
b.Push(last)
got := b.Match(p)
want := []*types.Log{last}
if !reflect.DeepEqual(want, got) {
t.Errorf("pattern %q: want %v, got %v", expr, messages(want), messages(got))
}
Expand Down