WIP: pass server_name to on_connect handler#105
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThreads an optional TLS SNI Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Pgwire as pgwire
participant Core as RustCore
participant Py as PythonWorker
participant CB as on_connect_cb
Client->>Pgwire: STARTUP / TLS handshake
Note over Pgwire: extract optional SNI -> server_name
Pgwire->>Core: connection established (ip, port, server_name?)
Core->>Py: WorkerMessage::Connect { conn_id, ip, port, server_name, responder }
Py->>CB: on_connect(conn_id, ip, port, server_name=... or None)
CB-->>Py: BoolCallbackResult
Py-->>Core: responder response
Core-->>Pgwire: finalize connect (allow/deny)
Pgwire-->>Client: continue or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ybrs
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
1196-1214: Critical: Missingserver_nameargument inon_connectcall.The
on_connectcall at line 1202 is missing the requiredserver_nameargument, causing the compilation failure reported in the CI pipeline. This is the non-authenticated connection path, which should also support SNI-based routing.Apply this diff to retrieve and pass
server_name:let _ = sender.send(id); } let addr = client.socket_addr(); + // Try to get TLS SNI server_name (if any) from client metadata + let server_name = client + .metadata() + .get("server_name") + .cloned(); let allowed = self .py_worker - .on_connect(id, addr.ip().to_string(), addr.port()) + .on_connect(id, addr.ip().to_string(), addr.port(), server_name) .await; if !allowed.allowed {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Cargo.toml(1 hunks)pysrc/riffq/connection.py(2 hunks)src/lib.rs(5 hunks)tests/test_on_connect.py(1 hunks)tests/test_on_connect_custom_error.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pysrc/riffq/connection.py (2)
tests/test_on_connect.py (1)
handle_connect(18-19)tests/test_on_connect_custom_error.py (1)
handle_connect(15-16)
tests/test_on_connect_custom_error.py (2)
pysrc/riffq/connection.py (2)
handle_connect(37-38)handle_connect(82-85)tests/test_on_connect.py (1)
handle_connect(18-19)
tests/test_on_connect.py (2)
pysrc/riffq/connection.py (2)
handle_connect(37-38)handle_connect(82-85)tests/test_on_connect_custom_error.py (1)
handle_connect(15-16)
🪛 GitHub Actions: CI
src/lib.rs
[error] 1202-1202: Step failed: cargo test --verbose. Rust compiler error E0061: on_connect called with 3 arguments, but function signature requires 4 (connection_id: u64, ip: String, port: u16, server_name: Option). Provide the missing server_name argument in the call.
🪛 Ruff (0.13.3)
pysrc/riffq/connection.py
37-37: Unused method argument: ip
(ARG002)
37-37: Unused method argument: port
(ARG002)
37-37: Unused method argument: server_name
(ARG002)
tests/test_on_connect_custom_error.py
15-15: Unused function argument: conn_id
(ARG001)
15-15: Unused function argument: ip
(ARG001)
15-15: Unused function argument: port
(ARG001)
15-15: Unused function argument: server_name
(ARG001)
tests/test_on_connect.py
18-18: Unused function argument: conn_id
(ARG001)
18-18: Unused function argument: ip
(ARG001)
18-18: Unused function argument: port
(ARG001)
18-18: Unused function argument: server_name
(ARG001)
🔇 Additional comments (8)
src/lib.rs (4)
97-97: LGTM!The addition of
server_name: Option<String>to theConnectmessage properly supports TLS SNI propagation.
716-748: LGTM!The
Connectmessage handler correctly:
- Destructures
server_namefrom the message- Passes it to Python kwargs as either the value or
py.None()- Maintains backward compatibility with handlers that don't use it
843-855: LGTM!The
on_connectmethod signature and implementation correctly accept and forwardserver_nameto the worker message.
1252-1259: LGTM!The server_name retrieval from client metadata is correctly implemented for the authenticated path.
pysrc/riffq/connection.py (2)
37-37: LGTM!The addition of
server_name=NonetoBaseConnection.handle_connectproperly supports SNI-based connection handling. The static analysis warnings about unused parameters can be safely ignored since this is an abstract base class method that subclasses may override to use these parameters.
82-85: LGTM!The
server_nameparameter is correctly added and forwarded to the connection'shandle_connectmethod.tests/test_on_connect_custom_error.py (1)
15-15: LGTM!The test's
handle_connectsignature correctly includes the optionalserver_nameparameter. The static analysis warnings about unused parameters can be safely ignored since this is a test fixture that doesn't need to use all parameters.tests/test_on_connect.py (1)
18-18: LGTM!The test's
handle_connectsignature correctly includes the optionalserver_nameparameter. The static analysis warnings about unused parameters can be safely ignored since this is a test fixture that doesn't need to use all parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib.rs (1)
718-742: LGTM! Consider simplifying the server_name handling.The logic correctly passes
server_nameto the Python callback. The conditional handling of theOptioncould be simplified.Consider this more concise approach:
let kwargs = PyDict::new(py); kwargs.set_item("callback", wrapper.clone_ref(py)).unwrap(); - if let Some(name) = server_name.clone() { - let _ = kwargs.set_item("server_name", name); - } else { - let _ = kwargs.set_item("server_name", py.None()); - } + let _ = kwargs.set_item("server_name", server_name.as_ref().into_py_any(py).unwrap());This leverages PyO3's
Optionconversion and avoids the unnecessaryclone().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/lib.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-tests
- GitHub Check: unit-tests
🔇 Additional comments (6)
src/lib.rs (6)
98-98: LGTM!The
server_namefield addition to theConnectvariant is correctly typed and consistent with the message structure.
606-607: Clarify the test module visibility change.The test module is now compiled in all builds instead of only during tests. This increases binary size and exposes test code unnecessarily.
Was this change intentional? If not, consider reverting:
-mod encode_tests { +#[cfg(test)] +mod encode_tests { use super::*;If this change is needed for external testing or debugging, please add a comment explaining the rationale.
845-845: LGTM!The signature change correctly adds the
server_nameparameter as a borrowed optional string, which is appropriate for this async function.
852-852: LGTM!The conversion from
Option<&str>toOption<String>is correctly implemented for the message payload.
1202-1206: LGTM!The
on_connectcall correctly retrieves and passes the TLS SNI server name viaclient.sni_server_name(). The comment clearly documents the intent.
1257-1257: LGTM!The
on_connectcall in the authentication path is consistent with the non-auth path, ensuringserver_nameis propagated in both startup flows.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pysrc/riffq/connection.py (1)
227-238: Critical: Missingserver_nameparameter in method signature.Line 238 references
server_namebut it's not defined as a parameter in the method signature on line 227. This will cause aNameErrorat runtime when the Rust layer invokes this handler with theserver_nameargument.Apply this diff to add the missing parameter:
- def handle_connect(self, connection_id: int, ip: str, port: int, callback: Callable[..., None] = lambda *a, **k: None) -> None: + def handle_connect(self, connection_id: int, ip: str, port: int, callback: Callable[..., None] = lambda *a, **k: None, server_name: str | None = None) -> None: """Forward a connect notification to the connection instance. Args: connection_id: Server assigned identifier for this client. ip: Remote peer IP address. port: Remote peer port. callback: Function to acknowledge handling. + server_name: Optional TLS SNI server name from the client. """ # logger.info(f"new connnection {connection_id} {ip} {port}") conn = self.get_connection(connection_id=connection_id) conn.handle_connect(ip, port, callback=callback, server_name=server_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)pysrc/riffq/connection.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pysrc/riffq/connection.py (3)
tests/test_on_connect_custom_error.py (1)
handle_connect(15-16)example/psql-redis/psql_redis.py (1)
handle_connect(273-275)tests/test_on_connect.py (1)
handle_connect(18-19)
🪛 Ruff (0.14.1)
pysrc/riffq/connection.py
92-92: Unused method argument: ip
(ARG002)
92-92: Unused method argument: port
(ARG002)
92-92: Unused method argument: server_name
(ARG002)
92-92: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
92-92: Unused lambda argument: a
(ARG005)
92-92: Unused lambda argument: k
(ARG005)
238-238: Undefined name server_name
(F821)
🔇 Additional comments (1)
Cargo.toml (1)
21-23: Good move to crates.io version.Using the published crates.io version
0.34.0improves build reproducibility compared to a git dependency. The commented alternative is fine to keep for reference.
| return callback(user == "user" and password == "secret") | ||
|
|
||
| def handle_connect(self, ip: str, port: int, callback: Callable[..., None] = lambda *a, **k: None) -> None: | ||
| def handle_connect(self, ip: str, port: int, server_name:str=None, callback: Callable[..., None] = lambda *a, **k: None) -> None: |
There was a problem hiding this comment.
Fix the type hint for server_name.
PEP 484 requires explicit Optional when the default value is None. The current implicit optional type hint should be made explicit.
Apply this diff:
- def handle_connect(self, ip: str, port: int, server_name:str=None, callback: Callable[..., None] = lambda *a, **k: None) -> None:
+ def handle_connect(self, ip: str, port: int, server_name: str | None = None, callback: Callable[..., None] = lambda *a, **k: None) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def handle_connect(self, ip: str, port: int, server_name:str=None, callback: Callable[..., None] = lambda *a, **k: None) -> None: | |
| def handle_connect(self, ip: str, port: int, server_name: str | None = None, callback: Callable[..., None] = lambda *a, **k: None) -> None: |
🧰 Tools
🪛 Ruff (0.14.1)
92-92: Unused method argument: ip
(ARG002)
92-92: Unused method argument: port
(ARG002)
92-92: Unused method argument: server_name
(ARG002)
92-92: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
92-92: Unused lambda argument: a
(ARG005)
92-92: Unused lambda argument: k
(ARG005)
🤖 Prompt for AI Agents
In pysrc/riffq/connection.py around line 92, the parameter server_name is
annotated as str but has a default None; update the type hint to Optional[str]
and ensure Optional is imported from typing (or typing import Optional alongside
Callable) so the annotation correctly reflects that None is an allowed value;
modify the function signature to use Optional[str] for server_name and add the
import if missing.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores