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

@@ -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

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

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,20 +332,27 @@ 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)
 					}
 					if tt.redisDoError != nil && !strings.Contains(err.Error(), tt.redisDoError.Error()) {
 						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)
 			}
 		})
 	}