Conversation
Signed-off-by: Alauda Bot <alaudabot@alauda.io>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Discussion point: cert-manager Certificate ownership This intent PR only consumes a Secret supplied by the deployment owner through My current preference is to keep issuance external in the core chart: it avoids a hard cert-manager dependency and leaves issuer, namespace, ownership, and rotation policy to the deployer. An opt-in Certificate template could be considered separately once those semantics are agreed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08def2881b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {{- if .Values.metrics.tls.enable }} | ||
| - --metrics-tls-enable | ||
| - --metrics-tls-cert-file=/etc/envoy-gateway/metrics-tls/tls.crt | ||
| - --metrics-tls-key-file=/etc/envoy-gateway/metrics-tls/tls.key |
There was a problem hiding this comment.
When metrics.tls.enable is set, these arguments make port 19001 TLS-only, but the chart's default pod annotations still contain only prometheus.io/scrape: "true" and the port, without prometheus.io/scheme: "https" (also visible in the generated TLS fixture). Installations relying on those annotations will therefore continue sending plaintext HTTP and lose all control-plane metrics after enabling this option; render HTTPS scrape metadata conditionally or require and document an explicit scraper override.
Useful? React with 👍 / 👎.
| # Control-plane Prometheus metrics configuration. This does not configure EnvoyProxy data-plane metrics. | ||
| metrics: | ||
| tls: | ||
| enable: false | ||
| secretRef: | ||
| name: "" |
There was a problem hiding this comment.
This introduces a new user-facing Helm capability, but the commit adds no fragment under release-notes/current/new_features/. The repository's release-note generator only compiles those fragments, so this feature will be omitted from the next release notes unless a corresponding entry is added.
Useful? React with 👍 / 👎.
|
@zirain 通过这种方式来给控制面的metrics加tls，这个方向可以被社区接受嘛。 |
Signed-off-by: Alauda Bot <alaudabot@alauda.io>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9854 +/- ##
==========================================
+ Coverage 76.29% 76.30% +0.01%
==========================================
Files 261 261
Lines 44588 44623 +35
==========================================
+ Hits 34019 34051 +32
- Misses 8312 8317 +5
+ Partials 2257 2255 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
What this PR does / why we need it:
This is an intent/prototype PR to discuss securing Envoy Gateway's control-plane Prometheus endpoint (
/metrics) with optional server-side TLS. It does not change EnvoyProxy data-plane metrics (/stats/prometheus).The deployment owner controls the feature through gateway-helm values:
When enabled, the chart mounts the user-provided Secret into the control-plane Pod and passes certificate paths to the metrics server. The server uses controller-runtime certwatcher so rotated Secret files are used for new TLS connections. Existing Prometheus registry composition, OTEL exporters, port 19001, and default HTTP behavior are preserved.
This PR is intentionally not a final API proposal. If the direction is accepted, the implementation details, naming, validation, observability integration, and compatibility behavior can be refined in follow-up work.
Open design question: cert-manager Certificate ownership
This prototype only consumes a Secret supplied by the deployment owner. Should gateway-helm optionally create a cert-manager
Certificateresource, or should certificate issuance remain entirely outside the chart? Keeping issuance external avoids a hard cert-manager dependency and leaves issuer/namespace/ownership policy to the deployment. An opt-in template could improve convenience, but would need explicit issuer configuration and behavior when cert-manager is absent.Which issue(s) this PR fixes:
Design discussion only; no issue is closed by this PR.
Verification
go test ./internal/metrics ./internal/cmd ./internal/envoygateway/confighelm lint charts/gateway-helm -f test/helm/gateway-helm/control-plane-metrics-tls.in.yamlmake helm-template.gateway-helmmake gen-checkreached Helm dependency generation but could not resolve the repository versions foralloy,loki, andopentelemetry-collectorin this environment.PR Checklist
/api.make gen-checkis blocked by external Helm repository version resolution; targeted checks pass.