feat: pass proxy url to admission webhook#2718
Merged
wilsonrivera merged 4 commits intomainfrom Apr 1, 2026
Merged
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
Contributor
WalkthroughThreads a new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/monograph/publishMonograph.ts (1)
159-160: Consider replacing sentinelundefinedpositional args with an options object.Line 159 uses
undefinedonly to reach the newwebhookProxyUrlpositional parameter. This is easy to break on future signature changes. A single options object (or overload) for trailing optional params would make call sites safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/monograph/publishMonograph.ts` around lines 159 - 160, The call in publishMonograph.ts is using a sentinel undefined to skip a positional parameter (currently passing "undefined, opts.webhookProxyUrl"); change the call to pass a single options object (e.g. { webhookProxyUrl: opts.webhookProxyUrl }) instead and update the callee's signature to accept an options parameter (or overload) so trailing optional parameters are named properties; specifically modify the function that is being invoked at this call site to accept and destructure an options object (including webhookProxyUrl) and update all call sites accordingly to remove the fragile undefined placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/services/AdmissionWebhookController.ts`:
- Around line 45-57: The proxy agent setup currently uses HttpsProxyAgent for
both protocols and swallows initialization errors; update the initialization to
create two agents—an HttpProxyAgent for plain HTTP and an HttpsProxyAgent for
HTTPS—using the provided proxyUrl, and if construction fails throw the error
instead of just logging (refer to the existing agent variable creation and
try/catch around new HttpsProxyAgent). Then pass both agents into axios.create
as httpAgent and httpsAgent respectively, set proxy: false in the axios config,
and ensure any caught exceptions during agent creation are re-thrown so the
service fails fast.
---
Nitpick comments:
In `@controlplane/src/core/bufservices/monograph/publishMonograph.ts`:
- Around line 159-160: The call in publishMonograph.ts is using a sentinel
undefined to skip a positional parameter (currently passing "undefined,
opts.webhookProxyUrl"); change the call to pass a single options object (e.g. {
webhookProxyUrl: opts.webhookProxyUrl }) instead and update the callee's
signature to accept an options parameter (or overload) so trailing optional
parameters are named properties; specifically modify the function that is being
invoked at this call site to accept and destructure an options object (including
webhookProxyUrl) and update all call sites accordingly to remove the fragile
undefined placeholder.
🪄 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: a15adbe4-4833-4813-bad4-6eff37dce94e
📒 Files selected for processing (27)
controlplane/src/core/bufservices/contract/createContract.tscontrolplane/src/core/bufservices/contract/updateContract.tscontrolplane/src/core/bufservices/feature-flag/createFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/deleteFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/enableFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/updateFeatureFlag.tscontrolplane/src/core/bufservices/federated-graph/createFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/migrateFromApollo.tscontrolplane/src/core/bufservices/federated-graph/moveFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/updateFederatedGraph.tscontrolplane/src/core/bufservices/graph/recomposeGraph.tscontrolplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.tscontrolplane/src/core/bufservices/monograph/publishMonograph.tscontrolplane/src/core/bufservices/monograph/updateMonograph.tscontrolplane/src/core/bufservices/proposal/createProposal.tscontrolplane/src/core/bufservices/proposal/updateProposal.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/fixSubgraphSchema.tscontrolplane/src/core/bufservices/subgraph/moveSubgraph.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/updateSubgraph.tscontrolplane/src/core/composition/composer.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/services/AdmissionWebhookController.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2718 +/- ##
===========================================
- Coverage 63.14% 46.96% -16.18%
===========================================
Files 249 1052 +803
Lines 26661 143453 +116792
Branches 0 9626 +9626
===========================================
+ Hits 16835 67377 +50542
- Misses 8449 74330 +65881
- Partials 1377 1746 +369
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/core/services/AdmissionWebhookController.ts (1)
49-54:⚠️ Potential issue | 🟠 MajorDo not continue without proxy after proxy initialization failure.
Line 53 only logs and continues. With
proxyUrlconfigured, this should throw (fail closed) so admission requests don’t silently bypass expected network controls.Suggested fix
if (proxyUrl) { try { httpAgent = new HttpProxyAgent(proxyUrl, {}); httpsAgent = new HttpsProxyAgent(proxyUrl, {}); } catch (e) { - logger.error(e, 'Failed to create proxy agent'); + logger.error(e, 'Failed to create proxy agent'); + throw new AdmissionError('Invalid webhook proxy URL configuration', e as Error); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/AdmissionWebhookController.ts` around lines 49 - 54, The proxy agent creation swallows errors and continues, allowing requests to bypass expected proxy controls; in the AdmissionWebhookController where proxyUrl is used to construct HttpProxyAgent/HttpsProxyAgent, detect when proxyUrl is configured and the agent constructors throw, log the error via logger.error as now and then rethrow the exception (or throw a new Error) so initialization fails fast instead of continuing; ensure the try/catch around new HttpProxyAgent(proxyUrl, {}) and new HttpsProxyAgent(proxyUrl, {}) either lets the error propagate or explicitly throws after logging to prevent the controller from starting without a working proxy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/webhooks/OrganizationWebhookService.ts`:
- Around line 141-147: If a proxyUrl is provided but
HttpProxyAgent/HttpsProxyAgent creation fails, do not continue; after catching
the error in the proxy initialization block (where proxyUrl, httpAgent,
httpsAgent are set and HttpProxyAgent/HttpsProxyAgent are constructed) log the
error with logger.error and then rethrow or throw a new Error to abort
construction so requests cannot bypass the proxy (ensure the thrown error
includes the original error details).
---
Duplicate comments:
In `@controlplane/src/core/services/AdmissionWebhookController.ts`:
- Around line 49-54: The proxy agent creation swallows errors and continues,
allowing requests to bypass expected proxy controls; in the
AdmissionWebhookController where proxyUrl is used to construct
HttpProxyAgent/HttpsProxyAgent, detect when proxyUrl is configured and the agent
constructors throw, log the error via logger.error as now and then rethrow the
exception (or throw a new Error) so initialization fails fast instead of
continuing; ensure the try/catch around new HttpProxyAgent(proxyUrl, {}) and new
HttpsProxyAgent(proxyUrl, {}) either lets the error propagate or explicitly
throws after logging to prevent the controller from starting without a working
proxy.
🪄 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: 899c8c72-b671-4582-8d8f-0d778fe471e3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
controlplane/package.jsoncontrolplane/src/core/services/AdmissionWebhookController.tscontrolplane/src/core/webhooks/OrganizationWebhookService.ts
✅ Files skipped from review due to trivial changes (1)
- controlplane/package.json
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.
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.