[feature] support Kafka client SCRAM authentication - #4320
goutamadwant wants to merge 3 commits into
Conversation
| if (!hasSaslAuthentication()) { | ||
| return StringUtils.isNotBlank(saslMechanism) | ||
| || StringUtils.isNotBlank(username) | ||
| || StringUtils.isNotBlank(password); |
There was a problem hiding this comment.
Could this cause the existing monitoring to become unavailable after the upgrade?
There was a problem hiding this comment.
@Duansg existing monitors remain backward compatible. Persisted monitor definitions have no values for the new fields, so securityProtocol and the SASL fields deserialize blank; KafkaProtocol.isInvalid() continues to accept that state, and getAdminClientProperties() emits only bootstrap.servers, matching the previous behavior.
The YAML default applies PLAINTEXT only when creating or editing a definition.
Aias00
left a comment
There was a problem hiding this comment.
Review: [feature] support Kafka client SCRAM authentication
Verdict: ✅ APPROVED — well-structured feature with solid test coverage.
What this PR does
Adds SCRAM (SCRAM-SHA-256/512) authentication support for the Kafka client collector:
KafkaProtocol: newsecurityProtocol,saslMechanism,username,passwordfields;passwordis@ToString.Exclude(no credential leak in logs);isInvalid()now validates the security protocol against a whitelist and requires credentials when SASL is selected.KafkaCollectImpl: builds SASL JAAS config, correctly escapes\and"viaescapeJaasValue, and folds credentials into theCacheIdentifierso different creds get distinct cachedAdminClients.app-kafka_client.yml: conditional fields (depend+hide) for SASL, wired into all 4 metric sets.- Docs (EN/ZH) updated; tests added for both collector and protocol.
Assessment
- Security: Credential escaping +
@ToString.Excludeare the right calls. The cache key now distinguishes by username/password/security protocol, preventing credential cross-contamination between monitors. - Correctness:
hasSaslAuthentication()safely handles a nullsecurityProtocol(no NPE viaString.equalsIgnoreCase(null)→ false).isInvalid()correctly treats a blank protocol as PLAINTEXT (backward compatible) and enforces all three SASL fields when SASL is on. - Tests: Good matrix — no-auth, SCRAM auth, JAAS escaping, cache isolation, whitelist validation, and the
toString-no-password guarantee.
Minor (non-blocking) suggestions
escapeJaasValuedoes not escape;, which would prematurely terminate the JAAS statement if present in a credential. Extreme edge case, but worth a comment or an additional escape.passwordYAML field has nolimit(username haslimit: 50); long passwords are plausible — consider adding one.SASL_SSLrelies on the JVM trust store with nossl.*properties supplied; the doc correctly warns about this, but a follow-up could surface truststore config. Acceptable for this scope.
No blocking issues. Nice contribution.
What's changed?
Adds optional SCRAM authentication to Kafka Client monitoring.
PLAINTEXT,SASL_PLAINTEXT, andSASL_SSLsecurity protocol optionsSCRAM-SHA-256andSCRAM-SHA-512with username and password credentialsCloses #4209
Verification
KafkaProtocolTest(23 tests)KafkaCollectTest(7 tests)YamlCheckScriptthrough the 22-module manager reactorChecklist
Add or update API
Not applicable: this change does not add or update an API. SCRAM validation, AdminClient property mapping, credential escaping, cache isolation, and plaintext compatibility are covered by unit tests.