metrics scraping with local authentication and authorization #701
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
In just the past week we have seen significant difficulties that would alleviated by this
Given the relatively small scope and risk since, we should prioritize this highly for our coming releases. Monitoring is extremely valuable when things are not behaving, but certain classes of failures have nearly complete monitoring blackouts at this time. |
|
|
||
| 1. The monitoriting operator creates a client cert/key pair valid against the kube-apiserver. | ||
| There is a CSR signer name specifically for this. | ||
| 2. The kube-controller-manager operator runs an approver that allows monitoring operator SA to get signed certificate for |
There was a problem hiding this comment.
Is the approver decision going to be based solely on exact username match (and maybe groups match?) or is there going to be any additional mechanism (like RBAC or something custom)?
There was a problem hiding this comment.
Is the approver decision going to be based solely on exact username match (and maybe groups match?) or is there going to be any additional mechanism (like RBAC or something custom)?
I think exact user match makes sense. The service-account for the monitoring operator can create a client cert for the metrics scraper. I think that permission is reasonable and happen to know exactly what identity that is.
| 4. The monitoring operator keeps these credentials up to date (the signer is only good for 30 days or so). | ||
| 5. The metrics scraper mounts the client cert/key pair and ensures that it is hot-reloadable or builds a process suicider. | ||
| There is library-go code to suicide on file changes. | ||
| 6. A participating metrics scraper target, can read the in-cluster client cert CA bundle and create an authenticator |
There was a problem hiding this comment.
Specify the location of the CM for readers unfamiliar with it.
| 7. A servicemonitor grows a way to specify using this client cert. | ||
| Technically this is optional. | ||
| If you find it too difficult, just *always* use the client-cert for scraping since the secrets never leave disk. | ||
| Those targets which don't support it will simply ignore the client-cert. |
There was a problem hiding this comment.
note: it's a shame the less-secure way is the default one, that means you have to opt-in your service to not receiving to the scraper SA's credentials. I wish we could simply default to the cert.
There was a problem hiding this comment.
note: it's a shame the less-secure way is the default one, that means you have to opt-in your service to not receiving to the scraper SA's credentials. I wish we could simply default to the cert.
An e2e test trying to find offenders could be used to close gaps. But I don't see that as essential to building out the initial capability.
|
|
||
| ## Drawbacks | ||
|
|
||
| I have to stop here to make dinner. |
|
|
||
| ## Proposal | ||
|
|
||
| 1. The monitoriting operator creates a client cert/key pair valid against the kube-apiserver. |
There was a problem hiding this comment.
s/monitoriting/monitoring
There was a problem hiding this comment.
"cluster monitoring" to be nit-picky.
|
/cc @openshift/openshift-team-monitoring |
| and a subjectaccessreview, then the metrics scraper can be rejected. | ||
| The subjectaccessreview (authorization) is already largely addressed, but service account tokens are still used | ||
| for scraping targets. | ||
| Tokens require an external network call that we can avoid by using client certificates. |
There was a problem hiding this comment.
question: as tokens are signed, wouldn't a local signature check suffice?
There was a problem hiding this comment.
as an after thought, potentially a signature-only check in addition to scoped tokens would potentially suffice?
At least the advantage would be that client-side no additional cert management would be necessary.
There was a problem hiding this comment.
question: as tokens are signed, wouldn't a local signature check suffice?
No. In general tokens are not signed. ServiceAccount tokens in particular are, but if you only check locally, then the serviceaccount tokens become irrevocable. It is important to be able to revoke tokens.
Can update the doc.
| ## Proposal | ||
|
|
||
| 1. The monitoriting operator creates a client cert/key pair valid against the kube-apiserver. | ||
| There is a CSR signer name specifically for this. |
There was a problem hiding this comment.
which one is it specifically? is there an existing one or do we have to implement a new?
There was a problem hiding this comment.
which one is it specifically? is there an existing one or do we have to implement a new?
kubernetes.io/kube-apiserver-client from https://kubernetes.io/docs/reference/access-authn-authz/certificate-signing-requests/ . Can update the doc.
|
|
||
| ## Proposal | ||
|
|
||
| 1. The monitoriting operator creates a client cert/key pair valid against the kube-apiserver. |
There was a problem hiding this comment.
"cluster monitoring" to be nit-picky.
| 2. The kube-controller-manager operator runs an approver that allows monitoring operator SA to get signed certificate for | ||
| The existing scraper identity. | ||
| 3. The monitoring operator puts the client cert/key pair into a secret. | ||
| 4. The monitoring operator keeps these credentials up to date (the signer is only good for 30 days or so). |
There was a problem hiding this comment.
I guess that the cluster monitoring operator needs to request a new client certificate before it reaches the expiration date, correct?
There was a problem hiding this comment.
I guess that the cluster monitoring operator needs to request a new client certificate before it reaches the expiration date, correct?
Yes. And because we only sign for 30 days or so.
| The existing scraper identity. | ||
| 3. The monitoring operator puts the client cert/key pair into a secret. | ||
| 4. The monitoring operator keeps these credentials up to date (the signer is only good for 30 days or so). | ||
| 5. The metrics scraper mounts the client cert/key pair and ensures that it is hot-reloadable or builds a process suicider. |
There was a problem hiding this comment.
Prometheus will automatically reload the certificate/key files if they change.
| 6. all control plane operators | ||
| 7. kube-controller-manager | ||
| 8. etcd | ||
| These are the ones I've updated already and we'll be able to get more. |
There was a problem hiding this comment.
All monitoring components (except Prometheus, Alertmanager and Thanos querier) also use kube-rbac-proxy to secure their metrics endpoint. The 3 others got scraped via oauth-proxy (because historical reasons I guess).
| 6. A participating metrics scraper target, can read the in-cluster client cert CA bundle and create an authenticator | ||
| using it. | ||
| The default delegated authentication stack does this and on openshift, the configmap is exposed to system:authenticated. | ||
| 7. A servicemonitor grows a way to specify using this client cert. |
There was a problem hiding this comment.
Not sure what you mean here. The service monitor allows to specify a client certificate/key either by file path in the Prometheus container or reference to a secret's key.
There was a problem hiding this comment.
Not sure what you mean here. The service monitor allows to specify a client certificate/key either by file path in the Prometheus container or reference to a secret's key.
That appears to be a reference to a secret in the namespace containing the target. If that were the case, then every target would have to have client cert credentials for scanning /metrics. We want to avoid this level of permissions for every component.
Is is possible to specify a client-cert that exists in the metric scraping namespace?
There was a problem hiding this comment.
The requirement is that the secret exists in the same namespace as the service monitor. The other option today would be to use the certFile and keyFile files but it assumes that the target knows the location of these files in the Prometheus container. It's not worse than what we do now for bearer tokens and CA but it's deprecated and we'll probably remove it eventually.
There was a problem hiding this comment.
And I'm not saying that we can't evolve things upstream :)
There was a problem hiding this comment.
The other option today would be to use the
certFileandkeyFilefiles but it assumes that the target knows the location of these files in the Prometheus container.
This option is more reasonable for the effort we're trying to complete here. Having every namespace with the power to scrape all metrics is not desireable.
There was a problem hiding this comment.
The requirement is that the secret exists in the same namespace as the service monitor.
@simonpasquier i think referencing the secret via reference rather than file would be desirable. Do I understand correctly you think about losening the restriction in prometheus-operator to be able to reference a secret from a different namespace?
There was a problem hiding this comment.
The requirement is that the secret exists in the same namespace as the service monitor.
@simonpasquier i think referencing the secret via reference rather than file would be desirable. Do I understand correctly you think about losening the restriction in prometheus-operator to be able to reference a secret from a different namespace?
I think an ideal way would be to say, "send me a client cert that identifies you as the scraper" without being able to specify exactly what. The exact mechanism can be niggled later. For the purposes of OCP, we can even hardcode it as a default since key material never leaves the scraper pod.
|
@deads2k i think we reached consensus generally 👍 do you mind to address the nits above so we can merge? I think we can add specifics as a follow-up? |
| 4. The monitoring operator keeps these credentials up to date (the signer is only good for 30 days or so). | ||
| 5. The metrics scraper mounts the client cert/key pair and ensures that it is hot-reloadable or builds a process suicider. | ||
| There is library-go code to suicide on file changes. | ||
| 6. A participating metrics scraper target, can read the in-cluster client cert CA bundle and create an authenticator |
There was a problem hiding this comment.
is it expected for the participating metrics scraper target to be able to hot-reload the client cert CA as well? (asking, as kube-rbac-proxy doesn't right now)
There was a problem hiding this comment.
is it expected for the participating metrics scraper target to be able to hot-reload the client cert CA as well? (asking, as kube-rbac-proxy doesn't right now)
Highly recommended. The delegating authenticator provided by kube in k8s.io/apiserver (vendoraable package) does this.
There was a problem hiding this comment.
i was about to ask if there's things to reuse, thanks!
|
@deads2k how about we wrap this enhancement up? i guess we reached consensus generally 🎉 |
|
/approve |
deads2k
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: simonpasquier The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| 1. The monitoriting operator creates a client cert/key pair valid against the kube-apiserver. | ||
| There is a CSR signer name specifically for this. | ||
| 2. The kube-controller-manager operator runs an approver that allows monitoring operator SA to get signed certificate for |
There was a problem hiding this comment.
We concretely ended up to be in the KCM https://github.com/openshift/cluster-policy-controller sidecar.
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
20 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
deads2k
force-pushed
the
monitoring-client-certs
branch
from
e2b2a2f
to
7888a17
Compare
|
New changes are detected. LGTM label has been removed. |
|
@deads2k: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
Monitoring needs to be reliable and is the very useful when trying to debug clusters in an already degraded state. We want to ensure that metrics scraping can always work if the scraper can reach the target, even if the kube-apiserver is unavailable or unreachable. To do this, we will combine a local authorizer (already merged in many binaries and the rbac-proxy) and client-cert based authentication to have a fully local authentication and authorization path for scraper targets.
If networking (or part of networking) is down and a scraper target cannot reach the kube-apiserver to verify a token and a subjectaccessreview, then the metrics scraper can be rejected. The subjectaccessreview (authorization) is already largely addressed, but service account tokens are still used for scraping targets. Tokens require an external network call that we can avoid by using client certificates. Gathering metrics, especially client metrics, from partially functionally clusters helps narrow the search area between kube-apiserver, etcd, kubelet, and SDN considerably.
In addition, this will significantly reduce the load on the kube-apiserver. We have observed in the CI cluster that token and subject access reviews are a significant percentage of all kube-apiserver traffic.
/assign @s-urbaniak
/assign @sym3tri