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.

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

Logstash pipelines #6480

Merged
merged 164 commits into from
Apr 6, 2023
Merged

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Mar 3, 2023

This PR allows configuration of Logstash pipeline.yml in CRD. pipeline.yml is an array of map of string/interface{} defining multiple pipelines. . in between the key is treated as string.

- pipeline.id: stdin_stdout
  config.string: input { stdin {} } output { stdout{} }
- pipeline.id: demo
  pipeline.workers: 1
  path.config: "/usr/share/logstash/pipeline"

Pipelines can be set from either inline in Pipelines or secret referencing inPipelinesRef.

This PR includes tests

  • e2e TestPipelineConfigRefLogstash, TestPipelineConfigLogstash
  • unit test

robbavey and others added 30 commits February 15, 2023 13:26
Initial commit of a simple operator.

The first operator will create:
* A Service exposing the logstash metrics API, so it can be used for monitoring purposes
* Secrets holding logstash.yml
* A StatefulSet deploying the logstash pods
* Pods with default liveness and readiness probes

The sample logstash yml file as located in config/samples/logstash/logstash.yaml will
create:

```
✗ kubectl tree logstash logstash-sample
NAMESPACE  NAME                                                  READY  REASON  AGE
default    Logstash/logstash-sample                              -              4m5s
default    ├─Secret/logstash-sample-ls-config                    -              4m4s
default    ├─Service/logstash-sample-ls-http                     -              4m5s
default    │ └─EndpointSlice/logstash-sample-ls-http-kwfsg       -              4m5s
default    └─StatefulSet/logstash-sample-ls                      -              4m4s
default      ├─ControllerRevision/logstash-sample-ls-79bd59f869  -              4m4s
default      ├─Pod/logstash-sample-ls-0                          True           3m59s
default      ├─Pod/logstash-sample-ls-1                          True           3m59s
default      └─Pod/logstash-sample-ls-2                          True           3m59s
```

And shows status:

```
✗ kubectl get logstash logstash-sample
NAME              AVAILABLE   EXPECTED   AGE    VERSION
logstash-sample   3           3          9m1s   8.6.1
```

Still TODO:

* Unit Tests
* End to end Tests
* Certificate handling on the HTTP Metrics API

Tidy up

Co-authored-by: Kaise Cheng <kaise.cheng@elastic.co>
Rudimentary tests for validation and naming
This reverts commit 8016b7b.

Remving e2e tests to get e2e tests prior to logstash working
This reverts commit b5c775e.

And re-adds the e2e tests
Add a test to make sure that Logstash is running, by testing the metrics API endpoint
and checking that the value of status is "green"
This commit fixes the readiness probe, by setting the default visibility of
the logstash API to 0.0.0.0, allowing access to the metrics API to the probe
test/e2e/logstash/pipeline_test.go Outdated Show resolved Hide resolved
test/e2e/logstash/pipeline_test.go Show resolved Hide resolved
test/e2e/logstash/pipeline_test.go Show resolved Hide resolved
@kaisecheng kaisecheng requested a review from barkbay April 5, 2023 09:58
Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Great stuff, just a couple of nits/suggestions around clarifying naming around PipelinesRef and ConfigRef

}

// MustParse parses the given pipeline content into a Pipelines.
// Expects content to be in YAML format. Panics on error.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add comment to state that this should be used in tests only

)

// ConfigRefWatchName returns the name of the watch registered on the secret referenced in `pipelinesRef`.
func ConfigRefWatchName(resource types.NamespacedName) string {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe PipelinesRef instead of ConfigRef?

Maybe for the filename here too

Suggested change
func ConfigRefWatchName(resource types.NamespacedName) string {
func PipelinesRefWatchName(resource types.NamespacedName) string {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lint suggested not to use Pipelines prefix. Renamed to ref.go and RefWatchName()


// ParseConfigRef retrieves the content of a secret referenced in `pipelinesRef`, sets up dynamic watches for that secret,
// and parses the secret content into a PipelinesConfig.
func ParseConfigRef(
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
func ParseConfigRef(
func ParsePipelinesRef(

@kaisecheng
Copy link
Contributor Author

@barkbay I think it is ready 2nd round

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work 👍

pkg/controller/logstash/pipelines/config.go Outdated Show resolved Hide resolved
pkg/controller/logstash/pipelines/config.go Outdated Show resolved Hide resolved
pkg/controller/logstash/pipelines/config.go Outdated Show resolved Hide resolved
pkg/controller/logstash/pipelines/config.go Outdated Show resolved Hide resolved
test/e2e/logstash/pipeline_test.go Outdated Show resolved Hide resolved
test/e2e/logstash/pipeline_test.go Outdated Show resolved Hide resolved
@@ -9,6 +9,8 @@ import (
"encoding/json"
"fmt"

"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/settings"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: import grouping seems off

kaisecheng and others added 7 commits April 6, 2023 11:12
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Co-authored-by: Michael Morello <michael.morello@gmail.com>
@kaisecheng kaisecheng merged commit 7ce5d9a into elastic:feature/logstash Apr 6, 2023
@robbavey
Copy link
Member

#6674 adds pipeline reload functionality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality :logstash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants