pkg/components: Refactor to reduce duplicate code #1845

RamLavi · 2024-08-06T11:45:03Z

What this PR does / why we need it:
currently sonarCloud reports a lot of code is duplicate on pkg/components (37.6%).
Refactoring the code to reduce duplicate to 0% and make thing more clear (all policies are visible in one screen, no endless scrolling).

Special notes for your reviewer:

Release note:

NONE

kubevirt-bot · 2024-08-06T11:45:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ramlavi. 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

RamLavi · 2024-08-06T11:51:57Z

@oshoval @phoracek PTAL

oshoval · 2024-08-06T11:57:57Z

pkg/components/components.go

+	var rules []rbacv1.PolicyRule
+
+	rules = append(rules,
+		newPolicyRule([]string{""}, []string{"events"}, []string{"update"}),


wdyt about this ?
easier to read imo please

newPolicyRule( []string{""}, []string{"events"}, []string{"update"} ),

ouch sonar is not happy from that
i guess we dont have a choice

tried: duplicity went up 18%
perhaps we could use new line only on lines that are too long?

either that, or just revert to the initial please (better imo)
whatever you prefer
thanks

I think we can continue with this change as is.

oshoval · 2024-08-06T12:13:26Z

beside cosmetics there are no changes right ?

RamLavi · 2024-08-06T12:27:10Z

beside cosmetics there are no changes right ?

It's only a refactor. No logic changed.

nunnatsa · 2024-08-11T09:02:52Z

pkg/components/components.go

+	var rules []rbacv1.PolicyRule
+
+	rules = append(rules,
+		newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),
+		newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),
+		newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),
+		newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),
+		newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),
+		newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),
+	)


why using append? append is pricey. I think you can initialize the slice with the required data; e.g.

Suggested change

var rules []rbacv1.PolicyRule

rules = append(rules,

newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),

newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),

newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),

newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),

newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),

newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),

)

rules := []rbacv1.PolicyRule{

newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),

newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),

newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),

newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),

newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),

newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),

}

nunnatsa · 2024-08-11T09:03:20Z

pkg/components/components.go

 	}
 	return role
 }

 func GetClusterRole(allowMultus bool) *rbacv1.ClusterRole {
+	var rules []rbacv1.PolicyRule


nunnatsa · 2024-08-11T09:03:52Z

pkg/components/components.go

+	var rules []rbacv1.PolicyRule
+
+	rules = append(rules,


RamLavi · 2024-08-13T09:36:18Z

Change: Rebase + removed @oshoval 's suggestion as it doesn't fix duplicate code issue

RamLavi · 2024-08-13T09:36:55Z

Change: Fix for @nunnatsa review

RamLavi · 2024-08-13T09:38:13Z

/hold
I want to manually check that no rbac was left out

oshoval · 2024-08-13T09:39:46Z

/hold I want to manually check that no rbac was left out

if any are left out, tests should find it, or we are missing tests, or those arent needed anymore
so if you find any, worth to check which of those reasons is the right one please

oshoval · 2024-08-13T09:56:03Z

you didnt rebase correctly, please check it
those seem to be missed, it will fail on OpenShift, but not on U/S (well it will also fail here as part of the deployment)
"virtualmachines/finalizers",
"virtualmachineinstances/finalizers",

Those need to be included please:
25b584e

RamLavi · 2024-08-13T10:40:08Z

virtualmachines/finalizers

thanks!

sonarqubecloud · 2024-08-13T10:41:11Z

Quality Gate passed

Issues
4 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 SonarCloud

oshoval · 2024-08-13T10:41:57Z

https://github.com/kubevirt/cluster-network-addons-operator/compare/968a49b0b0b0cee146b1409e44d9bebd457fd49e..cc6fe21598b5e551d09a187b03c9247b0dc4b3d3
you have here twice update

kubevirt-bot · 2024-11-11T14:44:42Z

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 · 2024-12-11T15:29:42Z

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

sonarqubecloud · 2024-12-15T07:48:02Z

Quality Gate passed

Issues
4 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

RamLavi · 2024-12-15T07:49:06Z

Change: Rebase

RamLavi · 2024-12-15T07:49:19Z

https://github.com/kubevirt/cluster-network-addons-operator/compare/968a49b0b0b0cee146b1409e44d9bebd457fd49e..cc6fe21598b5e551d09a187b03c9247b0dc4b3d3 you have here twice update

nice catch! - fixed. PTAL

RamLavi · 2024-12-15T07:50:37Z

/hold cancel

RamLavi · 2024-12-15T07:52:24Z

/remove-lifecycle rotten

This is done in order to eliminate sonarCloud issue. Signed-off-by: Ram Lavi <[email protected]>

RamLavi · 2025-02-10T09:18:31Z

Change: Rebase

sonarqubecloud · 2025-02-10T09:18:51Z

Quality Gate passed

Issues
4 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

nunnatsa

I think that if you can use the group names consts from the group themselves, it would be clearer and safer.

Added several examples inline.

nunnatsa · 2025-02-11T08:57:07Z

pkg/components/components.go

+		newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),
+		newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),
+		newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),
+		newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),
+		newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),
+		newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),
+		newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),


