Skip to content

fix(kms): set user-agent for auth api requests and improve error logging#525

Merged
kvinwang merged 2 commits intomasterfrom
feat-set-header-for-kms-auth-api-request
Mar 6, 2026
Merged

fix(kms): set user-agent for auth api requests and improve error logging#525
kvinwang merged 2 commits intomasterfrom
feat-set-header-for-kms-auth-api-request

Conversation

@Leechael
Copy link
Collaborator

@Leechael Leechael commented Mar 3, 2026

Summary

  • Add dstack-kms/<version> User-Agent header to all auth webhook requests, to avoid being blocked by WAF (e.g. Cloudflare)
  • Improve error logging on auth api response decode failure: log webhook URL, HTTP status code and response body for easier debugging

Test plan

  • Deploy KMS with webhook auth api and verify requests include proper User-Agent header
  • Trigger a decode failure and verify the error message contains status code and response body

@Leechael Leechael force-pushed the feat-set-header-for-kms-auth-api-request branch from c4a5835 to c0b0e12 Compare March 3, 2026 15:20
@Leechael Leechael force-pushed the feat-set-header-for-kms-auth-api-request branch from c0b0e12 to 79b8b8d Compare March 3, 2026 15:25
Copy link
Collaborator

@kvinwang kvinwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement on User-Agent and error logging! A few suggestions to take it further:

1. Extract generic http_get / http_post helpers

The current http_client() + manual response parsing can be unified into reusable helpers that handle User-Agent, status check, body truncation, and decode error context in one place:

async fn http_get<R: DeserializeOwned>(url: &str) -> Result<R> {
    send_request(reqwest::Client::new().get(url), url).await
}

async fn http_post<R: DeserializeOwned>(url: &str, body: &impl Serialize) -> Result<R> {
    send_request(reqwest::Client::new().post(url).json(body), url).await
}

async fn send_request<R: DeserializeOwned>(req: reqwest::RequestBuilder, url: &str) -> Result<R> {
    static USER_AGENT: &str = concat!("dstack-kms/", env!("CARGO_PKG_VERSION"));
    let response = req.header("User-Agent", USER_AGENT).send().await?;
    let status = response.status();
    let body = response.text().await?;
    let short_body = &body[..body.len().min(512)];
    if !status.is_success() {
        bail!("auth api {url} returned {status}: {short_body}");
    }
    serde_json::from_str(&body).with_context(|| {
        format!("failed to decode response from {url}, status={status}, body={short_body}")
    })
}

Then call sites become one-liners:

let resp: BootResponse = http_post(&url, &boot_info).await?;
let info: AuthApiInfoResponse = http_get(&webhook.url).await?;

This gives you:

  • Unified User-Agent across all requests
  • Status check — non-2xx bails immediately with url + status + body
  • Consistent decode error context — both is_app_allowed and get_info get the same treatment (currently only get_info has the improved error logging)
  • Body truncationshort_body caps at 512 bytes in error messages to avoid huge HTML pages blowing up logs
  • No separate http_client() function needed

2. Remove stray println!

The println!("url: {}", webhook.url) on the removed line — good catch removing it 👍

3. Minor: body truncation for error messages

If the webhook returns a large HTML error page, the full body in the error message could be noisy. The &body[..body.len().min(512)] in the suggestion above handles this. Note short_body is only used for error messages — serde_json::from_str still parses the full body.

- Unify User-Agent, status check, body truncation, and decode error
  context into a single send_request function
- Both is_app_allowed and get_info now share consistent error handling
- Truncate response body to 512 bytes in error messages to avoid
  noisy HTML pages in logs
- Remove separate http_client() function
Copy link
Collaborator

@kvinwang kvinwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kvinwang kvinwang merged commit 7da5d06 into master Mar 6, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants