LOG-3527: Collector to act as rsyslog server #2221
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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 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 kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jlarriba 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 |
| key_file = "/etc/collector/{{.ID}}/tls.key" | ||
| crt_file = "/etc/collector/{{.ID}}/tls.crt" | ||
|
|
||
| [transforms.{{.ID}}_split] |
There was a problem hiding this comment.
these transforms are specific to k8s audit logs. I'd drop them first and then see what remaps if any are needed.
There was a problem hiding this comment.
Yes, I will be working precisely on this, I am ramping up with Vector's transform syntax
| // Port the Service and the HTTP listener listen on. | ||
| // +kubebuilder:default:=10514 | ||
| // +optional | ||
| Port int32 `json:"port"` |
There was a problem hiding this comment.
any reason to support UDP?
There was a problem hiding this comment.
i will add UDP support while merging the common options in a ReceiverTypeSpec
| @@ -197,12 +200,14 @@ func verifyInputs(spec *loggingv1.ClusterLogForwarderSpec, status *loggingv1.Clu | |||
| badInput("inputspec cannot have a negative limit threshold") | |||
| case input.Receiver != nil && !extras[constants.VectorName]: | |||
| badInput("ReceiverSpecs are only supported for the vector log collector") | |||
| case input.Receiver != nil && input.Receiver.HTTP == nil: | |||
| badInput("ReceiverSpec must define an HTTP receiver") | |||
| case input.Receiver != nil && input.Receiver.HTTP == nil && input.Receiver.Syslog == nil: | |||
There was a problem hiding this comment.
I don't think we want both HTTP and Syslog defined for a receiver.
There was a problem hiding this comment.
Why not? They are intended for different things and serve different purposes.
There was a problem hiding this comment.
I'll defer to @alanconway for the final word, but I think it would complicate both upstream - k8s service - as well as downstream - vector xforms - plumbing if we try to support multiple protocols, or even multiple payload formats, over the same receiver.
There can be many receivers in a CLF though.
There was a problem hiding this comment.
Ah true, for sure we do not want to have two protocols over the same receiver. For having two different receivers we should create two different inputs. Im going to fix this.
internal/validations/clusterlogforwarder/validate_clusterlogforwarderspec_test.go
Outdated
Show resolved
Hide resolved
| @@ -120,6 +120,41 @@ var _ = Describe("Validate clusterlogforwarderspec", func() { | |||
| checkReceiver(&loggingv1.ReceiverSpec{}, `ReceiverSpecs are only supported for the vector log collector`, map[string]bool{}) | |||
| }) | |||
|
|
|||
| It("should fail validation for invalid Syslog receiver specs", func() { | |||
There was a problem hiding this comment.
may be we can collapse this together with the HTTP receiver test.
There was a problem hiding this comment.
We might, but I think it is more readable if we keep them separate, just to save some "if"s in there.
| checkReceiver(&loggingv1.ReceiverSpec{}, `ReceiverSpec must define an Syslog receiver`, map[string]bool{constants.VectorName: true}) | ||
| checkReceiver(&loggingv1.ReceiverSpec{}, `ReceiverSpecs are only supported for the vector log collector`, map[string]bool{}) | ||
| }) | ||
|
|
There was a problem hiding this comment.
need a negative test here for both HTTP and Syslog receivers defined together.
There was a problem hiding this comment.
i think HTTP and Syslog defined together is not something we would like to prevent
There was a problem hiding this comment.
Right, we should add this test
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@jlarriba: The following test failed, say
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. |
|
This PR has been superseded by a new one: #2235 There, I remove the need for an external Loki tenant, add a ReceiverTypeSpec and address the review comments. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
Description
The included modifications generate an rsyslog server configuration for Collector. This configuration does not currently apply any transformations and expects an "external" tenant existing in Loki to which send the logs to.
/cc syedriko
/assign jcantrill
Links