LOG-6863: safety parsing/read fields to avoid messing data in log event #3031
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vparfonov: This pull request references LOG-6863 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vparfonov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@vparfonov: This pull request references LOG-6863 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test all |
Makefile
Outdated
| RELATED_IMAGE_VECTOR=$(IMAGE_LOGGING_VECTOR) \ | ||
| RELATED_IMAGE_LOG_FILE_METRIC_EXPORTER=$(IMAGE_LOGFILEMETRICEXPORTER) \ | ||
| go test -race \ | ||
| ./test/functional/... \ | ||
| ./test/functional/outputs/syslog/... \ |
There was a problem hiding this comment.
This means we will only test this package and none of the others
There was a problem hiding this comment.
I add this for fast testing
Makefile
Outdated
| @@ -216,11 +216,11 @@ test-env: ## Echo test environment, useful for running tests outside of the Make | |||
| RELATED_IMAGE_LOG_FILE_METRIC_EXPORTER=$(IMAGE_LOGFILEMETRICEXPORTER) \ | |||
|
|
|||
| .PHONY: test-functional | |||
| test-functional: test-functional-benchmarker-vector | |||
| test-functional: | |||
There was a problem hiding this comment.
removing this dependency means we never compile the dev tool
| if s.Facility == "" { | ||
| s.Facility = `{._internal.syslog.facility || "user"}` | ||
| appendField := func(fieldName string, value *string, defaultVal string) { | ||
| if *value == "" { |
There was a problem hiding this comment.
Is it possible for "value" to be nil since its a pointer? Do we need to ensure it's non-nil? why do we need to pass a pointer string?
There was a problem hiding this comment.
The pointer we use to set the field in struct if need
| @@ -22,6 +20,15 @@ if .log_type == "audit" { | |||
| ._internal.syslog.severity = "informational" | |||
| ._internal.syslog.facility = "security" | |||
| } | |||
| _tmp, err = parse_json(string!(.message)) | |||
There was a problem hiding this comment.
I missing the intent of this section:
- parse message to temp
- error we set temp to the log event
- non error we set temp to the merge of the message and the log event
- set field parsed_msg
we do not appear to do anything with the parsed_msg so we do any of this?
There was a problem hiding this comment.
yes you are right here in this case it will not be used because will use default value, need to improve this part
There was a problem hiding this comment.
There was a problem hiding this comment.
By the way we can remove default calculation if value configured
Signed-off-by: Vitalii Parfonov <[email protected]>
Signed-off-by: Vitalii Parfonov <[email protected]>
|
/test all |
|
/retest |
|
@vparfonov: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
This PR addresses an issue where data in a Syslog log event could become corrupted after a
mergeoperation. Such behavior can lead to unpredictable and hard-to-diagnose issues.The fix involves
parsingandmergingdata into a temporary value, from which the necessary information is extracted. This ensures that the original log event remains structured and valid./cc
/assign
Links