feat(server): support custom tls config per endpoint - #552
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds TLS fields and validation to sync and validate tasks. It introduces shared HTTP and pooled HTTPS agents with TLS-based reuse, isolation, and LRU eviction. It redacts client keys in debug logs and adds unit and end-to-end TLS coverage. ChangesBackend TLS support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SyncValidate
participant getHttpsAgent
participant HTTPSBackend
participant releaseHttpsAgent
SyncValidate->>getHttpsAgent: provide TLS material
getHttpsAgent->>getHttpsAgent: reuse or create fingerprinted agent
getHttpsAgent->>HTTPSBackend: initialize backend with HTTPS agent
HTTPSBackend-->>SyncValidate: return backend response
SyncValidate->>releaseHttpsAgent: release acquired agent
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
bzp2010
added
test/api7
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
apps/cli/e2e/server/backend-tls.e2e-spec.ts (1)
10-12: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the pooled HTTPS agent is defined before comparing identities.
The fixtures exist, and
loadBackendreceives an options object withhttpsAgent. The identity assertions can still pass if the mocked calls exposeundefined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/e2e/server/backend-tls.e2e-spec.ts` around lines 10 - 12, Update the identity assertions in the backend TLS test to first assert that the pooled HTTPS agent returned or passed through by loadBackend is defined, then compare its identity. Use the existing httpsAgent-related value and preserve the current fixture and loadBackend setup.apps/cli/src/server/sync.ts (1)
51-63: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the handler-to-agent contract.
Both handlers should have tests that verify TLS fields reach
getHttpsAgent, do not reachloadBackend, and preserve secure defaults whentlsSkipVerifyis omitted.
apps/cli/src/server/sync.ts#L51-L63: cover sync backend initialization with CA and mTLS material.apps/cli/src/server/validate.ts#L50-L62: cover validate backend initialization with CA and mTLS material.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/server/sync.ts` around lines 51 - 63, Add test coverage for both backend initialization sites to verify the TLS field routing contract. In apps/cli/src/server/sync.ts around lines 51-63, create tests for the sync handler that confirm caCert, tlsClientCert, and tlsClientKey are passed to getHttpsAgent (not included in restOpts sent to loadBackend), and that secure defaults are applied when tlsSkipVerify is undefined. In apps/cli/src/server/validate.ts around lines 50-62, add identical coverage for the validate handler backend initialization with the same TLS field and default-behavior assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/e2e/server/backend-tls.e2e-spec.ts`:
- Around line 144-145: Update the no-TLS-failure assertions to remove the exact
500 status requirement and assert only that the response message does not
contain a certificate-verification error. Tighten both certificate-error regexes
by escaping the separator in “self-signed” so they match the literal Node error
text, while preserving the existing test independence.
In `@apps/cli/src/server/agent-pool.spec.ts`:
- Around line 127-137: Update the test around getHttpsAgent to refresh agentA
after creating agentB, then spy on agentB.destroy and add agentC so the pool
evicts agentB as the least-recently-used entry rather than merely the first
inserted one. Rename the test title and assertions to clearly describe and
verify this LRU behavior.
- Around line 87-89: Update the afterAll teardown around server.close so its
Promise callback accepts the close error and rejects when one is provided, while
resolving only on successful closure. This ensures server.close errors,
including ERR_SERVER_NOT_RUNNING, propagate from the test teardown.
- Around line 130-136: The agent pool’s LRU eviction must not immediately
destroy an evicted HttpsAgent while requests are active. Update the pool
disposal and request-tracking flow around getHttpsAgent, validate, and sync so
evicted agents are removed from cache lookup immediately but their destroy call
is deferred until all active requests drain. Replace the current eviction-only
destroy assertion with a regression test that holds a backend response during
eviction, verifies the request completes, and then confirms the agent is
destroyed.
- Around line 30-42: Extend the fingerprint test around fingerprintTlsMaterial
to include a tlsClientKey value and assert that changing it produces a different
fingerprint from base. Update the mTLS agent test to verify HttpsAgent.options
contains both tlsClientCert and tlsClientKey values, covering the assignments in
the agent creation flow.
In `@apps/cli/src/server/agent-pool.ts`:
- Around line 17-22: Update loggerMiddleware request-body handling to redact or
remove the tlsClientKey property before passing req.body to logger.log().
Preserve all other request fields and ensure the original body is not exposed in
debug JSON logs.
- Around line 31-37: Update the httpsAgentPool eviction handling to track each
HttpsAgent’s active request count and defer destruction of evicted agents until
that count reaches zero. Ensure request lifecycle paths increment and decrement
the count reliably, and destroy immediately only when an evicted agent has no
active requests; preserve normal LRU behavior for non-evicted agents.
In `@apps/cli/src/server/sync.ts`:
- Around line 51-53: Remove tlsSkipVerify from the backend options in both
handlers by destructuring it alongside caCert, tlsClientCert, and tlsClientKey
before spreading restOpts. Apply this change in apps/cli/src/server/sync.ts at
lines 51-53 and apps/cli/src/server/validate.ts at lines 50-52, leaving the
transport-specific value out of each loadBackend call.
- Around line 58-63: The /sync and /validate handlers must not honor
request-controlled tlsSkipVerify without HTTP-request authorization. Add the
existing authorization or authentication check before constructing the HTTPS
agent in the sync handler at apps/cli/src/server/sync.ts lines 58-63 and the
validate handler at apps/cli/src/server/validate.ts lines 57-62; only pass
tlsSkipVerify to getHttpsAgent after authorization, while preserving the current
behavior for authorized requests.
---
Nitpick comments:
In `@apps/cli/e2e/server/backend-tls.e2e-spec.ts`:
- Around line 10-12: Update the identity assertions in the backend TLS test to
first assert that the pooled HTTPS agent returned or passed through by
loadBackend is defined, then compare its identity. Use the existing
httpsAgent-related value and preserve the current fixture and loadBackend setup.
In `@apps/cli/src/server/sync.ts`:
- Around line 51-63: Add test coverage for both backend initialization sites to
verify the TLS field routing contract. In apps/cli/src/server/sync.ts around
lines 51-63, create tests for the sync handler that confirm caCert,
tlsClientCert, and tlsClientKey are passed to getHttpsAgent (not included in
restOpts sent to loadBackend), and that secure defaults are applied when
tlsSkipVerify is undefined. In apps/cli/src/server/validate.ts around lines
50-62, add identical coverage for the validate handler backend initialization
with the same TLS field and default-behavior assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cffcac2-865d-4982-a1b7-dc28fe676db5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/cli/e2e/server/backend-tls.e2e-spec.tsapps/cli/eslint.config.tsapps/cli/package.jsonapps/cli/src/server/agent-pool.spec.tsapps/cli/src/server/agent-pool.tsapps/cli/src/server/schema.tsapps/cli/src/server/sync.tsapps/cli/src/server/validate.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/server/agent-pool.spec.ts`:
- Around line 147-157: Balance every synthetic getHttpsAgent checkout in the LRU
and deferred-destruction tests by calling releaseHttpsAgent for each completed
lookup, including the calls around agentA, agentB, and agentC. Keep agentA
checked out only until the active-request assertion at the end of the
deferred-destruction scenario, and release all other agents immediately after
their modeled request completes.
In `@apps/cli/src/server/logger.ts`:
- Around line 29-31: Update the opts guard in the logger helper before the
`'tlsClientKey' in opts` check to require that opts is a non-null object,
returning body unchanged for primitives and other invalid values. Add a
regression test covering a body shaped as { task: { opts: 1 } } and verify
logging completes without throwing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9114eab-a96a-4c62-b098-6d69608b956c
📒 Files selected for processing (6)
apps/cli/e2e/server/backend-tls.e2e-spec.tsapps/cli/src/server/agent-pool.spec.tsapps/cli/src/server/agent-pool.tsapps/cli/src/server/logger.tsapps/cli/src/server/sync.tsapps/cli/src/server/validate.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/e2e/server/backend-tls.e2e-spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/cli/src/server/logger.spec.ts (2)
4-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression check for input immutability.
This test checks only the returned object. It would also pass if
redactRequestBodychangedbody.task.opts.tlsClientKeyin place. Retain the original body and verify that its key remains"SECRET"after redaction.Suggested assertion
- expect( - redactRequestBody({ + const body = { task: { opts: { backend: 'apisix', tlsClientKey: 'SECRET', tlsClientCert: 'cert' }, config: {}, }, - }), - ).toEqual({ + }; + expect(redactRequestBody(body)).toEqual({ task: { opts: { backend: 'apisix', tlsClientKey: '***', tlsClientCert: 'cert' }, config: {}, }, }); + expect(body.task.opts.tlsClientKey).toBe('SECRET');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/server/logger.spec.ts` around lines 4 - 18, The redactRequestBody test should also verify input immutability. In the test that redacts task.opts.tlsClientKey, retain the original request body before calling redactRequestBody and assert afterward that its tlsClientKey remains "SECRET", while preserving the existing returned-object assertions.
25-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the return value for malformed inputs.
The table checks only that
redactRequestBodydoes not throw. The helper also returns the original body whentask.optsis absent or invalid. Assert reference identity for each case so a regression that drops or rewrites malformed bodies cannot pass.Suggested assertion
- ])('does not throw for malformed body %j', (body) => { - expect(() => redactRequestBody(body)).not.toThrow(); + ])('returns malformed body unchanged without throwing', (body) => { + expect(redactRequestBody(body)).toBe(body);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/server/logger.spec.ts` around lines 25 - 35, The malformed-input cases in the redactRequestBody parameterized test only verify that no exception is thrown. Update the test to capture each input and assert redactRequestBody returns the exact same body reference for absent or invalid task.opts values, while retaining the no-throw assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/server/logger.spec.ts`:
- Around line 4-18: The redactRequestBody test should also verify input
immutability. In the test that redacts task.opts.tlsClientKey, retain the
original request body before calling redactRequestBody and assert afterward that
its tlsClientKey remains "SECRET", while preserving the existing returned-object
assertions.
- Around line 25-35: The malformed-input cases in the redactRequestBody
parameterized test only verify that no exception is thrown. Update the test to
capture each input and assert redactRequestBody returns the exact same body
reference for absent or invalid task.opts values, while retaining the no-throw
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba853444-a13a-4c56-bd68-86fa124166c3
📒 Files selected for processing (3)
apps/cli/src/server/agent-pool.spec.tsapps/cli/src/server/logger.spec.tsapps/cli/src/server/logger.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cli/src/server/logger.ts
- apps/cli/src/server/agent-pool.spec.ts
juzhiyuan
approved these changes
Aug 4, 2026
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
2 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
#537 introduced a feature that allows customizing TLS connection credentials for each endpoint on the ADC server. However, its implementation boundaries were unclear, and it modified code across multiple layers that should not have been involved, and the functionality is limited.
This PR provides comprehensive TLS capabilities (and only the cli/server implementation will be modified; the SDK and backend implementations will remain unchanged), including the ability to specify PEM CA and mTLS certificate pairs, and supports connection pools across different endpoints.
It uses a fingerprint mechanism that generates a hash based on the certificate and other factors. When an identical hash is detected, the connection pool will hit the same HttpsAgent instance, thereby using a cached connection. Different fingerprints will hit different HttpsAgent instances in a higher-level LRUCache, ensuring that connections using different certificates will not be incorrectly reused.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests