Repository navigation
fix(core): a cached client is shared only by callers with the same identity (SCHEMA-14) - #458
Merged
Merged
Conversation
…entity (SCHEMA-14) Story SCHEMA-14. Reviewed: r0:clean, r1:clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ElasticClientFactorynow shares a cached client only between callers whose effectiveelastic.*configuration is equal (SCHEMA-14). On 0.24.0 and main, the factory's three client caches (base, metrics, monitored) are keyed by the cluster URL alone (scheme://host:port): the auth method, user, password, API key and bearer token are ignored, so a caller configured with other credentials for the same URL receives the client built for the first caller, and that caller's privileges. The JDBC driver and ADBC build one client per connection through the factory, so two users of one cluster in one JVM were affected. The fix also stops one caller'sclose()from closing a client other callers still use, and stops monitored clients from leaking an actor system and a JVM shutdown hook each.What changes
ClientKeyis the URL plus an HMAC-SHA256 fingerprint of the whole effectiveelastic.*configuration (credentials, timeouts, scroll and schema-cache settings), computed by one function for the three caches and the custom-collector paths. The HMAC key is random per JVM, and the encoding is length-prefixed per value, so two configurations cannot collide by moving a separator and a leaked fingerprint is no offline password oracle.ClientKey.toStringprints the URL only. The configuration is parsed once percreate…()call (main parsed it twice).create,createWithMetricsandcreateWithMonitoringreturn the same cached instance for an equal configuration, as main does per URL;createWithCustomMetricsandcreateMonitoredWithCustomMetricsstill build a new wrapper per call, over the cached base client of that configuration.close()releases one share. A cached instance counts its holders:close()releases the caller's share and never closes the transport, or stops the monitor, that another caller still holds. Public return types are unchanged (package-privateSharedClient,SharedMetricsClient,SharedMonitoredClient; the custom paths return a per-call idempotent lease).ConcurrentHashMapremapping function). Without this, every rotated API key or token, and every user, would keep a client and its IO threads until JVM exit.shutdown()andclearCache()keep their meaning.shutdown();MonitoredElasticClient.shutdown()removes its own JVM shutdown hook. Main created one actor system and one hook per monitored client and never released them.ServiceLoaderiteration runs under a lock (aServiceLoaderis not safe for concurrent iteration).getCacheStatskeeps its three counts (now one entry per distinct configuration).Release notes
ElasticClientFactoryis no longer shared with a caller that configured other credentials (or other client settings) for the same cluster URL. 0.24.0 handed such a caller the first caller's client and privileges; the JDBC driver and ADBC, which open one client per connection, were affected.close()releases the caller's share; a client is really closed after 10 minutes without a holder (at the next client creation) or byshutdown().shutdown().clearCache()leaves the clients handed out before it usable but no longer managed: theirclose()does nothing andshutdown()does not close them (test use).MonitoredElasticClientgains private members only, no public signature changes and no case class changes arity.Acceptance evidence
JavaClientFactoryIdentitySpec): through every factory entry point and in both creation orders, a read-only user (basic credentials, and an API key restricted to a reader role) is refused on a protected index and on the version endpoint and answered on an open index, while the admin on the same URL is answered everywhere; a wrong password is refused with 401. Control on main: the second client answered exactly as the first.close()percreate…(), as the docs say; the JDBC driver, ADBC and the Flight producer each close once.getCacheStats, or in anytoString.*JavaClientFactoryIdentitySpec *JavaClientSpec *JavaClientSchemaConsumerSpec) 98/0; Docker-free client units on es6 REST, es7, es8 and es9 green; Scala 2.12 + 2.13 compile; CI lint line clean on the committed bytes.Downstream
The real drivers were run against this fix before opening the PR: 0.24.0 plus this commit (the cherry-pick applies cleanly; the factory,
MonitoredElasticClient,ElasticConfigandElasticCredentialsare unchanged since 0.24.0) was published locally under a throwaway version. The JDBC driver (throughDriverManager) and ADBC each ran one two-user test on ES 8 with security on — both users, both orders, one user closing while the other keeps answering — and each first checked that it had loaded the patched factory: JDBC 5/0, ADBC 5/0. Their class-loader isolation suites passed (13/0, 8/0). The throwaway artifacts were deleted. No change is needed in extensions, JDBC or arrow.Recorded, not fixed
elastic.*only: the licence mode, the extension registry and the akka settings are still read from the raw configuration of whoever first created a client for that key (as main does per URL).shutdown()directly on a shared monitored client stops monitoring for every holder (its scaladoc now steers callers toclose()).close()and its eviction re-opens a transport outside the cache.ElasticConfig.applylogs the configuration error on a parse failure, which may echo a value.Test plan
ClientKeySpec,ElasticClientCacheSpec,ElasticClientFactoryIsolationSpec(65/0) andcore/test(1640/0)JavaClientFactoryIdentitySpec,JavaClientSpec,JavaClientSchemaConsumerSpec(98/0)+ compile(2.12 and 2.13)🤖 Generated with Claude Code