From 3f3aaa5a39b785c98d8e05071463775def22ebda Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Sat, 9 Dec 2023 23:18:36 +0100 Subject: [PATCH] SyncAcls: move from added to updated (#10) * adds entra instance to test the a different flow * SyncAcls: move from added to updated - I've noticed running docker compose that if we have an existing user that would be updated, we delete it before. So here we will skip the delettion and allow it to be updated in a non-disruptive way * update tests --- configs/docker/docker-compose.yaml | 23 +++++++++++++++++ pkg/aclmanager/aclmanager.go | 41 ++++++++++++++++-------------- pkg/aclmanager/aclmanager_test.go | 34 +++++++++++++++++-------- 3 files changed, 69 insertions(+), 29 deletions(-) diff --git a/configs/docker/docker-compose.yaml b/configs/docker/docker-compose.yaml index 8c086e8..7c3935a 100644 --- a/configs/docker/docker-compose.yaml +++ b/configs/docker/docker-compose.yaml @@ -22,6 +22,18 @@ services: - "redis0001" - "6379" + redis0003: + image: redis:latest + container_name: redis0003 + volumes: + - ./redis:/usr/local/etc/redis:rw + command: + - "redis-server" + - "/usr/local/etc/redis/redis.conf" + - "--slaveof" + - "redis0001" + - "6379" + bedel_redis0001: image: ncode/bedel:dev container_name: bedel_redis0001 @@ -50,6 +62,17 @@ services: - "-l" - "info" + bedel_redis0003: + image: ncode/bedel:dev + container_name: bedel_redis0003 + depends_on: + - vault + command: + - "run" + environment: + ADDRESS: "redis0003:6379" + PASSWORD: "bedel-integration-test" + vault: image: hashicorp/vault:latest container_name: vault diff --git a/pkg/aclmanager/aclmanager.go b/pkg/aclmanager/aclmanager.go index 476ac0a..41c1453 100644 --- a/pkg/aclmanager/aclmanager.go +++ b/pkg/aclmanager/aclmanager.go @@ -165,10 +165,10 @@ func listAcls(ctx context.Context, client *redis.Client) (acls []string, err err } // SyncAcls connects to master node and syncs the acls to the current node -func (a *AclManager) SyncAcls(ctx context.Context, primary *AclManager) (added []string, deleted []string, err error) { +func (a *AclManager) SyncAcls(ctx context.Context, primary *AclManager) (updated []string, deleted []string, err error) { slog.Debug("Syncing acls") if primary == nil { - return added, deleted, fmt.Errorf("no primary found") + return updated, deleted, fmt.Errorf("no primary found") } sourceAcls, err := listAcls(ctx, primary.RedisClient) @@ -182,31 +182,34 @@ func (a *AclManager) SyncAcls(ctx context.Context, primary *AclManager) (added [ } // Map to keep track of ACLs to add - toAdd := make(map[string]struct{}) + toUpdate := make(map[string]string) for _, acl := range sourceAcls { - toAdd[acl] = struct{}{} + username := strings.Split(acl, " ")[1] + toUpdate[username] = acl } - // Delete ACLs not in source and remove from the toAdd map if present in destination + // Delete ACLs not in source and remove from the toUpdate map if present in destination for _, acl := range destinationAcls { username := strings.Split(acl, " ")[1] - if _, found := toAdd[acl]; found { - // If found in source, don't need to add, so remove from map - delete(toAdd, acl) - slog.Debug("ACL already in sync", "username", username) - } else { - // If not found in source, delete from destination - slog.Debug("Deleting ACL", "username", username) - if err := a.RedisClient.Do(context.Background(), "ACL", "DELUSER", username).Err(); err != nil { - return nil, nil, fmt.Errorf("error deleting acl: %v", err) + if currentAcl, found := toUpdate[username]; found { + if currentAcl == acl { + // If found in source, don't need to add, so remove from map + delete(toUpdate, username) + slog.Debug("ACL already in sync", "username", username) } - deleted = append(deleted, username) + continue + } + + // If not found in source, delete from destination + slog.Debug("Deleting ACL", "username", username) + if err := a.RedisClient.Do(context.Background(), "ACL", "DELUSER", username).Err(); err != nil { + return nil, nil, fmt.Errorf("error deleting acl: %v", err) } + deleted = append(deleted, username) } // Add remaining ACLs from source - for acl := range toAdd { - username := strings.Split(acl, " ")[1] + for username, acl := range toUpdate { slog.Debug("Syncing ACL", "username", username, "line", acl) command := strings.Split(filterUser.ReplaceAllString(acl, "ACL SETUSER "), " ") commandInterfce := make([]interface{}, len(command)) @@ -216,10 +219,10 @@ func (a *AclManager) SyncAcls(ctx context.Context, primary *AclManager) (added [ if err := a.RedisClient.Do(context.Background(), commandInterfce...).Err(); err != nil { return nil, nil, fmt.Errorf("error setting acl: %v", err) } - added = append(added, username) + updated = append(updated, username) } - return added, deleted, nil + return updated, deleted, nil } // Loop loops through the sync interval and syncs the acls diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index 1df135c..e915603 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/spf13/viper" "reflect" + "slices" "strings" "testing" "time" @@ -211,13 +212,13 @@ func TestListAcls_Error(t *testing.T) { assert.Error(t, err) } -func TestMirrorAcls(t *testing.T) { +func TestSyncAcls(t *testing.T) { tests := []struct { name string sourceAcls []interface{} destinationAcls []interface{} expectedDeleted []string - expectedAdded []string + expectedUpdated []string listAclsError error redisDoError error wantSourceErr bool @@ -228,8 +229,14 @@ func TestMirrorAcls(t *testing.T) { sourceAcls: []interface{}{"user acl1", "user acl2"}, destinationAcls: []interface{}{"user acl1", "user acl3"}, expectedDeleted: []string{"acl3"}, - expectedAdded: []string{"acl2"}, - wantSourceErr: false, + expectedUpdated: []string{"acl2"}, + }, + { + name: "ACLs synced with differences", + sourceAcls: []interface{}{"user acl1", "user acl2"}, + destinationAcls: []interface{}{"user acl1 something something", "user acl3"}, + expectedDeleted: []string{"acl3"}, + expectedUpdated: []string{"acl1", "acl2"}, }, { name: "ACLs synced with Error om SETUSER", @@ -241,7 +248,7 @@ func TestMirrorAcls(t *testing.T) { { name: "ACLs synced with Error on SETUSER", sourceAcls: []interface{}{"user acl1", "user acl2"}, - destinationAcls: []interface{}{"user acl1", "user acl3"}, + destinationAcls: []interface{}{"user acl1"}, redisDoError: fmt.Errorf("SETUSER"), wantSourceErr: false, wantDestinationErr: true, @@ -304,8 +311,8 @@ func TestMirrorAcls(t *testing.T) { destMock.ExpectDo("ACL", "DELUSER", username).SetVal("OK") } } - if tt.expectedAdded != nil { - for _, username := range tt.expectedAdded { + if tt.expectedUpdated != nil { + for _, username := range tt.expectedUpdated { if tt.wantDestinationErr && tt.redisDoError.Error() == "SETUSER" { destMock.ExpectDo("ACL", "SETUSER", username).SetErr(tt.redisDoError) continue @@ -325,7 +332,7 @@ func TestMirrorAcls(t *testing.T) { t.Errorf("mirrorAcls() error = %v, wantErr %v", err, tt.redisDoError) } } - if !tt.wantDestinationErr { + if tt.wantDestinationErr { if tt.listAclsError != nil && !strings.Contains(err.Error(), tt.listAclsError.Error()) { t.Errorf("mirrorAcls() error = %v, wantErr %v", err, tt.listAclsError) } @@ -333,12 +340,19 @@ func TestMirrorAcls(t *testing.T) { t.Errorf("mirrorAcls() error = %v, wantErr %v", err, tt.redisDoError) } } + if !tt.wantSourceErr && !tt.wantDestinationErr { + t.Errorf("mirrorAcls() error = %v, wantErr %v", err, tt.wantSourceErr) + } } + slices.Sort(added) + slices.Sort(tt.expectedUpdated) + slices.Sort(deleted) + slices.Sort(tt.expectedDeleted) if !reflect.DeepEqual(deleted, tt.expectedDeleted) { t.Errorf("mirrorAcls() deleted = %v, expectedDeleted %v", deleted, tt.expectedDeleted) } - if !reflect.DeepEqual(added, tt.expectedAdded) { - t.Errorf("mirrorAcls() added = %v, expectedAdded %v", deleted, tt.expectedDeleted) + if !reflect.DeepEqual(added, tt.expectedUpdated) { + t.Errorf("mirrorAcls() updated = %v, expectedUpdated %v", deleted, tt.expectedUpdated) } }) }