Fix Beat E2E tests issues #3325
Conversation
| allowHostPorts: false | ||
| allowPrivilegeEscalation: true | ||
| allowPrivilegedContainer: true | ||
| allowedCapabilities: [] | ||
| allowedCapabilities: |
There was a problem hiding this comment.
Can we add a comment to explain why these are necessary?
There was a problem hiding this comment.
Ditto for allowHostPID because it's not clear to me why it needs that, and also ditto for do we need to update the openshift docs to include these changes (my guess is yes)
| @@ -68,12 +72,40 @@ func getBeatKibanaRoles(associated commonv1.Associated) (string, error) { | |||
| ) | |||
| } | |||
|
|
|||
| if strings.Contains(beat.Spec.Type, ",") { | |||
| return "", fmt.Errorf("beat type %s should not contain a comma", beat.Spec.Type) | |||
There was a problem hiding this comment.
If it's a general thing about the Beats CRD it should probably live outside the association controller, and be also enforced by the Beat controller & webhook?
There was a problem hiding this comment.
It's already forbidden by the Type regex requirements. One here is just double checking. Do you think it would be worth to check it in a validation too?
There was a problem hiding this comment.
If it's in the OpenAPI validation it should never get through, so I think we can omit the check
There was a problem hiding this comment.
We seem to have an issue with that. But even without it, I'd like to keep another check close to the code just to make sure we are safe in case it'll get copied over somewhere not behind that regex check.
There was a problem hiding this comment.
Ah if the OpenAPI validation doesn't work we should still just check for it in the validation code we run at the beginning of the reconcile/webhook IMO rather than having type validation code scattered about
test/e2e/beat/config.go
Outdated
| @@ -196,6 +196,7 @@ processors: | |||
| hostPID: true # Required by auditd module | |||
| dnsPolicy: ClusterFirstWithHostNet | |||
| hostNetwork: true | |||
| automountServiceAccountToken: true | |||
There was a problem hiding this comment.
This is only necessary on some versions from your description, correct? Is it worth adding a comment here and updating the recipes/docs as well with this info? We call out other OpenShift specific configs
There was a problem hiding this comment.
@anyasabo, @sebgl: This is not OpenShift specific actually, only Beat version specific. I've added a comment, but I'd rather not add too much in the docs/samples for couple of reasons:
- if we do it for one setting, why we don't do it for all of them? it would a bit of work
- our docs start becoming Beats on k8s docs instead of just ECK docs
- we use those configs to test across all versions/environments, so we might end up with if statements in the comments :) we already turn off some things for old kind version, because it doesn't work there
In general I'd like to think about us testing Beats as a proof we can deploy them correctly and they work to some degree. I believe high fidelity documentation about what and why could be a rabbit hole for us and should live in Beat docs. WDYT?
There was a problem hiding this comment.
Ah gotcha my reading comprehension is poor. I think it's a blurry line for when it is useful to have beats documentation in our repo, but am good with your decision here 👍
Co-authored-by: Michael Morello <michael.morello@gmail.com>
|
jenkins test this please |
There was a problem hiding this comment.
When running the tests this one failed at leat once (I ran the test with 7.5.2):
--- FAIL: TestBeatKibanaRef/Verify_dashboards_installed (300.00s)
utils.go:84:
Error Trace: utils.go:84
Error: Received unexpected error:
expected Metricbeat dashboard [true], found dashboards [false]
Test: TestBeatKibanaRef/Verify_dashboards_installed
It just happened once, I have not been able to reproduce, so I'm not sure what to think about it...
Is there any scenario where the dashboards may not be installed ? (also re. this test there are a lot of Pods in a CrashloopBackoff state, but I guess it's because Kibana is not ready yet ?)
|
@barkbay It shouldn't happen. Logs from the Pod would be the most helpful I think. Crashing is due to two factors, but both should be temporary:
Maybe create a separate issue for flaky test? |
* Fix Beat E2E tests * Update config/e2e/filebeat.yaml Co-authored-by: Michael Morello <michael.morello@gmail.com> * PR fixes Co-authored-by: Michael Morello <michael.morello@gmail.com>
This PR fixes three Beats E2E tests issues:
automountServiceAccountTokento all Beat configs. Lower version Beats depended on/var/run/secrets/kubernetes.io/serviceaccount/namespaceto exists in the pod.kibana_adminorkibana_roleto do the dashboard setup below certain Beat version.