Account for discovered policies(kyverno, vapb etc) in aggregaterole #1907

yiraeChristineKim · 2025-01-09T18:07:00Z

Description

This will allow the discovered policies table to show kyverno policies(clusterPolicy, policy), gatekeeper mutations, validatingAdmissionPolicyBinding, policyreports for view users.

Related Issue

https://issues.redhat.com/browse/ACM-16116
https://issues.redhat.com/browse/ACM-16959

Changes Made

Add roles (gatekeeper mutation, kyverno policies, clusterpolicyReport)

Checklist

I have tested the changes locally and they are functioning as expected.
I have updated the documentation (if necessary) to reflect the changes.
I have added/updated relevant unit tests (if applicable).
I have ensured that my code follows the project's coding standards.
I have checked for any potential security issues and addressed them.
I have added necessary comments to the code, especially in complex or unclear sections.
I have rebased my branch on top of the latest main/master branch.

Additional Notes

Related: #1723

Reviewers

@JustinKuli @dhaiducek @gparvin

Definition of Done

openshift-ci · 2025-01-09T18:07:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

yiraeChristineKim · 2025-01-09T18:18:11Z

/cc @pshickeydev @joeg-pro

yiraeChristineKim · 2025-01-09T18:19:10Z

/cc @gparvin @JustinKuli @dhaiducek @manravi

JustinKuli · 2025-01-09T18:29:21Z

pkg/templates/multiclusterhub/base/clusterrole-admin-aggregate.yaml

+  resources: ["*"]
+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]
+- apiGroups: ["admissionregistration.k8s.io"]
+  resources: ["validatingadmissionpolicybindings"]


Only bindings? Or should these include just validatingadmissionpolicy?

We don't show validatingadmissionpolicy in discovered policy so

I added! Thanks validatingadmissionpolicy is connected to validatingadmissionpolicybindings so it is good to add this! Thanks!!

dhaiducek · 2025-01-09T18:38:05Z

pkg/templates/multiclusterhub/base/clusterrole-admin-aggregate.yaml

+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]
+- apiGroups: ["kyverno.io"]
+  resources: ["policies", "clusterpolicies"]
+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]


nit: Missing trailing newline

dhaiducek · 2025-01-09T18:38:10Z

pkg/templates/multiclusterhub/base/clusterrole-edit-aggregate.yaml

+  verbs: ["get", "list", "watch", "update", "patch"]
+- apiGroups: ["admissionregistration.k8s.io"]
+  resources: ["validatingadmissionpolicybindings"]
+  verbs: ["get", "list", "watch", "update", "patch"]


nit: Missing trailing newline

dhaiducek · 2025-01-09T18:38:15Z

pkg/templates/multiclusterhub/base/clusterrole-view-aggregate.yaml

+  verbs: ["get", "list", "watch"]
+- apiGroups: ["kyverno.io"]
+  resources: ["policies", "clusterpolicies"]
+  verbs: ["get", "list", "watch"]


nit: Missing trailing newline

dhaiducek · 2025-01-09T18:39:03Z

pkg/templates/multiclusterhub/base/clusterrole-edit-aggregate.yaml

+  verbs: ["get", "list", "watch", "update", "patch"]
+- apiGroups: ["admissionregistration.k8s.io"]
+  resources: ["validatingadmissionpolicybindings"]
+  verbs: ["get", "list", "watch", "update", "patch"]


Should this have Kvyerno also?

you mean validatingadmissionpolicybindings has kyverno?

No, sorry the lines here are unrelated. I mean kyverno.io is missing in the edit role.

I missed Thanks!

dhaiducek · 2025-01-09T18:40:30Z

pkg/templates/multiclusterhub/base/clusterrole-admin-aggregate.yaml

+- apiGroups: ["kyverno.io"]
+  resources: ["policies", "clusterpolicies"]
+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]


I could be swayed on Gatekeeper since Gatekeeper is included in an ACM subscription, but I'm not sure these permissions should allow anything other than a fetch for Kyverno objects since ACM doesn't include/support Kyverno.

Suggested change

- apiGroups: ["kyverno.io"]

resources: ["policies", "clusterpolicies"]

verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]

- apiGroups: ["kyverno.io"]

resources: ["policies", "clusterpolicies"]

verbs: ["get", "list", "watch"]

good point! Who should I ask this?

You could ask @gparvin?

I'm ok with get, list and watch but I don't think we want to go beyond those. What are we trying to do that needs more than just reading their resources?

@gparvin you are right. We need list, get (+watch maybe) to display on discovered policy table

@yiraeChristineKim Did we need more than get/list/watch on the other resources? Make sure you are requesting the least amount of permissions possible to reduce the possibility of security issues in this area.

We need get, list, and watch, but I primarily want to ensure compatibility with this PR. I am not entirely sure how the ClusterRole is being used

dhaiducek

LGTM!

gparvin · 2025-01-10T18:21:03Z

pkg/templates/multiclusterhub/base/clusterrole-admin-aggregate.yaml

  verbs: ["get", "list", "watch"]
 - apiGroups: ["submarineraddon.open-cluster-management.io"]
  resources: ["submarinerconfigs", "submarinerconfigs/status"]
  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]
+- apiGroups: ["mutations.gatekeeper.sh"]
+  resources: ["*"]
+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]


Double check whether get, list, watch is enough.

gparvin · 2025-01-10T18:22:18Z

pkg/templates/multiclusterhub/base/clusterrole-admin-aggregate.yaml

+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]
+- apiGroups: ["admissionregistration.k8s.io"]
+  resources: ["validatingadmissionpolicybindings", "validatingadmissionpolicies"]
+  verbs: ["create", "get", "list", "watch", "update", "delete", "deletecollection", "patch"]


Double check whether get, list, watch is enough. In the future I expect we could begin adopting VAP in the hub once it's supported across the OCP releases we run on, but for now I'm not sure why we would want the extra permissions.

so only get, list, watch? I am not sure why #1723 added extra verb to gatekeeper constraint

gparvin · 2025-01-10T18:23:01Z

pkg/templates/multiclusterhub/base/clusterrole-edit-aggregate.yaml

  verbs: ["get", "list", "watch"]
 - apiGroups: ["submarineraddon.open-cluster-management.io"]
  resources: ["submarinerconfigs", "submarinerconfigs/status"]
  verbs: ["get", "list", "watch", "update", "patch"]
+- apiGroups: ["mutations.gatekeeper.sh"]


If RBAC is reduced above, make sure update and patch are removed here too.

This will allow the discovered policies table to show kyverno policies(clusterPolicy, policy), gatekeeper mutations, validatingAdmissionPolicyBinding, policyreports for view users. Signed-off-by: yiraeChristineKim <[email protected]>

gparvin

Thank you!

openshift-ci · 2025-01-13T18:32:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, gparvin, yiraeChristineKim

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

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

sonarqubecloud · 2025-01-13T18:50:02Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

openshift-ci bot added the do-not-merge/work-in-progress label Jan 9, 2025

openshift-ci bot added the dco-signoff: yes label Jan 9, 2025

yiraeChristineKim force-pushed the dicovered-kyverno-gatekeeper-vapb branch 2 times, most recently from ea72ae6 to 1eab00b Compare January 9, 2025 18:14

openshift-ci bot requested review from joeg-pro and pshickeydev January 9, 2025 18:18

yiraeChristineKim marked this pull request as ready for review January 9, 2025 18:18

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 9, 2025

openshift-ci bot requested a review from cameronmwall January 9, 2025 18:18

openshift-ci bot requested review from dhaiducek, gparvin, JustinKuli and manravi January 9, 2025 18:19

JustinKuli reviewed Jan 9, 2025

View reviewed changes

dhaiducek suggested changes Jan 9, 2025

View reviewed changes

openshift-ci bot assigned dhaiducek Jan 9, 2025

yiraeChristineKim force-pushed the dicovered-kyverno-gatekeeper-vapb branch 3 times, most recently from d6f7706 to e48a233 Compare January 9, 2025 19:30

yiraeChristineKim requested review from dhaiducek and JustinKuli January 9, 2025 21:07

dhaiducek approved these changes Jan 9, 2025

View reviewed changes

openshift-ci bot added the lgtm label Jan 9, 2025

gparvin reviewed Jan 10, 2025

View reviewed changes

yiraeChristineKim force-pushed the dicovered-kyverno-gatekeeper-vapb branch from e48a233 to 535ee2c Compare January 13, 2025 18:25

openshift-ci bot removed the lgtm label Jan 13, 2025

yiraeChristineKim requested a review from gparvin January 13, 2025 18:31

gparvin approved these changes Jan 13, 2025

View reviewed changes

openshift-ci bot assigned gparvin Jan 13, 2025

openshift-ci bot added the lgtm label Jan 13, 2025

openshift-ci bot added the approved label Jan 13, 2025

openshift-merge-bot bot merged commit a215cef into stolostron:main Jan 13, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for discovered policies(kyverno, vapb etc) in aggregaterole #1907

Account for discovered policies(kyverno, vapb etc) in aggregaterole #1907

yiraeChristineKim commented Jan 9, 2025 •

edited

Loading

openshift-ci bot commented Jan 9, 2025

yiraeChristineKim commented Jan 9, 2025

yiraeChristineKim commented Jan 9, 2025

JustinKuli Jan 9, 2025

yiraeChristineKim Jan 9, 2025

yiraeChristineKim Jan 9, 2025

dhaiducek Jan 9, 2025

dhaiducek Jan 9, 2025

dhaiducek Jan 9, 2025

dhaiducek Jan 9, 2025

yiraeChristineKim Jan 9, 2025

dhaiducek Jan 9, 2025

yiraeChristineKim Jan 9, 2025

dhaiducek Jan 9, 2025

yiraeChristineKim Jan 9, 2025

dhaiducek Jan 9, 2025

gparvin Jan 9, 2025

yiraeChristineKim Jan 9, 2025

gparvin Jan 10, 2025

yiraeChristineKim Jan 10, 2025

dhaiducek left a comment

gparvin Jan 10, 2025

gparvin Jan 10, 2025

yiraeChristineKim Jan 10, 2025

gparvin Jan 10, 2025

gparvin left a comment

openshift-ci bot commented Jan 13, 2025

sonarqubecloud bot commented Jan 13, 2025

Account for discovered policies(kyverno, vapb etc) in aggregaterole #1907

Account for discovered policies(kyverno, vapb etc) in aggregaterole #1907

Conversation

yiraeChristineKim commented Jan 9, 2025 • edited Loading

Description

Related Issue

Changes Made

Checklist

Additional Notes

Reviewers

Definition of Done

openshift-ci bot commented Jan 9, 2025

yiraeChristineKim commented Jan 9, 2025

yiraeChristineKim commented Jan 9, 2025

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaiducek left a 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

Choose a reason for hiding this comment

gparvin left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 13, 2025

sonarqubecloud bot commented Jan 13, 2025

Quality Gate passed

yiraeChristineKim commented Jan 9, 2025 •

edited

Loading