feat(metricprovider): support self-signed certs for prometheus provider - #4964
feat(metricprovider): support self-signed certs for prometheus provider#4964abhyudayareddy wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4964 +/- ##
==========================================
- Coverage 85.20% 85.19% -0.01%
==========================================
Files 166 166
Lines 19453 19467 +14
==========================================
+ Hits 16575 16585 +10
- Misses 2027 2029 +2
- Partials 851 853 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Published E2E Test Results 4 files 4 suites 4h 11m 43s ⏱️ For more details on these failures, see this check. Results for commit 03a1829. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 635 tests 2 635 ✅ 3m 30s ⏱️ Results for commit 03a1829. ♻️ This comment has been updated with latest results. |
|
cc @argoproj/argo-rollouts-approvers — this is ready for review whenever you have a chance; CI is green. Thanks! |
|
Checking back in on this one — all checks are still green (CodeQL, unit tests, e2e across 1.32-1.35, SonarCloud). Let me know if there's anything I can do to help move it forward. Thanks! |
|
Another gentle nudge on this one — it's been ~3 weeks with no review. All 27 checks are still green (CodeQL, unit, e2e across k8s 1.32–1.35, SonarCloud), no conflicts with master. It's a small, self-contained change (self-signed cert support for the Prometheus metric provider, opt-in via config). Happy to rebase or adjust anything. Thanks! |
Adds an optional caCert field to the prometheus metric provider spec, allowing a PEM-encoded CA certificate bundle to be supplied so that TLS verification succeeds against a prometheus server using a private/self-signed certificate. Previously the only way to talk to such a server was to set insecure: true, which disables certificate verification entirely. caCert instead adds the given CA to the trusted pool used for verification, so TLS validation stays enabled. Relates to argoproj#2298 Signed-off-by: Abhyuday <abhyudayreddy@gmail.com>
The prior commit added PrometheusMetric.CACert to the Go types and regenerated most codegen artifacts, but missed the swagger spec and the UI's generated TypeScript models, which the CI 'Verify Codegen' job caught. Applying the exact diff that job's codegen run produced. Signed-off-by: Abhyuday <abhyudayreddy@gmail.com>
d663422 to
03a1829
Compare
|
1 participant
Problem
The prometheus metric provider only supports skipping TLS verification entirely via
insecure: true. There is no way to connect to a prometheus server that presents a certificate signed by a private/self-signed CA while keeping certificate verification enabled. Users who run their own CA (common for in-cluster prometheus, e.g. via OpenShift monitoring or self-managed clusters) are forced to disable TLS verification altogether to work around this, which is undesirable.Relates to #2298.
Root cause
NewPrometheusAPIinmetricproviders/prometheus/prometheus.goonly ever chose between two fixedhttp.Transports:secureTransport(default Go root CA trust store, full verification) andinsecureTransport(InsecureSkipVerify: true). There was no option to trust an additional/custom CA while still validating the server certificate against it.Fix
caCertfield toPrometheusMetric(pkg/apis/rollouts/v1alpha1/analysis_types.go), holding a PEM-encoded CA certificate bundle.newHTTPTransportWithCACertinmetricproviders/prometheus/prometheus.go, which parses the PEM bundle into anx509.CertPooland builds a transport withTLSClientConfig.RootCAsset to that pool (verification remains enabled, unlikeinsecure: true).NewPrometheusAPInow uses this transport whencaCertis set (andinsecureis not also set —insecurestill takes precedence, matching previous behavior of skipping verification when set).generated.pb.go/generated.proto(protobuf),openapi_generated.go, and the CRD manifests (manifests/crds/*.yaml,manifests/install.yaml,docs/features/kustomize/rollout_cr_schema.json) viamake gen-proto,make gen-openapi,make gen-crd, andmake manifests.docs/analysis/prometheus.md, alongside the existing "Skip TLS verification" section.Example
The certificate can be sourced from a Kubernetes Secret via the existing
valueFrom.secretKeyRefsupport for AnalysisTemplate arguments.Testing
metricproviders/prometheus/prometheus_test.go:TestNewPrometheusAPIWithCACert/TestNewPrometheusAPIWithInvalidCACert— valid/invalidcaCerthandling inNewPrometheusAPI.TestNewHTTPTransportWithCACert/TestNewHTTPTransportWithInvalidCACert— transport construction and PEM parsing.TestRunSuccessfullyWithCACert— end-to-end test usinghttptest.NewTLSServerwith a self-signed certificate; configurescaCertto the server's own certificate and asserts the query succeeds (proving TLS verification actually passes against the CA).TestRunErrorWithWrongCACert— same TLS server, but with an unrelated CA cert, asserting the connection is rejected (proving verification is not silently bypassed).go build ./...— passes.go vet ./metricproviders/... ./pkg/apis/...— passes.go test ./...— all 84 packages pass, no failures.golangci-lint run ./metricproviders/prometheus/... ./pkg/apis/rollouts/v1alpha1/...— 0 issues.git diff --exit-codeafter codegen only touches the expected files (no unrelated churn beyond the standard fully-rewritten gzipped proto file descriptor blob that any type change causes).Checklist