Add tencent_cls_fdw for Tencent Cloud Log Service#585
Open
wenerme wants to merge 5 commits intosupabase:mainfrom
Open
Add tencent_cls_fdw for Tencent Cloud Log Service#585wenerme wants to merge 5 commits intosupabase:mainfrom
wenerme wants to merge 5 commits intosupabase:mainfrom
Conversation
HTTP-based FDW using TC3-HMAC-SHA256 authentication. Supports SearchLog API with CQL query language. WHERE pushdown for _from/_to (timestamps) and _query. LogJson field auto-parsed for column extraction. Based on logflare_fdw template.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new Tencent Cloud Log Service (CLS) foreign data wrapper behind the feature flag Sequence DiagramsequenceDiagram
actor PostgreSQL as PostgreSQL
participant FDW as TencentClsFdw
participant Client as HTTP Client
participant API as Tencent CLS API
participant Converter as JSON->Rows
PostgreSQL->>FDW: new(server)
Note over FDW: init runtime, creds, region, endpoint, client
PostgreSQL->>FDW: begin_scan(quals, columns, options)
FDW->>FDW: build JSON payload\napply quals/options
FDW->>FDW: tc3_sign(secret_id, secret_key, service, host, ts, payload)
FDW->>Client: POST (signed headers + payload)
Client->>API: HTTP request
API-->>Client: HTTP response (JSON or error)
Client-->>FDW: response text
FDW->>FDW: enforce max response size\nparse JSON
FDW->>Converter: resp_to_rows(JSON)
Converter-->>FDW: Vec<Row>
loop iterate rows
PostgreSQL->>FDW: iter_scan()
FDW-->>PostgreSQL: next Row
end
PostgreSQL->>FDW: end_scan()
FDW->>FDW: clear scan state
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wrappers/src/fdw/tencent_cls_fdw/tencent_cls_fdw.rs`:
- Around line 332-345: The code silently substitutes empty strings when
get_vault_secret(...) returns None; change both secret_id and secret_key
branches to treat None as an error and fail fast: after calling
require_option("secret_id_id", &server.options)? and
require_option("secret_key_id", &server.options)?, call get_vault_secret(id_key)
and if it returns Some(value) use value, otherwise return an Err with a clear
message that includes the id_key (e.g., "vault secret not found for id X") so
connection setup fails immediately instead of proceeding with empty credentials;
update the same logic for secret_key using the "secret_key_id" key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 88e8f94c-2261-4f97-b222-24fa89c24a93
📒 Files selected for processing (5)
wrappers/Cargo.tomlwrappers/src/fdw/mod.rswrappers/src/fdw/tencent_cls_fdw/mod.rswrappers/src/fdw/tencent_cls_fdw/tencent_cls_fdw.rswrappers/src/fdw/tencent_cls_fdw/tests.rs
…ings Replace get_vault_secret().unwrap_or_default() with proper error handling for both secret_id and secret_key, so missing vault secrets fail fast with a clear error message instead of silently using empty credentials.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wrappers/src/fdw/tencent_cls_fdw/tencent_cls_fdw.rs`:
- Around line 70-75: The HTTP client lacks a request timeout and the response is
fully buffered via .text().await before checking body size; update create_client
to set a sensible request timeout on reqwest::Client::builder() (e.g., via
.timeout(Duration::from_secs(...))) and change the response handling that
currently uses .text().await to stream the body (e.g., via response.bytes_stream
or response.take) and enforce a MAX_BODY_SIZE when accumulating bytes so you
abort and return an error if the size limit is exceeded before converting to
text; reference the create_client function and the code paths that call
.text().await (replace with streaming+limit) and continue using
ClientBuilder/RetryTransientMiddleware as before.
- Around line 169-179: The header construction currently uses
HeaderValue::from_str(...).unwrap() for authorization and self.region which can
panic on invalid input; instead validate and map these conversions to a proper
FDW error path: replace the unwraps around HeaderValue::from_str(&authorization)
and HeaderValue::from_str(&self.region) (and similarly for timestamp) with
fallible handling (e.g., match or map_err) that returns an Err containing a
descriptive FDW error (or Error/Result used by the surrounding function) when
HeaderValue::from_str fails, so header insertion never panics but yields a
controlled error; locate the HeaderValue::from_str calls in the header-building
block (the headers.insert lines) and implement the conversion-to-HeaderValue
with explicit error mapping tied to the function's existing error type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 162397a5-b7ef-4934-968a-6ced495ae076
📒 Files selected for processing (2)
wrappers/src/fdw/tencent_cls_fdw/mod.rswrappers/src/fdw/tencent_cls_fdw/tencent_cls_fdw.rs
Comment on lines
+70
to
+75
| fn create_client() -> TencentClsFdwResult<ClientWithMiddleware> { | ||
| let client = reqwest::Client::builder().build()?; | ||
| let retry_policy = ExponentialBackoff::builder().build_with_max_retries(3); | ||
| Ok(ClientBuilder::new(client) | ||
| .with(RetryTransientMiddleware::new_with_policy(retry_policy)) | ||
| .build()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd wrappers/src/fdw/tencent_cls_fdw && \
echo "=== Lines 70-75 context ===" && \
sed -n '65,80p' tencent_cls_fdw.rs && \
echo -e "\n=== Client builder usage ===" && \
rg -n 'Client::builder|\.timeout|\.connect_timeout' tencent_cls_fdw.rs && \
echo -e "\n=== text().await usage ===" && \
rg -n -B2 -A4 '\.text\(\).*await' tencent_cls_fdw.rs && \
echo -e "\n=== max_response_size checks ===" && \
rg -n -B3 -A3 'max_response_size' tencent_cls_fdw.rsRepository: supabase/wrappers
Length of output: 836
🏁 Script executed:
cd wrappers/src/fdw/tencent_cls_fdw && \
echo "=== Lines 181-212 context ===" && \
sed -n '175,220p' tencent_cls_fdw.rsRepository: supabase/wrappers
Length of output: 1672
Add request timeout and enforce body-size limits before full buffering.
The HTTP client at line 71 has no timeout configuration, risking indefinite hangs on unresponsive backends. Additionally, the response at lines 197–198 is fully buffered with .text().await before the size check at line 203, allowing large responses to exhaust memory or block workers before validation occurs.
Proposed fix
+use std::time::Duration;
fn create_client() -> TencentClsFdwResult<ClientWithMiddleware> {
- let client = reqwest::Client::builder().build()?;
+ let client = reqwest::Client::builder()
+ .connect_timeout(Duration::from_secs(10))
+ .timeout(Duration::from_secs(30))
+ .build()?;
let retry_policy = ExponentialBackoff::builder().build_with_max_retries(3);
Ok(ClientBuilder::new(client)
.with(RetryTransientMiddleware::new_with_policy(retry_policy))
.build())
} let status = resp.status();
+ if let Some(content_len) = resp.content_length() {
+ if content_len as usize > self.max_response_size {
+ return Err(TencentClsFdwError::ResponseTooLarge(
+ content_len as usize,
+ self.max_response_size,
+ ));
+ }
+ }
+
+ let mut body = Vec::new();
+ let mut resp = resp;
+ while let Some(chunk) = resp
+ .chunk()
+ .await
+ .map_err(|e| TencentClsFdwError::RequestError(e))?
+ {
+ if body.len() + chunk.len() > self.max_response_size {
+ return Err(TencentClsFdwError::ResponseTooLarge(
+ body.len() + chunk.len(),
+ self.max_response_size,
+ ));
+ }
+ body.extend_from_slice(&chunk);
+ }
+ let text = String::from_utf8(body)
+ .map_err(|e| TencentClsFdwError::ApiError(format!("response is not valid UTF-8: {e}")))?;
- let text = resp
- .text()
- .await
- .map_err(|e| TencentClsFdwError::RequestError(e))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wrappers/src/fdw/tencent_cls_fdw/tencent_cls_fdw.rs` around lines 70 - 75,
The HTTP client lacks a request timeout and the response is fully buffered via
.text().await before checking body size; update create_client to set a sensible
request timeout on reqwest::Client::builder() (e.g., via
.timeout(Duration::from_secs(...))) and change the response handling that
currently uses .text().await to stream the body (e.g., via response.bytes_stream
or response.take) and enforce a MAX_BODY_SIZE when accumulating bytes so you
abort and return an error if the size limit is exceeded before converting to
text; reference the create_client function and the code paths that call
.text().await (replace with streaming+limit) and continue using
ClientBuilder/RetryTransientMiddleware as before.
…check - Add connect_timeout(10s) and request timeout(30s) to HTTP client - Replace HeaderValue::from_str().unwrap() with fallible error handling to avoid panics on non-ASCII input - Check Content-Length header before buffering full response body
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
1 participant
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.
Summary
tencent_cls_fdw) for querying Tencent Cloud Log Service (CLS) as foreign tables_from/_to(timestamp range) and_query(search expression)LogJsonfields for column extractionlogflare_fdwtemplate with CLS-specific adaptations