Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LOG-2988: Implement the output API to support sending logs to the Splunk #1605

Merged
merged 1 commit into from Oct 28, 2022
Merged

LOG-2988: Implement the output API to support sending logs to the Splunk #1605

merged 1 commit into from Oct 28, 2022

Conversation

vparfonov
Copy link
Contributor

@vparfonov vparfonov commented Aug 22, 2022 •
edited

Signed-off-by: Vitalii Parfonov vparfono@redhat.com

Description

Add support for sending logs to Spunk (HTTP Event Collector) (https://docs.splunk.com/Documentation/Splunk/latest/Data/FormateventsforHTTPEventCollector)

  • generator configuration for Vector Splunk sink
  • new output type splunk in the Cluster Logging Forwarder API
  • e2e test

Depends on PR ViaQ/vector#109 which will enable Splunk sink feature in Vector

/cc @vimalk78
/assign @jcantrill

/cherry-pick

Links

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 22, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2022
@jcantrill
Copy link
Contributor

/test all

jcantrill
jcantrill reviewed Aug 26, 2022
View reviewed changes
Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/hold

apis/logging/v1/output_types.go Outdated Show resolved Hide resolved
apis/logging/v1/output_types.go Outdated Show resolved Hide resolved
apis/logging/v1/output_types.go Outdated Show resolved Hide resolved
hack/cr-vector-unmanaged.yaml Outdated Show resolved Hide resolved
internal/generator/vector/output/splunk/splunk.go Outdated Show resolved Hide resolved
internal/generator/vector/output/splunk/splunk.go Outdated
if !hasTLS {
return []Element{}
}
} else if secret != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

dont think this is required either

Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to centralize this TLS logic somewhere.
@xperimental did you make any progress on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look like a lot of duplication of TLS logic code, i did similar to Kafka config

Copy link
Contributor

Choose a reason for hiding this comment

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

did you make any progress on that?

Sadly, no. The plan was to move the logic into the security package, so that outputs using the standard TLS options can benefit from de-duplication.

The kafka output is not a good example, because it does not actually use the standard TLS configuration of vector. I did not notice that in the original implementation, but my fix (#1591) is still not merged. The splink_hec output does seem to use the standard TLS options, so you could start consolidating the TLS options in for example Elasticsearch and Splunk to get to a reusable package.

Another change i did in the linked PR is to remove the hasTLS flag in the code and not make TLS dependent on the existence of a "secret" (passphrase, CA, certificate) but instead only on the used URL. I think this makes it more transparent to the user.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2022
@vparfonov vparfonov force-pushed the log2906 branch 2 times, most recently from 275e3cd to e461954 Compare August 31, 2022 16:37
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022
@openshift-ci openshift-ci bot added the midstream/Dockerfile A Dockerfile.in sync is needed with midstream label Aug 31, 2022
@vparfonov
Copy link
Contributor Author

/refresh

@vparfonov vparfonov changed the title LOG-2906: [spike] Investigate Vector log forwarding to Splunk WIP: LOG-2988: Investigate Vector log forwarding to Splunk Aug 31, 2022
@vparfonov
Copy link
Contributor Author

/test all

@vparfonov vparfonov changed the title WIP: LOG-2988: Investigate Vector log forwarding to Splunk LOG-2988: Investigate Vector log forwarding to Splunk Sep 1, 2022
@vparfonov vparfonov marked this pull request as ready for review September 1, 2022 12:21
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2022
@vparfonov
Copy link
Contributor Author

/test lint

3 similar comments
@vparfonov
Copy link
Contributor Author

/test lint

@vparfonov
Copy link
Contributor Author

/test lint

@vparfonov
Copy link
Contributor Author

/test lint

@vparfonov
Copy link
Contributor Author

/test e2e

2 similar comments
@vparfonov
Copy link
Contributor Author

/test e2e

@vparfonov
Copy link
Contributor Author

/test e2e

@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 8, 2022
@vparfonov
Copy link
Contributor Author

/test functional

4 similar comments
@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/retest-required

@vparfonov
Copy link
Contributor Author

/retest

3 similar comments
@jcantrill
Copy link
Contributor

/retest

@jcantrill
Copy link
Contributor

/retest

@jcantrill
Copy link
Contributor

/retest

jcantrill
jcantrill reviewed Oct 25, 2022
View reviewed changes
Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/hold
/retest

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2022
@vparfonov
LOG-2906: Add ability forward logs to Splunk output
14d7a7c 
Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
@jcantrill
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2022
jcantrill
jcantrill reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2022
@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/retest-required

@jcantrill
Copy link
Contributor

/retest

@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/retest-required

@jcantrill jcantrill mentioned this pull request Oct 27, 2022
Merged
@vparfonov
Copy link
Contributor Author

/test functional

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2022

@vparfonov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/functional 14d7a7c link true /test functional
ci/prow/e2e-ocp-next 14d7a7c link false /test e2e-ocp-next

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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 14d7a7c into openshift:master Oct 28, 2022
@vparfonov vparfonov deleted the log2906 branch October 28, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xperimental xperimental xperimental left review comments

@jcantrill jcantrill jcantrill left review comments

@alanconway alanconway alanconway requested changes

@cahartma cahartma Awaiting requested review from cahartma

Assignees

@jcantrill jcantrill

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. midstream/Dockerfile A Dockerfile.in sync is needed with midstream release/5.6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vparfonov @jcantrill @openshift-ci-robot @xperimental @alanconway @openshift-merge-robot