Move rbacs for config reader to a separate handler #1441

erkanerol · 2021-07-14T09:51:16Z

Signed-off-by: Erkan Erol eerol@redhat.com

With this PR, hco operator w,ill start reconciling hco.kubevirt.io:config-reader Role&RoleBinding strictly like other operands. After this PR, changes on these operands will be overwritten immediately without requiring any change on CDI or HCO CRs.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1963963

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Release note:

NONE

coveralls · 2021-07-14T09:55:00Z

Pull Request Test Coverage Report for Build 1033551036

113 of 129 (87.6%) changed or added relevant lines in 8 files are covered.
12 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.01%) to 70.102%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller/hyperconverged/hyperconverged_controller.go	0	2	0.0%
pkg/controller/operands/cdi.go	104	118	88.14%

Files with Coverage Reduction	New Missed Lines	%
pkg/controller/operands/cdi.go	1	90.6%
pkg/controller/operands/kubevirt.go	1	93.06%
pkg/controller/operands/vmImport.go	1	88.73%
pkg/controller/operands/cliDownload.go	2	89.71%
pkg/controller/operands/dashboard.go	2	78.65%
pkg/controller/operands/quickStart.go	2	82.41%
pkg/controller/operands/monitoring.go	3	86.12%

Totals
Change from base Build 1030817711:	0.01%
Covered Lines:	3088
Relevant Lines:	4405

💛 - Coveralls

hco-bot · 2021-07-14T11:26:58Z

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

kubevirt-bot · 2021-07-14T11:27:02Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp

In response to this:

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

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.

hco-bot · 2021-07-14T13:05:41Z

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

kubevirt-bot · 2021-07-14T13:05:44Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-gcp

In response to this:

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

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.

nunnatsa · 2021-07-15T05:09:23Z

We need to check if we can entirely take it out of the code, letting OLM manage it. The main problem with creating rbacs from the code is that HCO must have permissions for that, and we need to avoid such permmissions. Can we move it to the CSV?

erkanerol · 2021-07-15T06:56:29Z

We need to check if we can entirely take it out of the code, letting OLM manage it. The main problem with creating rbacs from the code is that HCO must have permissions for that, and we need to avoid such permmissions. Can we move it to the CSV?

@nunnatsa My understanding from the discussions in the PR below, it is not possible to create Role&RoleBinding with CSV
#498

nunnatsa · 2021-07-15T08:30:02Z

pkg/controller/operands/cdi.go

-
-	return nil
-}
+func (h *cdiHooks) postFound(req *common.HcoRequest, exists runtime.Object) error { return nil }


This is the one and only real implementation of the postFound function. Please remove it from the interface and delete all the empty implementations (and the function call too).

tiraboschi · 2021-07-15T09:02:18Z

We need to check if we can entirely take it out of the code, letting OLM manage it. The main problem with creating rbacs from the code is that HCO must have permissions for that, and we need to avoid such permmissions. Can we move it to the CSV?

@nunnatsa My understanding from the discussions in the PR below, it is not possible to create Role&RoleBinding with CSV
#498

If I correctly recall the specific problem was that a serviceAccountName was mandatory in the CSV while the console is impersonating the connected user while reading that config map.
Can you please double check if this is still relevant on CSV side?

nunnatsa · 2021-07-15T09:08:18Z

Can you please double check if this is still relevant on CSV side?

As far as I can see, this is still the case.

nunnatsa

Please see one comment about postFound

Signed-off-by: Erkan Erol <eerol@redhat.com>

It was necessary for configreader role&rolebinding in cdi hooks but we moved them to separate handlers now. We don't need this function anymore. Signed-off-by: Erkan Erol <eerol@redhat.com>

sonarcloud · 2021-07-15T10:25:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

nunnatsa

/approve

kubevirt-bot · 2021-07-15T11:04:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nunnatsa

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:

~~OWNERS~~ [nunnatsa]

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

hco-bot · 2021-07-15T12:03:16Z

hco-e2e-image-index-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-gcp

kubevirt-bot · 2021-07-15T12:03:23Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp

In response to this:

hco-e2e-image-index-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-gcp

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.

erkanerol · 2021-07-15T12:14:53Z

/retest

openshift-ci · 2021-07-15T13:31:33Z

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

Test name	Commit	Details	Rerun command
ci/prow/hco-e2e-image-index-gcp	`3cc9a24`	link	`/test hco-e2e-image-index-gcp`
ci/prow/hco-e2e-upgrade-index-aws	`3cc9a24`	link	`/test hco-e2e-upgrade-index-aws`
ci/prow/hco-e2e-kv-smoke-gcp	`3cc9a24`	link	`/test hco-e2e-kv-smoke-gcp`

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.

hco-bot · 2021-07-15T13:36:42Z

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

kubevirt-bot · 2021-07-15T13:36:59Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-gcp

In response to this:

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

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.

hco-bot · 2021-07-15T13:50:58Z

hco-e2e-upgrade-index-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-aws

kubevirt-bot · 2021-07-15T13:51:02Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-aws

In response to this:

hco-e2e-upgrade-index-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-aws

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.

tiraboschi · 2021-07-15T17:00:36Z

/retest

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 14, 2021

kubevirt-bot requested review from sradco and tiraboschi July 14, 2021 09:51

erkanerol changed the title ~~Move rbacs for config reader to separate handler~~ WIP: Move rbacs for config reader to separate handler Jul 14, 2021

erkanerol force-pushed the config-reader-controller branch from ffbac94 to 6c1e367 Compare July 14, 2021 09:57

erkanerol changed the title ~~WIP: Move rbacs for config reader to separate handler~~ WIP: Move rbacs for config reader to a separate handler Jul 14, 2021

erkanerol changed the title ~~WIP: Move rbacs for config reader to a separate handler~~ Move rbacs for config reader to a separate handler Jul 14, 2021

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2021

nunnatsa reviewed Jul 15, 2021

View reviewed changes

nunnatsa requested changes Jul 15, 2021

View reviewed changes

kubevirt-bot assigned nunnatsa Jul 15, 2021

Erkan Erol added 2 commits July 15, 2021 13:18

Move rbacs for config reader to separate handler

0d09559

Signed-off-by: Erkan Erol <eerol@redhat.com>

Remove postFound function from hcoResourceHooks

3cc9a24

It was necessary for configreader role&rolebinding in cdi hooks but we moved them to separate handlers now. We don't need this function anymore. Signed-off-by: Erkan Erol <eerol@redhat.com>

erkanerol force-pushed the config-reader-controller branch from 6c1e367 to 3cc9a24 Compare July 15, 2021 10:24

nunnatsa approved these changes Jul 15, 2021

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2021

kubevirt-bot merged commit 308b57f into kubevirt:main Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move rbacs for config reader to a separate handler #1441

Move rbacs for config reader to a separate handler #1441

erkanerol commented Jul 14, 2021 •

edited

coveralls commented Jul 14, 2021 •

edited

hco-bot commented Jul 14, 2021

kubevirt-bot commented Jul 14, 2021

hco-bot commented Jul 14, 2021

kubevirt-bot commented Jul 14, 2021

nunnatsa commented Jul 15, 2021 •

edited

erkanerol commented Jul 15, 2021

nunnatsa Jul 15, 2021 •

edited

erkanerol Jul 15, 2021

tiraboschi commented Jul 15, 2021

nunnatsa commented Jul 15, 2021

nunnatsa left a comment

sonarcloud bot commented Jul 15, 2021

nunnatsa left a comment

kubevirt-bot commented Jul 15, 2021

hco-bot commented Jul 15, 2021

kubevirt-bot commented Jul 15, 2021

erkanerol commented Jul 15, 2021

openshift-ci bot commented Jul 15, 2021 •

edited

hco-bot commented Jul 15, 2021

kubevirt-bot commented Jul 15, 2021

hco-bot commented Jul 15, 2021

kubevirt-bot commented Jul 15, 2021

tiraboschi commented Jul 15, 2021

Move rbacs for config reader to a separate handler #1441

Move rbacs for config reader to a separate handler #1441

Conversation

erkanerol commented Jul 14, 2021 • edited

coveralls commented Jul 14, 2021 • edited

Pull Request Test Coverage Report for Build 1033551036

💛 - Coveralls

hco-bot commented Jul 14, 2021

kubevirt-bot commented Jul 14, 2021

hco-bot commented Jul 14, 2021

kubevirt-bot commented Jul 14, 2021

nunnatsa commented Jul 15, 2021 • edited

erkanerol commented Jul 15, 2021

nunnatsa Jul 15, 2021 • edited

Choose a reason for hiding this comment

erkanerol Jul 15, 2021

Choose a reason for hiding this comment

tiraboschi commented Jul 15, 2021

nunnatsa commented Jul 15, 2021

nunnatsa left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 15, 2021

nunnatsa left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jul 15, 2021

hco-bot commented Jul 15, 2021

kubevirt-bot commented Jul 15, 2021

erkanerol commented Jul 15, 2021

openshift-ci bot commented Jul 15, 2021 • edited

hco-bot commented Jul 15, 2021

kubevirt-bot commented Jul 15, 2021

hco-bot commented Jul 15, 2021

kubevirt-bot commented Jul 15, 2021

tiraboschi commented Jul 15, 2021

erkanerol commented Jul 14, 2021 •

edited

coveralls commented Jul 14, 2021 •

edited

nunnatsa commented Jul 15, 2021 •

edited

nunnatsa Jul 15, 2021 •

edited

openshift-ci bot commented Jul 15, 2021 •

edited