You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@yunikorn.apache.org by ma...@apache.org on 2024/06/25 10:35:54 UTC
(yunikorn-core) branch master updated: [YUNIKORN-2657] Validate queue generated as part of the placement rules (#891)
This is an automated email from the ASF dual-hosted git repository.
mani pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
The following commit(s) were added to refs/heads/master by this push:
new ff2a881a [YUNIKORN-2657] Validate queue generated as part of the placement rules (#891)
ff2a881a is described below
commit ff2a881af8165518bf9493af2940bfca4fb1f83f
Author: Manikandan R <ma...@gmail.com>
AuthorDate: Tue Jun 25 16:05:33 2024 +0530
[YUNIKORN-2657] Validate queue generated as part of the placement rules (#891)
Closes: #891
Signed-off-by: Manikandan R <ma...@gmail.com>
---
pkg/common/configs/configvalidator.go | 15 ++-
pkg/common/configs/configvalidator_test.go | 16 ++-
pkg/common/errors.go | 27 +++++
pkg/scheduler/objects/queue.go | 7 +-
pkg/scheduler/objects/queue_test.go | 8 +-
pkg/scheduler/placement/fixed_rule.go | 8 +-
pkg/scheduler/placement/fixed_rule_test.go | 144 ++++++++++----------------
pkg/scheduler/placement/provided_rule.go | 19 +++-
pkg/scheduler/placement/provided_rule_test.go | 42 ++++++++
pkg/scheduler/placement/tag_rule.go | 20 +++-
pkg/scheduler/placement/tag_rule_test.go | 23 ++++
pkg/scheduler/placement/testrule.go | 7 ++
pkg/scheduler/placement/testrule_test.go | 38 +++++++
pkg/scheduler/placement/user_rule.go | 6 +-
pkg/scheduler/placement/user_rule_test.go | 142 +++++++++----------------
pkg/webservice/handlers.go | 6 +-
pkg/webservice/handlers_test.go | 15 ++-
17 files changed, 328 insertions(+), 215 deletions(-)
diff --git a/pkg/common/configs/configvalidator.go b/pkg/common/configs/configvalidator.go
index 0f3a774b..87003260 100644
--- a/pkg/common/configs/configvalidator.go
+++ b/pkg/common/configs/configvalidator.go
@@ -69,7 +69,7 @@ var DefaultPreemptionDelay = 30 * time.Second
// A queue can be a username with the dot replaced. Most systems allow a 32 character user name.
// The queue name must thus allow for at least that length with the replacement of dots.
-var QueueNameRegExp = regexp.MustCompile(`^[a-zA-Z0-9_-]{1,64}$`)
+var QueueNameRegExp = regexp.MustCompile(`^[a-zA-Z0-9_:#/@-]{1,64}$`)
// User and group name check: systems allow different things POSIX is the base but we need to be lenient and allow more.
// allow upper and lower case, add the @ and . (dot) and officially no length.
@@ -653,9 +653,9 @@ func checkQueues(queue *QueueConfig, level int) error {
// check this level for name compliance and uniqueness
queueMap := make(map[string]bool)
for _, child := range queue.Queues {
- if !QueueNameRegExp.MatchString(child.Name) {
- return fmt.Errorf("invalid child name '%s', a name must only have alphanumeric characters,"+
- " - or _, and be no longer than 64 characters", child.Name)
+ err = IsQueueNameValid(child.Name)
+ if err != nil {
+ return err
}
if queueMap[strings.ToLower(child.Name)] {
return fmt.Errorf("duplicate child name found with name '%s', level %d", child.Name, level)
@@ -673,6 +673,13 @@ func checkQueues(queue *QueueConfig, level int) error {
return nil
}
+func IsQueueNameValid(queueName string) error {
+ if !QueueNameRegExp.MatchString(queueName) {
+ return common.InvalidQueueName
+ }
+ return nil
+}
+
// Check the structure of the queue in the config:
// - exactly 1 root queue, added if missing
// - the parent flag is set on queues that are missing it
diff --git a/pkg/common/configs/configvalidator_test.go b/pkg/common/configs/configvalidator_test.go
index da7263ec..4a75ece5 100644
--- a/pkg/common/configs/configvalidator_test.go
+++ b/pkg/common/configs/configvalidator_test.go
@@ -2295,7 +2295,7 @@ func TestCheckQueues(t *testing.T) { //nolint:funlen
},
},
level: 0,
- expectedErrorMsg: "invalid child name 'thisQueueNameIsTooLongthisQueueNameIsTooLongthisQueueNameIsTooLong', a name must only have alphanumeric characters, - or _, and be no longer than 64 characters",
+ expectedErrorMsg: common.InvalidQueueName.Error(),
},
{
name: "Invalid Child Queue Name With Special Character",
@@ -2308,7 +2308,7 @@ func TestCheckQueues(t *testing.T) { //nolint:funlen
},
},
level: 0,
- expectedErrorMsg: "invalid child name 'queue_Name$', a name must only have alphanumeric characters, - or _, and be no longer than 64 characters",
+ expectedErrorMsg: common.InvalidQueueName.Error(),
},
{
name: "Valid Multiple Queues",
@@ -2414,3 +2414,15 @@ func TestCheckNodeSortingPolicy(t *testing.T) { //nolint:funlen
})
}
}
+
+func TestIsQueueNameValid(t *testing.T) {
+ assert.NilError(t, IsQueueNameValid("parent_Child_test-a_b_#_c_#_d_/_e@dom:ain"))
+ err := IsQueueNameValid("invalid!queue")
+ if err == nil {
+ t.Errorf("invalid queue name, validation should have failed. err is %v", err)
+ }
+ err = IsQueueNameValid("root.parent")
+ if err == nil {
+ t.Errorf("invalid queue name, validation should have failed. err is %v", err)
+ }
+}
diff --git a/pkg/common/errors.go b/pkg/common/errors.go
new file mode 100644
index 00000000..27ca0c74
--- /dev/null
+++ b/pkg/common/errors.go
@@ -0,0 +1,27 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package common
+
+import "errors"
+
+// Common errors
+var (
+ // InvalidQueueName returned when queue name is invalid
+ InvalidQueueName = errors.New("invalid queue name, max 64 characters consisting of alphanumeric characters and '-', '_', '#', '@', '/', ':' allowed")
+)
diff --git a/pkg/scheduler/objects/queue.go b/pkg/scheduler/objects/queue.go
index 4a168926..9ae65ce1 100644
--- a/pkg/scheduler/objects/queue.go
+++ b/pkg/scheduler/objects/queue.go
@@ -177,8 +177,11 @@ func NewDynamicQueue(name string, leaf bool, parent *Queue) (*Queue, error) {
return nil, fmt.Errorf("dynamic queue can not be added without parent: %s", name)
}
// name might not be checked do it here
- if !configs.QueueNameRegExp.MatchString(name) {
- return nil, fmt.Errorf("invalid queue name '%s', a name must only have alphanumeric characters, - or _, and be no longer than 64 characters", name)
+ if err := configs.IsQueueNameValid(name); err != nil {
+ return nil, err
+ }
+ if name == common.RecoveryQueue {
+ return nil, fmt.Errorf("dynamic queue cannot be root.@recovery@")
}
return newDynamicQueueInternal(name, leaf, parent)
}
diff --git a/pkg/scheduler/objects/queue_test.go b/pkg/scheduler/objects/queue_test.go
index 1b2c4d8c..318b4771 100644
--- a/pkg/scheduler/objects/queue_test.go
+++ b/pkg/scheduler/objects/queue_test.go
@@ -2192,7 +2192,7 @@ func TestNewDynamicQueue(t *testing.T) {
assert.Equal(t, childLeaf.preemptionPolicy, policies.DefaultPreemptionPolicy)
// case 1: non-leaf can't use template but it can inherit template from parent
- childNonLeaf, err := NewDynamicQueue("nonleaf", false, parent)
+ childNonLeaf, err := NewDynamicQueue("nonleaf_Test-a_b_#_c_#_d_/_e@dom:ain", false, parent)
assert.NilError(t, err, "failed to create dynamic queue: %v", err)
assert.Assert(t, reflect.DeepEqual(childNonLeaf.template, parent.template))
assert.Equal(t, len(childNonLeaf.properties), 0)
@@ -2201,6 +2201,12 @@ func TestNewDynamicQueue(t *testing.T) {
assert.Assert(t, childNonLeaf.prioritySortEnabled)
assert.Equal(t, childNonLeaf.priorityPolicy, policies.DefaultPriorityPolicy)
assert.Equal(t, childNonLeaf.preemptionPolicy, policies.DefaultPreemptionPolicy)
+
+ // case 2: invalid queue name
+ _, err = NewDynamicQueue("invalid!queue", false, parent)
+ if err == nil {
+ t.Errorf("new dynamic queue should have failed to create, err is %v", err)
+ }
}
func TestTemplateIsNotOverrideByParent(t *testing.T) {
diff --git a/pkg/scheduler/placement/fixed_rule.go b/pkg/scheduler/placement/fixed_rule.go
index 3e2e9216..a12e021c 100644
--- a/pkg/scheduler/placement/fixed_rule.go
+++ b/pkg/scheduler/placement/fixed_rule.go
@@ -68,6 +68,12 @@ func (fr *fixedRule) initialise(conf configs.PlacementRule) error {
if fr.queue == "" {
return fmt.Errorf("a fixed queue rule must have a queue name set")
}
+ parts := strings.Split(fr.queue, configs.DOT)
+ for _, part := range parts {
+ if err := configs.IsQueueNameValid(part); err != nil {
+ return err
+ }
+ }
fr.create = conf.Create
fr.filter = newFilter(conf.Filter)
// if we have a fully qualified queue name already we should not have a parent
@@ -92,7 +98,7 @@ func (fr *fixedRule) placeApplication(app *objects.Application, queueFn func(str
return "", nil
}
queueName := fr.queue
- // if the fixed queue is already fully qualified skip the parent check
+ // not fully qualified queue, run the parent rule if set
if !fr.qualified {
var parentName string
var err error
diff --git a/pkg/scheduler/placement/fixed_rule_test.go b/pkg/scheduler/placement/fixed_rule_test.go
index ce45c6d4..2e68ac6d 100644
--- a/pkg/scheduler/placement/fixed_rule_test.go
+++ b/pkg/scheduler/placement/fixed_rule_test.go
@@ -81,98 +81,46 @@ partitions:
tags := make(map[string]string)
app := newApplication("app1", "default", "ignored", user, tags, nil, "")
- // fixed queue that exists directly under the root
- conf := configs.PlacementRule{
- Name: "fixed",
- Value: "testqueue",
- }
- var fr rule
- fr, err = newRule(conf)
- if err != nil || fr == nil {
- t.Errorf("fixed rule create failed with queue name, err %v", err)
- }
- var queue string
- queue, err = fr.placeApplication(app, queueFunc)
- if queue != "root.testqueue" || err != nil {
- t.Errorf("fixed rule failed to place queue in correct queue '%s', err %v", queue, err)
- }
-
- // fixed queue that exists directly in hierarchy
- conf = configs.PlacementRule{
- Name: "fixed",
- Value: "root.testparent.testchild",
- }
- fr, err = newRule(conf)
- if err != nil || fr == nil {
- t.Errorf("fixed rule create failed with queue name, err %v", err)
- }
- queue, err = fr.placeApplication(app, queueFunc)
- if queue != "root.testparent.testchild" || err != nil {
- t.Errorf("fixed rule failed to place queue in correct queue '%s', err %v", queue, err)
- }
-
- // fixed queue that does not exists
- conf = configs.PlacementRule{
- Name: "fixed",
- Value: "newqueue",
- Create: true,
- }
- fr, err = newRule(conf)
- if err != nil || fr == nil {
- t.Errorf("fixed rule create failed with queue name, err %v", err)
- }
- queue, err = fr.placeApplication(app, queueFunc)
- if queue != "root.newqueue" || err != nil {
- t.Errorf("fixed rule failed to place queue in to be created queue '%s', err %v", queue, err)
- }
-
- // trying to place in a parent queue should not fail: failure happens on create in this case
- conf = configs.PlacementRule{
- Name: "fixed",
- Value: "root.testparent",
- }
- fr, err = newRule(conf)
- if err != nil || fr == nil {
- t.Errorf("fixed rule create failed with queue name, err %v", err)
- }
- queue, err = fr.placeApplication(app, queueFunc)
- if queue != "root.testparent" || err != nil {
- t.Errorf("fixed rule did fail with parent queue '%s', error %v", queue, err)
- }
-
- // trying to place in a child using a parent
- conf = configs.PlacementRule{
- Name: "fixed",
- Value: "testchild",
- Parent: &configs.PlacementRule{
- Name: "fixed",
- Value: "testparent",
- },
- }
- fr, err = newRule(conf)
- if err != nil || fr == nil {
- t.Errorf("fixed rule create failed with queue name, err %v", err)
- }
- queue, err = fr.placeApplication(app, queueFunc)
- if queue != "root.testparent.testchild" || err != nil {
- t.Errorf("fixed rule with parent queue should not have failed '%s', error %v", queue, err)
+ var tests = []struct {
+ name string
+ expectedQueue string
+ config configs.PlacementRule
+ nilError bool
+ }{
+ {"fixed queue that exists directly under the root", "root.testqueue", configs.PlacementRule{Name: "fixed", Value: "testqueue"}, true},
+ {"fixed queue that exists directly in hierarchy", "root.testparent.testchild", configs.PlacementRule{Name: "fixed", Value: "testparent.testchild"}, true},
+ {"fixed queue that does not exists", "root.newqueue", configs.PlacementRule{Name: "fixed", Value: "newqueue", Create: true}, true},
+ {"place in a parent queue should not fail: failure happens on create in this case", "root.testparent", configs.PlacementRule{Name: "fixed", Value: "root.testparent"}, true},
+ {"place in a child using a parent", "root.testparent.testchild", configs.PlacementRule{Name: "fixed", Value: "testchild", Parent: &configs.PlacementRule{Name: "fixed", Value: "testparent"}}, true},
+ {"invalid queue name", "", configs.PlacementRule{Name: "fixed", Value: "testqueue!>invalid<"}, false},
+ {"invalid queue name with full queue hierarchy", "", configs.PlacementRule{Name: "fixed", Value: "root.testparent!>invalid<test.testqueue"}, false},
+ {"deny filter type should got empty queue", "", configs.PlacementRule{Name: "fixed", Value: "testchild", Filter: configs.Filter{Type: filterDeny}}, true},
}
- // deny filter type should got got empty queue
- conf = configs.PlacementRule{
- Name: "fixed",
- Value: "testchild",
- Filter: configs.Filter{
- Type: filterDeny,
- },
- }
- fr, err = newRule(conf)
- if err != nil || fr == nil {
- t.Errorf("fixed rule create failed with queue name, err %v", err)
- }
- queue, err = fr.placeApplication(app, queueFunc)
- if queue != "" || err != nil {
- t.Errorf("fixed rule with deny filter type should got empty queue, err nil")
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var fr rule
+ fr, err = newRule(tt.config)
+ if tt.nilError {
+ if err != nil || fr == nil {
+ t.Errorf("fixed rule create failed with queue name, err %v", err)
+ }
+ var queue string
+ if tt.nilError {
+ queue, err = fr.placeApplication(app, queueFunc)
+ if queue != tt.expectedQueue || err != nil {
+ t.Errorf("fixed rule failed to place queue in correct queue '%s', err %v", queue, err)
+ }
+ } else {
+ _, err = fr.placeApplication(app, queueFunc)
+ if err == nil {
+ t.Errorf("fixed rule should have failed to place queue, err %v", err)
+ }
+ }
+ } else if err == nil {
+ t.Errorf("fixed rule should have failed with queue name, err %v", err)
+ }
+ })
}
}
@@ -248,6 +196,22 @@ func TestFixedRuleParent(t *testing.T) {
t.Errorf("fixed rule with non existing parent queue should created '%s', error %v", queue, err)
}
+ // trying to place in invalid child using a creatable parent
+ conf = configs.PlacementRule{
+ Name: "fixed",
+ Value: "testchild",
+ Create: true,
+ Parent: &configs.PlacementRule{
+ Name: "fixed",
+ Value: "test!invalid<tes>t",
+ Create: true,
+ },
+ }
+ _, err = newRule(conf)
+ if err == nil {
+ t.Errorf("fixed rule create should have failed with queue name, err %v", err)
+ }
+
// trying to place in a child using a parent which is defined as a leaf
conf = configs.PlacementRule{
Name: "fixed",
diff --git a/pkg/scheduler/placement/provided_rule.go b/pkg/scheduler/placement/provided_rule.go
index 834ff83c..fc911055 100644
--- a/pkg/scheduler/placement/provided_rule.go
+++ b/pkg/scheduler/placement/provided_rule.go
@@ -85,8 +85,21 @@ func (pr *providedRule) placeApplication(app *objects.Application, queueFn func(
}
var parentName string
var err error
- // if we have a fully qualified queue passed in do not run the parent rule
- if !strings.HasPrefix(queueName, configs.RootQueue+configs.DOT) {
+
+ // fully qualified queue, do not run the parent rule
+ if strings.HasPrefix(queueName, configs.RootQueue+configs.DOT) {
+ parts := strings.Split(queueName, configs.DOT)
+ for _, part := range parts {
+ if err = configs.IsQueueNameValid(part); err != nil {
+ return "", err
+ }
+ }
+ } else {
+ // not fully qualified queue
+ childQueueName := replaceDot(queueName)
+ if err = configs.IsQueueNameValid(childQueueName); err != nil {
+ return "", err
+ }
// run the parent rule if set
if pr.parent != nil {
parentName, err = pr.parent.placeApplication(app, queueFn)
@@ -113,7 +126,7 @@ func (pr *providedRule) placeApplication(app *objects.Application, queueFn func(
parentName = configs.RootQueue
}
// Make it a fully qualified queue
- queueName = parentName + configs.DOT + replaceDot(queueName)
+ queueName = parentName + configs.DOT + childQueueName
}
// Log the result before we check the create flag
log.Log(log.SchedApplication).Debug("Provided rule intermediate result",
diff --git a/pkg/scheduler/placement/provided_rule_test.go b/pkg/scheduler/placement/provided_rule_test.go
index 1a14d8f1..ee85e2f0 100644
--- a/pkg/scheduler/placement/provided_rule_test.go
+++ b/pkg/scheduler/placement/provided_rule_test.go
@@ -88,6 +88,21 @@ partitions:
t.Errorf("provided rule placed app in incorrect queue '%s', error %v", queue, err)
}
+ // trying to place in invalid queue
+ appInfo = newApplication("app1", "default", "root.unkno!wn", user, tags, nil, "")
+ conf = configs.PlacementRule{
+ Name: "provided",
+ Create: true,
+ }
+ pr, err = newRule(conf)
+ if err != nil || pr == nil {
+ t.Errorf("provided rule create failed, err %v", err)
+ }
+ _, err = pr.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("provided rule should have failed to place app, error %v", err)
+ }
+
conf = configs.PlacementRule{
Name: "provided",
Parent: &configs.PlacementRule{
@@ -114,6 +129,13 @@ partitions:
if queue != "root.testparent" || err != nil {
t.Errorf("provided rule placed in to be created queue with create false '%s', err %v", queue, err)
}
+
+ // invalid queue with parent rule (parent rule ignored)
+ appInfo = newApplication("app1", "default", "root.testp!arent", user, tags, nil, "")
+ _, err = pr.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("provided rule should have failed to place app, error %v", err)
+ }
}
func TestProvidedRuleParent(t *testing.T) {
@@ -188,6 +210,26 @@ func TestProvidedRuleParent(t *testing.T) {
t.Errorf("provided rule with non existing parent queue should create '%s', error %v", queue, err)
}
+ // trying to place in invalid queue using a creatable parent
+ conf = configs.PlacementRule{
+ Name: "provided",
+ Create: true,
+ Parent: &configs.PlacementRule{
+ Name: "fixed",
+ Value: "testparentnew",
+ Create: true,
+ },
+ }
+ pr, err = newRule(conf)
+ if err != nil || pr == nil {
+ t.Errorf("provided rule create failed, err %v", err)
+ }
+ appInfo = newApplication("app1", "default", "testc!hild", user, tags, nil, "")
+ _, err = pr.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("provided rule with non existing parent invalid queue should have failed to create, error %v", err)
+ }
+
// trying to place in a child using a parent which is defined as a leaf
conf = configs.PlacementRule{
Name: "provided",
diff --git a/pkg/scheduler/placement/tag_rule.go b/pkg/scheduler/placement/tag_rule.go
index 6bc54b42..4364548d 100644
--- a/pkg/scheduler/placement/tag_rule.go
+++ b/pkg/scheduler/placement/tag_rule.go
@@ -92,8 +92,20 @@ func (tr *tagRule) placeApplication(app *objects.Application, queueFn func(strin
var parentName string
var err error
queueName := tagVal
- // if we have a fully qualified queue in the value do not run the parent rule
- if !strings.HasPrefix(queueName, configs.RootQueue+configs.DOT) {
+ // fully qualified queue, do not run the parent rule
+ if strings.HasPrefix(queueName, configs.RootQueue+configs.DOT) {
+ parts := strings.Split(queueName, configs.DOT)
+ for _, part := range parts {
+ if err = configs.IsQueueNameValid(part); err != nil {
+ return "", err
+ }
+ }
+ } else {
+ // not fully qualified queue
+ childQueueName := replaceDot(tagVal)
+ if err = configs.IsQueueNameValid(childQueueName); err != nil {
+ return "", err
+ }
// run the parent rule if set
if tr.parent != nil {
parentName, err = tr.parent.placeApplication(app, queueFn)
@@ -119,10 +131,10 @@ func (tr *tagRule) placeApplication(app *objects.Application, queueFn func(strin
if parentName == "" {
parentName = configs.RootQueue
}
- queueName = parentName + configs.DOT + replaceDot(tagVal)
+ queueName = parentName + configs.DOT + childQueueName
}
// Log the result before we check the create flag
- log.Log(log.SchedApplication).Debug("Tag rule intermediate result",
+ log.Log(log.SchedApplication).Info("Tag rule intermediate result",
zap.String("application", app.ApplicationID),
zap.String("queue", queueName))
// get the queue object
diff --git a/pkg/scheduler/placement/tag_rule_test.go b/pkg/scheduler/placement/tag_rule_test.go
index 1c0e4909..6dff1a2b 100644
--- a/pkg/scheduler/placement/tag_rule_test.go
+++ b/pkg/scheduler/placement/tag_rule_test.go
@@ -104,6 +104,14 @@ partitions:
t.Errorf("tag rule failed to place queue in correct queue '%s', err %v", queue, err)
}
+ // tag invalid queue
+ tags = map[string]string{"label1": "test!queue"}
+ appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
+ _, err = tr.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("tag rule should have failed to place app, err %v", err)
+ }
+
// tag queue that does not exists
tags = map[string]string{"label1": "unknown"}
appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
@@ -120,6 +128,14 @@ partitions:
t.Errorf("tag rule did fail with qualified queue '%s', error %v", queue, err)
}
+ // tag invalid queue fully qualified
+ tags = map[string]string{"label1": "root.testparent.test!child"}
+ appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
+ _, err = tr.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("tag rule should have failed with fully qualified invalid queue, error %v", err)
+ }
+
// tag queue references recovery
tags = map[string]string{"label1": common.RecoveryQueueFull}
appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
@@ -154,6 +170,13 @@ partitions:
t.Errorf("tag rule with parent queue incorrect queue '%s', error %v", queue, err)
}
+ tags = map[string]string{"label1": "testchild", "label2": "testp!arent"}
+ appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
+ _, err = tr.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("tag rule with parent queue should have failed, error %v", err)
+ }
+
// deny filter type should got got empty queue
conf = configs.PlacementRule{
Name: "tag",
diff --git a/pkg/scheduler/placement/testrule.go b/pkg/scheduler/placement/testrule.go
index f06a145e..e27a6581 100644
--- a/pkg/scheduler/placement/testrule.go
+++ b/pkg/scheduler/placement/testrule.go
@@ -21,6 +21,7 @@ package placement
import (
"fmt"
"strconv"
+ "strings"
"github.com/apache/yunikorn-core/pkg/common/configs"
"github.com/apache/yunikorn-core/pkg/scheduler/objects"
@@ -70,6 +71,12 @@ func (tr *testRule) placeApplication(app *objects.Application, queueFn func(stri
return "", fmt.Errorf("nil app passed in")
}
if queuePath := app.GetQueuePath(); queuePath != "" {
+ parts := strings.Split(queuePath, configs.DOT)
+ for _, part := range parts {
+ if err := configs.IsQueueNameValid(part); err != nil {
+ return "", err
+ }
+ }
return replaceDot(queuePath), nil
}
return types.Test, nil
diff --git a/pkg/scheduler/placement/testrule_test.go b/pkg/scheduler/placement/testrule_test.go
index 990022fd..38453b31 100644
--- a/pkg/scheduler/placement/testrule_test.go
+++ b/pkg/scheduler/placement/testrule_test.go
@@ -20,6 +20,9 @@ package placement
import (
"strings"
+ "testing"
+
+ "gotest.tools/v3/assert"
"github.com/apache/yunikorn-core/pkg/common/configs"
"github.com/apache/yunikorn-core/pkg/common/security"
@@ -97,3 +100,38 @@ func newApplication(appID, partition, queueName string, ugi security.UserGroup,
}
return objects.NewApplication(siApp, ugi, eventHandler, rmID)
}
+
+func TestTestRulePlace(t *testing.T) {
+ // Create the structure for the test
+ data := `
+partitions:
+ - name: default
+ queues:
+ - name: testparent
+ queues:
+ - name: testchild
+`
+ err := initQueueStructure([]byte(data))
+ assert.NilError(t, err, "setting up the queue config failed")
+
+ tags := make(map[string]string)
+ user := security.UserGroup{
+ User: "test",
+ Groups: []string{},
+ }
+
+ conf := configs.PlacementRule{
+ Name: "test",
+ }
+ var pr rule
+ pr, err = newRule(conf)
+ if err != nil || pr == nil {
+ t.Errorf("test rule create failed, err %v", err)
+ }
+ appInfo := newApplication("app1", "default", "testchild", user, tags, nil, "")
+ var queue string
+ queue, err = pr.placeApplication(appInfo, queueFunc)
+ if queue != "testchild" || err != nil {
+ t.Errorf("test rule placed app in incorrect queue '%s', err %v", queue, err)
+ }
+}
diff --git a/pkg/scheduler/placement/user_rule.go b/pkg/scheduler/placement/user_rule.go
index 1e1f914d..21ff9b95 100644
--- a/pkg/scheduler/placement/user_rule.go
+++ b/pkg/scheduler/placement/user_rule.go
@@ -75,6 +75,10 @@ func (ur *userRule) placeApplication(app *objects.Application, queueFn func(stri
zap.Any("user", app.GetUser()))
return "", nil
}
+ childQueueName := replaceDot(userName)
+ if err := configs.IsQueueNameValid(childQueueName); err != nil {
+ return "", err
+ }
var parentName string
var err error
// run the parent rule if set
@@ -102,7 +106,7 @@ func (ur *userRule) placeApplication(app *objects.Application, queueFn func(stri
if parentName == "" {
parentName = configs.RootQueue
}
- queueName := parentName + configs.DOT + replaceDot(userName)
+ queueName := parentName + configs.DOT + childQueueName
// Log the result before we check the create flag
log.Log(log.SchedApplication).Debug("User rule intermediate result",
zap.String("application", app.ApplicationID),
diff --git a/pkg/scheduler/placement/user_rule_test.go b/pkg/scheduler/placement/user_rule_test.go
index b8d04048..1b686029 100644
--- a/pkg/scheduler/placement/user_rule_test.go
+++ b/pkg/scheduler/placement/user_rule_test.go
@@ -44,104 +44,46 @@ partitions:
assert.NilError(t, err, "setting up the queue config failed")
tags := make(map[string]string)
- user := security.UserGroup{
- User: "testchild",
- Groups: []string{},
- }
- appInfo := newApplication("app1", "default", "ignored", user, tags, nil, "")
- // user queue that exists directly under the root
- conf := configs.PlacementRule{
- Name: "user",
- }
- var ur rule
- ur, err = newRule(conf)
- if err != nil || ur == nil {
- t.Errorf("user rule create failed, err %v", err)
- }
- var queue string
- queue, err = ur.placeApplication(appInfo, queueFunc)
- if queue != "root.testchild" || err != nil {
- t.Errorf("user rule failed to place queue in correct queue '%s', err %v", queue, err)
- }
- // trying to place in a parent queue should fail on queue create not in the rule
- user = security.UserGroup{
- User: "testparent",
- Groups: []string{},
- }
- appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
- queue, err = ur.placeApplication(appInfo, queueFunc)
- if queue != "root.testparent" || err != nil {
- t.Errorf("user rule failed with parent queue '%s', error %v", queue, err)
- }
-
- user = security.UserGroup{
- User: "test.user",
- Groups: []string{},
- }
- appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
- queue, err = ur.placeApplication(appInfo, queueFunc)
- if queue == "" || err != nil {
- t.Errorf("user rule with dotted user should not have failed '%s', error %v", queue, err)
- }
-
- // user queue that exists directly in hierarchy
- conf = configs.PlacementRule{
- Name: "user",
- Parent: &configs.PlacementRule{
- Name: "fixed",
- Value: "testparent",
- },
- }
- user = security.UserGroup{
- User: "testchild",
- Groups: []string{},
- }
- appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
- ur, err = newRule(conf)
- if err != nil || ur == nil {
- t.Errorf("user rule create failed with queue name, err %v", err)
- }
- queue, err = ur.placeApplication(appInfo, queueFunc)
- if queue != "root.testparent.testchild" || err != nil {
- t.Errorf("user rule failed to place queue in correct queue '%s', err %v", queue, err)
- }
-
- // user queue that does not exists
- user = security.UserGroup{
- User: "unknown",
- Groups: []string{},
- }
- appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
-
- conf = configs.PlacementRule{
- Name: "user",
- Create: true,
- }
- ur, err = newRule(conf)
- if err != nil || ur == nil {
- t.Errorf("user rule create failed with queue name, err %v", err)
- }
- queue, err = ur.placeApplication(appInfo, queueFunc)
- if queue != "root.unknown" || err != nil {
- t.Errorf("user rule placed in to be created queue with create false '%s', err %v", queue, err)
+ var tests = []struct {
+ name string
+ user security.UserGroup
+ expectedQueue string
+ config configs.PlacementRule
+ nilError bool
+ }{
+ {"user queue that exists directly under the root", security.UserGroup{User: "testchild", Groups: []string{}}, "root.testchild", configs.PlacementRule{Name: "user"}, true},
+ {"trying to place in a parent queue should fail on queue create not in the rule", security.UserGroup{User: "testparent", Groups: []string{}}, "root.testparent", configs.PlacementRule{Name: "user"}, true},
+ {"user rule with dotted user should not have failed", security.UserGroup{User: "test.user", Groups: []string{}}, "root.test_dot_user", configs.PlacementRule{Name: "user"}, true},
+ {"user queue that exists directly in hierarchy", security.UserGroup{User: "testchild", Groups: []string{}}, "root.testparent.testchild", configs.PlacementRule{Name: "user", Parent: &configs.PlacementRule{Name: "fixed", Value: "testparent"}}, true},
+ {"user queue that does not exists", security.UserGroup{User: "unknown", Groups: []string{}}, "root.unknown", configs.PlacementRule{Name: "user", Create: true}, true},
+ {"user queue with oidc supported characters", security.UserGroup{User: "test.user@gmail.com", Groups: []string{}}, "root.test_dot_user@gmail_dot_com", configs.PlacementRule{Name: "user", Create: true}, true},
+ {"user queue with oidc supported characters", security.UserGroup{User: "http://domain.com/server1/testuser@cloudera.com", Groups: []string{}}, "root.http://domain_dot_com/server1/testuser@cloudera_dot_com", configs.PlacementRule{Name: "user", Create: true}, true},
+ {"invalid queue name", security.UserGroup{User: "invalid!us>er", Groups: []string{}}, "root.http://domain_dot_com/server1/testuser@cloudera_dot_com", configs.PlacementRule{Name: "user", Create: true}, false},
+ {"deny filter type should got empty queue", security.UserGroup{User: "unknown", Groups: []string{}}, "", configs.PlacementRule{Name: "user", Filter: configs.Filter{Type: filterDeny}}, true},
}
- // deny filter type should got got empty queue
- conf = configs.PlacementRule{
- Name: "user",
- Filter: configs.Filter{
- Type: filterDeny,
- },
- }
- ur, err = newRule(conf)
- if err != nil || ur == nil {
- t.Errorf("user rule create failed with queue name, err %v", err)
- }
- appInfo = newApplication("app1", "default", "ignored", user, tags, nil, "")
- queue, err = ur.placeApplication(appInfo, queueFunc)
- if queue != "" || err != nil {
- t.Errorf("user rule with deny filter type should got empty queue, err nil")
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var ur rule
+ ur, err = newRule(tt.config)
+ if err != nil || ur == nil {
+ t.Errorf("user rule create failed, err %v", err)
+ }
+ appInfo := newApplication("app1", "default", "ignored", tt.user, tags, nil, "")
+ var queue string
+ if tt.nilError {
+ queue, err = ur.placeApplication(appInfo, queueFunc)
+ if queue != tt.expectedQueue || err != nil {
+ t.Errorf("user rule failed to place queue in correct queue '%s', err %v", queue, err)
+ }
+ } else {
+ _, err = ur.placeApplication(appInfo, queueFunc)
+ if err == nil {
+ t.Errorf("user rule should have failed to place queue, err %v", err)
+ }
+ }
+ })
}
}
@@ -217,6 +159,16 @@ func TestUserRuleParent(t *testing.T) {
t.Errorf("user rule with non existing parent queue should create '%s', error %v", queue, err)
}
+ user1 := security.UserGroup{
+ User: "test!child",
+ Groups: []string{},
+ }
+ appInfo1 := newApplication("app1", "default", "unknown", user1, tags, nil, "")
+ _, err = ur.placeApplication(appInfo1, queueFunc)
+ if err == nil {
+ t.Errorf("user rule with non existing parent queue and invalid child queue should have failed, error %v", err)
+ }
+
// trying to place in a child using a parent which is defined as a leaf
conf = configs.PlacementRule{
Name: "user",
diff --git a/pkg/webservice/handlers.go b/pkg/webservice/handlers.go
index 6bb031b0..a66614dc 100644
--- a/pkg/webservice/handlers.go
+++ b/pkg/webservice/handlers.go
@@ -135,10 +135,8 @@ func validateQueue(queuePath string) error {
if queuePath != "" {
queueNameArr := strings.Split(queuePath, ".")
for _, name := range queueNameArr {
- if !configs.QueueNameRegExp.MatchString(name) && name != common.RecoveryQueue {
- return fmt.Errorf("problem in queue query parameter parsing as queue param "+
- "%s contains invalid queue name %s. Queue name must only have "+
- "alphanumeric characters, - or _, and be no longer than 64 characters except the recovery queue root.@recovery@", queuePath, name)
+ if err := configs.IsQueueNameValid(name); err != nil {
+ return err
}
}
}
diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go
index 3e72843a..a49df1a6 100644
--- a/pkg/webservice/handlers_test.go
+++ b/pkg/webservice/handlers_test.go
@@ -1241,11 +1241,11 @@ func TestGetPartitionQueuesHandler(t *testing.T) {
// test invalid queue name
req, err = http.NewRequest("GET", "/ws/v1/partition/default/queue/root.a", strings.NewReader(""))
assert.NilError(t, err, "HTTP request create failed")
- req = req.WithContext(context.WithValue(req.Context(), httprouter.ParamsKey, httprouter.Params{httprouter.Param{Key: "partition", Value: "default"}, httprouter.Param{Key: "queue", Value: "root.notexists@"}}))
+ req = req.WithContext(context.WithValue(req.Context(), httprouter.ParamsKey, httprouter.Params{httprouter.Param{Key: "partition", Value: "default"}, httprouter.Param{Key: "queue", Value: "root.notexists!"}}))
assert.NilError(t, err)
resp = &MockResponseWriter{}
getPartitionQueue(resp, req)
- assertQueueInvalid(t, resp, "root.notexists@", "notexists@")
+ assertQueueInvalid(t, resp, "root.notexists!", "notexists!")
// test queue is not exists
req, err = http.NewRequest("GET", "/ws/v1/partition/default/queue/root.a", strings.NewReader(""))
@@ -1696,13 +1696,13 @@ func TestGetApplicationHandler(t *testing.T) {
assert.NilError(t, err, "HTTP request create failed")
req5 = req5.WithContext(context.WithValue(req.Context(), httprouter.ParamsKey, httprouter.Params{
httprouter.Param{Key: "partition", Value: partitionNameWithoutClusterID},
- httprouter.Param{Key: "queue", Value: "root.test.test123@"},
+ httprouter.Param{Key: "queue", Value: "root.test.test123!"},
httprouter.Param{Key: "application", Value: "app-1"},
}))
assert.NilError(t, err, "Get Application Handler request failed")
resp5 := &MockResponseWriter{}
getApplication(resp5, req5)
- assertQueueInvalid(t, resp5, "root.test.test123@", "test123@")
+ assertQueueInvalid(t, resp5, "root.test.test123!", "test123!")
// test missing params name
req, err = http.NewRequest("GET", "/ws/v1/partition/default/queue/root.default/application/app-1", strings.NewReader(""))
@@ -1744,7 +1744,7 @@ func assertQueueInvalid(t *testing.T, resp *MockResponseWriter, invalidQueuePath
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
- assert.Equal(t, errInfo.Message, "problem in queue query parameter parsing as queue param "+invalidQueuePath+" contains invalid queue name "+invalidQueueName+". Queue name must only have alphanumeric characters, - or _, and be no longer than 64 characters except the recovery queue root.@recovery@", jsonMessageError)
+ assert.Equal(t, errInfo.Message, common.InvalidQueueName.Error(), jsonMessageError)
assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
}
@@ -1838,10 +1838,9 @@ func TestValidateQueue(t *testing.T) {
err := validateQueue("root.test.test123")
assert.NilError(t, err, "Queue path is correct but still throwing error.")
- invalidQueuePath := "root.test.test123@"
- invalidQueueName := "test123@"
+ invalidQueuePath := "root.test.test123!"
err1 := validateQueue(invalidQueuePath)
- assert.Error(t, err1, "problem in queue query parameter parsing as queue param "+invalidQueuePath+" contains invalid queue name "+invalidQueueName+". Queue name must only have alphanumeric characters, - or _, and be no longer than 64 characters except the recovery queue root.@recovery@")
+ assert.Error(t, err1, common.InvalidQueueName.Error())
err2 := validateQueue("root")
assert.NilError(t, err2, "Queue path is correct but still throwing error.")
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org