Grant authenticated users access to read kubevirt-storage-class-defaults #498

maya-r · 2020-03-15T18:31:02Z

Grant authenticated users access to read kubevirt-storage-class-defaults

aglitke

lgtm

deploy/cluster_role.yaml

mhenriks · 2020-03-18T19:56:38Z

deploy/cluster_role.yaml

+kind: Role
+metadata:
+  name: hco.kubevirt.io:config-reader
+  namespace: kubevirt-hyperconverged


not exactly sure the context in which this manifest is used but the other roles do not specify the namespace. So maybe also shouldn't here?

I want to give access to the configmap with namespace kubevirt-hyperconverged and name kubevirt-storage-class-defaults.
if I don't restrict the namespace then I'm granting access to an identically named configmap in anu namespace (if someone happens to use that exact name. unlikely, but I shouldn't do it)

djzager · 2020-03-18T20:04:28Z

In order to make sure that it gets included in all of the places it should (ie. downstream), we need to make it so that ./hack/build-manifests.sh properly creates this Role and RoleBinding.

However, even with that change, I don't think the Role will be installed when the HCO is installed via OLM. How exactly is this Role supposed to 1) make it onto a cluster 2) be used?

djzager · 2020-03-18T20:04:59Z

Is this something that the HCO should be creating in it's Reconcile()?

maya-r · 2020-03-24T15:53:11Z

Here's a version creating everything via reconcile. I added a functional test too.
I'm under the impression that we use Reconcile to bridge the gap between upstream vs. downstream namespace.
I might be able to live with a ClusterRoleBinding instead of a RoleBinding, because it does not need to be namespaced. But I don't know where to put it.

maya-r · 2020-04-05T14:01:58Z

Rebased to include the latest (unrelated, but fixing the tests issue) commits.

maya-r · 2020-04-13T11:45:21Z

ping

tiraboschi · 2020-04-14T12:23:43Z

pkg/controller/hyperconverged/hyperconverged_controller.go

+				APIGroups: []string{""},
+				Resources: []string{"configmaps"},
+				ResourceNames: []string{"kubevirt-storage-class-defaults"},
+				Verbs: []string{"get", "watch", "list"},


can you please run go fmt pkg/controller/hyperconverged/hyperconverged_controller.go and commit back the result?

tiraboschi · 2020-04-14T12:36:01Z

pkg/controller/hyperconverged/hyperconverged_controller.go

@@ -292,6 +293,8 @@ func (r *ReconcileHyperConverged) Reconcile(request reconcile.Request) (reconcil
 	for _, f := range []func(*hcov1alpha1.HyperConverged, logr.Logger, reconcile.Request) error{
 		r.ensureKubeVirtConfig,
 		r.ensureKubeVirtStorageConfig,
+		r.ensureKubeVirtStorageRole,
+		r.ensureKubeVirtStorageRoleBinding,


Why have we to do it at runtime via HCO?
Isn't OLM not enough for this?

I assume it can. I wasn't sure about how I'm expected to do something like this.
HCO seems to work with kubernetes too, whereas OLM is apparently specific to OpenShift.
Are we expected to add to the OLM files and have the kubernetes bits be generated?

Yes, something like that:
https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/deploy/cluster_role_binding.yaml and https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/deploy/cluster_role.yaml are used only on plain k8s deployment.

https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/deploy/olm-catalog/kubevirt-hyperconverged/1.1.0/kubevirt-hyperconverged-operator.v1.1.0.clusterserviceversion.yaml is instead the CSV for OLM based deployment.

both the set of files are generated running hack/build-manifests.sh according to what's provided by the components operators.
In order to consume a different component you have to patch go.mod and run go mod vendor and hack/config

In this case HCO is adding the ConfigMap, so it feels like the role should be something HCO adds. Is there somewhere that isn't auto-generated for appending to HCO-specific additions?

kubevirt-bot · 2020-06-07T15:05:21Z

@maya-r: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-hyperconverged-cluster-operator-e2e-k8s-1.17.0	030753652d3fe127ee8a7f5903681d76ca2aa830	link	`/test pull-hyperconverged-cluster-operator-e2e-k8s-1.17.0`

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.

ovirt-infra · 2020-06-07T19:42:16Z

All tests passed

maya-r · 2020-06-09T09:51:16Z

This looked like the right thing, I edited pkg/components/components.go and build-manifests.sh regenerated deploy/*.yaml, but the OLM CSV doesn't look like it picked up any changes.

tiraboschi · 2020-06-09T10:09:29Z

This looked like the right thing, I edited pkg/components/components.go and build-manifests.sh regenerated deploy/*.yaml, but the OLM CSV doesn't look like it picked up any changes.

You can eventually patch GetInstallStrategyBase functions.
But should this be part of what HCO is going to receive calling the csv-generator tool on CDI operator?

maya-r · 2020-06-09T10:18:25Z

Right now the configmap is generated by HCO in https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/pkg/controller/hyperconverged/hyperconverged_controller.go#L1502-L1524

I don't know why it's here (or why anything that it does is useful), but the web UI looks at this information

maya-r · 2020-06-16T06:31:56Z

So, I'm running into some trouble: OLM only lets me specify a serviceAccountName in a clusterPermissions.
The intention was to do this for the web UI, so maybe I should figure out what serviceAccountName is the web UI instead of trying to allow everyone access to this configMap?

ovirt-infra · 2020-06-17T06:42:56Z

All tests passed

maya-r · 2020-06-17T06:51:25Z

I went back to the version adding the Role and RoleBinding via the reconcile loop, because I don't think it's possible to add them via OLM.

Another option worth considering is granting access to the openshift-console serviceAccountName, that might have the expected effect too.

@orenc1, could you have a look at this pull request? I'm under the impression that @tiraboschi is away on holiday.

orenc1 · 2020-06-17T12:14:51Z

I went back to the version adding the Role and RoleBinding via the reconcile loop, because I don't think it's possible to add them via OLM.

Another option worth considering is granting access to the openshift-console serviceAccountName, that might have the expected effect too.

@orenc1, could you have a look at this pull request? I'm under the impression that @tiraboschi is away on holiday.

Hi @maya-r , we are using the OLM to set the required permissions for the service accounts of HCO's dependent operators. If the access for kubevirt-storage-class-defaults config map is only required by the console (and not a direct access by the user), I think it would be more "granular" to provide this Role&RoleBinding to the console ServiceAccount. That could be configured by OLM by specifying it in the CSV, instead of having HCO manage these permissions in runtime.

nunnatsa · 2020-06-17T13:05:33Z

pkg/controller/hyperconverged/hyperconverged_controller.go

@@ -469,6 +470,8 @@ func (r *ReconcileHyperConverged) ensureHco(req *hcoRequest) error {
 		r.ensureKubeVirtPriorityClass,
 		r.ensureKubeVirtConfig,
 		r.ensureKubeVirtStorageConfig,
+		r.ensureKubeVirtStorageRole,


In addition to the general comment about not doing it from reconcile - if there is no other choice, I would call these two methods from within ensureCDI and not as part of this loop. This is something that is not monitors by HCO as other resources.

Thanks for the review.
My original motivation to write the code this way is that ensureKubeVirtStorageConfig is currently implemented as its own reconcile function.
If the intention in rotating the code is to "don't test if the rolebinding exists in every reconcile iteration", I don't think that this is a good idea.
It would violate our rule to write reconcile such that we reach eventual consistency. This might negatively affect updates, as one example.
If it's just to make things tidy, I should probably combine it with ensureKubeVirtStorageConfig too.

orenc1 · 2020-06-17T13:16:55Z

Hi @maya-r , we are using the OLM to set the required permissions for the service accounts of HCO's dependent operators. If the access for kubevirt-storage-class-defaults config map is only required by the console (and not a direct access by the user), I think it would be more "granular" to provide this Role&RoleBinding to the console ServiceAccount. That could be configured by OLM by specifying it in the CSV, instead of having HCO manage these permissions in runtime.

I would recommend you to test by manually configuring the required Roles&RoleBindings of this configmap for the console ServiceAccount and see if it solves the issue. If so, add the relevant section in csv-generator of CDI so it will end up in HCO's CSV.

maya-r · 2020-06-18T15:03:10Z

I got a response from Tomas Jelinek that the web interface accesses objects in the cluster as the logged in user (I had some trouble experimenting in the web interface).
It isn't possible to add the rights to the serviceAccountName and get the same result.
So this version of the pull request that is adding it via reconcile is the best I can do, I can't achieve the same result using OLM.

orenc1 · 2020-06-18T15:34:41Z

OK.
I would suggest to nest r.ensureKubeVirtStorageConfig and r.ensureKubeVirtStorageRole executions under ensureCDI, as Nahshon said, because these resources are strictly related to Storage.
I would also wait for @tiraboschi who returns on Monday for another review.

tiraboschi · 2020-06-23T16:53:11Z

Yes, I also agree with @nunnatsa comment: all the methods called in the first loop in ensureHco method have a specific signature because the code there will use it to check the conditions reported by component operators and eventually the status of an upgrade.
We don't have anything like that for r.ensureKubeVirtStorageRole and r.ensureKubeVirtStorageRoleBinding so I agree that they should be called from r.ensureCDI so that you can use a simpler signature.
Probably, in a separate PR, we should do the same for r.ensureKubeVirtStorageConfig and other Config Maps created there.

orenc1 · 2020-06-30T11:49:35Z

Yes, I also agree with @nunnatsa comment: all the methods called in the first loop in ensureHco method have a specific signature because the code there will use it to check the conditions reported by component operators and eventually the status of an upgrade.
We don't have anything like that for r.ensureKubeVirtStorageRole and r.ensureKubeVirtStorageRoleBinding so I agree that they should be called from r.ensureCDI so that you can use a simpler signature.
Probably, in a separate PR, we should do the same for r.ensureKubeVirtStorageConfig and other Config Maps created there.

@maya-r ping?

ovirt-infra · 2020-06-30T21:16:10Z

All tests passed

- Add a role/rolebinding for this purpose, using reconcile. This is not done using OLM because OLM clusterPermissions only allows adding permissions to a serviceAccountName. - Add a functional test ensuring unprivileged users can read that configmap but not others. - Cargo cult add the role/rolebinding to the unit tests wherever we test all elements created by reconcile Signed-off-by: Maya Rashish <mrashish@redhat.com>

This means they can use a simpler function signature Signed-off-by: Maya Rashish <mrashish@redhat.com>

ovirt-infra · 2020-07-07T09:57:15Z

All tests passed

orenc1 · 2020-07-07T11:08:03Z

/approve
/lgtm

kubevirt-bot · 2020-07-07T11:08:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: orenc1

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~~ [orenc1]

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

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Mar 15, 2020

kubevirt-bot requested review from tiraboschi and yuvalturg March 15, 2020 18:31

aglitke reviewed Mar 18, 2020

View reviewed changes

mhenriks reviewed Mar 18, 2020

View reviewed changes

deploy/cluster_role.yaml Outdated Show resolved Hide resolved

mhenriks reviewed Mar 18, 2020

View reviewed changes

maya-r force-pushed the rbac branch from 5d762f3 to 661bb4b Compare March 24, 2020 15:51

kubevirt-bot added size/L and removed size/M labels Mar 24, 2020

This comment has been minimized.

Sign in to view

maya-r force-pushed the rbac branch from 661bb4b to acffec7 Compare March 24, 2020 17:44

tiraboschi force-pushed the rbac branch from acffec7 to b1d1fe5 Compare April 4, 2020 14:23

maya-r force-pushed the rbac branch from b1d1fe5 to 0307536 Compare April 5, 2020 14:00

mareklibra mentioned this pull request Apr 9, 2020

kubevirt: add sample Role to grant privileges for non-admin to run v2v openshift/console#4965

Merged

mareklibra mentioned this pull request Apr 14, 2020

kubevirt: Add Import VM to dev-console openshift/console#4941

Merged

5 tasks

tiraboschi requested changes Apr 14, 2020

View reviewed changes

kubevirt-bot assigned tiraboschi Apr 14, 2020

maya-r force-pushed the rbac branch 2 times, most recently from 02fd070 to 7365560 Compare June 7, 2020 19:40

maya-r force-pushed the rbac branch from 7365560 to 173bb92 Compare June 17, 2020 06:40

nunnatsa reviewed Jun 17, 2020

View reviewed changes

maya-r force-pushed the rbac branch from 173bb92 to 3e93cbe Compare June 30, 2020 21:13

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020

maya-r added 2 commits July 7, 2020 12:05

Call ensureKubeVirtStorage{Role,RoleBinding} from ensureCDI.

fa387cd

This means they can use a simpler function signature Signed-off-by: Maya Rashish <mrashish@redhat.com>

maya-r force-pushed the rbac branch from 3e93cbe to fa387cd Compare July 7, 2020 09:54

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020

kubevirt-bot assigned orenc1 Jul 7, 2020

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

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

kubevirt-bot merged commit 67d2eec into kubevirt:master Jul 7, 2020

maya-r mentioned this pull request Jul 7, 2020

Simplify ensureKubeVirtStorageConfig #690

Merged

nunnatsa mentioned this pull request Oct 1, 2020

[Release 1.2] Bump NETWORK_ADDONS to v0.42.2 #850

Closed

erkanerol mentioned this pull request Jul 15, 2021

Move rbacs for config reader to a separate handler #1441

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grant authenticated users access to read kubevirt-storage-class-defaults #498

Grant authenticated users access to read kubevirt-storage-class-defaults #498

maya-r commented Mar 15, 2020

aglitke left a comment

mhenriks Mar 18, 2020

maya-r Mar 19, 2020

djzager commented Mar 18, 2020

djzager commented Mar 18, 2020

maya-r commented Mar 24, 2020

This comment has been minimized.

maya-r commented Apr 5, 2020 •

edited

maya-r commented Apr 13, 2020

tiraboschi Apr 14, 2020

tiraboschi Apr 14, 2020

maya-r Apr 15, 2020

tiraboschi Apr 15, 2020

maya-r Apr 15, 2020

kubevirt-bot commented Jun 7, 2020 •

edited

ovirt-infra commented Jun 7, 2020

maya-r commented Jun 9, 2020 •

edited

tiraboschi commented Jun 9, 2020

maya-r commented Jun 9, 2020

maya-r commented Jun 16, 2020

ovirt-infra commented Jun 17, 2020

maya-r commented Jun 17, 2020

orenc1 commented Jun 17, 2020

nunnatsa Jun 17, 2020 •

edited

maya-r Jun 18, 2020

orenc1 commented Jun 17, 2020

maya-r commented Jun 18, 2020

orenc1 commented Jun 18, 2020 •

edited

tiraboschi commented Jun 23, 2020

orenc1 commented Jun 30, 2020

ovirt-infra commented Jun 30, 2020

ovirt-infra commented Jul 7, 2020

orenc1 commented Jul 7, 2020

kubevirt-bot commented Jul 7, 2020

Grant authenticated users access to read kubevirt-storage-class-defaults #498

Grant authenticated users access to read kubevirt-storage-class-defaults #498

Conversation

maya-r commented Mar 15, 2020

aglitke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager commented Mar 18, 2020

djzager commented Mar 18, 2020

maya-r commented Mar 24, 2020

This comment has been minimized.

maya-r commented Apr 5, 2020 • edited

maya-r commented Apr 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-bot commented Jun 7, 2020 • edited

ovirt-infra commented Jun 7, 2020

maya-r commented Jun 9, 2020 • edited

tiraboschi commented Jun 9, 2020

maya-r commented Jun 9, 2020

maya-r commented Jun 16, 2020

ovirt-infra commented Jun 17, 2020

maya-r commented Jun 17, 2020

orenc1 commented Jun 17, 2020

nunnatsa Jun 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenc1 commented Jun 17, 2020

maya-r commented Jun 18, 2020

orenc1 commented Jun 18, 2020 • edited

tiraboschi commented Jun 23, 2020

orenc1 commented Jun 30, 2020

ovirt-infra commented Jun 30, 2020

ovirt-infra commented Jul 7, 2020

orenc1 commented Jul 7, 2020

kubevirt-bot commented Jul 7, 2020

maya-r commented Apr 5, 2020 •

edited

kubevirt-bot commented Jun 7, 2020 •

edited

maya-r commented Jun 9, 2020 •

edited

nunnatsa Jun 17, 2020 •

edited

orenc1 commented Jun 18, 2020 •

edited