Add advertisedPlaintextPort to DruidNode for sidecar proxy support#19598
Add advertisedPlaintextPort to DruidNode for sidecar proxy support#19598saithal-confluent wants to merge 2 commits into
Conversation
Allow Druid nodes to advertise a different plaintext port (e.g. an envoy sidecar port) for peer discovery while binding Jetty to the actual service port. This enables east-west mTLS via a sidecar proxy without changing Druid's bind configuration. The new `advertisedPlaintextPort` JSON property on DruidNode controls the port advertised during peer discovery. When unset (or <= 0), it falls back to `plaintextPort` preserving existing behaviour.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 3 of 3 changed files.
This is an automated review by Codex GPT-5.5
| @JsonProperty("enableTlsPort") boolean enableTlsPort, | ||
| @JsonProperty("labels") @Nullable Map<String, String> labels | ||
| @JsonProperty("labels") @Nullable Map<String, String> labels, | ||
| @JsonProperty("advertisedPlaintextPort") Integer advertisedPlaintextPort |
There was a problem hiding this comment.
[P1] Preserve the labels-taking DruidNode constructor
Adding advertisedPlaintextPort to the existing @JsonCreator constructor removes the previous nine-argument overload that accepted labels. Existing source in DruidNodeTest still calls new DruidNode(..., labels), for example the equality and serde tests, and those calls no longer match any constructor, so the server module will not compile. Please keep a nine-argument overload that delegates with a null advertisedPlaintextPort, or update all local call sites while preserving source compatibility intentionally.
There was a problem hiding this comment.
Restored the 9-arg overload so existing call sites keep compiling. The new field is reachable via the 10-arg constructor instead of 9-arg one as it would make existing null-literal calls ambiguous.
Thanks for the review @FrankChen021 , could you please take a look again?
Restore the nine-argument DruidNode(..., labels) overload that was dropped when advertisedPlaintextPort was added to the @JsonCreator constructor, and update the new advertisedPlaintextPort tests to use the ten-argument form. Without this the server module did not compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
Follow-up reviewed. The constructor compatibility concern is addressed: the nine-argument labels overload is restored, and the advertised plaintext port remains on the ten-argument constructor path.
Reviewed 3 of 3 changed files.
This is an automated review by Codex GPT-5.5
2 participants
Description
Allow Druid nodes to advertise a different plaintext port for peer discovery while binding Jetty to the actual service port.
Motivation
When running Druid with a sidecar proxy (e.g. Envoy for east-west mTLS), the port the proxy listens on differs from the port Jetty binds to. Without this change, peer discovery advertises the Jetty bind port, so other nodes try to connect directly — bypassing the sidecar — and mTLS fails.
Changes
advertisedPlaintextPortfield toDruidNode(JSON property:advertisedPlaintextPort,@Max(0xffff))getHostAndPort()returns the advertised port instead ofplaintextPortplaintextPort— no behaviour change for existing deploymentsServiceLocationto use the advertised portTesting
mvn test -pl server -Dtest=DruidNodeTestmvn test -pl server(full server module)This is a backport of an internal change validated in production.