✨ Add RBAC files for metrics authentication and authorization #2116
Conversation
metal3-io-bot
added
the
size/M
|
@camilamacedo86 I could identify these RBACs. Please check if this is sufficient. Thanks for your feedback so far. |
| @@ -9,3 +9,12 @@ resources: | |||
| - role_binding.yaml | |||
| - leader_election_role.yaml | |||
| - leader_election_role_binding.yaml | |||
There was a problem hiding this comment.
It seems accurate for me, but to ensure that the metrics is working as should be you might want use the same tests which are now scaffolded by kubebuilder, see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/test/e2e/e2e_test.go#L166-L235
There was a problem hiding this comment.
it seems adapting the e2e would not only require the e2e tests but also the prometheus operator and such. Planning to do it in a followup. @lentzi90 wdyt?
There was a problem hiding this comment.
You do not need prometheus operator.
You can remove this part and do not apply the ServiceMonitor
You can check in kubernetes-sigs/kubebuilder#3907 Steps to Verify Metrics with curl that is what you need. Just get the token and the steps that does
kubectl create clusterrolebinding <project-name>-metrics-binding \
--clusterrole=<project-name>-metrics-reader \
--serviceaccount=<project-name>-system:<project-name>-controller-manager
export TOKEN=$(kubectl create token operator-controller-controller-manager -n olmv1-system)
echo $TOKEN
kubectl run curl-metrics --rm -it --restart=Never \
--image=curlimages/curl:7.87.0 -n <project>-system -- /bin/sh
Call the metrics enpoint using the TOKEN and check the HTTP OK
curl -v -k -H "Authorization: Bearer $TOKEN" https://<my-project-name>-controller-manager-metrics-service.<my-project-name>-system.svc.cluster.local:8443/metrics
There was a problem hiding this comment.
That sounds good! @kashifest can you try it?
There was a problem hiding this comment.
So, from the example above https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/test/e2e/e2e_test.go#L166-L235
You can mainly do the tests by copying it and:
- Just remove https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/test/e2e/e2e_test.go#L180C1-L183C68
- Ensure that instead of project-v4 it uses the name of your project
There was a problem hiding this comment.
Honestly I have no clue now why the test is failing, heres a screenshot of my local test where everything is happy
There was a problem hiding this comment.
It is missing the command. You can test it locally with the fixture test bu doing make test-e2e. It is fast and simple, no extra config needed.
There was a problem hiding this comment.
command was there and it was failing even then, I removed it for testing
There was a problem hiding this comment.
make test-e2e doesnt work as is, you need to build images also manually before it?
There was a problem hiding this comment.
Right, sorry. You need to do
IMG=quay.io/metal3-io/baremetal-operator:e2e make docker
make test-e2e
kashifest
force-pushed
the
kashif/add-missing-rbac-metrics
branch
6 times, most recently
from
5e84514 to
c0c2f01
Compare
metal3-io#2102 has introduced controller-runtime's WithAuthenticationAndAuthorization filter which also requires extra RBAC roles and role bindings for metrics authentication and authorization. This PR adds those. Signed-off-by: Kashif Khan <[email protected]>
kashifest
force-pushed
the
kashif/add-missing-rbac-metrics
branch
5 times, most recently
from
e7b25b8 to
c1e6906
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@camilamacedo86: adding LGTM is restricted to approvers and reviewers in OWNERS files. In 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86 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 |
kashifest
force-pushed
the
kashif/add-missing-rbac-metrics
branch
from
c1e6906 to
c693110
Compare
Signed-off-by: Kashif Khan <[email protected]>
kashifest
force-pushed
the
kashif/add-missing-rbac-metrics
branch
3 times, most recently
from
b1e40a7 to
703156f
Compare
Signed-off-by: Kashif Khan <[email protected]>
kashifest
force-pushed
the
kashif/add-missing-rbac-metrics
branch
from
703156f to
763acbb
Compare
#2102 has introduced controller-runtime's
WithAuthenticationAndAuthorizationfilter which also requires extra RBAC roles and role bindings for metrics authentication and authorization. This PR adds those.