feat: implement cmpd's PolicyRules #8328

cjc7373 · 2024-10-24T14:56:40Z

Fixes #8310. Things done in this PR:

implements cmpd's PolicyRules
changes the semantics of serviceAccountName in cluster and component CR. KB now does not create rbac resources if user has specified a service account.
serviceaccount is now within a component's level, with a name of kb-<clusterName>-<compName>.

Addon changes (like update pg addon's cmpd policyRule since we removed kubeblocks-patroni-pod-role) will be addressed in apecloud/kubeblocks-addons#1197.

codecov · 2024-10-25T07:25:23Z

Codecov Report

Attention: Patch coverage is 66.50485% with 69 lines in your changes missing coverage. Please review.

Project coverage is 60.30%. Comparing base (c206d0e) to head (9e56c89).

Files with missing lines	Patch %	Lines
controllers/apps/transformer_component_rbac.go	72.44%	21 Missing and 14 partials ⚠️
controllers/apps/transformer_component_deletion.go	59.61%	14 Missing and 7 partials ⚠️
pkg/controller/factory/builder.go	21.42%	11 Missing ⚠️
controllers/apps/transformer_cluster_deletion.go	50.00%	0 Missing and 1 partial ⚠️
pkg/controller/component/synthesize_component.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8328      +/-   ##
==========================================
+ Coverage   60.04%   60.30%   +0.25%     
==========================================
  Files         376      377       +1     
  Lines       45858    45915      +57     
==========================================
+ Hits        27535    27688     +153     
+ Misses      15739    15621     -118     
- Partials     2584     2606      +22

Flag	Coverage Δ
unittests	`60.30% <66.50%> (+0.25%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

controllers/apps/transformer_component_rbac.go

deploy/helm/templates/rbac/cluster_pod_required_role.yaml

controllers/apps/transformer_cluster_deletion.go

controllers/apps/transformer_component_rbac.go

cjc7373 · 2024-11-14T07:17:03Z

relevant addon PR: apecloud/kubeblocks-addons#1197

apis/apps/v1/cluster_types.go

leon-inf · 2024-11-20T10:05:04Z

apis/apps/v1/cluster_types.go

-	// The service account will be bound to a default role named "kubeblocks-cluster-pod-role" which is installed together with KubeBlocks.
-	// If not specified, KubeBlocks automatically assigns a default ServiceAccount named "kb-{cluster.name}"
+	// If not specified, KubeBlocks automatically creates a default ServiceAccount named
+	// "kb-{cluster.name}-{component.name}", bound to a role with rules defined in ComponentDefinition's


As a further improvement, components with the same cmp can share a default service account.

So that the rbac provision can be moved to cmpd controller?

I ignored a problem. Cmpd is not namespaced, but serviceaccount/rolebinding is. ~~So that it is not feasible to move rbac provision to cmpd controller.~~ Cmpd controller can watch a component to trigger reconcile. The reconcilation related to rbac may look like this:

It lists all components that references the cmpd. Find all the namespaces of them (list 1).

It lists all managed rbac resources (serviceaccount/rolebinding/role) using label selector. Find all the namespaces of them (list 2).

Create/update rbac resources in list 1.

Delete rbac resouces in list 2 but not in list 1.

controllers/apps/transformer_component_utils.go

…onent

cjc7373 · 2024-11-29T06:38:45Z

After a second thought, rbac provision will remain in component controller.

To share rbac resources among multiple components, these rbac resources will use the component which first creates them as their owner. If the component has been deleted, their owner will be transfered to another component which shares a same cmpd. If no such component can be found, they will be deleted.

Another note: equality.Semantic is used to replace reflect.DeepEqual to avoid the false alarm of something like reflect.DeepEqual([]v1.LocalObjectReference(nil), []v1.LocalObjectReference{})

…onent

leon-inf · 2024-12-03T09:49:43Z

controllers/apps/transformer_component_rbac.go

-	if serviceAccount == nil {
-		transCtx.Logger.V(1).Info("buildServiceAccounts returns serviceAccount nil")
+	if !viper.GetBool(constant.EnableRBACManager) {
+		transCtx.EventRecorder.Event(transCtx.Cluster, corev1.EventTypeWarning, EventReasonRBACManager, "RBAC manager is disabled")


No need to initiate a warning event, logging or a normal event is sufficient.

leon-inf · 2024-12-03T09:52:05Z

controllers/apps/transformer_component_rbac.go

-			string(ictrlutil.ErrorTypeNotFound), fmt.Sprintf("ServiceAccount %s is not exist", serviceAccount.Name))
-		return ictrlutil.NewRequeueError(time.Second, "RBAC manager is disabled, but service account is not exist")
+	var err error
+	if serviceAccountName == "" {


Even if the service account is specified by user, should we consider setting a finalizer to prevent accidental deletion by the user?

leon-inf · 2024-12-03T09:53:12Z

controllers/apps/transformer_component_rbac.go

-		return nil
+	graphCli, _ := transCtx.Client.(model.GraphClient)
+
+	if err := cleanOldResource(transCtx, graphCli, dag, synthesizedComp); err != nil {


This can be implemented when transitioning old version clusters to version 1.0.

Where is the transitioning code?

leon-inf · 2024-12-03T09:55:22Z

controllers/apps/component_controller.go

 			Owns(&corev1.ServiceAccount{})
-	} else {
-		b.Watches(&rbacv1.RoleBinding{}, handler.EnqueueRequestsFromMapFunc(r.filterComponentResources)).
-			Watches(&corev1.ServiceAccount{}, handler.EnqueueRequestsFromMapFunc(r.filterComponentResources))


How should it be handled when the service account is specified by user?

It's not owned by the component controller.

IMO a service account specified by user should not be modified by KB, even if it's deleted by user. So kb doesn't need to receive events from these service accounts.

leon-inf · 2024-12-03T10:01:33Z

pkg/controller/factory/builder.go

+	if len(rules) == 0 {
+		return nil
+	}
+	return builder.NewRoleBuilder(synthesizedComp.Namespace, saName).


If the role is shared within a cmpd, the name should not use the service account name, as it might be specified by user.

Ok, I'll use cmpd's name as role/rolebinding's name.

leon-inf · 2024-12-03T10:03:18Z

pkg/controller/factory/builder.go

+		return nil
+	}
+	return builder.NewRoleBuilder(synthesizedComp.Namespace, saName).
+		AddLabelsInMap(constant.GetCompLabels(synthesizedComp.ClusterName, synthesizedComp.Name)).


And these labels become meaningless because they do not belong to any specific object.

controllers/apps/transformer_component_deletion.go

wangyelei · 2024-12-06T05:51:26Z

controllers/apps/transformer_component_deletion.go

+			return false, err
+		}
+		// if any, transfer ownership to any other component
+		for _, otherComp := range compList.Items {


skip deleting component

github-actions bot added the size/XL Denotes a PR that changes 500-999 lines. label Oct 24, 2024

cjc7373 changed the title ~~feature: implement cmpd's PolicyRules~~ feat: implement cmpd's PolicyRules Oct 24, 2024

apecloud-bot added the area/user-interaction label Oct 24, 2024

apecloud-bot requested a review from realzyy October 24, 2024 15:05

cjc7373 marked this pull request as ready for review October 28, 2024 07:30

cjc7373 requested review from leon-inf, Y-Rookie and a team as code owners October 28, 2024 07:30

zjx20 reviewed Oct 30, 2024

View reviewed changes

controllers/apps/transformer_component_rbac.go Show resolved Hide resolved

cjc7373 commented Oct 31, 2024

View reviewed changes

deploy/helm/templates/rbac/cluster_pod_required_role.yaml Outdated Show resolved Hide resolved

zjx20 approved these changes Oct 31, 2024

View reviewed changes

apecloud-bot added the approved PR Approved Test label Oct 31, 2024

Y-Rookie reviewed Nov 1, 2024

View reviewed changes

controllers/apps/transformer_cluster_deletion.go Outdated Show resolved Hide resolved

Y-Rookie reviewed Nov 1, 2024

View reviewed changes

controllers/apps/transformer_component_rbac.go Outdated Show resolved Hide resolved

cjc7373 added 15 commits November 12, 2024 17:03

wip

17e60fc

cleanup

8a37f6b

wip

8dfd349

cleanup kubeblocks-kb-agent-pod-role

a80d8f0

refactor

092e869

unit test

7169d42

fix tests

1aea7fe

fix tests

6282bd3

cleanup

b262349

api doc

ad2e09c

static check

a57f03b

doc

618a559

cleanup

b3e4c6f

fix: wrong kind

208cf73

fix

155b67a

apecloud-bot removed the approved PR Approved Test label Nov 14, 2024

cjc7373 added 2 commits November 14, 2024 15:09

fix test

8416e61

typo

7581dc6

leon-inf reviewed Nov 20, 2024

View reviewed changes

apis/apps/v1/cluster_types.go Outdated Show resolved Hide resolved

leon-inf reviewed Nov 20, 2024

View reviewed changes

controllers/apps/transformer_component_utils.go Outdated Show resolved Hide resolved

cjc7373 added 7 commits November 22, 2024 16:33

Merge remote-tracking branch 'origin/main' into feature/rbac-for-comp…

e5587b3

…onent

clean code

1b7bc7c

Merge remote-tracking branch 'origin/main' into feature/rbac-for-comp…

7235c5e

…onent

Merge remote-tracking branch 'origin/main' into feature/rbac-for-comp…

5e68101

…onent

refine description

287bb06

change sa naming rule, share rbac resources among components

70deaa9

fix unit test

8a5a122

github-actions bot added size/XXL Denotes a PR that changes 1000+ lines. and removed size/XL Denotes a PR that changes 500-999 lines. labels Nov 29, 2024

cjc7373 added 2 commits November 29, 2024 14:21

typo

f55de76

fix test

3bceb13

cjc7373 added 3 commits December 3, 2024 16:45

Merge remote-tracking branch 'origin/main' into feature/rbac-for-comp…

8fc1b2f

…onent

clean up

e04467b

clean up

9e56c89

leon-inf reviewed Dec 3, 2024

View reviewed changes

wangyelei reviewed Dec 6, 2024

View reviewed changes

controllers/apps/transformer_component_deletion.go Show resolved Hide resolved

wangyelei reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement cmpd's PolicyRules #8328

feat: implement cmpd's PolicyRules #8328

cjc7373 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

cjc7373 commented Nov 14, 2024

leon-inf Nov 20, 2024

cjc7373 Nov 22, 2024

cjc7373 Nov 27, 2024 •

edited

Loading

cjc7373 commented Nov 29, 2024 •

edited

Loading

leon-inf Dec 3, 2024

leon-inf Dec 3, 2024

leon-inf Dec 3, 2024 •

edited

Loading

cjc7373 Dec 4, 2024

leon-inf Dec 3, 2024

leon-inf Dec 3, 2024

cjc7373 Dec 4, 2024

leon-inf Dec 3, 2024

cjc7373 Dec 4, 2024 •

edited

Loading

leon-inf Dec 3, 2024

wangyelei Dec 6, 2024

feat: implement cmpd's PolicyRules #8328

Are you sure you want to change the base?

feat: implement cmpd's PolicyRules #8328

Conversation

cjc7373 commented Oct 24, 2024 • edited Loading

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

cjc7373 commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjc7373 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

cjc7373 commented Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leon-inf Dec 3, 2024 • edited Loading

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

cjc7373 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjc7373 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

cjc7373 Nov 27, 2024 •

edited

Loading

cjc7373 commented Nov 29, 2024 •

edited

Loading

leon-inf Dec 3, 2024 •

edited

Loading

cjc7373 Dec 4, 2024 •

edited

Loading