✨ Re-introduce authn/authz for metrics endpoint #3968
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
joelanford
force-pushed
the
authn-authz-controller-runtime-metrics
branch
from
e2b8a3a to
7797875
Compare
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
joelanford
force-pushed
the
authn-authz-controller-runtime-metrics
branch
from
7797875 to
15d8fa6
Compare
docs/book/src/cronjob-tutorial/testdata/project/config/default/kustomization.yaml
Show resolved
Hide resolved
docs/book/src/cronjob-tutorial/testdata/project/config/prometheus/monitor.yaml
Show resolved
Hide resolved
| SecureServing: secureMetrics, | ||
| TLSOpts: tlsOpts, | ||
| BindAddress: metricsAddr, | ||
| FilterProvider: filters.WithAuthenticationAndAuthorization, |
There was a problem hiding this comment.
Would we have an option to give the users the choice to opt-in or not to use this one?
What about we scaffold it commented and add comments in the code to allow people opt-in to use it if they wish OR we have a plugin that do it ?
I am not sure what would be the best I just felt that now that finally we clean it up we have the chance to do that properly and give the chance for the users opt-in or not so that we are able to properly address the feedbacks. WDYT?
There was a problem hiding this comment.
Have you good idea as always :-) ?
So users reported that they do not like kube-rbac-proxy in the past and would like to not see that by default (i.e. #3482) others shared that mainly only use network-policies so I was wonder if now we could grow it in a way that give more power to users opt-in or not.
There was a problem hiding this comment.
IMO the two ideal states are:
- Metrics listening on localhost only
- Metrics listening via TLS with a valid cert chain and this (or a similar) filter.
Do you agree with that? If so, I think that means that the cert-manager overlay would actually be the thing that controls the flags that are passed into the controller container to enable the TLS listen address and the authn/authz filters.
There was a problem hiding this comment.
Yes, I totally agree.
I think the next step would be add an option to patch the cert-manager config as well
| # If you want to expose the metric endpoint of your controller-manager uncomment the following line. | ||
| #- path: manager_metrics_patch.yaml | ||
| # target: | ||
| # kind: Deployment |
There was a problem hiding this comment.
Could we not keep this one so that we allow people disable if they wish?
We can let enable with :8443 instead of :8080 by default
| if _, err := t.Kubectl.WithInput(string(output)).Command("delete", "-f", "-"); err != nil { | ||
| warnError(err) | ||
| } | ||
| } |
| } | ||
|
|
||
| // GenerateV4WithoutMetrics implements a go/v4 plugin project defined by a TestContext. | ||
| func GenerateV4WithoutMetrics(kbc *utils.TestContext) { |
There was a problem hiding this comment.
I think would be great we keep a test to ensure that the metrics are not exposed and we have an option to opt-out from it that works.
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
| @@ -70,17 +70,11 @@ resources: | |||
| #- ../certmanager | |||
| # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'. | |||
| #- ../prometheus | |||
| # [METRICS] To enable the controller manager metrics service, uncomment the following line. | |||
| #- metrics_service.yaml | |||
| # [METRICS] Expose the controller manager metrics service. | |||
There was a problem hiding this comment.
Nit:
We can probably remind users of role binding for scraping, such as:
# To scrape metrics e.g. via Prometheus the client needs the ClusterRole `metrics-reader`
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, Kavinjsir 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 |
k8s-ci-robot
added
the
approved
|
Hi @joelanford AS we spoke I am closing this one in favor of #4003 Again, thank you a lot for your contribution and amazing help.!!! |
No description provided.