Issue 3512: To limit kubevirt-controller service ccount from modifying CRs

[yuhaohaoyu]

What this PR does / why we need it:

The purpose of the PR is to address the problem discussed in issue 3512, i.e. to prevent the service account kubevirt-controller from modifying or deleting custom resources in the API Group kubevirt.io.

One reason that the problem existed was that the the RBAC role associated with the service account kubevirt-controller can do anything to all the resources in the API Group kubevirt.io.

The solution follows the discussion in the issue: to refactor the RBAC rule associated to the kubevirt-controller role by spelling out action verbs to each of the 6 resources in the API group. In this PR, the permission associated with the RBAC role kubevirt-controller are summarized as below:

Resource API	API Group	Verbs
vm	kubevirt.io	get, list, watch, patch
vmi	kubevirt.io	get, list, watch, patch
vmim	kubevirt.io	create, get, list, watch, patch
vmpreset	kubevirt.io	list, watch
kubevirt	kubevirt.io	get, list, watch

The PR includes an addition of 6 tests to tests/access_test.go that essentially runs the command
kubectl auth can-i delete vm --as system:serviceaccount:kubevirt:kubevirt-controller
for the 8 verbs against the 6 resource objects.

The PR contains a minor code cleaning up that replaced some recurring string literals by constants in pkg/virt-operator/creation/rbac/operator.go to align to the practice of other files under the same directory, e.g. pkg/virt-operator/creation/rbac/handler.go.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3512

Special notes for your reviewer:
This is a joint effort with @JinjunXiong and authors' first PR to this project and detailed review feedback from all aspects of the PR are highly appreciated.

Release note:

NONE

[openshift-ci-robot]

Hi @yuhaohaoyu. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[kubevirt-bot]

Hi @yuhaohaoyu. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[yuhaohaoyu]

/assign @rmohr

Again, our first PR, we are simply following the instruction from an email sent by the kubevirt-bot.

I noticed that the 'continuous-integration/travis-ci/pr' could not complete. The associated details show unknown reason for the termination of the building process, with no linkage to the PR. I will wait for further notice.

[xpivarc]

/assign @rmohr

Again, our first PR, we are simply following the instruction from an email sent by the kubevirt-bot.

I noticed that the 'continuous-integration/travis-ci/pr' could not complete. The associated details show unknown reason for the termination of the building process, with no linkage to the PR. I will wait for further notice.

Hi, we disabled builds with Travis. It should go away if you rebase on master.

[yuhaohaoyu]

(author keeping track of the git ops. welcome comment so to get the concise work flow):

• CI test failed to build without linkage to the PR submission
• (author) received email from xpivarc (notifications@github.com) with suggestion to do : "it should go away if you rebase on master"
• (author) did these git operations on the private dev machine (rhel-8.2) (there were no further change since the last push that directly corresponded to the PR submission).

> git branch
* master
> git remote -v
origin  https://github.com/yuhaohaoyu/kubevirt.git (fetch)
origin  https://github.com/yuhaohaoyu/kubevirt.git (push)
upstream        https://github.com/kubevirt/kubevirt.git (fetch)
upstream        https://github.com/kubevirt/kubevirt.git (push)

> git fetch --all
> git merge --no-commit upstream/master
> git commit --signoff
> git rebase upstream/master
> git push -f

Then github automatically started the CI again.

[yuhaohaoyu]

/assign @JinjunXiong

[danielBelenky]

/retest

[danielBelenky]

Overall looks good! Thanks for this PR.
Added a comment and a suggestion.

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign danielbelenky
You can assign the PR to them by writing /assign @danielbelenky in a comment when ready.

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

[yuhaohaoyu]

Made following changes to the code to address review comments from @danielBelenky

• Removed the [test_id:xxx] part in the test messages
• Reorganized the code in the table describe block for clarity (suggested by review comments)

[yuhaohaoyu]

/assign @danielBelenky

[yuhaohaoyu]

The latest update in the pkg/virt-operator/creation/rbac/controller.go was to specify 2 rules (A and B) as below. The only change (compared with the previous commit) was adding the verb delete into the Rule A. This change reduced the failed functests from 22 to 2 (to be verified after the CI test). However this change made Rule A a superset of Rule B. I am keeping Rule B in the code to reflect the attempted rule extension. The reason that I could not use Rule B only was discussed with @xpivarc earlier in this PR.

Rule A

                        {
                                APIGroups: []string{
                                        "kubevirt.io",
                                },
                                Resources: []string{
                                        "*",
                                },
                                Verbs: []string{
                                        "get", "list", "watch", "patch", "update", "create", "delete",
                                },
                        },

Rule B

                        {
                                APIGroups: []string{
                                        "kubevirt.io",
                                },
                                Resources: []string{
                                        "kubevirts",
                                        "virtualmachineinstancemigrations",
                                        "virtualmachineinstancepresets",
                                        "virtualmachineinstancereplicasets",
                                        "virtualmachineinstances",
                                        "virtualmachines",
                                },
                                Verbs: []string{
                                        "get", "list", "watch", "patch", "update", "create", "delete",
                                },
                        },

With the change of adding the verb delete to Rule A, following previously failed functests got to pass in my dev environment. The test_id of these functests are:

[test_id:1524]
[test_id:1525]
[test_id:1526]
[test_id:1527]
[test_id:1530]
[test_id:2035]
[test_id:2186]
[test_id:2187]
[test_id:2189]
[test_id:3163]
[test_id:3164]
[test_id:3176]
[test_id:4645]
[test_id:3085]
[test_id:3226]
[test_id:3466]

[yuhaohaoyu]

To fix the last two functional tests: 3145, 4642, I had to eventually revert the rule to: resources: "*", verbs: "*", which was the case before this PR. I choose to leave restrictive rule in place for future reference. Below is a test I run against the two functional tests:

Rule 1 Resources	Rule 1 Verbs	Rule 2 Resources	Rule 2 Verbs	Test
*	*	6 subjects	7 verbs	Pass
*	8 verbs	6 subjects	7 verbs	Fail
*	8 verbs	6 subjects	*	Fail
*	8 verbs	6 subjects	8 verbs	Fail

The exercise would also suggest:

1. In term of specification of the verbs in a RBAC rule, "*" covers more than the 8 verbs {"get", "list", "watch", "patch", "update", "create", "delete", "deleteconnection"}. Is there any other verb?
2. In term of specification of resource subjects in the RBAC rule in the kubevirt.io API group, "*" appears to be a superset of {vm, vmi, vmrs, vmpreset, vmim, kv}.

[yuhaohaoyu]

As a summary, the effort to refactor the RBAC rule of the kubevirt-controller ClusterRole with regard to the access of the resources in the kubevirt\.io API group did not find a more restrictive substitute. However, through the functional testing process (CI) in the scope of this PR, I learnt a few loose relationship of the specification of the rule(s) and some functional tests. They are explained as below:

Before the PR, all CI tests pass:

Rules tmp-name	API Group	Subject	Verbs
orig	kubevirt.io	*	*

Refactoring that allow cluster-deploy script to bring up the kv, but fails 22 functional tests in the 3 e2e CI tests. The failed tests were in vm_test, vm_watch_test, subresource_api_test, expose_test, pausing_test.

Rules tmp-name	API Group	Subject	Verbs
rule1-A	kubevirt.io	*	get, list, watch, patch, update, create
rule1-B	kubevirt.io	kv, vm, vmi, vmirs, vmipreset, vmim	get, list, watch, patch, update, create, delete

Note there are not many room for tweaking the specification between the orig rule and rule1 rule. To log the tweaking briefly, we used these loosely defined terms,

Term	Stands for
6-verbs	a specification of verbs that including get, watch, list, patch, update, create
7-verbs	a specification of verbs that including get, watch, list, patch, update, create, delete
8-verbs	a specification of verbs that including get, watch, list, patch, update, create, deletecollection
6-subjects	a specification of rule subjects that including kv, vm, vmi, vmirs, vmipreset, vmim

With these terms the rule tweaking is logged as below

Id	rule1-A subj.	rule1-A verb	rule1-B subs.	rule1-B verb	Failed tests
1	*	6-verb	6-subj	7-verb	22 fails (vm_test, etc.)
2	*	7-verb	6-subj	7-verb	2 fails (id: 3145, 4642)
3	*	8-verb	6-subj	8-verb	2 fails
4	*	8-verb	6-subj	*	2 fails
5	*	*	6-subj	7-verb	0 fail

The main conclusion from this table is that in order to keep the current CI tests, the rule has to use * for both the resources and verbs fields in the code.

The secondary findings (not explained) are:

1. In term of specification of the verbs in a RBAC rule, "*" covers more than the 8 verbs {"get", "list", "watch", "patch", "update", "create", "delete", "deleteconnection"}. Is there any other verb we have declared?
2. In term of specification of resource subjects in the RBAC rule in the kubevirt.io API group, "*" appears to be a superset of {vm, vmi, vmrs, vmpreset, vmim, kv}.

Following the minimalism practice, there is no reason to merge this PR. The code changes are:

• pkg/virt-operator/creationg/rbac/controller.go : to leave trace of this attempt.
• pkg/virt-operator/creationg/rbac/operator.go : cleanup, no functionality change.
• tests/acctest_test.go , tests/acctest.go: a fast test targeted the rule in discussion.

[kubevirt-bot]

@yuhaohaoyu: 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/test-infra repository.

[kubevirt-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[kubevirt-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

[kubevirt-bot]

@yuhaohaoyu: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubevirt-e2e-kind-1.17-sriov	b70b576	link	/test pull-kubevirt-e2e-kind-1.17-sriov
pull-kubevirt-e2e-k8s-1.20-sig-network	b70b576	link	/test pull-kubevirt-e2e-k8s-1.20-sig-network

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.

[kubevirt-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

[kubevirt-bot]

@kubevirt-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

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.