feat: support TCP responses that do not contain CR or LF markers - #1602
feat: support TCP responses that do not contain CR or LF markers#1602mkl262 wants to merge 1 commit into
Conversation
b28e0bf to
02c251a
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the TCP probe query/response handling so regex-based expectations can succeed even when the target protocol response doesn’t include CR/LF line terminators (e.g., banner-style responses).
Changes:
- Replaces
bufio.Scanner-based line scanning with abufio.Readerand a newreadUntilRegexpMatchhelper to support matching without newline delimiters. - Updates TCP query-response logic to expand/send based on the matched bytes returned by the new reader-based implementation.
- Adds targeted unit tests for
readUntilRegexpMatchand an integration-style TCP probe test covering “no newline” banners.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| prober/query_response.go | Introduces readUntilRegexpMatch and switches query/response reading to a buffered reader to support non-newline-terminated responses. |
| prober/query_response_test.go | Adds unit tests validating the new regexp-reading helper for newline and non-newline inputs. |
| prober/tcp_test.go | Adds a TCP probe test ensuring regex matching works when the server banner has no line terminator. |
Comments suppressed due to low confidence (1)
prober/query_response.go:195
- Using bufio.Reader.Read here can return fewer than len(expectBytes) bytes without an error (especially if some bytes are already buffered), which makes ExpectBytes matching flaky/incorrect. Use io.ReadFull (or io.ReadAtLeast) to reliably read the exact number of bytes before comparing.
// Try to read same number of bytes as expected.
data := make([]byte, len(expectBytes))
n, err := reader.Read(data)
if err != nil {
logger.Error("Error reading from connection", "err", err)
return false
}
logger.Debug("Read bytes", "bytes", data)
if n < len(expectBytes) {
logger.Error("Read less data than expected", "expected", expectBytes, "bytes", data)
return false
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@bwplotka Are you ok with the changes copilot recommended? and are there any other changes you want me to make? |
|
Yes, those looks correct |
d9f4afa to
1a93e52
Compare
|
I've committed the changes. |
|
@bwplotka Are there any further changes that you think are needed? |
if a TCP response is matched by regex continue to next step, dont wait for end-of-line markers Add basic test to query_response.go Signed-off-by: Michael Kanchuker <michaelkanchuker@gmail.com>
|
I believe this is a breaking change without a configuration to enable this option. For example with SMTP some servers split 220 greeting lines into multiple packets as an anti-spam measure, with this change the probe may match incorrectly. |
dgl
left a comment
There was a problem hiding this comment.
This just can't work as written. One way to make this work is for each QueryResponse have a flag that says whether this is binary or line based.
Using regexps to parse binary data is very https://xkcd.com/1313/ -- the main problem is a regexp can be greedy, but you don't have a delimiter here, so you don't know where the binary data ends. There's not an issue associated with this, not sure if discussions happened elsewhere; one way to handle this would be to only allow a binary regexp match as the final item in query_responses, but given the lack of an issue I'm not sure if that works for the use case here.
| if len(line) > bufio.MaxScanTokenSize { | ||
| return line, nil, fmt.Errorf("read line exceeds %d bytes", bufio.MaxScanTokenSize) | ||
| } | ||
| data = data[idx+1:] |
There was a problem hiding this comment.
More fundamentally this is throwing away data if a read returns more than one line.
What this PR does / Which issue(s) does the PR fix:
if a TCP response is matched by regex continue to next step, dont wait for end-of-line markers
Add basic test to query_response.go
Does this PR introduce a user-facing change?
Checklist
release-notessection of PR Desc.