-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Check license x-pack #11296
Check license x-pack #11296
Conversation
After a walk I will take another way, I am trying to find an OK way without a hack. |
I did a bit of user testing:
The error, however, is not very clear:
|
@ph IMO we can go with this solution for now, given the time constraints. It's probably the minimal diff to make it happen and the code is simple enough. I'd be worried about a larger refactoring this late into the game. |
@tsg @andrewkroh I've made the changes and tweak the error handling/messaging. |
Username: cli.GetEnvOr("ES_USER", ""), | ||
Password: cli.GetEnvOr("ES_PASS", ""), | ||
Username: "myelastic", // NOTE: I will refactor this in a followup PR | ||
Password: "changeme", |
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.
@tsg, @andrewkroh I will do a followup on this, I didn't want to waste time on docker-compose environment variable and slow the merge. Since we need to add integration test for this new scenario we can fix it at the same time.
Co-Authored-By: ph <[email protected]>
Need backport to 6.7 and 7.0. |
Create an issue for the flaky metricbeat test #11336 |
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.
LGTM.
* Check License for basic or better (cherry picked from commit b0a2eb4)
* Check License for basic or better (cherry picked from commit b0a2eb4)
@graphaelli This might affects apm or not, I will do a followup to make the check cleaner. |
It certainly will, thanks for the heads up. cc @elastic/apm-server |
This has caused some issues with our AWS Elasticsearch Service cluster. |
Checkout my comment in #11607. There are separate releases of Beats that do not include any x-pack features and therefore do not need that |
You released this change with oblique release notes and error messages that obfuscate the true issue. It's painful and disappointing to be hurt by your fight against another company. |
What error message did you get? This is the message I suggested. I thought it was pretty clear, but if you have suggestions let us know.
We should add more details to the release notes. There's not really enough detail that for someone unfamiliar to understand the impact of the change or how to handle it. We'll fix that. |
That message is very good. I would have liked to receive it. Instead I see:
One tweak to your message I suggest would be to use the words "default" and "OSS" which are how the two builds of filebeat are distinguished. For example:
Putting it together with the raw message I received:
In other words, the message is added by the same part of code which adds "cannot retrieve the elasticsearch license" as further context. |
It looks like the reason you didn't get this error message is because the failure occurs before the actual license check, as the HTTP request was not authorized correctly: Problem with the authorization error is that Beats can not tell if it is talking with AWS Elasticsearch, OSS Elasticsearch, or XPack Elasticsearch. Or maybe a proxy doing authentication. Therefore we can not do any license check and should not suggest the version a user is running, so to not cause more confusion. I wonder about the raw HTTP response. Maybe we can find something in the HTTP response headers, so to create a more informative error message similar to what you suggested. Reading the code we will print a complete warning if 400 or 405 status code is received. Have you tried the OSS version? Did you get any errors? |
AWS ES response for this URL doesn't have many tells:
Is this license check the first HTTP request made to the ES endpoint? If the entire cluster is protected with HTTP auth, libbeat would notice before getting to the license check. |
Looking at the discussion and the header output not sure I can give more information or details about that situation in that specific case. Let's say that you connect to _xpack with the right credentials what do you get? |
It seems like we always get a 401 from AWS, even if authentication is configured correctly. It's a little tricky, as we don't really want to hack around wrong status codes, making assumptions about the actual service in use. |
* Fix index template always being overwritten - elastic/beats#11671 * Perform Basic license check on Elasticsearch connect - elastic/beats#11296 + elastic/beats#11649
Early look on the way of doing it.
This strategy has some limitations, we cannot set a specific license for a specific call but for the current need, I think it's OK.
I am currently in the process or splittings the Elasticsearch output module into a pipeline client an elasticsearch client, I will try to revisit the strategy there.
Note: I've tried to use the normal callbacks, but I've decided against for two reasons:
I am writing test but I wanted to have an early look.