refactor(opensearchtransport)!: drop IncludeDedicatedClusterManagers, exclude DCMs from routing unconditionally

BREAKING CHANGE: remove the IncludeDedicatedClusterManagers config flag from
opensearch.Config and opensearchtransport.Config.

v5 no longer offers an opt-in for routing to dedicated cluster managers. They are
kept in the connection inventory (allConns) for discovery reuse and eviction, but
never selected for request traffic. A connection is skipped during selection when
it is a dedicated cluster manager unless it is a user-supplied seed, so discovery
still bootstraps against a dedicated-cluster-manager seed via the seed-fallback
pool. This replaces the flag-gated excludeDCM pool field with an intrinsic
conn.seed check, removing the per-pool bool and the policyConfig plumbing.

RoundRobinPolicy now excludes dedicated cluster managers from its DiscoveryUpdate
add path unconditionally.

Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>

Author: sean-

CHANGELOG.md

@@ -236,13 +236,15 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ### Removed
 
+- **BREAKING**: Remove the `IncludeDedicatedClusterManagers` config flag from `opensearch.Config` and `opensearchtransport.Config`. Dedicated cluster managers (`cluster_manager` role with no work roles) are now unconditionally excluded from request routing: they are kept in the connection inventory for discovery reuse/eviction but never selected for query traffic (a user-supplied seed remains selectable so discovery can bootstrap against it). The flag previously defaulted to excluding them; opting them into routing is no longer supported. Delete any `IncludeDedicatedClusterManagers` field from your config (it is a compile error otherwise) ([#1004](https://github.com/opensearch-project/opensearch-go/pull/1004))
 - Remove deprecated `(*opensearch.Client).Perform` and `(*opensearchtransport.Transport).Perform`; `Stream(*http.Request) (*http.Response, error)` is now the sole method on `opensearchtransport.Interface`. Custom transport implementations must implement `Stream` instead of `Perform`. The `opensearch.Streamer` opt-in interface and `opensearch.ErrTransportMissingMethodStream` sentinel are removed. ([#872](https://github.com/opensearch-project/opensearch-go/issues/872))
 - **BREAKING**: Remove the `EnableMetrics` config flag from `opensearch.Config` and `opensearchtransport.Config`. The detailed-metrics snapshot (per-connection enumeration, per-policy breakdowns, and router cache state) is now always available; `Metrics()` returns the full snapshot unconditionally. The flag's only remaining purpose after [#891](https://github.com/opensearch-project/opensearch-go/issues/891) was to gate the detailed path, which now does its work lazily and lock-free at call time and so costs nothing until `Metrics()` is called. Delete any `EnableMetrics` field from your config (it is a compile error otherwise); see [`UPGRADING_V5.md`](UPGRADING_V5.md#enablemetrics-removed). ([#892](https://github.com/opensearch-project/opensearch-go/issues/892))
 - Remove backport.yml and dependabot_pr.yml as we are not using backport app anymore
 - Stop emitting `opensearchapi.Client` sub-client fields that have no operations routed to them. `cmd/osgen` now emits a sub-client only when at least one operation targets it, dropping the previously-empty `Script`, `ComponentTemplate`, `IndexTemplate`, `Template`, and `DataStream` fields. Index-template and data-stream operations are reached through `client.Indices.*` (e.g. `client.Indices.PutIndexTemplate`, `client.Indices.CreateDataStream`); stored-script operations remain top-level on `Client`
 
 ### Fixed
 
+- Fix an unbounded connection/heap leak in node discovery when the cluster has a dedicated cluster manager (`cluster_manager` role with no work roles). The node was filtered out of the `allConns` inventory while the router received the unfiltered added/removed diffs, so `findConnectionByURL` never matched it: a new `*Connection` was created every discovery cycle and the stale one was never evicted, accumulating without bound in the round-robin fallback pool whose `checkDead` health checks repopulated a per-connection `poolRegistry` `sync.Map` each cycle (leak rate scaled with discovery frequency). `allConns` is now the full connection inventory so discovery reuses and evicts symmetrically, and dedicated cluster managers are excluded at request-routing selection instead: `RoundRobinPolicy` skips them in its `DiscoveryUpdate` add path and `multiServerPool.Next()` skips non-seed dedicated cluster managers during selection (including the no-router fallback). A user-supplied seed is exempt, so discovery still bootstraps against a dedicated cluster manager seed. See also the `IncludeDedicatedClusterManagers` removal under Removed ([#1004](https://github.com/opensearch-project/opensearch-go/pull/1004))
 - Fix `cmd/osgen` silently dropping typed structs on Go type-name collisions, and add a completeness guard so future collisions fail generation instead of degrading output. Two distinct spec schemas that derived the same Go name were reduced to one by the type registry, dropping the other to raw `json.RawMessage` (or mis-typing a field): the search `profile` container collided with the per-search `SearchProfile` item, and multiple response bodies in the `flow_framework.common` and `security_analytics.findings` groups all derived `<Group>Resp`. Colliding refs are now disambiguated via documented override tables (`typeNameCollisions` / `respTypeNameCollisions`), and a panic-guard requires any new colliding ref to be enumerated. Regenerating restores typed responses for the affected operations (e.g. flow_framework `search`/`search_state`, security_analytics `search_finding_correlations`, and the search response's `profile` field), and a response schema also referenced structurally (a search hit's `_source`) is now emitted as a standalone type instead of dangling ([#989](https://github.com/opensearch-project/opensearch-go/pull/989))
 - Fix plugin dispatch methods discarding the transport response, leaving `Inspect().Response` nil on every typed plugin response. The generated plugin dispatch template dropped the `*opensearch.Response` returned by `request()` (`if _, err := request(...)`); it now assigns it (`resp.response, err = request(...)`), matching the core client. Also serialize the discovery-path warmup recalculation and ready-list partitioning in `createOrUpdateMultiNodePoolWithLock` under the pool write lock -- `recalculateWarmupParamsWithLock`/`getWarmupParamsWithLock` (renamed to reflect the requirement) and the `mu.activeCount` write touched `mu`-guarded fields without holding `pool.mu`, racing `resurrectWithLock` (follow-up to [#981](https://github.com/opensearch-project/opensearch-go/pull/981)) ([#989](https://github.com/opensearch-project/opensearch-go/pull/989))
 - Fix node discovery hijacking the request stream with unverified, unreachable discovered nodes and masking the user-supplied seed-URL fallback. When discovered `publish_address` values are unroutable from the client (NAT'd or misconfigured clusters, e.g. a Kubernetes stack cluster in CI), a freshly discovered but never-health-checked node could be served to requests as a zombie -- failing every request with `no route to host` -- instead of returning `ErrNoConnections` and cascading to the reachable seed URL. Connections are now considered available for routing only when they are a user-supplied seed (assumed reachable) or a discovered node confirmed reachable, and every routing policy and pool (round-robin, role, coordinator, index/doc router, single-server, and multi-server pools) consistently honors that gate on both the enabled-bit and connection-selection paths, so the seed fallback serves requests until a discovered node health-checks clean ([#952](https://github.com/opensearch-project/opensearch-go/pull/952), [#954](https://github.com/opensearch-project/opensearch-go/pull/954), [#956](https://github.com/opensearch-project/opensearch-go/pull/956))

opensearchtransport/discovery_integration_internal_test.go

@@ -444,12 +444,12 @@ func TestDiscoverNodesWithNewRoleValidation(t *testing.T) {
 	}
 }
 
-// TestIncludeDedicatedClusterManagersConfiguration verifies the configurable behavior
-func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
+// TestDedicatedClusterManagersExcludedFromRouting verifies that dedicated
+// cluster managers are held in the connection inventory but never routed to.
+func TestDedicatedClusterManagersExcludedFromRouting(t *testing.T) {
 	tests := []struct {
-		name                            string
-		includeDedicatedClusterManagers bool
-		nodes                           map[string][]string // nodeName -> roles
+		name  string
+		nodes map[string][]string // nodeName -> roles
 		// expectedInInventory lists nodes that must appear in the allConns pool,
 		// which holds every discovered node regardless of role.
 		expectedInInventory []string
@@ -458,18 +458,7 @@ func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
 		expectedNotRoutable []string
 	}{
 		{
-			name:                            "IncludeDedicatedClusterManagers enabled - all nodes routable",
-			includeDedicatedClusterManagers: true,
-			nodes: map[string][]string{
-				"cm-only":   {RoleClusterManager},
-				"data-node": {RoleData},
-			},
-			expectedInInventory: []string{"cm-only", "data-node"},
-			expectedNotRoutable: []string{},
-		},
-		{
-			name:                            "IncludeDedicatedClusterManagers disabled (default) - dedicated CM in inventory but not routable",
-			includeDedicatedClusterManagers: false,
+			name: "dedicated cluster manager in inventory but not routable",
 			nodes: map[string][]string{
 				"cm-only":   {RoleClusterManager},
 				"data-node": {RoleData},
@@ -479,8 +468,7 @@ func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
 			expectedNotRoutable: []string{"cm-only"},
 		},
 		{
-			name:                            "Mixed roles with CM always routable regardless of setting",
-			includeDedicatedClusterManagers: false,
+			name: "mixed cluster_manager and data role is routable",
 			nodes: map[string][]string{
 				"cm-data": {RoleClusterManager, RoleData},
 				"dummy":   {RoleData}, // Add second node to avoid single connection pool
@@ -497,9 +485,8 @@ func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
 
 			u := mockhttp.GetOpenSearchURL(t)
 			c, err := New(Config{
-				URLs:                            []*url.URL{u},
-				Transport:                       transport,
-				IncludeDedicatedClusterManagers: tt.includeDedicatedClusterManagers,
+				URLs:      []*url.URL{u},
+				Transport: transport,
 			})
 			require.NoError(t, err)
 

opensearchtransport/discovery_internal_test.go

@@ -1177,12 +1177,12 @@ func TestDiscoverNodesWithNewRoleValidation(t *testing.T) {
 	}
 }
 
-// TestIncludeDedicatedClusterManagersConfiguration verifies the configurable behavior
-func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
+// TestDedicatedClusterManagersExcludedFromRouting verifies that dedicated
+// cluster managers are held in the connection inventory but never routed to.
+func TestDedicatedClusterManagersExcludedFromRouting(t *testing.T) {
 	tests := []struct {
-		name                            string
-		includeDedicatedClusterManagers bool
-		nodes                           map[string][]string // nodeName -> roles
+		name  string
+		nodes map[string][]string // nodeName -> roles
 		// expectedInInventory lists nodes that must appear in the allConns pool.
 		// The inventory holds every discovered node regardless of role so that
 		// discovery can reuse and evict connections; dedicated cluster managers
@@ -1193,18 +1193,7 @@ func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
 		expectedNotRoutable []string
 	}{
 		{
-			name:                            "IncludeDedicatedClusterManagers enabled - all nodes routable",
-			includeDedicatedClusterManagers: true,
-			nodes: map[string][]string{
-				"cm-only":   {RoleClusterManager},
-				"data-node": {RoleData},
-			},
-			expectedInInventory: []string{"cm-only", "data-node"},
-			expectedNotRoutable: []string{},
-		},
-		{
-			name:                            "IncludeDedicatedClusterManagers disabled (default) - dedicated CM in inventory but not routable",
-			includeDedicatedClusterManagers: false,
+			name: "dedicated cluster manager in inventory but not routable",
 			nodes: map[string][]string{
 				"cm-only":   {RoleClusterManager},
 				"data-node": {RoleData},
@@ -1214,8 +1203,7 @@ func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
 			expectedNotRoutable: []string{"cm-only"},
 		},
 		{
-			name:                            "Mixed roles with CM always routable regardless of setting",
-			includeDedicatedClusterManagers: false,
+			name: "mixed cluster_manager and data role is routable",
 			nodes: map[string][]string{
 				"cm-data": {RoleClusterManager, RoleData},
 				"dummy":   {RoleData}, // Add second node to avoid single connection pool
@@ -1314,8 +1302,7 @@ func TestIncludeDedicatedClusterManagersConfiguration(t *testing.T) {
 			// Use the seed address for discovery
 			urls := []*url.URL{{Scheme: "http", Host: seedAddr}}
 			c, err := New(Config{
-				URLs:                            urls,
-				IncludeDedicatedClusterManagers: tt.includeDedicatedClusterManagers,
+				URLs: urls,
 			})
 			require.NoError(t, err)
 

opensearchtransport/doc.go

@@ -176,8 +176,8 @@ Enable automatic cluster discovery to maintain current node information:
 		Router: router,
 	})
 
-The discovery process respects node roles and can exclude dedicated cluster manager nodes
-from request routing (controlled by IncludeDedicatedClusterManagers configuration).
+The discovery process keeps dedicated cluster manager nodes in the connection
+inventory for reuse and eviction, but excludes them from request routing.
 
 When a request fails (transport error or retryable HTTP status), the failing connection is
 marked with a needsCatUpdate flag that excludes it from scored routing candidate sets.

opensearchtransport/opensearchtransport.go

@@ -199,13 +199,6 @@ type Config struct {
 
 	DiscoverNodesInterval time.Duration
 
-	// IncludeDedicatedClusterManagers includes dedicated cluster manager nodes in request routing.
-	// When false (default), dedicated cluster manager nodes are excluded from client requests,
-	// following best practices and matching the Java client's NodeSelector.SKIP_DEDICATED_CLUSTER_MASTERS behavior.
-	// When true, all nodes including dedicated cluster managers can receive client requests.
-	// Default: false (excludes dedicated cluster managers for better performance)
-	IncludeDedicatedClusterManagers bool
-
 	// DiscoveryHealthCheckRetries sets the number of health check retries during node discovery.
 	// During cold start, health checks are performed asynchronously without blocking.
 	// During running cluster discovery, health checks are performed with retries before adding nodes.
@@ -470,11 +463,10 @@ type Transport struct {
 	discoverNodesInterval time.Duration
 	verifyDeadAfter       time.Duration
 
-	includeDedicatedClusterManagers bool
-	discoveryHealthCheckRetries     int
-	healthCheckTimeout              time.Duration
-	healthCheckMaxRetries           int
-	healthCheckJitter               float64
+	discoveryHealthCheckRetries int
+	healthCheckTimeout          time.Duration
+	healthCheckMaxRetries       int
+	healthCheckJitter           float64
 
 	resurrectTimeoutInitial      time.Duration
 	resurrectTimeoutMax          time.Duration
@@ -955,11 +947,10 @@ func New(cfg Config) (*Transport, error) {
 		discoverNodesInterval: cfg.DiscoverNodesInterval,
 		verifyDeadAfter:       verifyDeadAfter,
 
-		includeDedicatedClusterManagers: cfg.IncludeDedicatedClusterManagers,
-		discoveryHealthCheckRetries:     cfg.DiscoveryHealthCheckRetries,
-		healthCheckTimeout:              healthCheckTimeout,
-		healthCheckMaxRetries:           healthCheckMaxRetries,
-		healthCheckJitter:               healthCheckJitter,
+		discoveryHealthCheckRetries: cfg.DiscoveryHealthCheckRetries,
+		healthCheckTimeout:          healthCheckTimeout,
+		healthCheckMaxRetries:       healthCheckMaxRetries,
+		healthCheckJitter:           healthCheckJitter,
 
 		resurrectTimeoutInitial:      resurrectTimeoutInitial,
 		resurrectTimeoutMax:          resurrectTimeoutMax,
@@ -1179,22 +1170,21 @@ func New(cfg Config) (*Transport, error) {
 	// Configure policy settings for all policies in the router
 	if client.router != nil {
 		config := policyConfig{
-			ctx:                             client.ctx,
-			resurrectTimeoutInitial:         client.resurrectTimeoutInitial,
-			resurrectTimeoutMax:             client.resurrectTimeoutMax,
-			resurrectTimeoutFactorCutoff:    client.resurrectTimeoutFactorCutoff,
-			minimumResurrectTimeout:         client.minimumResurrectTimeout,
-			jitterScale:                     client.jitterScale,
-			serverMaxNewConnsPerSec:         client.serverMaxNewConnsPerSec,
-			clientsPerServer:                client.clientsPerServer,
-			healthCheck:                     client.healthCheck,
-			observer:                        client.observer.Load(),
-			poolInfoReady:                   &client.poolInfoReady,
-			clusterSearchCwnd:               &client.clusterSearch.cwnd,
-			activeListCap:                   client.activeListCapConfig,
-			standbyPromotionChecks:          client.standbyPromotionChecks,
-			includeDedicatedClusterManagers: client.includeDedicatedClusterManagers,
-			metrics:                         client.metrics,
+			ctx:                          client.ctx,
+			resurrectTimeoutInitial:      client.resurrectTimeoutInitial,
+			resurrectTimeoutMax:          client.resurrectTimeoutMax,
+			resurrectTimeoutFactorCutoff: client.resurrectTimeoutFactorCutoff,
+			minimumResurrectTimeout:      client.minimumResurrectTimeout,
+			jitterScale:                  client.jitterScale,
+			serverMaxNewConnsPerSec:      client.serverMaxNewConnsPerSec,
+			clientsPerServer:             client.clientsPerServer,
+			healthCheck:                  client.healthCheck,
+			observer:                     client.observer.Load(),
+			poolInfoReady:                &client.poolInfoReady,
+			clusterSearchCwnd:            &client.clusterSearch.cwnd,
+			activeListCap:                client.activeListCapConfig,
+			standbyPromotionChecks:       client.standbyPromotionChecks,
+			metrics:                      client.metrics,
 		}
 		// Use type assertion to check if the router (which is a Policy) implements policyConfigurable
 		if configurablePolicy, ok := client.router.(policyConfigurable); ok {
@@ -2726,7 +2716,6 @@ func (c *Transport) newMultiServerPoolFromClientWithLock(name string, m *metrics
 		metrics:                      m,
 		activeListCapConfig:          c.activeListCapConfig,
 		standbyPromotionChecks:       c.standbyPromotionChecks,
-		excludeDCM:                   !c.includeDedicatedClusterManagers,
 	}
 	pool.mu.activeListCap = c.activeListCap
 	pool.mu.healthCheck = c.healthCheck