fix based on reviews

Author: Michael

pkg/common/security/acl.go

@@ -46,12 +46,15 @@ func (a *ACL) setAllAllowed(part string) {
 	a.allAllowed = part == common.Wildcard
 }
 
-// set the user list in the ACL, invalid user names are ignored
-func (a *ACL) setUsers(userList []string) {
+// set the user list in the ACL, invalid user names are ignored.
+// If the silence flag is set to true, the function will not log when setting the users.
+func (a *ACL) setUsers(userList []string, silence bool) {
 	a.users = make(map[string]bool)
 	// special case if the user list is just the wildcard
 	if len(userList) == 1 && userList[0] == common.Wildcard {
-		log.Log(log.Security).Info("user list is wildcard, allowing all access")
+		if !silence {
+			log.Log(log.Security).Info("user list is wildcard, allowing all access")
+		}
 		a.allAllowed = true
 		return
 	}
@@ -64,7 +67,7 @@ func (a *ACL) setUsers(userList []string) {
 		// check the users validity
 		if userNameRegExp.MatchString(user) {
 			a.users[user] = true
-		} else {
+		} else if !silence {
 			log.Log(log.Security).Info("ignoring user in ACL definition",
 				zap.String("user", user))
 		}
@@ -102,7 +105,7 @@ func (a *ACL) setGroups(groupList []string) {
 }
 
 // create a new ACL from scratch
-func NewACL(aclStr string) (ACL, error) {
+func NewACL(aclStr string, silence bool) (ACL, error) {
 	acl := ACL{}
 	if aclStr == "" {
 		return acl, nil
@@ -116,7 +119,7 @@ func NewACL(aclStr string) (ACL, error) {
 	// trim and check for wildcard
 	acl.setAllAllowed(aclStr)
 	// parse users and groups
-	acl.setUsers(strings.Split(fields[0], common.Separator))
+	acl.setUsers(strings.Split(fields[0], common.Separator), silence)
 	if len(fields) == 2 {
 		acl.setGroups(strings.Split(fields[1], common.Separator))
 	}

pkg/common/security/acl_test.go

@@ -155,7 +155,7 @@ func TestACLCreate(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.input, func(t *testing.T) {
-			got, err := NewACL(tt.input)
+			got, err := NewACL(tt.input, false)
 			if err != nil {
 				t.Errorf("parsing failed for string: %s", tt.input)
 			}
@@ -177,7 +177,7 @@ func TestNewACLErrorCase(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.caseName, func(t *testing.T) {
-			if _, err := NewACL(tt.acl); err == nil {
+			if _, err := NewACL(tt.acl, false); err == nil {
 				t.Errorf("parsing %s string should be failed", tt.acl)
 			}
 		})
@@ -238,7 +238,7 @@ func TestACLAccess(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("vistor %v, acl %s", tt.visitor, tt.acl), func(t *testing.T) {
-			acl, err := NewACL(tt.acl)
+			acl, err := NewACL(tt.acl, false)
 			if err != nil {
 				t.Error("the number of space should not be more than 2 because the number of categories only include users and groups")
 			}

pkg/scheduler/objects/queue.go

@@ -128,7 +128,7 @@ func NewConfiguredQueue(conf configs.QueueConfig, parent *Queue, silence bool) (
 	sq.maxRunningApps = conf.MaxApplications
 
 	// update the properties
-	if err := sq.applyConf(conf); err != nil {
+	if err := sq.applyConf(conf, silence); err != nil {
 		return nil, errors.Join(errors.New("configured queue creation failed: "), err)
 	}
 
@@ -312,21 +312,22 @@ func filterParentProperty(key string, value string) string {
 func (sq *Queue) ApplyConf(conf configs.QueueConfig) error {
 	sq.Lock()
 	defer sq.Unlock()
-	return sq.applyConf(conf)
+	return sq.applyConf(conf, false)
 }
 
 // applyConf applies all the properties to the queue from the config.
-// lock free call, must be called holding the queue lock or during create only
-func (sq *Queue) applyConf(conf configs.QueueConfig) error {
+// lock free call, must be called holding the queue lock or during create only.
+// If the silence flag is set to true, the function will not log when setting the users.
+func (sq *Queue) applyConf(conf configs.QueueConfig, silence bool) error {
 	// Set the ACLs
 	var err error
-	sq.submitACL, err = security.NewACL(conf.SubmitACL)
+	sq.submitACL, err = security.NewACL(conf.SubmitACL, silence)
 	if err != nil {
 		log.Log(log.SchedQueue).Error("parsing submit ACL failed this should not happen",
 			zap.Error(err))
 		return err
 	}
-	sq.adminACL, err = security.NewACL(conf.AdminACL)
+	sq.adminACL, err = security.NewACL(conf.AdminACL, silence)
 	if err != nil {
 		log.Log(log.SchedQueue).Error("parsing admin ACL failed this should not happen",
 			zap.Error(err))

pkg/scheduler/objects/queue_test.go

@@ -2340,11 +2340,11 @@ func TestResetRunningState(t *testing.T) {
 	parent.MarkQueueForRemoval()
 	assert.Assert(t, parent.IsDraining(), "parent should be marked as draining")
 	assert.Assert(t, leaf.IsDraining(), "leaf should be marked as draining")
-	err = parent.applyConf(emptyConf)
+	err = parent.applyConf(emptyConf, false)
 	assert.NilError(t, err, "failed to update parent")
 	assert.Assert(t, parent.IsRunning(), "parent should be running again")
 	assert.Assert(t, leaf.IsDraining(), "leaf should still be marked as draining")
-	err = leaf.applyConf(emptyConf)
+	err = leaf.applyConf(emptyConf, false)
 	assert.NilError(t, err, "failed to update leaf")
 	assert.Assert(t, leaf.IsRunning(), "leaf should be running again")
 }

pkg/scheduler/partition.go

@@ -132,7 +132,7 @@ func (pc *PartitionContext) initialPartitionFromConfig(conf configs.PartitionCon
 
 	// We need to pass in the locked version of the GetQueue function.
 	// Placing an application will not have a lock on the partition context.
-	pc.placementManager = placement.NewPlacementManager(conf.PlacementRules, pc.GetQueue)
+	pc.placementManager = placement.NewPlacementManager(conf.PlacementRules, pc.GetQueue, silence)
 	// get the user group cache for the partition
 	pc.userGroupCache = security.GetUserGroupCache("")
 	pc.updateNodeSortingPolicy(conf, silence)

pkg/scheduler/placement/placement.go

@@ -43,11 +43,11 @@ type AppPlacementManager struct {
 	locking.RWMutex
 }
 
-func NewPlacementManager(rules []configs.PlacementRule, queueFunc func(string) *objects.Queue) *AppPlacementManager {
+func NewPlacementManager(rules []configs.PlacementRule, queueFunc func(string) *objects.Queue, silence bool) *AppPlacementManager {
 	m := &AppPlacementManager{
 		queueFn: queueFunc,
 	}
-	if err := m.initialise(rules); err != nil {
+	if err := m.initialise(rules, silence); err != nil {
 		log.Log(log.Config).Error("Placement manager created without rules: not active", zap.Error(err))
 	}
 	return m
@@ -66,25 +66,30 @@ func (m *AppPlacementManager) GetRulesDAO() []*dao.RuleDAO {
 
 // UpdateRules sets the rules for an active placement manager
 func (m *AppPlacementManager) UpdateRules(rules []configs.PlacementRule) error {
-	if err := m.initialise(rules); err != nil {
+	if err := m.initialise(rules, false); err != nil {
 		log.Log(log.Config).Info("Placement manager rules not reloaded", zap.Error(err))
 		return err
 	}
 	return nil
 }
 
 // initialise the rules from a parsed config.
-func (m *AppPlacementManager) initialise(rules []configs.PlacementRule) error {
-	log.Log(log.Config).Info("Building new rule list for placement manager")
+// If the silence flag is set to true, the function will not log.
+func (m *AppPlacementManager) initialise(rules []configs.PlacementRule, silence bool) error {
+	if !silence {
+		log.Log(log.Config).Info("Building new rule list for placement manager")
+	}
 	// build temp list from new config
-	tempRules, err := buildRules(rules)
+	tempRules, err := buildRules(rules, silence)
 	if err != nil {
 		return err
 	}
 	m.Lock()
 	defer m.Unlock()
 
-	log.Log(log.Config).Info("Activated rule set in placement manager")
+	if !silence {
+		log.Log(log.Config).Info("Activated rule set in placement manager")
+	}
 	m.rules = tempRules
 	// all done manager is initialised
 	for rule := range m.rules {
@@ -203,11 +208,14 @@ func (m *AppPlacementManager) PlaceApplication(app *objects.Application) error {
 
 // buildRules builds a new rule set based on the config.
 // If the rule set is correct and can be used the new set is returned.
-// If any error is encountered a nil array is returned and the error set
-func buildRules(rules []configs.PlacementRule) ([]rule, error) {
+// If any error is encountered a nil array is returned and the error set.
+// If the silence flag is set to true, the function will not log.
+func buildRules(rules []configs.PlacementRule, silence bool) ([]rule, error) {
 	// empty list should result in a single "provided" rule
 	if len(rules) == 0 {
-		log.Log(log.Config).Info("Placement manager configured without rules: using implicit provided rule")
+		if !silence {
+			log.Log(log.Config).Info("Placement manager configured without rules: using implicit provided rule")
+		}
 		rules = []configs.PlacementRule{{
 			Name:   types.Provided,
 			Create: false,

pkg/scheduler/placement/placement_test.go

@@ -34,20 +34,20 @@ import (
 // basic test to check if no rules leave the manager unusable
 func TestManagerNew(t *testing.T) {
 	// basic info without rules, manager should not init
-	man := NewPlacementManager(nil, queueFunc)
+	man := NewPlacementManager(nil, queueFunc, false)
 	assert.Equal(t, 2, len(man.rules), "wrong rule count for new placement manager, no config")
 	assert.Equal(t, types.Provided, man.rules[0].getName(), "wrong name for implicit provided rule")
 	assert.Equal(t, types.Recovery, man.rules[1].getName(), "wrong name for implicit recovery rule")
 
 	// fail update rules with unknown rule
 	rules := []configs.PlacementRule{{Name: "unknown", Create: true}}
-	man = NewPlacementManager(rules, queueFunc)
+	man = NewPlacementManager(rules, queueFunc, false)
 	assert.Equal(t, 0, len(man.rules), "wrong rule count for new placement manager, no config")
 }
 
 func TestManagerInit(t *testing.T) {
 	// basic info without rules, manager should implicitly init
-	man := NewPlacementManager(nil, queueFunc)
+	man := NewPlacementManager(nil, queueFunc, false)
 	assert.Equal(t, 2, len(man.rules), "wrong rule count for nil rules config")
 	ruleDAOs := man.GetRulesDAO()
 	assert.Equal(t, 2, len(ruleDAOs), "wrong DAO count for nil rules config")
@@ -56,7 +56,7 @@ func TestManagerInit(t *testing.T) {
 
 	// try to init with empty list should do the same
 	var rules []configs.PlacementRule
-	err := man.initialise(rules)
+	err := man.initialise(rules, false)
 	assert.NilError(t, err, "Failed to initialize empty placement rules")
 	assert.Equal(t, 2, len(man.rules), "wrong rule count for empty rules config")
 	ruleDAOs = man.GetRulesDAO()
@@ -67,7 +67,7 @@ func TestManagerInit(t *testing.T) {
 	rules = []configs.PlacementRule{
 		{Name: "unknown"},
 	}
-	err = man.initialise(rules)
+	err = man.initialise(rules, false)
 	if err == nil {
 		t.Error("initialise with 'unknown' rule list should have failed")
 	}
@@ -81,7 +81,7 @@ func TestManagerInit(t *testing.T) {
 	rules = []configs.PlacementRule{
 		{Name: "test"},
 	}
-	err = man.initialise(rules)
+	err = man.initialise(rules, false)
 	assert.NilError(t, err, "failed to init existing manager")
 	ruleDAOs = man.GetRulesDAO()
 	assert.Equal(t, 2, len(ruleDAOs), "wrong DAO count for manager with test rule")
@@ -90,7 +90,7 @@ func TestManagerInit(t *testing.T) {
 
 	// update the manager: remove rules implicit state is reverted
 	rules = []configs.PlacementRule{}
-	err = man.initialise(rules)
+	err = man.initialise(rules, false)
 	assert.NilError(t, err, "Failed to re-initialize empty placement rules")
 	assert.Equal(t, 2, len(man.rules), "wrong rule count for empty rules config")
 	ruleDAOs = man.GetRulesDAO()
@@ -99,7 +99,7 @@ func TestManagerInit(t *testing.T) {
 	assert.Equal(t, ruleDAOs[1].Name, types.Recovery, "expected recovery rule as second rule")
 
 	// check if we handle a nil list
-	err = man.initialise(nil)
+	err = man.initialise(nil, false)
 	assert.NilError(t, err, "Failed to re-initialize nil placement rules")
 	assert.Equal(t, 2, len(man.rules), "wrong rule count for nil rules config")
 	ruleDAOs = man.GetRulesDAO()
@@ -117,7 +117,7 @@ func TestManagerInit(t *testing.T) {
 			},
 		},
 	}
-	err = man.initialise(rules)
+	err = man.initialise(rules, false)
 	assert.NilError(t, err, "Failed to re-initialize nil placement rules")
 	assert.Equal(t, 2, len(man.rules), "wrong rule count for nil rules config")
 	ruleDAOs = man.GetRulesDAO()
@@ -128,7 +128,7 @@ func TestManagerInit(t *testing.T) {
 
 func TestManagerUpdate(t *testing.T) {
 	// basic info without rules, manager should not init
-	man := NewPlacementManager(nil, queueFunc)
+	man := NewPlacementManager(nil, queueFunc, false)
 	// update the manager
 	rules := []configs.PlacementRule{
 		{Name: "test"},
@@ -172,7 +172,7 @@ func TestManagerBuildRule(t *testing.T) {
 	rules := []configs.PlacementRule{
 		{Name: "test"},
 	}
-	ruleObjs, err := buildRules(rules)
+	ruleObjs, err := buildRules(rules, false)
 	if err != nil {
 		t.Errorf("test rule build should not have failed, err: %v", err)
 	}
@@ -188,7 +188,7 @@ func TestManagerBuildRule(t *testing.T) {
 			},
 		},
 	}
-	ruleObjs, err = buildRules(rules)
+	ruleObjs, err = buildRules(rules, false)
 	if err != nil || len(ruleObjs) != 2 {
 		t.Errorf("test rule build should not have failed and created 2 top level rule, err: %v, rules: %v", err, ruleObjs)
 	} else {
@@ -203,7 +203,7 @@ func TestManagerBuildRule(t *testing.T) {
 		{Name: "user"},
 		{Name: "test"},
 	}
-	ruleObjs, err = buildRules(rules)
+	ruleObjs, err = buildRules(rules, false)
 	if err != nil || len(ruleObjs) != 3 {
 		t.Errorf("rule build should not have failed and created 3 rules, err: %v, rules: %v", err, ruleObjs)
 	} else if ruleObjs[0].getName() != rules[0].Name || ruleObjs[1].getName() != rules[1].Name || ruleObjs[2].getName() != "recovery" {
@@ -230,7 +230,7 @@ partitions:
 	err := initQueueStructure([]byte(data))
 	assert.NilError(t, err, "setting up the queue config failed")
 	// basic info without rules, manager should init
-	man := NewPlacementManager(nil, queueFunc)
+	man := NewPlacementManager(nil, queueFunc, false)
 	if man == nil {
 		t.Fatal("placement manager create failed")
 	}
@@ -362,7 +362,7 @@ partitions:
 			Value:  "namespace",
 			Create: true},
 	}
-	man := NewPlacementManager(rules, queueFunc)
+	man := NewPlacementManager(rules, queueFunc, false)
 	if man == nil {
 		t.Fatal("placement manager create failed")
 	}
@@ -503,7 +503,7 @@ partitions:
 			Value:  "root.custom",
 			Create: true},
 	}
-	man1 := NewPlacementManager(rules, queueFunc)
+	man1 := NewPlacementManager(rules, queueFunc, false)
 	if man1 == nil {
 		t.Fatal("placement manager create failed")
 	}
@@ -559,7 +559,7 @@ partitions:
 	err := initQueueStructure([]byte(data))
 	assert.NilError(t, err, "setting up the queue config failed")
 	// basic info without rules, manager should init
-	man := NewPlacementManager(nil, queueFunc)
+	man := NewPlacementManager(nil, queueFunc, false)
 	if man == nil {
 		t.Fatal("placement manager create failed")
 	}