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

Add Beat config library #3406

Merged
merged 9 commits into from
Jul 17, 2020
Merged

Conversation

david-kow
Copy link
Contributor

@david-kow david-kow commented Jul 8, 2020

This PR introduces a configuration library for Beat CRD. Yaml files in config/recipes/beats are self-contained and should be fully functional in non-secured cluster (PSP/SCC is out of scope).

$ kubectl apply -f config/recipes/beats # due to conflicting naming not all examples will be deployed
$ kubectl get es,kb,apm,ent,beat -A
NAMESPACE   NAME                                                       HEALTH   NODES   VERSION   PHASE   AGE
default     elasticsearch.elasticsearch.k8s.elastic.co/elasticsearch   green    3       7.8.0     Ready   2m39s

NAMESPACE   NAME                                  HEALTH   NODES   VERSION   AGE
default     kibana.kibana.k8s.elastic.co/kibana   green    1       7.8.0     2m39s

NAMESPACE   NAME                                   HEALTH   AVAILABLE   EXPECTED   TYPE          VERSION   AGE
default     beat.beat.k8s.elastic.co/auditbeat     green    3           3          auditbeat     7.8.0     2m39s
default     beat.beat.k8s.elastic.co/filebeat      green    3           3          filebeat      7.8.0     2m39s
default     beat.beat.k8s.elastic.co/heartbeat     green    1           1          heartbeat     7.8.0     2m38s
default     beat.beat.k8s.elastic.co/journalbeat   green    3           3          journalbeat   7.8.0     2m38s
default     beat.beat.k8s.elastic.co/metricbeat    green    1           1          metricbeat    7.8.0     2m38s
default     beat.beat.k8s.elastic.co/packetbeat    green    3           3          packetbeat    7.8.0     2m37s

Each recipe is tested as an E2E test with custom (per recipe) setup and checks. Docs linking to those recipes will come as a separate PR. Related: #3319.

@botelastic botelastic bot added the triage label Jul 8, 2020
@david-kow david-kow added the >feature Adds or discusses adding a feature to the product label Jul 8, 2020
@botelastic botelastic bot removed the triage label Jul 8, 2020
securityContext:
runAsUser: 0
# If using Red Hat OpenShift uncomment this:
#privileged: true
Copy link
Contributor

Choose a reason for hiding this comment

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

The privileged field does not exist in the PodSecurityContext. It is only available in the container's SecurityContext.

Also I'm wondering if it is required for all versions of Openshift since it's seems to work out of the box on Openshift 4 according to our e2e tests ? I think it is required on Openshift 3.11 to bypass SELinux on the hostPath volume.

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've fixed the placement, I'll try to test at some point to see if only 3.11 is affected. FYI, the comment was taken from Beats examples, but you are right, it works in our E2E tests.

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

Looks really good! I wonder if it would make sense to have a short description for each recipe as a README in addition to the docs link coming later.