What do you think about defining a core group variable, e.g. var coreGroup = []string{""} and then use it instead of the empty string group? i.e.

Suggested change

newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "create", "update"}),

newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),

newPolicyRule([]string{""}, []string{"namespaces"}, []string{"update", "get", "patch"}),

newPolicyRule([]string{""}, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),

newPolicyRule([]string{""}, []string{"configmaps"}, []string{"patch"}),

newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),

newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"apps"}, []string{"daemonsets"}, []string{"get", "create", "update", "delete"}),

newPolicyRule(coreGroup, []string{"configmaps"}, []string{"get", "create", "update"}),

newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"delete"}),

newPolicyRule(coreGroup, []string{"namespaces"}, []string{"update", "get", "patch"}),

newPolicyRule(coreGroup, []string{"serviceaccounts"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"monitoring.coreos.com"}, []string{"prometheusrules", "servicemonitors"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"rbac.authorization.k8s.io"}, []string{"roles", "rolebindings"}, []string{"get", "create", "update", "delete"}),

newPolicyRule([]string{"policy"}, []string{"poddisruptionbudgets"}, []string{"get", "delete"}),

newPolicyRule(coreGroup, []string{"configmaps"}, []string{"patch"}),

newPolicyRule([]string{"coordination.k8s.io"}, []string{"leases"}, []string{"get", "list", "watch", "create", "update", "patch", "delete"}),

newPolicyRule([]string{"cert-manager.io"}, []string{"certificates", "issuers"}, []string{"get", "create", "update", "delete"}),

nunnatsa · 2025-02-11T09:16:55Z

pkg/components/components.go

+	rules := []rbacv1.PolicyRule{
+		newPolicyRule([]string{""}, []string{"events"}, []string{"update"}),
+		newPolicyRule([]string{""}, []string{"pods", "pods/status"}, []string{"get", "update", "list", "watch"}),
+		newPolicyRule([]string{"events.k8s.io"}, []string{"events"}, []string{"create", "patch", "update"}),


if you'll import eventsv1 "k8s.io/api/events/v1",

you could use:

Suggested change

newPolicyRule([]string{"events.k8s.io"}, []string{"events"}, []string{"create", "patch", "update"}),

newPolicyRule([]string{eventsv1.GroupName}, []string{"events"}, []string{"create", "patch", "update"}),

nunnatsa · 2025-02-11T09:17:30Z

pkg/components/components.go

+		newPolicyRule([]string{""}, []string{"nodes", "nodes/status"}, []string{"get", "update", "patch"}),
+		newPolicyRule([]string{""}, []string{"configmaps"}, []string{"get", "delete"}),
+		newPolicyRule([]string{""}, []string{"secrets"}, []string{"list", "watch", "create", "update"}),
+		newPolicyRule([]string{"admissionregistration.k8s.io"}, []string{"validatingwebhookconfigurations", "mutatingwebhookconfigurations"}, []string{"list", "watch"}),


Same, with admissionregistrationv1 "k8s.io/api/admissionregistration/v1"

nunnatsa · 2025-02-11T09:18:17Z

pkg/components/components.go

+		newPolicyRule([]string{""}, []string{"secrets"}, []string{"list", "watch", "create", "update"}),
+		newPolicyRule([]string{"admissionregistration.k8s.io"}, []string{"validatingwebhookconfigurations", "mutatingwebhookconfigurations"}, []string{"list", "watch"}),
+		newPolicyRule([]string{""}, []string{"services"}, []string{"get", "create", "update", "list", "watch"}),
+		newPolicyRule([]string{"kubevirt.io"}, []string{"virtualmachines"}, []string{"get", "list", "watch", "update"}),


same with kvcore "kubevirt.io/api/core"

nunnatsa · 2025-02-11T09:18:25Z

pkg/components/components.go

+		newPolicyRule([]string{"authentication.k8s.io"}, []string{"tokenreviews"}, []string{"create"}),
+		newPolicyRule([]string{"authorization.k8s.io"}, []string{"subjectaccessreviews"}, []string{"create"}),
+		newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"get", "create", "update"}),
+		newPolicyRule([]string{"kubevirt.io"}, []string{"virtualmachineinstances"}, []string{"get", "list", "watch"}),


same with kvcore "kubevirt.io/api/core"

nunnatsa · 2025-02-11T09:19:15Z

pkg/components/components.go

+		newPolicyRule([]string{"authentication.k8s.io"}, []string{"tokenreviews"}, []string{"create"}),
+		newPolicyRule([]string{"authorization.k8s.io"}, []string{"subjectaccessreviews"}, []string{"create"}),


