feat(plugin-native-sidecar): Add internal communication auth layer to sidecar http-clients#27184
Merged
pdabre12 merged 4 commits intoprestodb:masterfrom Mar 13, 2026
Merged
Conversation
Contributor
Reviewer's GuideAdds an internal-authenticated communication layer for native sidecar-related HTTP clients and propagates coordinator internal communication settings (via a new AuthClientConfigs value object) into function namespaces, expression optimizers, session property providers, and plan checker providers, including wiring through server/test infrastructure and validating via expanded tests. Sequence diagram for coordinator auth config propagation to sidecar HTTP clientssequenceDiagram
participant PrestoServer
participant Injector
participant InternalCommunicationConfig
participant NodeInfo
participant AuthClientConfigs
participant StaticFunctionNamespaceStore
participant FunctionAndTypeManager
participant NativeFunctionNamespaceManagerFactory
participant NativeSidecarCommunicationModule
participant CommonInternalCommunicationModule
participant InternalAuthenticationManager
participant SidecarHttpClient
PrestoServer->>Injector: getInstance(InternalCommunicationConfig)
PrestoServer->>Injector: getInstance(NodeInfo)
PrestoServer->>AuthClientConfigs: createAuthClientConfigs(InternalCommunicationConfig, NodeInfo)
PrestoServer-->>AuthClientConfigs: authClientConfigs
PrestoServer->>Injector: getInstance(StaticFunctionNamespaceStore)
PrestoServer->>StaticFunctionNamespaceStore: loadFunctionNamespaceManagers(authClientConfigs)
StaticFunctionNamespaceStore->>FunctionAndTypeManager: loadFunctionNamespaceManager(..., authClientConfigs)
FunctionAndTypeManager->>NativeFunctionNamespaceManagerFactory: create(catalogName, properties, FunctionNamespaceManagerContext(..., authClientConfigs))
NativeFunctionNamespaceManagerFactory->>NativeSidecarCommunicationModule: new NativeSidecarCommunicationModule(authClientConfigs)
NativeFunctionNamespaceManagerFactory->>CommonInternalCommunicationModule: bindInternalAuth(binder, authClientConfigs)
CommonInternalCommunicationModule->>InternalAuthenticationManager: new InternalAuthenticationManager(authClientConfigs.getSharedSecret(), authClientConfigs.getNodeId(), authClientConfigs.isInternalJwtEnabled())
CommonInternalCommunicationModule->>SidecarHttpClient: bindGlobalFilter(InternalAuthenticationManager)
PrestoServer->>Injector: getInstance(ExpressionOptimizerManager)
PrestoServer->>ExpressionOptimizerManager: loadExpressionOptimizerFactories(authClientConfigs)
note over ExpressionOptimizerManager,NativeSidecarCommunicationModule: Similar flow for expression optimizer, session property providers, and plan checker providers, each passing AuthClientConfigs into NativeSidecarCommunicationModule and binding InternalAuthenticationManager on their HTTP clients
Class diagram for AuthClientConfigs propagation and contextsclassDiagram
class AuthClientConfigs {
+String nodeId
+String keyStorePath
+String keyStorePassword
+String trustStorePath
+String trustStorePassword
+Optional~String~ excludeCipherSuites
+Optional~String~ includedCipherSuites
+boolean internalJwtEnabled
+Optional~String~ sharedSecret
+static AuthClientConfigs defaultAuthClientConfigs(String nodeId)
+String getNodeId()
+String getKeyStorePath()
+String getKeyStorePassword()
+String getTrustStorePath()
+String getTrustStorePassword()
+Optional~String~ getExcludeCipherSuites()
+Optional~String~ getIncludedCipherSuites()
+boolean isInternalJwtEnabled()
+Optional~String~ getSharedSecret()
}
class ExpressionOptimizerContext {
-NodeManager nodeManager
-RowExpressionSerde rowExpressionSerde
-FunctionMetadataManager functionMetadataManager
-StandardFunctionResolution functionResolution
-AuthClientConfigs authClientConfigs
+ExpressionOptimizerContext(NodeManager nodeManager, RowExpressionSerde rowExpressionSerde, FunctionMetadataManager functionMetadataManager, StandardFunctionResolution functionResolution, AuthClientConfigs authClientConfigs)
+NodeManager getNodeManager()
+RowExpressionSerde getRowExpressionSerde()
+FunctionMetadataManager getFunctionMetadataManager()
+StandardFunctionResolution getFunctionResolution()
+AuthClientConfigs getAuthClientConfigs()
}
class FunctionNamespaceManagerContext {
-TypeManager typeManager
-NodeManager nodeManager
-FunctionMetadataManager functionMetadataManager
-AuthClientConfigs authClientConfigs
+FunctionNamespaceManagerContext(TypeManager typeManager, NodeManager nodeManager, FunctionMetadataManager functionMetadataManager, AuthClientConfigs authClientConfigs)
+TypeManager getTypeManager()
+NodeManager getNodeManager()
+FunctionMetadataManager getFunctionMetadataManager()
+AuthClientConfigs getAuthClientConfigs()
}
class SessionPropertyContext {
-Optional~TypeManager~ typeManager
-Optional~NodeManager~ nodeManager
-AuthClientConfigs authClientConfigs
+SessionPropertyContext(Optional~TypeManager~ typeManager, Optional~NodeManager~ nodeManager, AuthClientConfigs authClientConfigs)
+NodeManager getNodeManager()
+TypeManager getTypeManager()
+AuthClientConfigs getAuthClientConfigs()
}
class PlanCheckerProviderContext {
-SimplePlanFragmentSerde simplePlanFragmentSerde
-NodeManager nodeManager
-AuthClientConfigs authClientConfigs
+PlanCheckerProviderContext(SimplePlanFragmentSerde simplePlanFragmentSerde, NodeManager nodeManager, AuthClientConfigs authClientConfigs)
+SimplePlanFragmentSerde getSimplePlanFragmentSerde()
+NodeManager getNodeManager()
+AuthClientConfigs getAuthClientConfigs()
}
class CommonInternalCommunicationModule {
+void setup(Binder binder)
+static void bindHttpClientDefaults(Binder binder, AuthClientConfigs authClientConfigs)
+static void bindInternalAuth(Binder binder, AuthClientConfigs authClientConfigs)
}
class InternalAuthenticationManager {
-boolean internalJwtEnabled
-byte[] hmac
-String nodeId
-Optional~String~ sharedSecret
+InternalAuthenticationManager(InternalCommunicationConfig config, NodeInfo nodeInfo)
+InternalAuthenticationManager(Optional~String~ sharedSecret, String nodeId, boolean internalJwtEnabled)
+Request filterRequest(Request request)
+Optional~String~ getSharedSecret()
}
class NativeSidecarCommunicationModule {
-AuthClientConfigs authClientConfigs
+NativeSidecarCommunicationModule(AuthClientConfigs authClientConfigs)
+void configure(Binder binder)
}
class RestBasedCommunicationModule {
-AuthClientConfigs authClientConfigs
+RestBasedCommunicationModule(AuthClientConfigs authClientConfigs)
+void configure(Binder binder)
}
class FunctionAndTypeManager {
+void loadFunctionNamespaceManager(String functionNamespaceManagerName, String catalogName, Map~String,String~ properties, NodeManager nodeManager, AuthClientConfigs authClientConfigs)
}
class ExpressionOptimizerManager {
+void loadExpressionOptimizerFactories(AuthClientConfigs authClientConfigs)
+void loadExpressionOptimizerFactory(File configurationFile, AuthClientConfigs authClientConfigs)
+void loadExpressionOptimizerFactory(String factoryName, String optimizerName, Map~String,String~ properties, AuthClientConfigs authClientConfigs)
}
class SessionPropertyManager {
+void loadSessionPropertyProviders(AuthClientConfigs authClientConfigs)
+void loadSessionPropertyProvider(String sessionPropertyProviderName, Map~String,String~ properties, Optional~TypeManager~ typeManager, Optional~NodeManager~ nodeManager, AuthClientConfigs authClientConfigs)
}
class StaticFunctionNamespaceStore {
+void loadFunctionNamespaceManagers(AuthClientConfigs authClientConfigs)
+void loadFunctionNamespaceManagers(Map~String,Map~String,String~~ catalogProperties, AuthClientConfigs authClientConfigs)
}
class PlanCheckerProviderManager {
+void loadPlanCheckerProviders(NodeManager nodeManager, AuthClientConfigs authClientConfigs)
+void loadPlanCheckerProvider(String planCheckerProviderName, Map~String,String~ properties, NodeManager nodeManager, AuthClientConfigs authClientConfigs)
}
class NativeSidecarExpressionInterpreter {
-HttpClient httpClient
+HttpClient getHttpClient()
}
class NativePlanChecker {
-HttpClient httpClient
+HttpClient getHttpClient()
}
class NativeExpressionOptimizer {
-NativeSidecarExpressionInterpreter rowExpressionInterpreterService
+NativeSidecarExpressionInterpreter getRowExpressionInterpreterService()
}
AuthClientConfigs --> ExpressionOptimizerContext : provided_to
AuthClientConfigs --> FunctionNamespaceManagerContext : provided_to
AuthClientConfigs --> SessionPropertyContext : provided_to
AuthClientConfigs --> PlanCheckerProviderContext : provided_to
AuthClientConfigs --> CommonInternalCommunicationModule : used_by
AuthClientConfigs --> NativeSidecarCommunicationModule : ctor_arg
AuthClientConfigs --> RestBasedCommunicationModule : ctor_arg
AuthClientConfigs --> InternalAuthenticationManager : ctor_arg
CommonInternalCommunicationModule --> InternalAuthenticationManager : creates
CommonInternalCommunicationModule --> HttpClientConfig : config_defaults
StaticFunctionNamespaceStore --> FunctionAndTypeManager : uses
FunctionAndTypeManager --> FunctionNamespaceManagerContext : creates
ExpressionOptimizerManager --> ExpressionOptimizerContext : creates
SessionPropertyManager --> SessionPropertyContext : creates
PlanCheckerProviderManager --> PlanCheckerProviderContext : creates
NativeExpressionOptimizer --> NativeSidecarExpressionInterpreter : uses
NativeSidecarExpressionInterpreter --> HttpClient : uses
NativePlanChecker --> HttpClient : uses
NativeSidecarCommunicationModule --> InternalAuthenticationManager : binds_filter
RestBasedCommunicationModule --> InternalAuthenticationManager : binds_filter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
pdabre12
force-pushed
the
internal-auth-sidecar
branch
4 times, most recently
from
12826cf to
e44db6c
Compare
pdabre12
requested review from
a team,
elharo,
feilong-liu,
jaystarshot and
shrinidhijoshi
as code owners
prestodb-ci
requested review from
a team,
jkhaliqi and
sh-shamsan
and removed request for
a team
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- When merging coordinator properties into the various manager/optimizer/session provider property maps (e.g., in DistributedQueryRunner, StaticFunctionNamespaceStore, SessionPropertyManager, PlanCheckerProviderManager, ExpressionOptimizerManager),
putAll(server.getAllCoordinatorProperties())is called after copying the existing properties, which means coordinator properties override per-component settings on key collisions; consider reversing the merge order or documenting this precedence explicitly if the intention is to let component-specific configuration win. - The
testInternalAuthenticationFilterhelper asserts that there is exactly oneHttpRequestFilterand that it is anInternalAuthenticationManager; this will make tests brittle if additional filters are ever added—consider instead asserting that at least one filter is anInternalAuthenticationManagerwithout constraining the overall list size or position.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When merging coordinator properties into the various manager/optimizer/session provider property maps (e.g., in DistributedQueryRunner, StaticFunctionNamespaceStore, SessionPropertyManager, PlanCheckerProviderManager, ExpressionOptimizerManager), `putAll(server.getAllCoordinatorProperties())` is called after copying the existing properties, which means coordinator properties override per-component settings on key collisions; consider reversing the merge order or documenting this precedence explicitly if the intention is to let component-specific configuration win.
- The `testInternalAuthenticationFilter` helper asserts that there is exactly one `HttpRequestFilter` and that it is an `InternalAuthenticationManager`; this will make tests brittle if additional filters are ever added—consider instead asserting that at least one filter is an `InternalAuthenticationManager` without constraining the overall list size or position.
## Individual Comments
### Comment 1
<location path="presto-main-base/src/main/java/com/facebook/presto/sql/expressions/ExpressionOptimizerManager.java" line_range="103-104" />
<code_context>
checkArgument(!optimizerName.equals(DEFAULT_EXPRESSION_OPTIMIZER_NAME), "Cannot name an expression optimizer instance %s", DEFAULT_EXPRESSION_OPTIMIZER_NAME);
Map<String, String> properties = new HashMap<>(loadProperties(configurationFile));
+ properties.putAll(allCoordinatorProperties);
String factoryName = properties.remove(EXPRESSION_MANAGER_FACTORY_NAME);
checkArgument(!isNullOrEmpty(factoryName), "%s does not contain %s", configurationFile, EXPRESSION_MANAGER_FACTORY_NAME);
</code_context>
<issue_to_address>
**🚨 issue (security):** Expression optimizer configs now get all coordinator properties merged in, which could both override local settings and expose unrelated configuration.
Merging the full coordinator property set into the optimizer config (and doing so after the file) can both override optimizer-specific settings and unnecessarily expose unrelated coordinator properties, including potential secrets, to optimizer factories. If factories only require a few values, consider limiting what is passed (e.g., via a dedicated namespace or a filtered subset) and/or changing the merge order so optimizer config values reliably take precedence.
</issue_to_address>
### Comment 2
<location path="presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java" line_range="166-167" />
<code_context>
injector.getInstance(PluginManager.class).loadPlugins();
+ // get all coordinator configs
+ Map<String, String> allCoordinatorConfigs = injector.getInstance(ConfigurationFactory.class).getProperties();
+
ServerConfig serverConfig = injector.getInstance(ServerConfig.class);
</code_context>
<issue_to_address>
**🚨 question (security):** Passing the full coordinator configuration into multiple plugin managers could unintentionally leak sensitive settings.
`allCoordinatorConfigs` is now exposed to multiple extension points (function namespaces, session properties, plan checkers, expression optimizers), giving plugins broad access to all coordinator properties, potentially including credentials or other sensitive values. If you only need specific settings (e.g., internal communication config), consider passing a filtered subset (by prefix or explicit allowlist) instead of the full `ConfigurationFactory` properties.
</issue_to_address>
### Comment 3
<location path="presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java" line_range="789-795" />
<code_context>
.collect(Collectors.toList());
}
+
+ private static void testInternalAuthenticationFilter(HttpClient httpClient)
+ {
+ // check if filter present
+ List<HttpRequestFilter> filters = ((JettyHttpClient) httpClient).getRequestFilters();
+ assertEquals(filters.size(), 1);
+ assertTrue(filters.get(0) instanceof InternalAuthenticationManager);
+ }
}
</code_context>
<issue_to_address>
**suggestion (testing):** Make the internal-auth filter assertion more robust and less implementation-specific
This helper currently assumes the client is always `JettyHttpClient` and that there is exactly one filter, `InternalAuthenticationManager`. That makes the test brittle if another filter is added or the client implementation changes. Prefer asserting that the filter list contains an `InternalAuthenticationManager`, e.g.:
```java
List<HttpRequestFilter> filters = ((JettyHttpClient) httpClient).getRequestFilters();
assertTrue(filters.stream().anyMatch(filter -> filter instanceof InternalAuthenticationManager));
```
You can still assert the list is non-empty if desired, but relying on `size() == 1` will cause unnecessary test failures when unrelated filters are added.
```suggestion
private static void testInternalAuthenticationFilter(HttpClient httpClient)
{
// check if filter present
List<HttpRequestFilter> filters = ((JettyHttpClient) httpClient).getRequestFilters();
assertFalse(filters.isEmpty(), "Expected at least one HttpRequestFilter to be configured");
assertTrue(
filters.stream().anyMatch(filter -> filter instanceof InternalAuthenticationManager),
"Expected an InternalAuthenticationManager filter to be configured");
}
```
</issue_to_address>
### Comment 4
<location path="presto-main-base/src/test/java/com/facebook/presto/sql/planner/sanity/TestPlanCheckerProviderManager.java" line_range="44-45" />
<code_context>
.setPlanCheckerConfigurationDir(new File("src/test/resources/plan-checkers"));
PlanCheckerProviderManager planCheckerProviderManager = new PlanCheckerProviderManager(new JsonCodecSimplePlanFragmentSerde(JsonCodec.jsonCodec(SimplePlanFragment.class)), planCheckerProviderManagerConfig);
planCheckerProviderManager.addPlanCheckerProviderFactory(new TestingPlanCheckerProviderFactory());
- planCheckerProviderManager.loadPlanCheckerProviders(new TestingNodeManager());
+ planCheckerProviderManager.loadPlanCheckerProviders(new TestingNodeManager(), emptyMap());
assertEquals(planCheckerProviderManager.getPlanCheckerProviders(), ImmutableList.of(TESTING_PLAN_CHECKER_PROVIDER));
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for merging coordinator properties into plan checker provider configuration
`PlanCheckerProviderManager.loadPlanCheckerProviders` now merges `allCoordinatorConfigs` into each provider’s properties, but these tests only pass `emptyMap()` and don’t validate that behavior. Please add a test where a property is omitted from the plan checker config file but provided via `allCoordinatorConfigs`, and have `TestingPlanCheckerProviderFactory` (or the resulting `PlanCheckerProvider`) assert that the property is present. That will verify the new coordinator config propagation end-to-end.
Suggested implementation:
```java
import static com.facebook.presto.sql.planner.sanity.TestPlanCheckerProviderManager.TestingPlanCheckerProvider.TESTING_PLAN_CHECKER_PROVIDER;
import static com.facebook.presto.testing.assertions.Assert.assertEquals;
import static java.util.Collections.emptyMap;
import java.util.Map;
import com.facebook.presto.sql.planner.sanity.PlanCheckerProviderManager.PlanCheckerProviderFactory;
import com.facebook.presto.sql.planner.sanity.PlanCheckerProviderManager.PlanCheckerProvider;
import com.google.common.collect.ImmutableMap;
```
```java
PlanCheckerProviderManagerConfig planCheckerProviderManagerConfig = new PlanCheckerProviderManagerConfig()
.setPlanCheckerConfigurationDir(new File("src/test/resources/plan-checkers"));
PlanCheckerProviderManager planCheckerProviderManager = new PlanCheckerProviderManager(new JsonCodecSimplePlanFragmentSerde(JsonCodec.jsonCodec(SimplePlanFragment.class)), planCheckerProviderManagerConfig);
planCheckerProviderManager.loadPlanCheckerProviders(new TestingNodeManager(), emptyMap());
}
@Test
public void testCoordinatorConfigsMergedIntoPlanCheckerProviderConfiguration()
{
// Coordinator-level config that is not present in the plan checker config file
Map<String, String> allCoordinatorConfigs = ImmutableMap.of(
"coordinator-only-property", "coordinator-value");
PlanCheckerProviderManagerConfig planCheckerProviderManagerConfig = new PlanCheckerProviderManagerConfig()
.setPlanCheckerConfigurationDir(new File("src/test/resources/plan-checkers"));
PlanCheckerProviderManager planCheckerProviderManager = new PlanCheckerProviderManager(
new JsonCodecSimplePlanFragmentSerde(JsonCodec.jsonCodec(SimplePlanFragment.class)),
planCheckerProviderManagerConfig);
TestingPlanCheckerProviderFactory factory = new TestingPlanCheckerProviderFactory();
planCheckerProviderManager.addPlanCheckerProviderFactory(factory);
planCheckerProviderManager.loadPlanCheckerProviders(new TestingNodeManager(), allCoordinatorConfigs);
// Verify that the PlanCheckerProvider received the merged coordinator configs
assertEquals(factory.getLastProperties().get("coordinator-only-property"), "coordinator-value");
}
public static class TestingPlanCheckerProviderFactory
```
```java
public static class TestingPlanCheckerProviderFactory
implements PlanCheckerProviderFactory
{
private volatile Map<String, String> lastProperties;
@Override
public String getName()
{
return "testing";
}
@Override
public PlanCheckerProvider create(Map<String, String> properties)
{
this.lastProperties = ImmutableMap.copyOf(properties);
return TESTING_PLAN_CHECKER_PROVIDER;
}
public Map<String, String> getLastProperties()
{
return lastProperties;
}
}
```
The above edits assume the following, which you may need to align with the actual codebase:
1. `PlanCheckerProviderManager` exposes nested interfaces `PlanCheckerProviderFactory` and `PlanCheckerProvider` with a `create(Map<String, String> properties)` method and a `getName()` method on the factory. If the real signatures differ (e.g., different package, non-nested interfaces, different method names/parameters), update the imports and `TestingPlanCheckerProviderFactory` accordingly.
2. If `TestingPlanCheckerProviderFactory` already has an implementation elsewhere in the file, merge the `lastProperties` field, `getLastProperties()` accessor, and properties-capturing logic into the existing implementation instead of replacing it wholesale.
3. If `@Test` is not yet imported in this file, add `import org.testng.annotations.Test;` (or the appropriate test framework import) alongside the other imports.
4. If `PlanCheckerProviderManager.loadPlanCheckerProviders(...)` has a different signature than `(TestingNodeManager, Map<String,String>)`, adapt the call site in the new test to the actual method signature while still passing `allCoordinatorConfigs` through.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...-main-base/src/main/java/com/facebook/presto/sql/expressions/ExpressionOptimizerManager.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java
Outdated
Show resolved
Hide resolved
...native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java
Show resolved
Hide resolved
...ase/src/test/java/com/facebook/presto/sql/planner/sanity/TestPlanCheckerProviderManager.java
Show resolved
Hide resolved
tdcmeehan
reviewed
Mar 10, 2026
| Injector injector = app | ||
| .doNotInitializeLogging() | ||
| .setRequiredConfigurationProperties(config) | ||
| .noStrictConfig() |
Contributor
Author
There was a problem hiding this comment.
Yes, its needed because we are passing down the internal communication configs here too.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The logic for
extractInternalCommunicationConfigsis duplicated betweenPrestoServerandTestingPrestoServer; consider extracting this into a shared utility to avoid drift and keep the set of propagated keys consistent. - In
InternalAuthenticationManager, thesharedSecretfield is not initialized in the ctor that takesInternalCommunicationConfigandNodeInfo, sogetSharedSecret()can returnnullor throw; either initializesharedSecretthere or document that it is only valid for the alternate constructor and guard calls accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for `extractInternalCommunicationConfigs` is duplicated between `PrestoServer` and `TestingPrestoServer`; consider extracting this into a shared utility to avoid drift and keep the set of propagated keys consistent.
- In `InternalAuthenticationManager`, the `sharedSecret` field is not initialized in the ctor that takes `InternalCommunicationConfig` and `NodeInfo`, so `getSharedSecret()` can return `null` or throw; either initialize `sharedSecret` there or document that it is only valid for the alternate constructor and guard calls accordingly.
## Individual Comments
### Comment 1
<location path="presto-main/src/main/java/com/facebook/presto/server/testing/TestingPrestoServer.java" line_range="959-968" />
<code_context>
throw new IllegalArgumentException("Presto announcement not found: " + announcements);
}
+
+ private static Map<String, String> extractInternalCommunicationConfigs(Map<String, String> properties)
+ {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ properties.forEach((key, value) -> {
+ // Requests through plugin HTTP clients should be treated as coming from the same node as the coordinator,
+ // so we propagate the required node properties here.
+ if (key.startsWith("internal-communication.") || key.equals("node.id") || key.equals("node.environment")) {
+ builder.put(key, value);
+ }
+ });
+
+ return builder.build();
+ }
}
</code_context>
<issue_to_address>
**suggestion:** The helper for extracting internal communication configs is duplicated; consider centralizing it to avoid divergence.
`extractInternalCommunicationConfigs` now exists both in `PrestoServer` and `TestingPrestoServer` with only minor differences. Consolidating into a shared utility (or reusing the `PrestoServer` version) will reduce duplication and prevent them from drifting if the set of propagated `internal-communication.*` or node properties changes.
Suggested implementation:
```java
import com.facebook.presto.server.PrestoServer;
import com.facebook.presto.server.testing.TestingPrestoServer;
```
```java
authClientConfigs = PrestoServer.extractInternalCommunicationConfigs(serverConfig);
```
```java
```
To fully centralize the helper and avoid duplication, you will also need to:
1. In `presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java`, change the helper signature from:
`private static Map<String, String> extractInternalCommunicationConfigs(Map<String, String> properties)`
to:
`public static Map<String, String> extractInternalCommunicationConfigs(Map<String, String> properties)`.
2. Ensure the import I referenced in the first SEARCH/REPLACE block matches your existing import section; adjust the searched line if your current imports differ (e.g., you may not have an explicit `TestingPrestoServer` import line to anchor on).
3. If the initialization of `authClientConfigs` in `TestingPrestoServer` uses a slightly different expression (e.g., wraps `serverConfig` in `ImmutableMap.copyOf`), update the SEARCH pattern accordingly, keeping the replacement call to `PrestoServer.extractInternalCommunicationConfigs(...)`.
</issue_to_address>
### Comment 2
<location path="presto-main/src/main/java/com/facebook/presto/server/testing/TestingPrestoServer.java" line_range="963-964" />
<code_context>
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ for (Map.Entry<String, String> entry : properties.entrySet()) {
+ // The requests coming through the plugin http-clients should be considered as coming thru the same node as the coordinator, hence adding required node properties here.
+ if (entry.getKey().startsWith("internal-communication.") || entry.getKey().equals("node.id") || entry.getKey().equals("node.environment")) {
+ builder.put(entry.getKey(), entry.getValue());
</code_context>
<issue_to_address>
**nitpick (typo):** Minor wording/spacing issues in the comment that could be cleaned up for clarity.
This comment has minor nits (`thru` → `through`, double space before `as`). Suggested wording: `The requests coming through the plugin HTTP clients should be considered as coming from the same node as the coordinator, hence adding the required node properties here.`
```suggestion
for (Map.Entry<String, String> entry : properties.entrySet()) {
// The requests coming through the plugin HTTP clients should be considered as coming from the same node as the coordinator, hence adding the required node properties here.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-main/src/main/java/com/facebook/presto/server/testing/TestingPrestoServer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/testing/TestingPrestoServer.java
Outdated
Show resolved
Hide resolved
pdabre12
force-pushed
the
internal-auth-sidecar
branch
from
e47c6fb to
16f7a95
Compare
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In InternalAuthenticationManager, the constructor that takes InternalCommunicationConfig/NodeInfo never assigns the sharedSecret field, so getSharedSecret() can return null and NPE; consider initializing sharedSecret there (e.g., Optional.ofNullable(internalCommunicationConfig.getSharedSecret())) to keep the two construction paths consistent.
- The pattern of building an allProperties map by copying properties and then putAll(authClientConfigs) is duplicated in several places (DistributedQueryRunner.loadFunctionNamespaceManager/loadExpressionOptimizer/loadSessionPropertyProvider/loadPlanCheckerProviderManager, StaticFunctionNamespaceStore, SessionPropertyManager, PlanCheckerProviderManager, ExpressionOptimizerManager); consider extracting a small helper or utility to centralize this merge logic and avoid inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In InternalAuthenticationManager, the constructor that takes InternalCommunicationConfig/NodeInfo never assigns the sharedSecret field, so getSharedSecret() can return null and NPE; consider initializing sharedSecret there (e.g., Optional.ofNullable(internalCommunicationConfig.getSharedSecret())) to keep the two construction paths consistent.
- The pattern of building an allProperties map by copying properties and then putAll(authClientConfigs) is duplicated in several places (DistributedQueryRunner.loadFunctionNamespaceManager/loadExpressionOptimizer/loadSessionPropertyProvider/loadPlanCheckerProviderManager, StaticFunctionNamespaceStore, SessionPropertyManager, PlanCheckerProviderManager, ExpressionOptimizerManager); consider extracting a small helper or utility to centralize this merge logic and avoid inconsistencies.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
It's okay, we are just passing down the properties and adding it to a map in all these places. |
pdabre12
force-pushed
the
internal-auth-sidecar
branch
from
16f7a95 to
9690c34
Compare
… sidecar http-clients
pdabre12
force-pushed
the
internal-auth-sidecar
branch
from
9690c34 to
3c4f209
Compare
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the various Bootstrap wiring points (e.g., NativeExpressionOptimizerFactory, NativePlanCheckerProviderFactory, NativeSystemSessionPropertyProviderFactory, RestBasedFunctionNamespaceManagerFactory, NativeFunctionNamespaceManagerFactory) you call setRequiredConfigurationProperties twice—once with the plugin-specific config and once with authClientConfigs; consider merging these maps before constructing the Bootstrap so the precedence is explicit and you avoid relying on the internal behavior of multiple calls.
- InternalAuthenticationManager now stores sharedSecret in a field but only initializes it in the Optional constructor; the @Inject constructor never sets this field, so getSharedSecret() can return null or NPE—either initialize the field consistently in both constructors or guard the getter accordingly.
- The new extractInternalCommunicationConfigs hack in PrestoServer/TestingPrestoServer is central to how auth config flows into sidecar clients; it would be safer and clearer to replace this with a dedicated immutable config/POJO that encapsulates the required properties rather than passing around a loosely-typed Map<String, String>.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the various Bootstrap wiring points (e.g., NativeExpressionOptimizerFactory, NativePlanCheckerProviderFactory, NativeSystemSessionPropertyProviderFactory, RestBasedFunctionNamespaceManagerFactory, NativeFunctionNamespaceManagerFactory) you call setRequiredConfigurationProperties twice—once with the plugin-specific config and once with authClientConfigs; consider merging these maps before constructing the Bootstrap so the precedence is explicit and you avoid relying on the internal behavior of multiple calls.
- InternalAuthenticationManager now stores sharedSecret in a field but only initializes it in the Optional<String> constructor; the @Inject constructor never sets this field, so getSharedSecret() can return null or NPE—either initialize the field consistently in both constructors or guard the getter accordingly.
- The new extractInternalCommunicationConfigs hack in PrestoServer/TestingPrestoServer is central to how auth config flows into sidecar clients; it would be safer and clearer to replace this with a dedicated immutable config/POJO that encapsulates the required properties rather than passing around a loosely-typed Map<String, String>.
## Individual Comments
### Comment 1
<location path="presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/expressions/NativeExpressionOptimizerFactory.java" line_range="58-61" />
<code_context>
new RestBasedFunctionNamespaceManagerModule(catalogName),
new RestSqlFunctionExecutorsModule());
Injector injector = app
.doNotInitializeLogging()
.setRequiredConfigurationProperties(config)
+ .setRequiredConfigurationProperties(context.getAuthClientConfigs())
.initialize();
return injector.getInstance(RestBasedFunctionNamespaceManager.class);
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling setRequiredConfigurationProperties twice likely drops the optimizer-specific config
The second `.setRequiredConfigurationProperties(context.getAuthClientConfigs())` call overrides the earlier optimizer-related properties, so the optimizer factory may miss required settings (e.g., sidecar endpoint, timeouts). Please merge the optimizer and auth client configuration maps and call `setRequiredConfigurationProperties` once. The same pattern appears in other factories (NativePlanCheckerProviderFactory, NativeSystemSessionPropertyProviderFactory, NativeFunctionNamespaceManagerFactory) and should be updated similarly.
</issue_to_address>
### Comment 2
<location path="presto-function-namespace-managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManagerFactory.java" line_range="63-66" />
<code_context>
new RestBasedFunctionNamespaceManagerModule(catalogName),
new RestSqlFunctionExecutorsModule());
Injector injector = app
.doNotInitializeLogging()
.setRequiredConfigurationProperties(config)
+ .setRequiredConfigurationProperties(context.getAuthClientConfigs())
.initialize();
return injector.getInstance(RestBasedFunctionNamespaceManager.class);
</code_context>
<issue_to_address>
**issue (bug_risk):** Rest function namespace manager config is also being overwritten by auth configs
The second `setRequiredConfigurationProperties` with `context.getAuthClientConfigs()` overwrites the earlier `config` map, so catalog-specific settings for the Rest function namespace manager may be lost and cause runtime misconfiguration. Build a single combined map of `config` and `context.getAuthClientConfigs()` (with a clear precedence rule on key conflicts) and pass it once to `setRequiredConfigurationProperties` instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
.../src/main/java/com/facebook/presto/sidecar/expressions/NativeExpressionOptimizerFactory.java
Outdated
Show resolved
Hide resolved
...ava/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManagerFactory.java
Outdated
Show resolved
Hide resolved
tdcmeehan
previously approved these changes
Mar 12, 2026
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The TLS/auth-related wiring in NativeSidecarCommunicationModule and RestBasedCommunicationModule is nearly identical (configBinder for HttpClientConfig, binding AuthClientConfigs, InternalAuthenticationManager, InternalAuthenticationFilter); consider extracting a shared helper/module to reduce duplication and the risk of the two diverging.
- In presto-function-namespace-managers/pom.xml, the ignoredNonTestScopedDependency entry for
com.facebook.airlift:jaxrshas a leading space (" com.facebook.airlift:jaxrs"), which may prevent the Maven dependency plugin from matching it correctly; remove the space so the ignore rule actually applies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The TLS/auth-related wiring in NativeSidecarCommunicationModule and RestBasedCommunicationModule is nearly identical (configBinder for HttpClientConfig, binding AuthClientConfigs, InternalAuthenticationManager, InternalAuthenticationFilter); consider extracting a shared helper/module to reduce duplication and the risk of the two diverging.
- In presto-function-namespace-managers/pom.xml, the ignoredNonTestScopedDependency entry for `com.facebook.airlift:jaxrs` has a leading space (`" com.facebook.airlift:jaxrs"`), which may prevent the Maven dependency plugin from matching it correctly; remove the space so the ignore rule actually applies.
## Individual Comments
### Comment 1
<location path="presto-function-namespace-managers/pom.xml" line_range="267" />
<code_context>
+ <configuration>
+ <ignoredNonTestScopedDependencies>
+ <ignoredNonTestScopedDependency>jakarta.ws.rs:jakarta.ws.rs-api</ignoredNonTestScopedDependency>
+ <ignoredNonTestScopedDependency> com.facebook.airlift:jaxrs</ignoredNonTestScopedDependency>
+ </ignoredNonTestScopedDependencies>
+ </configuration>
</code_context>
<issue_to_address>
**issue (bug_risk):** Remove leading whitespace in ignoredNonTestScopedDependency coordinate
` com.facebook.airlift:jaxrs` includes a leading space, so it won’t match the actual dependency coordinate and may not be ignored by the plugin. Please remove the space so it matches `com.facebook.airlift:jaxrs` consistently with the other entry.
</issue_to_address>
### Comment 2
<location path="presto-internal-communication/src/main/java/com/facebook/presto/server/InternalAuthenticationManager.java" line_range="47" />
<code_context>
private final boolean internalJwtEnabled;
private final byte[] hmac;
private final String nodeId;
+ private Optional<String> sharedSecret;
@Inject
</code_context>
<issue_to_address>
**suggestion:** Make sharedSecret final for consistency and immutability
`sharedSecret` is only set in the constructor and never mutated. Making it `private final Optional<String> sharedSecret;` and assigning it in both constructors preserves immutability and simplifies thread-safety and reasoning about this class.
Suggested implementation:
```java
private final boolean internalJwtEnabled;
private final byte[] hmac;
private final String nodeId;
private final Optional<String> sharedSecret;
```
```java
public InternalAuthenticationManager(Optional<String> sharedSecret, String nodeId, boolean internalJwtEnabled)
{
this.sharedSecret = requireNonNull(sharedSecret, "sharedSecret is null");
requireNonNull(nodeId, "nodeId is null");
this.internalJwtEnabled = internalJwtEnabled;
if (internalJwtEnabled) {
```
You will also need to ensure that `sharedSecret` is assigned in the `@Inject` constructor:
1. Inside `public InternalAuthenticationManager(InternalCommunicationConfig internalCommunicationConfig, NodeInfo nodeInfo)`, add an assignment such as:
`this.sharedSecret = requireNonNull(internalCommunicationConfig.getSharedSecret(), "sharedSecret is null");`
(or the appropriate accessor from `InternalCommunicationConfig`).
2. Make sure this assignment happens exactly once in that constructor to satisfy the `final` field requirement.
</issue_to_address>
### Comment 3
<location path="presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java" line_range="789-798" />
<code_context>
.collect(Collectors.toList());
}
+
+ private static void testInternalAuthenticationFilter(HttpClient httpClient)
+ {
+ // check if filter present
+ List<HttpRequestFilter> filters = ((JettyHttpClient) httpClient).getRequestFilters();
+
+ InternalAuthenticationManager authenticationManager = filters.stream()
+ .filter(InternalAuthenticationManager.class::isInstance)
+ .map(InternalAuthenticationManager.class::cast)
+ .findFirst()
+ .orElseThrow(() -> new AssertionError("InternalAuthenticationManager filter not found"));
+
+ // Verify that the test shared secret is propagated all the way through
+ assertTrue(authenticationManager.getSharedSecret().isPresent());
+ assertEquals(authenticationManager.getSharedSecret().get(), "internal-shared-secret");
+ }
}
</code_context>
<issue_to_address>
**suggestion (testing):** Tighten the assertion on the number of InternalAuthenticationManager filters to catch misconfiguration or duplication
The helper currently only checks that at least one `InternalAuthenticationManager` is present and has the expected shared secret. To make the test resilient to accidental duplicates, collect all such filters and assert there is exactly one before using it:
```java
List<InternalAuthenticationManager> managers = filters.stream()
.filter(InternalAuthenticationManager.class::isInstance)
.map(InternalAuthenticationManager.class::cast)
.collect(toImmutableList());
assertEquals(1, managers.size(), "Expected exactly one InternalAuthenticationManager filter");
InternalAuthenticationManager authenticationManager = managers.get(0);
...
```
Suggested implementation:
```java
private static void testInternalAuthenticationFilter(HttpClient httpClient)
{
// check if filter present
List<HttpRequestFilter> filters = ((JettyHttpClient) httpClient).getRequestFilters();
List<InternalAuthenticationManager> managers = filters.stream()
.filter(InternalAuthenticationManager.class::isInstance)
.map(InternalAuthenticationManager.class::cast)
.collect(toImmutableList());
assertEquals(managers.size(), 1, "Expected exactly one InternalAuthenticationManager filter");
InternalAuthenticationManager authenticationManager = managers.get(0);
// Verify that the test shared secret is propagated all the way through
assertTrue(authenticationManager.getSharedSecret().isPresent());
assertEquals(authenticationManager.getSharedSecret().get(), "internal-shared-secret");
}
```
1. Add (or reuse if already present) the static import for `toImmutableList` at the top of the file:
`import static com.google.common.collect.ImmutableList.toImmutableList;`
2. Ensure any existing `assertEquals` static import is compatible with the new call signature `(actual, expected, message)`; if the project convention is `(expected, actual, message)`, flip the first two arguments accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...al-communication/src/main/java/com/facebook/presto/server/InternalAuthenticationManager.java
Outdated
Show resolved
Hide resolved
...native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In AuthClientConfigs.defaultAuthClientConfigs you populate key/trust store paths and passwords with empty strings, which will get propagated into HttpClientConfig via bindHttpClientDefaults; consider using null/Optional and only setting these fields when actually configured to avoid unintentionally overriding HttpClient defaults with empty values in non-TLS/internal-auth scenarios.
- InternalAuthenticationManager.getSharedSecret() is a public method returning the shared secret; even though it is annotated @VisibleForTesting, it can be used in production code—consider reducing its visibility or moving the verification into a dedicated test helper to avoid exposing sensitive configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In AuthClientConfigs.defaultAuthClientConfigs you populate key/trust store paths and passwords with empty strings, which will get propagated into HttpClientConfig via bindHttpClientDefaults; consider using null/Optional and only setting these fields when actually configured to avoid unintentionally overriding HttpClient defaults with empty values in non-TLS/internal-auth scenarios.
- InternalAuthenticationManager.getSharedSecret() is a public method returning the shared secret; even though it is annotated @VisibleForTesting, it can be used in production code—consider reducing its visibility or moving the verification into a dedicated test helper to avoid exposing sensitive configuration.
## Individual Comments
### Comment 1
<location path="presto-main-base/src/test/java/com/facebook/presto/sql/expressions/TestExpressionOptimizerManager.java" line_range="91" />
<code_context>
manager.addExpressionOptimizerFactory(getExpressionOptimizerFactory("foo"));
manager.addExpressionOptimizerFactory(getExpressionOptimizerFactory("bar"));
- manager.loadExpressionOptimizerFactories();
+ manager.loadExpressionOptimizerFactories(defaultAuthClientConfigs(pluginNodeManager.getCurrentNode().getNodeIdentifier()));
assertOptimizedExpression("1+1", "2", ImmutableMap.of());
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that AuthClientConfigs are correctly propagated into ExpressionOptimizerContext
The updated tests now pass `defaultAuthClientConfigs(...)` into `loadExpressionOptimizerFactories`, but they still only check optimizer selection and config-file errors. To verify the new wiring, please update `getExpressionOptimizerFactory("foo")` in the test to assert that `context.getAuthClientConfigs()` is non-null and that its nodeId matches `pluginNodeManager.getCurrentNode().getNodeIdentifier()`.
Suggested implementation:
```java
private ExpressionOptimizerFactory getExpressionOptimizerFactory(String name)
{
return new ExpressionOptimizerFactory()
{
@Override
public String getName()
{
return name;
}
@Override
public ExpressionOptimizer create(Expression expression, Session session, Metadata metadata, SqlParser parser, ExpressionAnalysis analysis, Map<NodeRef<Expression>, Type> types, ExpressionOptimizerContext context)
{
if ("foo".equals(name)) {
assertNotNull(context.getAuthClientConfigs(), "AuthClientConfigs should be propagated into ExpressionOptimizerContext for 'foo' optimizer");
assertEquals(
context.getAuthClientConfigs().getNodeId(),
pluginNodeManager.getCurrentNode().getNodeIdentifier(),
"AuthClientConfigs.nodeId should match current plugin node identifier");
}
return new ExpressionOptimizer()
{
@Override
public RowExpression optimize(Expression expression, Session session, TypeProvider types)
{
return new RowExpressionOptimizer(metadata, parser).optimize(expression, session, types);
}
};
}
};
}
```
1. Ensure the following static imports exist at the top of `TestExpressionOptimizerManager.java` (or equivalent TestNG/JUnit assertions used in the rest of the file):
- `import static org.testng.Assert.assertEquals;`
- `import static org.testng.Assert.assertNotNull;`
2. If `AuthClientConfigs` exposes the node identifier via a different accessor than `getNodeId()` (e.g., `getNodeIdentifier()` or `getNodeId().toString()`), adjust the `assertEquals` call accordingly to compare the same types (e.g., both as `String` or both as the same value object type).
3. If the signature of `ExpressionOptimizerFactory.create(...)` or `ExpressionOptimizerContext` in your code differs slightly (e.g., different type parameters or additional arguments), adapt the method signature and parameter list in the SEARCH/REPLACE block to match your existing code before applying the change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...n-base/src/test/java/com/facebook/presto/sql/expressions/TestExpressionOptimizerManager.java
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- AuthClientConfigs currently accepts several potentially null String fields (e.g., keyStorePath, trustStorePath) without nullability annotations or validation and then propagates them into HttpClientConfig via bindHttpClientDefaults; consider either making these Optional, adding requireNonNull checks, or conditionally setting them to avoid overwriting existing defaults with nulls.
- The new CommonInternalCommunicationModule.bindInternalAuth binds HttpClientConfig global defaults and InternalAuthenticationManager in addition to the existing setup() bindings; it would be helpful to clearly separate or consolidate these code paths to avoid accidental double-binding or inconsistent behavior between modules using the module directly vs. the static helper.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- AuthClientConfigs currently accepts several potentially null String fields (e.g., keyStorePath, trustStorePath) without nullability annotations or validation and then propagates them into HttpClientConfig via bindHttpClientDefaults; consider either making these Optional, adding requireNonNull checks, or conditionally setting them to avoid overwriting existing defaults with nulls.
- The new CommonInternalCommunicationModule.bindInternalAuth binds HttpClientConfig global defaults and InternalAuthenticationManager in addition to the existing setup() bindings; it would be helpful to clearly separate or consolidate these code paths to avoid accidental double-binding or inconsistent behavior between modules using the module directly vs. the static helper.
## Individual Comments
### Comment 1
<location path="presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/expressions/NativeExpressionOptimizerFactory.java" line_range="56-58" />
<code_context>
+ new NativeSidecarCommunicationModule(context.getAuthClientConfigs()),
new NativeExpressionsModule(context.getNodeManager(), context.getRowExpressionSerde(), context.getFunctionMetadataManager(), context.getFunctionResolution()));
Injector injector = app
- .noStrictConfig()
.doNotInitializeLogging()
.setRequiredConfigurationProperties(config)
.quiet()
</code_context>
<issue_to_address>
**question (bug_risk):** Dropping `noStrictConfig()` changes config validation behavior and may break existing configs.
Unused or unknown config properties will now cause startup failures instead of being ignored, which can break deployments or tests that reuse .properties files with extra entries. If this stricter behavior isn’t explicitly intended, consider restoring `noStrictConfig()`; otherwise, document the new requirement and ensure affected configs are cleaned up.
</issue_to_address>
### Comment 2
<location path="presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/sessionpropertyproviders/NativeSystemSessionPropertyProviderFactory.java" line_range="44-46" />
<code_context>
+ new NativeSidecarCommunicationModule(context.getAuthClientConfigs()),
new NativeExpressionsModule(context.getNodeManager(), context.getRowExpressionSerde(), context.getFunctionMetadataManager(), context.getFunctionResolution()));
Injector injector = app
- .noStrictConfig()
.doNotInitializeLogging()
.setRequiredConfigurationProperties(config)
.quiet()
</code_context>
<issue_to_address>
**question (bug_risk):** Config strictness change for session property provider may reject previously valid configs.
Removing `.noStrictConfig()` here makes config validation stricter for the native system session property provider and may cause startup failures if any unexpected properties are present. Consider keeping `noStrictConfig()` unless you intentionally want strict config scoping and have confirmed existing configs won’t be rejected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
.../src/main/java/com/facebook/presto/sidecar/expressions/NativeExpressionOptimizerFactory.java
Show resolved
Hide resolved
...book/presto/sidecar/sessionpropertyproviders/NativeSystemSessionPropertyProviderFactory.java
Show resolved
Hide resolved
Contributor
Author
The plugins would need to ensure that accidental double-binding is avoided. |
tdcmeehan
approved these changes
Mar 13, 2026
This was referenced Mar 31, 2026
15 tasks
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
3 participants
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.
Description
Add internal communication auth layer to sidecar http-clients.
Motivation and Context
Enables secure internal communication between worker and coordinator when coordinator talks to the worker via the sidecar http-clients.
Impact
No impact
Test Plan
Unit tests included
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Integrate coordinator configuration into native sidecar components and expression optimizers to enable internal-authenticated HTTP communication across sidecar-related services.
New Features:
Enhancements:
Build:
Tests:
Summary by Sourcery
Introduce a shared internal-auth configuration object and propagate it into native sidecar and related components to secure their internal HTTP communication.
New Features:
Enhancements:
Build:
Tests: