OCPSTRAT-1852: Address review feedback for hosted control plane metrics enhancement#1948
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@csrwng: This pull request references OCPSTRAT-1852 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
- Switch authentication from bearer token + TokenReview to mTLS client certificate verification using the existing metrics-client-certs and cluster-signer-ca infrastructure - Use honorLabels: true on PodMonitor endpoints instead of metricRelabelings to handle label conflicts between proxy-injected and target labels - Fix imprecise source_pod references to use the standard pod label - Update user story to reference AlertingRule CRs and user-defined monitoring instead of PrometheusRule Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
csrwng
force-pushed
the
hosted-cluster-metrics-followup-2
branch
from
8dde61b to
f96b5ce
Compare
|
@jan--f ptal |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
| CSR controller when it reaches 20-25% remaining lifetime. | ||
| 2. The `metrics-client-certs` Secret is already mounted into the Prometheus pod | ||
| at `/etc/prometheus/secrets/metrics-client-certs/` (containing `tls.crt` and | ||
| `tls.key`). The PodMonitor's `tlsConfig` references these paths for client |
There was a problem hiding this comment.
(nit) it could also leverage the tls-client-certificate-auth scrape class which would inject the right cert/key files without hypershift needing to know about them.
There was a problem hiding this comment.
Good call, updated the PodMonitor to use scrapeClass: tls-client-certificate-auth and simplified the tlsConfig to only serverName. Also updated the prose in the authentication flow and HCCO resource descriptions to reflect this.
| keySecret: | ||
| name: metrics-client-certs | ||
| key: tls.key | ||
| serverName: <metrics-proxy-route-hostname> |
There was a problem hiding this comment.
using scrapeClass we should only need serverName.
There was a problem hiding this comment.
Done, removed ca, cert, and keySecret from the tlsConfig — only serverName remains now.
Leverage CMO's tls-client-certificate-auth scrape class to inject client cert/key and CA automatically, simplifying the PodMonitor's tlsConfig to only require serverName. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
@csrwng: 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. |
5 participants
Summary
metrics-client-certsandcluster-signer-cainfrastructurehonorLabels: trueon PodMonitor endpoints instead ofmetricRelabelingsto handle label conflicts between proxy-injected and target labelssource_podreferences to use the standardpodlabelAlertingRuleCRs and user-defined monitoring instead ofPrometheusRuleTest plan
metrics-client-certsCSR flow andcluster-signer-caverificationhonorLabels: trueandtlsConfigwith client cert referencessource_pod,bearerTokenFile, orTokenReviewreferences remain in active design sections🤖 Generated with Claude Code