Same with authenticationv1 "k8s.io/api/authentication/v1"

fix: the 2nd one should be withauthorizationv1 "k8s.io/api/authorization/v1"

nunnatsa · 2025-02-11T09:20:10Z

pkg/components/components.go

+		newPolicyRule([]string{"kubevirt.io"}, []string{"virtualmachines"}, []string{"get", "list", "watch", "update"}),
+		newPolicyRule([]string{"authentication.k8s.io"}, []string{"tokenreviews"}, []string{"create"}),
+		newPolicyRule([]string{"authorization.k8s.io"}, []string{"subjectaccessreviews"}, []string{"create"}),
+		newPolicyRule([]string{"apps"}, []string{"deployments"}, []string{"get", "create", "update"}),


same with appsv1 "k8s.io/api/apps/v1"

nunnatsa · 2025-02-11T09:21:12Z

pkg/components/components.go

+		newPolicyRule([]string{"k8s.cni.cncf.io"}, []string{"ipamclaims"}, []string{"get", "list", "watch", "create", "update"}),
+		newPolicyRule([]string{"k8s.cni.cncf.io"}, []string{"network-attachment-definitions"}, []string{"get", "list", "watch", "create", "delete", "update"}),


same with cnicncf "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io"

kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 6, 2024

kubevirt-bot requested a review from phoracek August 6, 2024 11:45

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Aug 6, 2024

kubevirt-bot requested a review from qinqon August 6, 2024 11:45

kubevirt-bot added the size/XL label Aug 6, 2024

oshoval reviewed Aug 6, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2024

nunnatsa reviewed Aug 11, 2024

View reviewed changes

pkg/components/components.go Outdated

Comment on lines 479 to 439

var rules []rbacv1.PolicyRule

rules = append(rules,

Copy link

Contributor

nunnatsa Aug 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

RamLavi force-pushed the refactor_components branch from 5d31b26 to 6896c42 Compare August 13, 2024 09:35

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024

RamLavi force-pushed the refactor_components branch from 6896c42 to 968a49b Compare August 13, 2024 09:36

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024

RamLavi force-pushed the refactor_components branch from 968a49b to cc6fe21 Compare August 13, 2024 10:40

kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 11, 2024

kubevirt-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 11, 2024

RamLavi force-pushed the refactor_components branch from cc6fe21 to 519dec3 Compare December 15, 2024 07:47

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2024

kubevirt-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 15, 2024

pkg/components: Refactor to reduce duplicate code

c70c726

This is done in order to eliminate sonarCloud issue. Signed-off-by: Ram Lavi <[email protected]>

RamLavi force-pushed the refactor_components branch from 519dec3 to c70c726 Compare February 10, 2025 09:17

nunnatsa reviewed Feb 11, 2025

View reviewed changes

	newPolicyRule([]string{"events.k8s.io"}, []string{"events"}, []string{"create", "patch", "update"}),
	newPolicyRule([]string{eventsv1.GroupName}, []string{"events"}, []string{"create", "patch", "update"}),

		newPolicyRule([]string{"authentication.k8s.io"}, []string{"tokenreviews"}, []string{"create"}),
		newPolicyRule([]string{"authorization.k8s.io"}, []string{"subjectaccessreviews"}, []string{"create"}),

		newPolicyRule([]string{"k8s.cni.cncf.io"}, []string{"ipamclaims"}, []string{"get", "list", "watch", "create", "update"}),
		newPolicyRule([]string{"k8s.cni.cncf.io"}, []string{"network-attachment-definitions"}, []string{"get", "list", "watch", "create", "delete", "update"}),

pkg/components: Refactor to reduce duplicate code #1845

Are you sure you want to change the base?

pkg/components: Refactor to reduce duplicate code #1845

Conversation

RamLavi commented Aug 6, 2024 • edited Loading

kubevirt-bot commented Aug 6, 2024

RamLavi commented Aug 6, 2024

oshoval Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval commented Aug 6, 2024

RamLavi commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi commented Aug 13, 2024

RamLavi commented Aug 13, 2024 • edited Loading

RamLavi commented Aug 13, 2024

oshoval commented Aug 13, 2024

oshoval commented Aug 13, 2024 • edited Loading

RamLavi commented Aug 13, 2024

sonarqubecloud bot commented Aug 13, 2024

Quality Gate passed

oshoval commented Aug 13, 2024

kubevirt-bot commented Nov 11, 2024

kubevirt-bot commented Dec 11, 2024

sonarqubecloud bot commented Dec 15, 2024

Quality Gate passed

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Dec 15, 2024

RamLavi commented Feb 10, 2025

sonarqubecloud bot commented Feb 10, 2025

Quality Gate passed

nunnatsa 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

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

RamLavi commented Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

oshoval Aug 6, 2024 •

edited

Loading

RamLavi commented Aug 13, 2024 •

edited

Loading

oshoval commented Aug 13, 2024 •

edited

Loading