LOG-6860: Support Splunk Metadata keys in ClusterLogForwarder #2999
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vparfonov: This pull request references LOG-6860 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set. 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. |
|
/lgtm |
|
New changes are detected. LGTM label has been removed. |
|
[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 |
Signed-off-by: Vitalii Parfonov <[email protected]>
| @@ -338,6 +338,15 @@ type ClusterLogForwarderList struct { | |||
| Items []ClusterLogForwarder `json:"items"` | |||
| } | |||
|
|
|||
| // FieldPath represents a path to find a value for a given field. The format must a value that can be converted to a | |||
There was a problem hiding this comment.
...The format must be a value...
api/observability/v1/output_types.go
Outdated
| @@ -1045,6 +1045,49 @@ type Splunk struct { | |||
| // +kubebuilder:validation:Pattern:=`^(([a-zA-Z0-9-_.\/])*(\{(\.[a-zA-Z0-9_]+|\."[^"]+")+((\|\|)(\.[a-zA-Z0-9_]+|\.?"[^"]+")+)*\|\|"[^"]*"\})*)*$` | |||
| // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Index",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"} | |||
| Index string `json:"index,omitempty"` | |||
|
|
|||
| //Fields to be added to Splunk index | |||
There was a problem hiding this comment.
IndexedFields This should match the field name per golang guidelines.
IndexedFields are the list of fields to be indexed by Splunk. //Please add a short comment matching "splunks" warning about using this feature. Maybe we should also add a note about all fields are converted to a string
| @@ -80,3 +85,71 @@ func SecretFrom(secretKey *v1.SecretReference) string { | |||
| } | |||
| return "" | |||
| } | |||
|
|
|||
| // generateQuotedPathSegmentArrayStr generates the final string of the array of array of path segments | |||
| payload := string(spec.Splunk.PayloadKey) | ||
| for _, v := range spec.Splunk.IndexedFields { | ||
| if !strings.HasPrefix(string(v), payload) { | ||
| results = append(results, fmt.Sprintf("Indexed field: %s not part of payload: %s", v, payload)) |
There was a problem hiding this comment.
are we trying to restrict they payload field from being part of indexed fields? If so then the error should read that it can not or is not allowed
There was a problem hiding this comment.
Indexed fields can't be outside of a given payload
| var _ = Describe("validating Splunk", func() { | ||
| Context("#ValidateSplunkMetadata", func() { | ||
|
|
||
| It("should fail validation if meet indexed field not in payload", func() { |
There was a problem hiding this comment.
If i read this validation correctly, the "event" is always the entire vector log event (e.g. .)? And you can only index fields that are part of what gets added to .? The splunk docs would suggest otherwise but I guess vector's encoding does not allow defining the "event" content separate from "indexed field values"? Maybe the "indexed" fields at the root level would need to be added to the "exclude" fields?
There was a problem hiding this comment.
This checks not related to the Splunk requirements directly, more about our procedure how we remap/calculate fields. If indexed fields will be outside the given payload, we can't remap it correctly. So this warning to check given settings. I think adding "indexed" fields to exclude filter not make sens, we have .log_source and .log_type these should be enough.
| if err != nil { | ||
| return "", err | ||
| } | ||
| println("\n\n\n\n\n\n\n") |
There was a problem hiding this comment.
should these be log statements?
|
|
||
| //checking Splunk index fields with 'index=${index} | stats count by ${field_name}' | ||
| //not direct checking to be sure fields sent as indexed | ||
| //if filed is indexed will return counts 2*logs_count or logs_count otherwise |
| Entry("should send audit logs with default source ", "", "", obs.InputTypeAudit), | ||
| Entry("should send journal logs with default source ", "", "", obs.InputTypeInfrastructure)) | ||
|
|
||
| DescribeTable("with user defined payloadKey", func(payloadKey, expSourceType string, expression *regexp.Regexp) { |
There was a problem hiding this comment.
Please add a test for the audit log defined in the feature request and see how closely we can get to matching the request. I don't believe we wish to test indexing every annotation but I'm interested to see how we populate "event" with the audit event message and maybe index a few other fields
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Signed-off-by: Vitalii Parfonov <[email protected]>
Signed-off-by: Vitalii Parfonov <[email protected]>
|
/test functional-target |
1 similar comment
|
/test functional-target |
Signed-off-by: Vitalii Parfonov <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Signed-off-by: Vitalii Parfonov <[email protected]>
|
/test functional-target |
Signed-off-by: Vitalii Parfonov <[email protected]>
|
/test functional-target |
1 similar comment
|
/test functional-target |
Signed-off-by: Vitalii Parfonov <[email protected]>
|
/test functional-target |
4 similar comments
|
/test functional-target |
|
/test functional-target |
|
/test functional-target |
|
/test functional-target |
|
@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 addressed to extend the
ClusterLogForwarderto supportSplunkmetadata fieldswhen forwarding logsso that we enable better integration with
Splunk./cc @Clee2691 @cahartma
/assign @jcantrill
/cherry-pick
Links