Add TLS to Jaeger dev deployment via OpenShift serving certs#361
Add TLS to Jaeger dev deployment via OpenShift serving certs#361vimalk78 wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Jaeger deployment script now provisions an OpenShift serving certificate, configures the OTLP gRPC receiver for TLS, mounts the certificate and generated configuration, and labels the displayed endpoint as TLS-enabled. ChangesJaeger OTLP gRPC TLS
Sequence Diagram(s)sequenceDiagram
participant deploy-jaeger.sh
participant OpenShift
participant Jaeger
deploy-jaeger.sh->>OpenShift: Annotate jaeger-otlp-grpc for serving certificate
OpenShift-->>deploy-jaeger.sh: Create serving-certificate secret
deploy-jaeger.sh->>OpenShift: Apply Jaeger config and mount certificate secret
OpenShift->>Jaeger: Start TLS-enabled OTLP gRPC receiver
deploy-jaeger.sh->>OpenShift: Print TLS OTLP gRPC endpoint
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 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 `@hack/deploy-jaeger.sh`:
- Around line 23-25: Update the oc set volume command in deploy-jaeger.sh so it
no longer suppresses failures with a generic fallback. Before adding the volume,
explicitly detect whether the serving-cert volume already exists and skip the
add only in that case; otherwise propagate failures from oc set volume,
including invalid secrets, permission errors, and mount conflicts.
- Line 45: Update the OTLP gRPC endpoint printed by the deploy script to use the
service-serving certificate hostname `jaeger-otlp-grpc.$NAMESPACE.svc:4317`,
removing `.cluster.local` while preserving the TLS annotation and port.
- Around line 17-21: Update the Jaeger deployment configuration around the
existing oc set env command to either pin jaegertracing/jaeger to a
v1-compatible image tag or migrate the configuration to Jaeger v2 settings.
Ensure the selected approach enables OTLP gRPC TLS and preserves the configured
certificate and key paths from CERT_MOUNT.
🪄 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: Enterprise
Run ID: 233083b0-f182-4afa-92af-52cfcb3b8151
📒 Files selected for processing (1)
hack/deploy-jaeger.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
5d46892 to
429f5e5
Compare
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 `@hack/deploy-jaeger.sh`:
- Line 72: Update the JSON patch operation in the Jaeger deployment patch
command to use `add` instead of `replace` for
`/spec/template/spec/containers/0/args`, so it creates the missing field on
initial deployment and replaces it safely on reruns.
- Line 83: Update the OTLP gRPC endpoint printed by the deployment script to
include the https:// scheme before the Jaeger service hostname, while preserving
the existing namespace and port values.
🪄 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: Enterprise
Run ID: 187fd2fd-39ba-43a1-92fc-272acd089c01
📒 Files selected for processing (1)
hack/deploy-jaeger.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| oc set volume deployment/jaeger -n "$NAMESPACE" --add --overwrite --name=config --type=configmap --configmap-name="$CONFIG_CM" --mount-path=/etc/jaeger | ||
| oc set volume deployment/jaeger -n "$NAMESPACE" --add --overwrite --name=serving-cert --type=secret --secret-name="$CERT_SECRET" --mount-path="$CERT_MOUNT" --read-only | ||
|
|
||
| oc patch deployment jaeger -n "$NAMESPACE" --type=json -p '[{"op":"replace","path":"/spec/template/spec/containers/0/args","value":["--config","/etc/jaeger/config.yaml"]}]' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Use JSON Patch add for container arguments.
A deployment created on line 16 has no containers[0].args, so replace fails and aborts the script. add creates the field and also replaces it on reruns.
Proposed fix
-oc patch deployment jaeger -n "$NAMESPACE" --type=json -p '[{"op":"replace","path":"/spec/template/spec/containers/0/args","value":["--config","/etc/jaeger/config.yaml"]}]'
+oc patch deployment jaeger -n "$NAMESPACE" --type=json -p '[{"op":"add","path":"/spec/template/spec/containers/0/args","value":["--config","/etc/jaeger/config.yaml"]}]'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc patch deployment jaeger -n "$NAMESPACE" --type=json -p '[{"op":"replace","path":"/spec/template/spec/containers/0/args","value":["--config","/etc/jaeger/config.yaml"]}]' | |
| oc patch deployment jaeger -n "$NAMESPACE" --type=json -p '[{"op":"add","path":"/spec/template/spec/containers/0/args","value":["--config","/etc/jaeger/config.yaml"]}]' |
🤖 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 `@hack/deploy-jaeger.sh` at line 72, Update the JSON patch operation in the
Jaeger deployment patch command to use `add` instead of `replace` for
`/spec/template/spec/containers/0/args`, so it creates the missing field on
initial deployment and replaces it safely on reruns.
| echo "" | ||
| echo "Jaeger UI: http://$ROUTE" | ||
| echo "OTLP gRPC: jaeger-otlp-grpc.$NAMESPACE.svc.cluster.local:4317" | ||
| echo "OTLP gRPC: jaeger-otlp-grpc.$NAMESPACE.svc.cluster.local:4317 (TLS)" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Publish an https:// OTLP endpoint.
The linked tracing client enables TLS only when OTEL_EXPORTER_OTLP_ENDPOINT starts with https; copying this bare endpoint produces a plaintext connection attempt against the TLS listener.
-echo "OTLP gRPC: jaeger-otlp-grpc.$NAMESPACE.svc.cluster.local:4317 (TLS)"
+echo "OTLP gRPC: https://jaeger-otlp-grpc.$NAMESPACE.svc.cluster.local:4317 (TLS)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "OTLP gRPC: jaeger-otlp-grpc.$NAMESPACE.svc.cluster.local:4317 (TLS)" | |
| echo "OTLP gRPC: https://jaeger-otlp-grpc.$NAMESPACE.svc.cluster.local:4317 (TLS)" |
🤖 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 `@hack/deploy-jaeger.sh` at line 83, Update the OTLP gRPC endpoint printed by
the deployment script to include the https:// scheme before the Jaeger service
hostname, while preserving the existing namespace and port values.
Source: Linked repositories
|
@vimalk78: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
insecure: false, so Jaeger needs a valid TLS certjaeger-otlp-grpcservice withserving-cert-secret-namefor auto-generated certsCOLLECTOR_OTLP_GRPC_TLS_*env varsTest plan
hack/deploy-jaeger.shand confirm Jaeger starts with TLS on port 4317🤖 Generated with Claude Code