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

Introduce Beat CRD #3041

Merged
merged 82 commits into from
Jun 8, 2020
Merged

Introduce Beat CRD #3041

merged 82 commits into from
Jun 8, 2020

Conversation

david-kow
Copy link
Contributor

@david-kow david-kow commented May 12, 2020

This PR introduces first iteration of CRD for Beats and it's updated to reflect most recent changes.

Look & feel

Filebeat

$ kubectl get beat filebeat-sample
NAME              HEALTH   AVAILABLE   EXPECTED   TYPE       VERSION   AGE
filebeat-sample   green    3           3          filebeat   7.6.2     2m7s

$ kubectl tree beat filebeat-sample
NAMESPACE  NAME                                                             READY  REASON  AGE
default    Beat/filebeat-sample                                             -              19s
default    ├─DaemonSet/filebeat-sample-beat-filebeat                        -              16s
default    │ ├─ControllerRevision/filebeat-sample-beat-filebeat-7f7667d58b  -              16s
default    │ ├─Pod/filebeat-sample-beat-filebeat-pnx85                      True           16s
default    │ ├─Pod/filebeat-sample-beat-filebeat-wmnvb                      True           16s
default    │ └─Pod/filebeat-sample-beat-filebeat-x5rtf                      True           16s
default    ├─Secret/filebeat-sample-beat-es-ca                              -              17s
default    ├─Secret/filebeat-sample-beat-filebeat-config                    -              17s
default    ├─Secret/filebeat-sample-beat-user                               -              17s
default    └─ServiceAccount/elastic-operator-beat-filebeat-sample-d7fb      -              17s

Heartbeat

$ kubectl get beat heartbeat-sample
NAME               HEALTH   AVAILABLE   EXPECTED   TYPE        VERSION   AGE
heartbeat-sample   green    1           1          heartbeat   7.6.2     10h

$ kubectl tree beat heartbeat-sample
NAMESPACE  NAME                                                          READY  REASON  AGE
default    Beat/heartbeat-sample                                         -              10h
default    ├─Deployment/heartbeat-sample-beat-otherbeat                  -              10h
default    │ └─ReplicaSet/heartbeat-sample-beat-otherbeat-c8645b56       -              10h
default    │   └─Pod/heartbeat-sample-beat-otherbeat-c8645b56-cr4f8      True           10h
default    ├─Secret/heartbeat-sample-beat-es-ca                          -              10h
default    ├─Secret/heartbeat-sample-beat-otherbeat-config               -              10h
default    ├─Secret/heartbeat-sample-beat-user                           -              10h
default    └─ServiceAccount/elastic-operator-beat-heartbeat-sample-8d7e  -              10h

Features

  • native support for Filebeat and Metricbeat
  • initial support for other Beats
  • support for daemonsets and deployments
  • providing Beat status
  • basic webhook/non-webhook validation
  • rollout for config/version changes
  • labelling for all created resources
  • support for pod template/config overriding
  • basic apm/events/logging
  • RBAC for autodiscovery setup (can be disabled via operator flag, separate issue at Improve setup of ClusterRole and ClusterRoleBinding for autodiscover #3134)

Additional notes

State

This is the first pass, meta-issue was created (#3040) for additional context and work tracking. There is couple of open issues to discuss PSP/SCC treatment (#3133), default/merging config (#3042) and autodiscover RBAC (#3134).

Testing

I was able to run Filebeat, Metricbeat and Heartbeat successfully, change config, upgrade versions. Some E2E are added for couple basic scenarios and SmokeTest runs with Filebeat now. Tested on OpenShift as well. There is a facility introduced that helps with querying ES and checking whether Beats events really appear in ES. In E2E tests autodiscover RBAC is managed by the operator, while the correct PSP is granted to the operator similarly to other stack products. This will be improved to grant additional permissions only to Beat pods and avoid escalating operator privileges unnecessarily.

Also, at some point, tested manually with:

  • Filebeat config targeting specific namespaces, labels
  • output overriden
  • multiple Beats, multiple Beats per namespace

Beat support

Only filebeat and metricbeat are supported natively. Other beats can be deployed, but only when both image and config are provided.

Config handling

As this deserves an issue of it's own to discuss (#3042), I'll just sum up what is currently in the code: if no spec.config is provided we use a static, default config from controller/common/beat/filebeat/config.go, otherwise we use the provided config. The only merging we do is the output if elasticsearchRef was provided.

@david-kow david-kow added >feature Adds or discusses adding a feature to the product v1.2.0 labels May 12, 2020
@david-kow david-kow mentioned this pull request May 12, 2020
cmd/manager/main.go Outdated Show resolved Hide resolved
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.

Just did a first review, I'll do some additional tests.

config/operator/global/rbac.yaml Show resolved Hide resolved
config/samples/beat/filebeat_es_kibana.yaml Outdated Show resolved Hide resolved
pkg/apis/beat/v1beta1/beat_types.go Outdated Show resolved Hide resolved
pkg/apis/beat/v1beta1/beat_types.go Outdated Show resolved Hide resolved
pkg/apis/beat/v1beta1/webhook.go Show resolved Hide resolved
pkg/controller/common/beat/otherbeat/otherbeat.go Outdated Show resolved Hide resolved
pkg/controller/common/beat/otherbeat/otherbeat.go Outdated Show resolved Hide resolved
pkg/controller/common/beat/autodiscovery.go Outdated Show resolved Hide resolved
pkg/controller/common/beat/autodiscovery.go Outdated Show resolved Hide resolved
pkg/controller/common/beat/autodiscovery.go Outdated Show resolved Hide resolved
David Kowalski added 14 commits May 21, 2020 06:57
First pass at Beats in ECK, features include:
- native support for Filebeat (daemonset only)
- initial support for other Beats (deployment only)
- RBAC for autodiscovery setup (can be disabled via operator flag)
- providing Beat status
- basic webhook/non-webhook validation
- rollout for config/version changes
- labelling for all created resources
- support for pod template/config overriding
- basic apm/events/logging
@david-kow david-kow force-pushed the beat_crd branch 2 times, most recently from 08c6598 to d4c538e Compare May 21, 2020 05:09
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

Great work! The code is very clean now with the recent refactoring 👍
I left some comments, mostly for minor cosmetic changes. I'm fine with merging early and creating follow-up issues where necessary.

cmd/manager/main.go Outdated Show resolved Hide resolved
- JSONPath: .status.expectedNodes
description: Expected nodes
name: expected
type: integer
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a single nodes (corresponding to available nodes) instead of expectedNodes and availableNodes in other CRDs. Let's be consistent?

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 think we did want to change that in the past (#1786). I'd be for keeping both and improving the status on other resources. It's following the pattern of built-in resources, gives the users more information and it comes free. I'd leave it as is unless you feel strongly about it. (I'm open to naming changes.)

config/crds/all-crds.yaml Outdated Show resolved Hide resolved
config/dev/elastic-psp.yaml Show resolved Hide resolved
config/dev/elastic-psp.yaml Outdated Show resolved Hide resolved
pkg/controller/common/association/association.go Outdated Show resolved Hide resolved
test/e2e/test/beat/pod_builder.go Outdated Show resolved Hide resolved
test/e2e/test/beat/pod_builder.go Show resolved Hide resolved
test/e2e/test/beat/steps.go Show resolved Hide resolved
test/e2e/test/beat/steps.go Outdated Show resolved Hide resolved
@anyasabo
Copy link
Contributor

anyasabo commented Jun 5, 2020

Just to track from our out of band conversation yesterday, I think we do want to release this as v1alpha1 at first

Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

LGTM. Awesome work @david-kow!
I left a few comments again. I think I'd prefer we merge soon and build on top with smaller PRs, rather than iterate here for too long.

test/e2e/test/beat/pod_builder.go Show resolved Hide resolved
test/e2e/test/beat/pod_builder.go Outdated Show resolved Hide resolved
hosts:
- https://${HOSTNAME}:10249
metricsets:
- proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why we remove those? Is it moving us away from https://raw.githubusercontent.com/elastic/beats/7.7/deploy/kubernetes/metricbeat-kubernetes.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filebeat doesn't need any API permissions other than autodiscover provides, but Metricbeat does. Removing the modules/metricsets that need those permissions. This will be addressed by implementation of #3190, but not fixing it here to avoid solving the same problem twice. Those modules/metricsets will be brought back in another PR.

config/samples/beat/filebeat_es_kibana_apm.yaml Outdated Show resolved Hide resolved
pkg/controller/beat/common/pod.go Show resolved Hide resolved
field.NewPath("spec").Child("image"),
"Image is required if Beat type is not one of [filebeat, metricbeat]"),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed on Slack: it's about what we support or not. If users specify type: packetbeat they may feel like ECK supports running packetbeat, which is not true.

@@ -24,6 +24,8 @@ const (
ElasticsearchImage Image = "elasticsearch/elasticsearch"
KibanaImage Image = "kibana/kibana"
EnterpriseSearchImage Image = "enterprise-search/enterprise-search"
FilebeatImage Image = "beats/filebeat"
MetricbeatImage Image = "beats/metricbeat"
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed on Slack: it's about what we "support". If the user is allowed to specify type: packetbeat they may feel like we officially support it, which isn't true.

@david-kow
Copy link
Contributor Author

Thanks for reviews everyone!

@david-kow david-kow merged commit 2c6b487 into elastic:master Jun 8, 2020
@david-kow david-kow deleted the beat_crd branch June 8, 2020 10:53
@david-kow david-kow mentioned this pull request Jun 8, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature Adds or discusses adding a feature to the product release-highlight Candidate for the ECK release highlight summary v1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants