feat: local-env-setup should use HTTPS by default with a self-signed cert#1162
Open
manik3160 wants to merge 11 commits into
Open
feat: local-env-setup should use HTTPS by default with a self-signed cert#1162manik3160 wants to merge 11 commits into
manik3160 wants to merge 11 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSwitches the local Istio gateway from HTTP to HTTPS by introducing a cert-manager self-signed CA chain ( ChangesLocal gateway TLS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
manik3160
force-pushed
the
local-env-https
branch
from
d07b39d to
cf05093
Compare
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
manik3160
force-pushed
the
local-env-https
branch
from
cf05093 to
c497a31
Compare
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
- Updated gateway listeners on port 8080 to use 8443 and HTTPS - Extended local gateway TLS certificate with wildcard SANs for internal routes - Changed default listenerPort in e2e tests from 8080 to 8443 Signed-off-by: Manik <maniksingh3606@gmail.com>
- Added patch-broker-local-ca target to Makefile - Added step to conformance.yaml and local-env-setup to patch the dynamically created broker deployment with the local Cert-Manager self-signed CA. - This ensures the broker can verify the gateway's TLS certificate when making internal hairpin requests (e.g. tools/call). Signed-off-by: Manik <maniksingh3606@gmail.com>
manik3160
force-pushed
the
local-env-https
branch
from
e1236d4 to
d27da83
Compare
Contributor
|
Thanks for the contribution, @manik3160! You currently have other non-draft PR(s) open: To help us review and merge changes as efficiently as possible, we ask contributors to focus on one PR at a time. Activity on this project can be high, and maintainers have other priorities outside the project, so having a single active PR helps everyone get changes landed faster. This PR has been moved to draft. Once your other PR(s) are merged or closed, mark this one as ready for review and we will take a look. Note This is an experimental process and may change or need manual intervention while we trial it. |
coderabbitai
Bot
added
high-risk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
build/dev.mk (1)
47-50: ⚡ Quick winPrefer CA-based TLS verification over
-kindev-test.Using
-kdisables cert validation and can mask local TLS trust issues. Keep insecure mode opt-in, and default to validating with the generated CA bundle.Proposed change
- curl -k -X POST \ + curl --cacert out/certs/ca.crt -X POST \ -H "Content-Type: application/json" \ -d '{"jsonrpc":"2.0","method":"initialize","params":{},"id":1}' \ $(GATEWAY_SCHEME)://mcp.127-0-0-1.sslip.io:$(KIND_HOST_PORT_MCP_GATEWAY)/mcp🤖 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 `@build/dev.mk` around lines 47 - 50, The curl command in the dev-test target uses the `-k` flag which disables TLS certificate validation. Remove the `-k` flag from the curl invocation and instead add a `--cacert` option that points to the generated CA bundle file to enable proper certificate validation by default. This ensures TLS trust issues are surfaced rather than masked during development testing.
🤖 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 @.github/workflows/conformance.yaml:
- Line 31: The NODE_TLS_REJECT_UNAUTHORIZED environment variable set to "0" in
the conformance workflow job is globally disabling TLS certificate validation,
which weakens the conformance test signal. Remove the
NODE_TLS_REJECT_UNAUTHORIZED: "0" line from the environment variables and
instead configure NODE_EXTRA_CA_CERTS to point to the local CA certificate file
that needs to be trusted, allowing proper TLS verification to remain enabled
while still trusting the required certificate authority.
In `@Makefile`:
- Around line 81-83: The GATEWAY_SCHEME variable defined here does not actually
control gateway behavior - it only affects printed URLs. To fix this, locate
where the local-env-setup target (and related targets around lines 740-745)
hard-enables TLS resources and HTTPS gateway flow, and make these conditional on
the value of GATEWAY_SCHEME. When GATEWAY_SCHEME is set to http, ensure that TLS
resources are not enabled and the gateway uses HTTP instead of HTTPS, so the
override actually changes gateway behavior rather than just the constructed
URLs.
- Around line 483-486: Remove the `|| true` at the end of the kubectl get secret
command (which extracts the CA certificate) to ensure the target fails loudly if
CA extraction fails. Additionally, replace the JSON patch operations that use
the add operation on volumes, volumeMounts, and command arrays with idempotent
alternatives such as using replace operations or conditionally checking for
existing elements before adding, so that repeated executions of this target do
not duplicate or fail when the same elements already exist in the deployment
specification.
---
Nitpick comments:
In `@build/dev.mk`:
- Around line 47-50: The curl command in the dev-test target uses the `-k` flag
which disables TLS certificate validation. Remove the `-k` flag from the curl
invocation and instead add a `--cacert` option that points to the generated CA
bundle file to enable proper certificate validation by default. This ensures TLS
trust issues are surfaced rather than masked during development testing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad53acb1-6839-4eff-ad20-b2f38c6c2cf6
📒 Files selected for processing (11)
.github/workflows/conformance.yamlMakefilebuild/ci.mkbuild/dev.mkbuild/info.mkbuild/inspect.mkconfig/istio/gateway/gateway.yamlconfig/istio/gateway/local-gateway-tls.yamlconfig/istio/gateway/nodeport.yamltests/e2e/builders.gotests/e2e/commons.go
| CONFORMANCE_VERSION: "0.1.15" | ||
| MCP_URL: "http://mcp.127-0-0-1.sslip.io:8001/mcp" | ||
| MCP_URL: "https://mcp.127-0-0-1.sslip.io:8001/mcp" | ||
| NODE_TLS_REJECT_UNAUTHORIZED: "0" |
There was a problem hiding this comment.
Avoid globally disabling Node TLS verification in CI.
NODE_TLS_REJECT_UNAUTHORIZED: "0" turns off certificate validation for all Node traffic in this job, which can hide real TLS trust failures and weakens the conformance signal. Prefer adding the local CA to trust (NODE_EXTRA_CA_CERTS) and keeping verification enabled.
🤖 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 @.github/workflows/conformance.yaml at line 31, The
NODE_TLS_REJECT_UNAUTHORIZED environment variable set to "0" in the conformance
workflow job is globally disabling TLS certificate validation, which weakens the
conformance test signal. Remove the NODE_TLS_REJECT_UNAUTHORIZED: "0" line from
the environment variables and instead configure NODE_EXTRA_CA_CERTS to point to
the local CA certificate file that needs to be trusted, allowing proper TLS
verification to remain enabled while still trusting the required certificate
authority.
Comment on lines
+81
to
+83
| # default scheme for the local gateway URL (override to http for plain HTTP) | ||
| GATEWAY_SCHEME ?= https | ||
|
|
There was a problem hiding this comment.
GATEWAY_SCHEME=http is not actually wired to an HTTP gateway path.
local-env-setup still hard-enables TLS resources and HTTPS gateway flow regardless of GATEWAY_SCHEME, so the override currently only changes printed/constructed URLs, not gateway behavior.
Suggested direction
+# enforce supported schemes
+ifeq ($(filter $(GATEWAY_SCHEME),http https),)
+$(error GATEWAY_SCHEME must be 'http' or 'https')
+endif
+
local-env-setup: setup-cluster-base ## Setup complete local demo environment with Kind, Istio, MCP Gateway, and test servers
`@echo` "========================================="
`@echo` "Setting up Local Demo Environment"
`@echo` "========================================="
- "$(MAKE)" cert-manager-install
- "$(MAKE)" deploy-gateway
- $(KUBECTL) apply -f config/istio/gateway/local-gateway-tls.yaml
- @$(KUBECTL) wait --for=condition=Ready certificate/mcp-gateway-local-cert -n gateway-system --timeout=60s
+ifeq ($(GATEWAY_SCHEME),https)
+ "$(MAKE)" cert-manager-install
+ "$(MAKE)" deploy-gateway
+ $(KUBECTL) apply -f config/istio/gateway/local-gateway-tls.yaml
+ @$(KUBECTL) wait --for=condition=Ready certificate/mcp-gateway-local-cert -n gateway-system --timeout=60s
+else
+ "$(MAKE)" deploy-gateway-http
+endif
"$(MAKE)" deploy
- "$(MAKE)" patch-broker-local-ca
+ifeq ($(GATEWAY_SCHEME),https)
+ "$(MAKE)" patch-broker-local-ca
+endifAlso applies to: 740-745
🤖 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 `@Makefile` around lines 81 - 83, The GATEWAY_SCHEME variable defined here does
not actually control gateway behavior - it only affects printed URLs. To fix
this, locate where the local-env-setup target (and related targets around lines
740-745) hard-enables TLS resources and HTTPS gateway flow, and make these
conditional on the value of GATEWAY_SCHEME. When GATEWAY_SCHEME is set to http,
ensure that TLS resources are not enabled and the gateway uses HTTP instead of
HTTPS, so the override actually changes gateway behavior rather than just the
constructed URLs.
Comment on lines
+483
to
+486
| @kubectl get secret private-ca-keypair -n cert-manager -o jsonpath='{.data.ca\.crt}' | base64 -d > out/certs/ca.crt || true | ||
| @kubectl create secret generic gateway-ca-bundle -n mcp-system --from-file=ca.crt=out/certs/ca.crt --dry-run=client -o yaml | kubectl apply -f - | ||
| @kubectl patch deployment mcp-gateway -n mcp-system --type='json' -p='[{"op":"add","path":"/spec/template/spec/volumes/-","value":{"name":"gateway-ca","secret":{"secretName":"gateway-ca-bundle"}}},{"op":"add","path":"/spec/template/spec/containers/0/volumeMounts/-","value":{"name":"gateway-ca","mountPath":"/certs/gateway-ca.crt","subPath":"ca.crt","readOnly":true}},{"op":"add","path":"/spec/template/spec/containers/0/command/-","value":"--gateway-ca-cert=/certs/gateway-ca.crt"}]' | ||
| @kubectl rollout status deployment mcp-gateway -n mcp-system --timeout=60s |
There was a problem hiding this comment.
patch-broker-local-ca can silently continue on bad CA input and is not idempotent.
|| true on CA extraction hides failures, and repeated JSON add patches can duplicate/fail on volume/mount/arg insertion in reruns.
Proposed hardening
patch-broker-local-ca: ## Patch broker deployment to trust local self-signed CA
`@echo` "Patching broker-router deployment to trust local self-signed CA..."
`@kubectl` wait --for=condition=Available deployment/mcp-gateway -n mcp-system --timeout=120s
- `@kubectl` get secret private-ca-keypair -n cert-manager -o jsonpath='{.data.ca\.crt}' | base64 -d > out/certs/ca.crt || true
+ `@set` -euo pipefail; mkdir -p out/certs; \
+ kubectl get secret private-ca-keypair -n cert-manager -o jsonpath='{.data.ca\.crt}' | base64 -d > out/certs/ca.crt; \
+ test -s out/certs/ca.crt
`@kubectl` create secret generic gateway-ca-bundle -n mcp-system --from-file=ca.crt=out/certs/ca.crt --dry-run=client -o yaml | kubectl apply -f -
- `@kubectl` patch deployment mcp-gateway -n mcp-system --type='json' -p='[{"op":"add","path":"/spec/template/spec/volumes/-","value":{"name":"gateway-ca","secret":{"secretName":"gateway-ca-bundle"}}},{"op":"add","path":"/spec/template/spec/containers/0/volumeMounts/-","value":{"name":"gateway-ca","mountPath":"/certs/gateway-ca.crt","subPath":"ca.crt","readOnly":true}},{"op":"add","path":"/spec/template/spec/containers/0/command/-","value":"--gateway-ca-cert=/certs/gateway-ca.crt"}]'
+ `@kubectl` get deployment mcp-gateway -n mcp-system -o jsonpath='{.spec.template.spec.volumes[*].name}' | grep -qw gateway-ca || \
+ kubectl patch deployment mcp-gateway -n mcp-system --type='json' -p='[{"op":"add","path":"/spec/template/spec/volumes/-","value":{"name":"gateway-ca","secret":{"secretName":"gateway-ca-bundle"}}}]'
+ `@kubectl` get deployment mcp-gateway -n mcp-system -o jsonpath='{.spec.template.spec.containers[0].volumeMounts[*].name}' | grep -qw gateway-ca || \
+ kubectl patch deployment mcp-gateway -n mcp-system --type='json' -p='[{"op":"add","path":"/spec/template/spec/containers/0/volumeMounts/-","value":{"name":"gateway-ca","mountPath":"/certs/gateway-ca.crt","subPath":"ca.crt","readOnly":true}}]'
+ `@kubectl` get deployment mcp-gateway -n mcp-system -o jsonpath='{.spec.template.spec.containers[0].command}' | grep -qw -- '--gateway-ca-cert=/certs/gateway-ca.crt' || \
+ kubectl patch deployment mcp-gateway -n mcp-system --type='json' -p='[{"op":"add","path":"/spec/template/spec/containers/0/command/-","value":"--gateway-ca-cert=/certs/gateway-ca.crt"}]'
`@kubectl` rollout status deployment mcp-gateway -n mcp-system --timeout=60s🤖 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 `@Makefile` around lines 483 - 486, Remove the `|| true` at the end of the
kubectl get secret command (which extracts the CA certificate) to ensure the
target fails loudly if CA extraction fails. Additionally, replace the JSON patch
operations that use the add operation on volumes, volumeMounts, and command
arrays with idempotent alternatives such as using replace operations or
conditionally checking for existing elements before adding, so that repeated
executions of this target do not duplicate or fail when the same elements
already exist in the deployment specification.
Signed-off-by: Manik <maniksingh3606@gmail.com>
…oute Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
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.
The e2e test infrastructure already uses HTTPS with a private CA certificate, but
make local-setup-envstill defaults to plain HTTP. This PR updates the local development environment to match the e2e setup and use HTTPS by default with a self-signed certificate.Closes #1142
Changes
config/istio/gateway/local-gateway-tls.yamlto provision a self-signed CA and TLS certificate for the local gatewaymcplistener to default toHTTPSwith TLS termination30080to the new8443HTTPS listenerlocal-env-setupMakefile target to installcert-managerand apply the local TLS resourcesci-setuptarget to also apply the new TLS resource so the gateway programs successfullyGATEWAY_SCHEMEvariable (defaulting tohttps) to update all CLI URL output references (inspect, info, dev)Existing users can still override to plain HTTP by running
make local-env-setup GATEWAY_SCHEME=http.Summary by CodeRabbit
New Features
Chores
Tests
Documentation
/mcppath.