Migrations: Revert remove ACL migration #5942

N-o-Z · 2023-05-23T12:01:23Z

Linked Issue

Closes #5941

Change Description

Introduce ACL migration as part of a rolling upgrade

nopcoder · 2023-05-26T10:02:53Z

pkg/kv/migrations/migrations_test.go

+	ctx := context.Background()
+
+	mig := migrations.NewACLsMigrator(nil, false)
+
+	for _, tt := range tests {


Suggested change

ctx := context.Background()

mig := migrations.NewACLsMigrator(nil, false)

for _, tt := range tests {

ctx := context.Background()

mig := migrations.NewACLsMigrator(nil, false)

for _, tt := range tests {

nopcoder · 2023-05-26T10:03:10Z

pkg/kv/migrations/migrations_test.go

+			permission, err := mig.ComputePermission(ctx, tt.Actions)
+
+			if permission != tt.Permission {


Suggested change

permission, err := mig.ComputePermission(ctx, tt.Actions)

if permission != tt.Permission {

permission, err := mig.ComputePermission(ctx, tt.Actions)

if permission != tt.Permission {

nopcoder · 2023-05-26T10:04:23Z

pkg/kv/migrations/migrations_test.go

+			permission, err := mig.ComputePermission(ctx, tt.Actions)
+
+			if permission != tt.Permission {
+				t.Errorf("Got permission %s when expecting %s", permission, tt.Permission)
+			}
+
+			if !errors.Is(err, tt.Err) {
+				t.Errorf("Got error %s but expected %s", err, tt.Err)
+			}


Need to verify expected error first

In case there is an err we need to skip permissions check

nopcoder · 2023-05-26T10:09:09Z

pkg/kv/migrations/migrations_test.go

+// #####################################################################################
+//																																										 #
+//												 				rbac_to_acl                                          #
+//																																										 #


Can we replace these headers with function doc or package level docs.
There are some examples (https://tip.golang.org/doc/comment)

nopcoder · 2023-05-26T10:12:02Z

pkg/kv/migrations/migrations_test.go

+	for i, a := range perms {
+		for j, b := range perms {
+			t.Run(fmt.Sprintf("%s:%s", a, b), func(t *testing.T) {


Suggest not to create a subtest for each check - just print the combination in the error.

nopcoder · 2023-05-26T10:39:21Z

pkg/kv/migrations/rbac_to_acl.go

+
+	// handle migrate within ACLs
+	if version == kv.ACLMigrateVersion {
+		if force {


Update using force flag should be handled by migrate level, not on a specific migrate to acl, as it is relevant no matter to which level you like to force the update - the level just have to be less or equal to the latest compiled with the binary.

It's more complicated then that. The force was designed specifically for ACLMigrationVersion (blocking migration). It was not designed for a rolling upgrade. I suggest to use the force flag per migration

nopcoder · 2023-05-26T10:44:43Z

pkg/kv/migrations/utils.go

+func updateKVSchemaVersion(ctx context.Context, kvStore kv.Store, version uint) error {
+	err := kv.SetDBSchemaVersion(ctx, kvStore, version)
+	if err != nil {
+		return fmt.Errorf("failed to upgrade version, to fix this re-run migration: %w", err)
+	}
+	return nil
+}


Would keep this one as part of rbac_to_acl as it looks like another helper for the specific code

This is being used in both import_premissions.py and rbac_to_acl.py.

nopcoder · 2023-05-26T10:46:30Z

pkg/kv/migrations/rbac_to_acl.go

+	return addedActionsSlice
+}
+
+// BroaderPermission returns true if a offers strictly more permissions that b.


Suggested change

// BroaderPermission returns true if a offers strictly more permissions that b.

// BroaderPermission returns true if a offers strictly more permissions that b. Unknown ACLPermission will panic.

nopcoder · 2023-05-26T10:51:37Z

pkg/kv/migrations/rbac_to_acl.go

+	addedActions := make(map[string]struct{}, len(allAllowedActions))
+	for _, action := range permissions.Actions {
+		if someActionMatches(action, allAllowedActions) && !someActionMatches(action, alreadyAllowedActions) {
+			addedActions[action] = struct{}{}
+		}
+	}
+	addedActionsSlice := make([]string, 0, len(addedActions))
+	for action := range addedActions {
+		addedActionsSlice = append(addedActionsSlice, action)
+	}
+	return addedActionsSlice


If we are going over a unique array of actions, to verify someActionMatches - why do we need a map (like we need to make sure to have unique keys) and transform it to a slice at the end.
Looks like we just need to add the relevant actions to a slice and return.

I'd rather not touch this code. It was written by @guy-har and already been battle tested.

Thanks - will look into it in a different PR.

nopcoder · 2023-05-26T10:52:45Z

pkg/kv/migrations/rbac_to_acl.go

+	var (
+		allAllowedActions map[string]struct{}
+	)


Suggested change

var (

allAllowedActions map[string]struct{}

)

var allAllowedActions map[string]struct{}

nopcoder

LGTM - minor suggestions about the code comment

nopcoder · 2023-05-29T08:19:00Z

pkg/kv/migrations/migrations_test.go

-//												 				rbac_to_acl                                          #
-//																																										 #
-// #####################################################################################
+// # rbac_to_acl test code


Suggested change

// # rbac_to_acl test code

// TestGetMinPermission - rbac_to_acl test code

nopcoder · 2023-05-29T08:19:25Z

pkg/kv/migrations/migrations_test.go

+// # import_permissions test code
+


Suggested change

// # import_permissions test code

// TestMigrateImportPermissions - import_permissions test code

nopcoder · 2023-05-29T08:20:40Z

pkg/kv/migrations/rbac_to_acl.go

+	addedActions := make(map[string]struct{}, len(allAllowedActions))
+	for _, action := range permissions.Actions {
+		if someActionMatches(action, allAllowedActions) && !someActionMatches(action, alreadyAllowedActions) {
+			addedActions[action] = struct{}{}
+		}
+	}
+	addedActionsSlice := make([]string, 0, len(addedActions))
+	for action := range addedActions {
+		addedActionsSlice = append(addedActionsSlice, action)
+	}
+	return addedActionsSlice


Thanks - will look into it in a different PR.

guy-har

Looks good, requesting changes due to the "warning" and "force" behavior that changed

guy-har · 2023-05-31T06:43:47Z

pkg/kv/migrations/rbac_to_acl.go

+	_, err = rbacToACL(ctx, authService, true, updateTime, func(groupID string, acl model.ACL, warn error) {
+		groupReports = append(groupReports, Warning{GroupID: groupID, ACL: acl, Warn: warn})
+	})


IIUC you are forcing it even if you have errors.
AFAIK this is not the expected behavior, it might perform unwanted changes (such as upgrading users to admin)

Migrations: Revert remove ACL migration

203f3bd

N-o-Z added include-changelog PR description should be included in next release changelog team/versioning-engine Team versioning engine labels May 23, 2023

N-o-Z requested review from guy-har, Jonathan-Rosenberg and a team May 23, 2023 12:01

N-o-Z self-assigned this May 23, 2023

Gofmt

c0e4f8e

nopcoder requested changes May 26, 2023

View reviewed changes

CR Fixes

90b6fec

N-o-Z requested a review from nopcoder May 28, 2023 08:57

nopcoder approved these changes May 29, 2023

View reviewed changes

guy-har requested changes May 31, 2023

View reviewed changes

N-o-Z added 2 commits May 31, 2023 11:36

CR Fixes

7233b98

CR Fixes 2

85719c5

guy-har approved these changes May 31, 2023

View reviewed changes

N-o-Z enabled auto-merge (squash) May 31, 2023 08:43

N-o-Z merged commit acbc0dc into master May 31, 2023

N-o-Z deleted the task/add-acl-migration branch May 31, 2023 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrations: Revert remove ACL migration #5942

Migrations: Revert remove ACL migration #5942

N-o-Z commented May 23, 2023

nopcoder May 26, 2023

nopcoder May 26, 2023

nopcoder May 26, 2023

nopcoder May 26, 2023

nopcoder May 26, 2023

nopcoder May 26, 2023

N-o-Z May 28, 2023

nopcoder May 26, 2023

N-o-Z May 28, 2023

nopcoder May 26, 2023

nopcoder May 26, 2023

N-o-Z May 28, 2023 •

edited

Loading

nopcoder May 29, 2023

nopcoder May 26, 2023

nopcoder left a comment

nopcoder May 29, 2023

nopcoder May 29, 2023

nopcoder May 29, 2023

guy-har left a comment

guy-har May 31, 2023

		permission, err := mig.ComputePermission(ctx, tt.Actions)

		if permission != tt.Permission {

	// BroaderPermission returns true if a offers strictly more permissions that b.
	// BroaderPermission returns true if a offers strictly more permissions that b. Unknown ACLPermission will panic.

	// # rbac_to_acl test code
	// TestGetMinPermission - rbac_to_acl test code

	// # import_permissions test code
	// TestMigrateImportPermissions - import_permissions test code

Migrations: Revert remove ACL migration #5942

Migrations: Revert remove ACL migration #5942

Conversation

N-o-Z commented May 23, 2023

Linked Issue

Closes #5941

Change Description

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

N-o-Z May 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder 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

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z May 28, 2023 •

edited

Loading