-
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
[Metricbeat][Aerospike] Add support for basic auth #41233
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
This pull request is now in conflicts. Could you fix it? 🙏
|
Spell username toghether, as suggested. Co-authored-by: Ishleen Kaur <[email protected]>
BTW may I ask what's exactly failing in the check? I don't have access to buildkite output. |
Hi thanks for sending the screenshot. The method is correct. When I test it locally, I receive this error:
Meaning that when perform the test the Aerospike is not initialized yet. Testing it multiple times some times passes sometimes not without a clear reason. The Dockerfile simply checks if the port is open, but I am afraid it's not sufficient. We probably need to improve the HEALTHCHECK to make sure that Aerospike is up-and-running (for real) or roll-back to the goold-old version 3.9.0 that worked before without particular issues. (As a test I rolled back to version 3.9.0 in the below commit) |
Aerospike opens the port **before** Aerospike is actually ready Use the image aerospike/aerospike-server-enterprise instead of aerospike because it contains the aerospike tools
Hi there, Now the command |
@herrBez looks like the Aerospike python integration test is failing
|
@@ -47,6 +57,24 @@ func ParseClientPolicy(config Config) (*as.ClientPolicy, error) { | |||
clientPolicy.TlsConfig = tlsconfig.ToConfig() | |||
} | |||
|
|||
if len(config.User) > 0 && len(config.Password) > 0 { |
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.
[NIT]
As you're checking for empty string, it's more readable do it explicitly:
if len(config.User) > 0 && len(config.Password) > 0 { | |
if config.User != "" && config.Password != "" { |
Name: "Password is set and user is not set", | ||
Config: Config{ | ||
Password: samplePassword, | ||
}, | ||
expectedClientPolicy: as.NewClientPolicy(), | ||
expectedErr: nil, |
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 don't know much about Aerospike, but shouldn't it be an error case? Just setting the password?
Proposed commit message
[Metricbeat][Aerospike] Add support for Basic Auth and update aerospike-client-go dependency
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
No user impact the old configuration still works
Author's Checklist
Context
As of October 14, 2024, only Aerospike DB version 6.1 and above is supported by the vendor. More details can be found here: Aerospike Platform Support.
The supported versions of the Aerospike client libraries are listed here: Aerospike Client Library Matrix.
Currently, Beats integrates version 1.27.1 of the aerospike-client-go library, which was released in 2017 and is no longer supported by the vendor.
In this pull request (PR), we upgrade the dependency to version 7 and add support for Basic Authentication for Enterprise Edition (EE) of Aerospike,
Please note that Aerospike version 7 introduced several changes to the metrics (some metrics that the metricset is using have been renamed, and others removed). Details can be found here: Aerospike 7.0 Metrics Changes. To keep the scope of this PR focused, I have opted to implement this change first and will submit a separate PR to address the metrics changes (I have already implemented the code for the change).
Final note, we distinguish between CE (community edition) and EE (enterprise edition) also in the docker images.
How to test this PR locally
username: admin
andpassword: admin
:aerospike-basic-auth.conf
:Aerospike 7 config-file
Aerospike 6 config-file
Export the following variable
export AEROSPIKE_VERSION=ee-6.4.0.7_2
to test with version 6 of AerospikeUse the following docker-compose.yaml
./metricbeat test modules aerospike
Please note that since some metrics have been renamed with Aerospike 7, the answer with this version will contain some empty metrics.
Related issues
Use cases
Monitor an Aerospike Cluster protected by Basic Auth
Screenshots
N/A
Logs
Not relevant