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-3527: Collector to act as rsyslog server #2235

Merged
merged 1 commit into from Nov 30, 2023
Merged

LOG-3527: Collector to act as rsyslog server #2235

merged 1 commit into from Nov 30, 2023

Conversation

jlarriba
Copy link
Contributor

@jlarriba jlarriba commented Nov 7, 2023 •
edited

The included modifications generate an rsyslog server configuration for Collector. This configuration does not currently apply any transformations and send the logs succesfully to Loki, which stores them.

It also applies the OutputTypeSpec to receivers so they can share common parameters between the receivers, defining a ReceiverTypeSpec.

/cc @syedriko @alanconway
/assign @jcantrill

Links
JIRA: https://issues.redhat.com/browse/LOG-3527

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 7, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 7, 2023 •
edited by openshift-ci bot

@jlarriba: This pull request references LOG-3527 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 epic to target either version "4.8." or "openshift-4.8.", but it targets "Logging 5.9.0" instead.

In response to this:

The included modifications generate an rsyslog server configuration for Collector. This configuration does not currently apply any transformations and send the logs succesfully to Loki, which stores them.

It also applies the OutputTypeSpec to receivers so they can share common parameters between the receivers, defining a ReceiverTypeSpec.

/cc syedriko, aconway
/assign jcantrill

Links
JIRA: https://issues.redhat.com/browse/LOG-3527

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.

@jlarriba jlarriba mentioned this pull request Nov 7, 2023
Closed
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 7, 2023 •
edited by openshift-ci bot

@jlarriba: This pull request references LOG-3527 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 epic to target either version "4.8." or "openshift-4.8.", but it targets "Logging 5.9.0" instead.

In response to this:

The included modifications generate an rsyslog server configuration for Collector. This configuration does not currently apply any transformations and send the logs succesfully to Loki, which stores them.

It also applies the OutputTypeSpec to receivers so they can share common parameters between the receivers, defining a ReceiverTypeSpec.

/cc @syedriko @alanconway
/assign @jcantrill

Links
JIRA: https://issues.redhat.com/browse/LOG-3527

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.

@jlarriba
Copy link
Contributor Author

jlarriba commented Nov 7, 2023

/retest

@jlarriba jlarriba force-pushed the syslog_infra branch 2 times, most recently from 731f941 to 667e539 Compare November 8, 2023 11:22
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2023
@syedriko
Copy link
Contributor

@jlarriba please squash and rebase

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2023
@jlarriba
Copy link
Contributor Author

@syedriko squashed and rebased

jcantrill
jcantrill reviewed Nov 13, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
internal/collector/collector.go Outdated Show resolved Hide resolved
internal/collector/collector.go Outdated Show resolved Hide resolved
apis/logging/v1/input_receiver_types.go Show resolved Hide resolved
apis/logging/v1/input_receiver_types.go Show resolved Hide resolved
internal/generator/vector/input/inputs.go Outdated Show resolved Hide resolved
internal/generator/vector/output/loki/loki.go Outdated Show resolved Hide resolved
internal/logstore/lokistack/logstore_lokistack.go Outdated Show resolved Hide resolved
internal/validations/clusterlogforwarder/validate_clusterlogforwarderspec.go Outdated Show resolved Hide resolved
alanconway
alanconway suggested changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

Very solid PR. API change requested is to move the location of "port". Sgreed with some minor changes suggested b y @jcantrill. The open question is proper setting of log_type/tenant which I think we need to hash out some more.

apis/logging/v1/input_receiver_types.go Show resolved Hide resolved
apis/logging/v1/input_receiver_types.go Outdated Show resolved Hide resolved
apis/logging/v1/input_receiver_types.go Show resolved Hide resolved
internal/collector/collector.go Outdated Show resolved Hide resolved
internal/logstore/lokistack/logstore_lokistack.go Outdated Show resolved Hide resolved
@jcantrill
Copy link
Contributor

/hold

@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 Nov 13, 2023
@jlarriba jlarriba force-pushed the syslog_infra branch 2 times, most recently from 98b1830 to 1d5d424 Compare November 14, 2023 11:22
@jlarriba
Copy link
Contributor Author

/retest

@jlarriba jlarriba force-pushed the syslog_infra branch 2 times, most recently from 4ed00d8 to 7b0725c Compare November 28, 2023 11:55
@jlarriba
Copy link
Contributor Author

/retest

2 similar comments
@jlarriba
Copy link
Contributor Author

/retest

@jlarriba
Copy link
Contributor Author

/retest

jcantrill
jcantrill reviewed Nov 28, 2023
View reviewed changes
internal/generator/vector/conf/pipelines.go Outdated Show resolved Hide resolved
internal/generator/vector/output/loki/loki.go Outdated Show resolved Hide resolved
internal/generator/vector/source/syslog_receiver.go Outdated Show resolved Hide resolved
internal/network/service.go Outdated Show resolved Hide resolved
internal/validations/clusterlogforwarder/validate_clusterlogforwarder_service_account_test.go Show resolved Hide resolved
internal/validations/clusterlogforwarder/validate_clusterlogforwarderspec.go
case isHTTPReceiver(input) && input.Receiver.HTTP.Format != loggingv1.FormatKubeAPIAudit:
badInput("invalid format specified for HTTP receiver")
case isHTTPReceiver(input) && !validPort(input.Receiver.HTTP.Port):
case input.Receiver != nil && input.Receiver.ReceiverTypeSpec == nil:
Copy link
Contributor

Choose a reason for hiding this comment

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

consider moving these to a separate file which is probably long overdue for this set of validations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of moving them into a separate file, I moved them to a separate func in the same file: validateReceivers, which will get validated just after Inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been trying nad this implies a lot of changes. Could we consider this validation re-org as candidate for its own PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

This entire validation should be refactored. I was thinking the 'receiver' validations could be moved out, but if the work is too invasive, please ignore this comment.

internal/validations/clusterlogforwarder/validate_clusterlogforwarderspec_test.go Outdated Show resolved Hide resolved
@jlarriba jlarriba force-pushed the syslog_infra branch 4 times, most recently from 34a5485 to 77428cf Compare November 29, 2023 14:54
@jcantrill
Copy link
Contributor

/approve

@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 Nov 29, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, jlarriba

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2023
@jlarriba
Copy link
Contributor Author

/retest

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 30, 2023

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

@cahartma
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 9270994 into openshift:master Nov 30, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcantrill jcantrill jcantrill left review comments

@syedriko syedriko syedriko left review comments

@alanconway alanconway alanconway requested changes

@cahartma cahartma Awaiting requested review from cahartma

@vimalk78 vimalk78 Awaiting requested review from vimalk78

Assignees

@jcantrill jcantrill

@cahartma cahartma

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. release/5.9
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jlarriba @openshift-ci-robot @syedriko @jcantrill @cahartma @alanconway @openshift-merge-robot