Added rules to ClusterRole

[rbaturov]

We need to modify the ClusterRole to grant the required permissions for the manager pod to access the /metrics endpoint. This is essential for e2e testing, as we will run curl commands from within the manager container to interact with the endpoint.

This is needed to support:
https://issues.redhat.com/browse/CNF-10142

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rbaturov
Once this PR has been reviewed and has the lgtm label, please assign ffromani for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Tal-or]

I don't see that we're consuming the auth_proxy_role.yaml

[titzhak@fedora numaresources-operator]$ grep -R auth_proxy_role.yaml
config/rbac/kustomization.yaml:#- auth_proxy_role.yaml

Anyway, if this file is supposed to be consumed only on testing environment I would make sure it deployed only there and not on production.

[Tal-or]

Not needed. you may commit without this file

[rbaturov]

I don't see that we're consuming the auth_proxy_role.yaml

[titzhak@fedora numaresources-operator]$ grep -R auth_proxy_role.yaml
config/rbac/kustomization.yaml:#- auth_proxy_role.yaml

Anyway, if this file is supposed to be consumed only on testing environment I would make sure it deployed only there and not on production.

Thanks for the fast reply.
I decided to add this to the default cluster role that is used for the operator, rather than the proxy ClusterRole as this is a more correct approach.

[Tal-or]

I don't see that we're consuming the auth_proxy_role.yaml

[titzhak@fedora numaresources-operator]$ grep -R auth_proxy_role.yaml
config/rbac/kustomization.yaml:#- auth_proxy_role.yaml

Anyway, if this file is supposed to be consumed only on testing environment I would make sure it deployed only there and not on production.

Thanks for the fast reply. I decided to add this to the default cluster role that is used for the operator, rather than the proxy ClusterRole as this is a more correct approach.

But you said it's being used only for e2e tests isn't it?

[rbaturov]

I don't see that we're consuming the auth_proxy_role.yaml

[titzhak@fedora numaresources-operator]$ grep -R auth_proxy_role.yaml
config/rbac/kustomization.yaml:#- auth_proxy_role.yaml

Anyway, if this file is supposed to be consumed only on testing environment I would make sure it deployed only there and not on production.

Thanks for the fast reply. I decided to add this to the default cluster role that is used for the operator, rather than the proxy ClusterRole as this is a more correct approach.

But you said it's being used only for e2e tests isn't it?

Yes you are right. now that I think of this, @ffromani what do you guys think if I'll just use the ClusterRole auth_proxy_client_clusterrole which does the same and add a clusterorlebinding for this under kind-ci and close this PR?

[Tal-or]

This or you can add the specific content you need using MergePatch:
https://stackoverflow.com/questions/71496560/kustomize-using-strategic-merge-patch-on-helmreleases

[rbaturov]

This or you can add the specific content you need using MergePatch: https://stackoverflow.com/questions/71496560/kustomize-using-strategic-merge-patch-on-helmreleases
Sounds good to me.
@ffromani WDYT about this idea?

[ffromani]

@rbaturov this is the only missing RBAC changeset to enable the testing per our offline discussion?

[rbaturov]

@rbaturov this is the only missing RBAC changeset to enable the testing per our offline discussion?

Yes, this is the only change needed to test the operator metrics endpoint for (manager container).
For RTE metrics endpoint, (which is a separate discussion) we need to think of a different resolution to allow the worker pod to access its endpoint. here are two ideas I have:

1. Add and apply a clusterrole and clusterorlebinding solely for accessing endpoint in the metrics test iteself
2. Alternatively, apply both (clusterrole and clusterorlebinding) by default in pkg/metrics/manifests/yaml

[ffromani]

@rbaturov this is the only missing RBAC changeset to enable the testing per our offline discussion?

Yes, this is the only change needed to test the operator metrics endpoint for (manager container). For RTE metrics endpoint, (which is a separate discussion) we need to think of a different resolution to allow the worker pod to access its endpoint. here are two ideas I have:

1. Add and apply a `clusterrole` and `clusterorlebinding` solely for accessing endpoint  in the metrics test iteself

2. Alternatively, apply both (`clusterrole` and `clusterorlebinding`) by default in `pkg/metrics/manifests/yaml`

ok then this is good to go in main branch for everyone, not just for CI/testing. If upgrade is no bother (as I expect, honestly) than this change can also be backported.

[rbaturov]

@rbaturov this is the only missing RBAC changeset to enable the testing per our offline discussion?

Yes, this is the only change needed to test the operator metrics endpoint for (manager container). For RTE metrics endpoint, (which is a separate discussion) we need to think of a different resolution to allow the worker pod to access its endpoint. here are two ideas I have:

1. Add and apply a `clusterrole` and `clusterorlebinding` solely for accessing endpoint  in the metrics test iteself

2. Alternatively, apply both (`clusterrole` and `clusterorlebinding`) by default in `pkg/metrics/manifests/yaml`

ok then this is good to go in main branch for everyone, not just for CI/testing. If upgrade is no bother (as I expect, honestly) than this change can also be backported.

So can we merge this?

[rbaturov]

/hold
If we already decide to enable this permission by default to the main branch, I think a better approach would be to add this to the auth_proxy_role.yaml instead. Since we consume the auth_proxy_role.yaml also on RTE (lin #1035), this means that also the RTE worker pods would have same permission, thus preparing infrastructure to run these tests without any additional adjustments.

[openshift-merge-robot]

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.

[openshift-ci]

@rbaturov: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-e2e-install-hypershift	d078332	link	true	/test ci-e2e-install-hypershift

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-sigs/prow repository. I understand the commands that are listed here.