-
Notifications
You must be signed in to change notification settings - Fork 5k
Add tests for beatsauth extension 1/2 #46723
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
base: main
Are you sure you want to change the base?
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this tests but I think they are in the wrong place. As is, they could live with the beatsauth extension itself because they don't depend on anything in Beats.
However, the best version of these tests uses the configuration translation in agent to generate the auth extension configuration and the exporter configuration (I think you'd want the client configuration part of the exporter only to verify there are no TLS parameter conflicts that cause problems).
So I think these tests should move to Elastic Agent. Probably all the configuration translation code should be moved to Elastic Agent but you don't need to do that here.
for _, m := range sm.Metrics { | ||
switch d := m.Data.(type) { | ||
case metricdata.Sum[int64]: | ||
if len(d.DataPoints) >= 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be looking at one of the specific metrics, probably bulk.index.total
or bulk.create.total
depending on which on increments on an initial write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment in the other locations.
One of the metrics incrementing indicating indexing actually failed shouldn't allow the test to pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using "bulk.create.ok" metric now - since this is incremented only when the the event is successfully indexed
https://github.com/elastic/mock-es/blob/main/pkg/api/api.go#L328
Now that I've read #46428, you can get pretty close to this once that PR is merged into this one, if you were able to use the translation output for the extension and exporter as the base for the collector component configuration in the tests. Also since this says 1/2 in the description I assume there are more tests coming for things like proxies which are as critical as TLS, possibly in a separate PR. |
Proposed commit message
This PR adds test for translated Elasticsearch exporter configurations works with:
mTLS
,ca_trusted_fingerprint
andverififcation_mode
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Related issues