test/e2e/test/run.go Outdated Show resolved Hide resolved
customize := func(builder beat.Builder) beat.Builder {
return builder.
WithRoles(beat.PSPClusterRoleName, beat.AutodiscoverClusterRoleName).
WithESValidations(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a bit odd that we define test validations as part of the builder, that builds the test subjects, but I guess this is a general problem in our e2e setup at the moment.

test/e2e/test/helper/yaml.go Outdated Show resolved Hide resolved
test/e2e/test/helper/yaml.go Outdated Show resolved Hide resolved
david-kow and others added 2 commits July 9, 2020 16:05
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
@david-kow david-kow requested a review from pebrc July 14, 2020 09:01
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

I tested on an unsecured k8s cluster (so no PSP) a few RBAC issues popped up but otherwise LGTM

config/recipes/beats/README.md Outdated Show resolved Hide resolved
config/recipes/beats/README.md Outdated Show resolved Hide resolved
config/recipes/beats/README.md Outdated Show resolved Hide resolved
config/recipes/beats/auditbeat_hosts.yaml Outdated Show resolved Hide resolved
config/recipes/beats/journalbeat_hosts.yaml Outdated Show resolved Hide resolved
config/recipes/beats/packetbeat_dns_http.yaml Outdated Show resolved Hide resolved
david-kow and others added 2 commits July 14, 2020 14:47
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
@david-kow
Copy link
Contributor Author

I went and added missing RBAC, but then I realized that those kubernetes processors are not adding anything to the events, so I removed them.

In case of auditbeat and journalbeat I'm not sure if kubernetes metadata makes sense at all as they interact with hosts. For packetbeat, I think it would make sense to annotate events with src/dst containers, but it doesn't seem to happen and I'm not sure if this is a Beat limitation or something is wrong with the config. I think that I'd leave that for a future improvement.

@anyasabo
Copy link
Contributor

In case of auditbeat and journalbeat I'm not sure if kubernetes metadata makes sense at all as they interact with hosts.

k8s nodes still have labels and annotations, so I could see how it might happen, but going by your message it does not 🤷‍♀️ maybe worth opening a docs issue just to clarify what resources/beats actually add metadata

@anyasabo
Copy link
Contributor

anyasabo commented Jul 14, 2020

Yaml files in config/recipes/beats are self-contained

The three filebeat examples share a beat name so overlap each other, I was a bit confused at first when the k8s metadata was not being added. Maybe this is fine, but we probably want to update the initial PR if so. Otherwise we may want to change the name of the beats

@anyasabo
Copy link
Contributor

anyasabo commented Jul 14, 2020

Don't think this is a blocker but: I'm at least having trouble with the auditbeat recipe, the only events in the dashboard are

2020-07-14T22:11:16.890Z ERROR [auditd] auditd/audit_linux.go:155 Failure receiving audit events {"error": "failed to set audit PID. An audit process is already running (PID 152)"}

Even after I deleted the dset and let ECK recreate it. So hopped on and saw

sabo@gke-sabo-dev-cluster-default-pool-c24f3bc2-ggb1 ~ $ ps aux | grep 152
root 152 0.0 0.8 340984 262828 ? Ss Jul08 0:35 /usr/lib/systemd/systemd-journald

And of course @jordansissel is forever in our hearts and already ran into the same issue:
elastic/beats#8523

I wonder if we want to just link to that as a known COS issue. I can also see the argument for including the fix in a recipe though since we have a lot of GKE customers.

config/recipes/beats/README.md Outdated Show resolved Hide resolved
config/recipes/beats/README.md Show resolved Hide resolved
@@ -0,0 +1,39 @@
# Beats Configuration Library

This directory contains yaml manifests with example configurations for Beats. These manifests are self-contained and work out-of-the-box on any non-secured Kubernetes cluster. All of them contain three-node Elasticsearch cluster and single Kibana instance. All Beat configurations set up Kibana dashboards if they are available for a given Beat and all required RBAC resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention how the Elasticsearch spec is not suitable to a production environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure people will notice the root README, it's worth repeating here imo.

@@ -0,0 +1,39 @@
# Beats Configuration Library
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering how those examples will be referenced from the documentation on the website. I think there's value in listing all the examples explicitly in the docs (ie. copying this markdown file content, basically).

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to include the samples in the documentation a long time ago but the docs build only checks out the docs directory so that wasn't successful. We'll either have to move the samples inside the docs directory or figure out the magic incantation required by the docs build to checkout more directories.

Copy link
Contributor Author

@david-kow david-kow Jul 16, 2020

Choose a reason for hiding this comment

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

I planned to copy it to the docs almost 1:1. The only thing I'd add in the docs is kubectl apply command for each example.

config/recipes/beats/README.md Outdated Show resolved Hide resolved
@pebrc
Copy link
Collaborator

pebrc commented Jul 15, 2020

Don't think this is a blocker but: I'm at least having trouble with the auditbeat recipe, the only events in the dashboard are

I forgot to mention I tested on AKS where I did not run into the auditbeat issue @anyasabo found.

@david-kow
Copy link
Contributor Author

Yaml files in config/recipes/beats are self-contained

The three filebeat examples share a beat name so overlap each other, I was a bit confused at first when the k8s metadata was not being added. Maybe this is fine, but we probably want to update the initial PR if so. Otherwise we may want to change the name of the beats

But is "self-contained" implying they won't overlap? I didn't mean to imply the latter. I'd be reluctant to rename, because we'd also need to rename RBAC pieces and ES/Kibana for consistency. Right now they are fairly easy to ready, but if we start encoding more things into the names they might get more complicated.

@exekias
Copy link

exekias commented Jul 16, 2020

@ChrsMark FYI

@anyasabo
Copy link
Contributor

But is "self-contained" implying they won't overlap? I didn't mean to imply the latter.

I think that combined with the suggestion to just:

kubectl apply -f config/recipes/beats

to review is what threw me off. I'm good with leaving the names as is, just a note for other reviewers 👍

Copy link
Contributor

@anyasabo anyasabo left a comment

Choose a reason for hiding this comment

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

I'm good with this as is but would suggest a couple changes:

  • adding a note in the readme about packetbeat+gke w/ COS being broken, or including another recipe that works for GKE
  • adding comments justifying the host network usage in beats where it's not necessary (which I think is everything but packetbeat)

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM!

@david-kow david-kow merged commit 2ce2160 into elastic:master Jul 17, 2020
@david-kow david-kow deleted the beat_recipes_library branch July 17, 2020 08:36
@charith-elastic charith-elastic added :ci Things related to Continuous Integration, automation and releases >docs Documentation >test Related to unit/integration/e2e tests and removed >feature Adds or discusses adding a feature to the product :ci Things related to Continuous Integration, automation and releases labels Jul 17, 2020
david-kow added a commit to david-kow/cloud-on-k8s that referenced this pull request Jul 17, 2020
* Add Beat config library

* Fix commented security context placement

* Apply suggestions from code review

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Add beats config library readme

* Apply suggestions from code review

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Remove add_kubernetes_metadata processor

* PR fixes

* Add note about namespace expectations for heartbeat example

* Add hostNetwork comments, add fix for GKE

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
david-kow added a commit to david-kow/cloud-on-k8s that referenced this pull request Jul 17, 2020
* Add Beat config library

* Fix commented security context placement

* Apply suggestions from code review

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Add beats config library readme

* Apply suggestions from code review

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Remove add_kubernetes_metadata processor

* PR fixes

* Add note about namespace expectations for heartbeat example

* Add hostNetwork comments, add fix for GKE

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
david-kow added a commit that referenced this pull request Jul 17, 2020
… (#3459)

* Add Beat config library (#3406)

* Add Beat config library

* Fix commented security context placement

* Apply suggestions from code review

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Add beats config library readme

* Apply suggestions from code review

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Remove add_kubernetes_metadata processor

* PR fixes

* Add note about namespace expectations for heartbeat example

* Add hostNetwork comments, add fix for GKE

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

* Fix recipes E2E tests (#3462)

* Add permissions for e2e runner necessary for recipes testing

* Add a note about examples purpose

* Sync Beat examples descriptions between readme and docs

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs Documentation >test Related to unit/integration/e2e tests v1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants