Flight transport TLS cert hot-reload

[rishabhmaurya]

Description

DefaultSslContextProvider had a // TODO - handle certificates reload — when the Security plugin reloaded certs via PUT /_plugins/_security/api/ssl/{transport,http}/reloadcerts, the Flight server and client kept serving the old cert until node restart.

ReloadableSslContext — new SslContext wrapper that calls supplier.get().newEngine() on every connection, so reloaded certs are picked up immediately. Reload chain: reloadcerts API → Security plugin updates SslContextHandler → provider.parameters() reads it → ReloadableSslContext.newEngine() serves the new cert.

DefaultSslContextProvider — wraps server/client contexts in ReloadableSslContext, removing the TODO.

FlightTransport — removed FlightClient cache; initiateChannel now creates a FlightClient per connection (called once per node by ClusterConnectionManager) and FlightClientChannel.close() owns its lifecycle — matching the per-connection pattern of SecureNetty4Transport.

Tests

ReloadableSslContextTests — in-JVM TLS handshake before/after swapping the supplier; asserts cert serial changes on the next connection.

ReloadableSslContextFlightIT — real FlightServer/FlightClient; swaps supplier, forces reconnect via ManagedChannel.enterIdle(), asserts new cert serial is served.

How I tested it

Tested on a 2-node cluster with Security plugin (ssl_cert_reload_enabled: true). A script records cert serials on both ports (Flight 9401/9402), generates a new self-signed cert, swaps it on disk, calls reloadcerts, and asserts the serial changed on every Flight port — without restarting either node.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5f295a4)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak Risk In initiateChannel, if FlightClientChannel constructor throws an exception after the try block starts, the client is closed in the catch block. However, if an exception occurs between client creation (line 315-323) and the try block (line 325), the client won't be closed, potentially leaking resources. protected TcpChannel initiateChannel(DiscoveryNode node) throws IOException { TransportAddress publishAddress = node.getStreamAddress(); String address = publishAddress.getAddress(); int flightPort = publishAddress.address().getPort(); Location location = sslContextProvider != null ? Location.forGrpcTls(address, flightPort) : Location.forGrpcInsecure(address, flightPort); HeaderContext context = new HeaderContext(); ClientHeaderMiddleware.Factory factory = new ClientHeaderMiddleware.Factory(context, getVersion()); FlightClient client = OSFlightClient.builder() .allocator(clientAllocator) .location(location) .channelType(ServerConfig.clientChannelType()) .eventLoopGroup(workerEventLoopGroup) .sslContext(sslContextProvider != null ? sslContextProvider.getClientSslContext() : null) .executor(clientExecutor) .intercept(factory) .build(); try { return new FlightClientChannel( boundAddress, client, node, location, context, DEFAULT_PROFILE, getResponseHandlers(), threadPool, this.inboundHandler.getMessageListener(), namedWriteableRegistry, statsCollector, config ); } catch (Exception e) { try { client.close(); } catch (Exception ce) { e.addSuppressed(ce); } throw e; } } Null Parameter Both buildServerSslContext and buildClientSslContext call provider.parameters(null).get(). The null parameter should be validated or documented to ensure it's intentional and won't cause issues in the provider implementation. SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null).get(); return SslContextBuilder.forServer(parameters.keyManagerFactory().get()) .sslProvider(SslProvider.valueOf(parameters.sslProvider().get().toUpperCase(Locale.ROOT))) .clientAuth(ClientAuth.valueOf(parameters.clientAuth().get().toUpperCase(Locale.ROOT))) .protocols(parameters.protocols()) .ciphers(parameters.cipherSuites(), SupportedCipherSuiteFilter.INSTANCE) .sessionCacheSize(0) .sessionTimeout(0) .applicationProtocolConfig( new ApplicationProtocolConfig( ApplicationProtocolConfig.Protocol.ALPN, ApplicationProtocolConfig.SelectorFailureBehavior.NO_ADVERTISE, ApplicationProtocolConfig.SelectedListenerFailureBehavior.ACCEPT, ApplicationProtocolNames.HTTP_2, ApplicationProtocolNames.HTTP_1_1 ) ) .trustManager(parameters.trustManagerFactory().get()) .build(); } catch (SSLException e) { throw new RuntimeException(e); } } private static SslContext buildClientSslContext(SecureTransportSettingsProvider provider, Settings settings) { try { SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null).get(); Exception Handling The close() method catches and logs exceptions from client.close() but continues execution. Consider whether suppressed exceptions should be tracked or if close failures should be propagated in certain scenarios. try { client.close(); } catch (Exception e) { logger.warn("Failed to close FlightClient for node [" + node.getId() + "]", e); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5f295a4

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle empty Optional safely The code calls .get() on an Optional without checking if it's present, which will throw NoSuchElementException if empty. This could cause unexpected failures during SSL context initialization. Add a check or use orElseThrow() with a descriptive exception message. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java [78] -SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null).get(); +SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null) + .orElseThrow(() -> new IllegalStateException("Failed to retrieve secure transport parameters")); Suggestion importance[1-10]: 6 __ Why: The .get() call on Optional without checking could throw NoSuchElementException. Using orElseThrow() with a descriptive message would provide better error handling and clearer failure diagnostics.	Low
General	Avoid catching overly broad exceptions The exception handling in initiateChannel catches a generic Exception which is too broad. This could inadvertently catch and rethrow critical errors like OutOfMemoryError or ThreadDeath. Consider catching more specific exceptions like IOException or creating a custom exception type. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/transport/FlightTransport.java [340-347] -} catch (Exception e) { +} catch (IOException | RuntimeException e) { try { client.close(); } catch (Exception ce) { e.addSuppressed(ce); } throw e; } Suggestion importance[1-10]: 5 __ Why: Catching Exception is broad, but the code properly handles cleanup and rethrows. Using more specific exception types like IOException | RuntimeException would be better practice, though the current implementation is functional.	Low
	Handle client close exceptions properly The close() method swallows exceptions from client.close(), which could mask critical cleanup failures. Consider rethrowing the exception or at least logging it at ERROR level to ensure visibility of resource cleanup issues that might lead to resource leaks. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/transport/FlightClientChannel.java [152-156] try { client.close(); } catch (Exception e) { - logger.warn("Failed to close FlightClient for node [" + node.getId() + "]", e); + logger.error("Failed to close FlightClient for node [" + node.getId() + "]", e); + throw new RuntimeException("FlightClient cleanup failed", e); } Suggestion importance[1-10]: 4 __ Why: While logging at ERROR level may be appropriate, rethrowing the exception from a close() method could cause issues for callers expecting idempotent cleanup. The current WARN level is reasonable for a best-effort cleanup scenario.	Low

---

Previous suggestions

Suggestions up to commit 0e23fc7

Category	Suggestion	Impact
Possible issue	Safely unwrap Optional SSL parameters Calling get() on an Optional without checking if it's present can throw NoSuchElementException if the parameters are absent. This could cause the SSL context creation to fail unexpectedly. Verify the Optional is present before calling get() or use orElseThrow() with a descriptive exception. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java [78] -SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null).get(); +SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null) + .orElseThrow(() -> new IllegalStateException("SSL parameters not available")); Suggestion importance[1-10]: 6 __ Why: Valid concern about calling get() on an Optional without checking. Using orElseThrow() with a descriptive message would improve error handling. However, the impact is moderate since this likely fails fast during initialization rather than at runtime.	Low
	Validate SSL context before creating engine If contextSupplier.get() returns null or throws an exception during cert reload, the newEngine() methods will fail with NullPointerException or propagate the exception, potentially breaking active connections. Add null checks and exception handling to provide graceful degradation or fallback to the initial context. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/ReloadableSslContext.java [48-56] @Override public SSLEngine newEngine(ByteBufAllocator alloc) { - return contextSupplier.get().newEngine(alloc); + SslContext ctx = contextSupplier.get(); + if (ctx == null) { + throw new IllegalStateException("SSL context supplier returned null"); + } + return ctx.newEngine(alloc); } @Override public SSLEngine newEngine(ByteBufAllocator alloc, String peerHost, int peerPort) { - return contextSupplier.get().newEngine(alloc, peerHost, peerPort); + SslContext ctx = contextSupplier.get(); + if (ctx == null) { + throw new IllegalStateException("SSL context supplier returned null"); + } + return ctx.newEngine(alloc, peerHost, peerPort); } Suggestion importance[1-10]: 5 __ Why: Adding null checks for contextSupplier.get() is a reasonable defensive programming practice. However, if the supplier is properly initialized (as it appears to be in DefaultSslContextProvider), null should never occur. The suggestion adds safety but may be unnecessary given the design.	Low
General	Handle client close failures properly The close() method catches all exceptions but continues silently after logging. If closing the client fails, the channel may remain in an inconsistent state. Consider rethrowing the exception or setting a flag to prevent further operations on this channel. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/transport/FlightClientChannel.java [152-156] try { client.close(); } catch (Exception e) { - logger.warn("Failed to close FlightClient for node [" + node.getId() + "]", e); + logger.error("Failed to close FlightClient for node [" + node.getId() + "]", e); + throw new RuntimeException("Failed to close FlightClient", e); } Suggestion importance[1-10]: 4 __ Why: The suggestion to rethrow exceptions on client close failure is overly aggressive. Logging a warning is appropriate for cleanup operations in a close() method, as throwing exceptions could disrupt shutdown sequences. Changing to logger.error might be reasonable, but rethrowing is not.	Low

Suggestions up to commit 1ee835b

Category	Suggestion	Impact
Possible issue	Handle missing TLS parameters safely The .get() call on an Optional can throw NoSuchElementException if the parameters are absent. This could cause the server or client to fail during initialization or reload. Add proper error handling or validation to ensure parameters are present before calling .get(). plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java [78] -SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null).get(); +SecureTransportSettingsProvider.SecureTransportParameters parameters = provider.parameters(null) + .orElseThrow(() -> new IllegalStateException("TLS parameters are not available")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that calling .get() on an Optional without checking can throw NoSuchElementException. However, the impact is moderate since this is likely a configuration issue that would be caught during initialization, not a runtime bug in normal operation.	Medium
General	Preserve SSL exception context properly Wrapping SSLException in a generic RuntimeException loses important context about the TLS configuration failure. Use a more specific exception type or preserve the original exception type to help with debugging and error handling. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java [97-99] } catch (SSLException e) { - throw new RuntimeException(e); + throw new IllegalStateException("Failed to build SSL context", e); } Suggestion importance[1-10]: 5 __ Why: While using IllegalStateException provides slightly better semantics than generic RuntimeException, both preserve the original exception as the cause. The improvement is marginal and mainly stylistic.	Low
	Use parameterized logging for efficiency String concatenation in logging statements can create unnecessary string objects even when the log level is disabled. Use parameterized logging to improve performance and avoid unnecessary allocations during normal operation. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/transport/FlightClientChannel.java [152-156] try { client.close(); } catch (Exception e) { - logger.warn("Failed to close FlightClient for node [" + node.getId() + "]", e); + logger.warn("Failed to close FlightClient for node [{}]", node.getId(), e); } Suggestion importance[1-10]: 4 __ Why: The suggestion is correct about using parameterized logging for better performance. However, the impact is minor since this is only executed during error conditions (client close failures), not in hot paths.	Low

[github-actions]

Persistent review updated to latest commit 0e23fc7

[github-actions]

Persistent review updated to latest commit 5f295a4

[github-actions]

❌ Gradle check result for 5f295a4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❕ Gradle check result for 5f295a4: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 56.86275% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.24%. Comparing base (c856e62) to head (5f295a4).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...light/bootstrap/tls/DefaultSslContextProvider.java	0.00%	10 Missing ⚠️
...search/arrow/flight/transport/FlightTransport.java	62.96%	8 Missing and 2 partials ⚠️
...ch/arrow/flight/transport/FlightClientChannel.java	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20700      +/-   ##
============================================
- Coverage     73.25%   73.24%   -0.02%     
- Complexity    71966    72003      +37     
============================================
  Files          5781     5782       +1     
  Lines        329414   329427      +13     
  Branches      47531    47530       -1     
============================================
- Hits         241307   241281      -26     
- Misses        68741    68820      +79     
+ Partials      19366    19326      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.