Implement Aerospike client reauth and LDAP-specific failure handling#72
Implement Aerospike client reauth and LDAP-specific failure handling#72eappere wants to merge 2 commits into
Conversation
| return hex.EncodeToString(hasher.Sum(nil)) | ||
| } | ||
|
|
||
| func ensureAerospikeClientRefreshed(e *AerospikeEndpoint) (bool, error) { |
There was a problem hiding this comment.
you should return only errror.
Not being connected after refresh should be considered as an errror.
It will simplify all call sites
There was a problem hiding this comment.
Not being connected after refresh should be considered as an errror.
While this is going to generate logs, ultimately this should be OK, as it will classify error as latency and availability error.
There was a problem hiding this comment.
Done; also the error is ignored (since the function can only do anything every auth refresh interval).
Due to previous implementation details of the probe and the alerts, this results in "data unavailable" alert (not technically incorrect), rather than "availability < 100%"
| } | ||
|
|
||
| func (e *AerospikeEndpoint) Connect() error { | ||
| func ldapSpecificResultCode(err error) (int, string, bool) { |
There was a problem hiding this comment.
I don't understand interest of tracking ldap specific errors. Either connection is working or not.
There was a problem hiding this comment.
There are two reasons auth could fail:
- client side due to us passing invalid credentials
- server-side when Aerospike server contacts LDAPS server on our behalf
Considering the second case almost always means that other users are impacted (like last Thursday), and thus almost always causes a BES, I find it desirable to have a separate metric and a separate alert, something like:
increase(blackbox_prober_aerospike_ldap_specific_failures_total[4m]) > 0
for 10min
severity=page
I have reproduced the incident on aerospikes99 and my changes and new metric catch that cleanly
There was a problem hiding this comment.
Latest commit can be removed
eappere
force-pushed
the
fix-aerospike-ldap
branch
from
351d7c6 to
258765f
Compare
Set 2min threshold. While less than durability check interval (10min), it is higher than latency check interval (500ms)
…_total Unwrap the error code and check if it is in the 90..99 range. If that is the case, it is a LDAP-specific failure, not just some generic "invalid creds" error code and we can quickly raise a paging alert.
eappere
force-pushed
the
fix-aerospike-ldap
branch
from
258765f to
0a7f5ee
Compare
| return fmt.Errorf("error: given endpoint is not an aerospike endpoint") | ||
| } | ||
|
|
||
| // Ignore the error to cause probing check failure; LDAP failures are logged anyway. |
There was a problem hiding this comment.
Still, in case of non ldap error, it won't be logged at all, leading to not knowing why we were unable to login during checks failure.
At least log an error message and let the check fails ?
| } | ||
|
|
||
| // Ignore the error to cause probing check failure; LDAP failures are logged anyway | ||
| // We get data_unavailable when LDAP failures happen, as the gauges are set unconditionally |
|
I think it will be misleading that reconnection make Latency and Durability check fails. |
3 participants
This PR adds periodic Aerospike client reauthentication, and specifically improves visibility into LDAP-specific failures.
Previously, we wouldn't detect LDAP failures our users would face as sessions are kept alive for up to 10 days (by default on Aerospike).
Changes:
client_config.reauth_intervalto Aerospike endpoint config (default: 2m)blackbox_prober_aerospike_client_cluster_stats(as these metrics are client-side)90..99range- Added
blackbox_prober_aerospike_ldap_specific_failures_totalso that we can be paged as soon as possible (we cansumthe hypothetical alert per DC to avoid alert fatigue)