Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support kubernetes equality-based ops for field selector metadata.name #13476

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion persist/sqldb/selector.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
func BuildArchivedWorkflowSelector(selector db.Selector, tableName, labelTableName string, t dbType, options utils.ListOptions, count bool) (db.Selector, error) {
selector = selector.
And(namespaceEqual(options.Namespace)).
And(nameEqual(options.Name)).
And(nameOperator(options.Name, options.NameOperator)).
And(namePrefixClause(options.NamePrefix)).
And(startedAtFromClause(options.MinStartedAt)).
And(startedAtToClause(options.MaxStartedAt))
Expand Down
20 changes: 19 additions & 1 deletion persist/sqldb/workflow_archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,25 @@ func nameEqual(name string) db.Cond {
return db.Cond{}
}

func nameNotEqual(name string) db.Cond {
if name != "" {
return db.Cond{"name !=": name}
}
return db.Cond{}
}

func nameOperator(name, op string) db.Cond {
if name != "" {
switch op {
case "!=":
return nameNotEqual(name)
case "=", "==", "":
return nameEqual(name)
}
}
return db.Cond{}
}

func namePrefixClause(namePrefix string) db.Cond {
if namePrefix != "" {
return db.Cond{"name LIKE": namePrefix + "%"}
Expand Down Expand Up @@ -389,7 +408,6 @@ func (r *workflowArchive) GetWorkflowForEstimator(namespace string, requirements
FinishedAt: v1.Time{Time: awf.FinishedAt},
},
}, nil

}

func (r *workflowArchive) DeleteWorkflow(uid string) error {
Expand Down
43 changes: 35 additions & 8 deletions server/utils/list_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ import (
)

type ListOptions struct {
Namespace, Name, NamePrefix string
MinStartedAt, MaxStartedAt time.Time
LabelRequirements labels.Requirements
Limit, Offset int
ShowRemainingItemCount bool
StartedAtAscending bool
Namespace, Name, NameOperator, NamePrefix string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to unify this with #13160 , which I need to re-review

MinStartedAt, MaxStartedAt time.Time
LabelRequirements labels.Requirements
Limit, Offset int
ShowRemainingItemCount bool
StartedAtAscending bool
}

func (l ListOptions) WithLimit(limit int) ListOptions {
Expand Down Expand Up @@ -71,6 +71,7 @@ func BuildListOptions(options metav1.ListOptions, ns, namePrefix string) (ListOp
// note that for backward compatibility, the field selector 'metadata.namespace' is also supported for now
namespace := ns // optional
name := ""
nameOperator := ""
minStartedAt := time.Time{}
maxStartedAt := time.Time{}
showRemainingItemCount := false
Expand All @@ -91,8 +92,13 @@ func BuildListOptions(options metav1.ListOptions, ns, namePrefix string) (ListOp
return ListOptions{}, status.Errorf(codes.InvalidArgument,
"'namespace' query param (%q) and fieldselector 'metadata.namespace' (%q) are both specified and contradict each other", namespace, fieldSelectedNamespace)
}
} else if strings.HasPrefix(selector, "metadata.name=") {
name = strings.TrimPrefix(selector, "metadata.name=")
} else if strings.HasPrefix(selector, "metadata.name") {
var ok bool
nameOperator, name, ok = splitTerm(selector)
if !ok {
return ListOptions{}, status.Errorf(codes.InvalidArgument,
"unsupported fieldselector 'metadata.name' %s", selector)
}
} else if strings.HasPrefix(selector, "spec.startedAt>") {
minStartedAt, err = time.Parse(time.RFC3339, strings.TrimPrefix(selector, "spec.startedAt>"))
if err != nil {
Expand Down Expand Up @@ -122,6 +128,7 @@ func BuildListOptions(options metav1.ListOptions, ns, namePrefix string) (ListOp
return ListOptions{
Namespace: namespace,
Name: name,
NameOperator: nameOperator,
NamePrefix: namePrefix,
MinStartedAt: minStartedAt,
MaxStartedAt: maxStartedAt,
Expand All @@ -131,3 +138,23 @@ func BuildListOptions(options metav1.ListOptions, ns, namePrefix string) (ListOp
ShowRemainingItemCount: showRemainingItemCount,
}, nil
}

const (
notEqualOperator = "!="
doubleEqualOperator = "=="
equalOperator = "="
)

var termOperators = []string{notEqualOperator, doubleEqualOperator, equalOperator}

func splitTerm(term string) (op, rhs string, ok bool) {
for i := range term {
remaining := term[i:]
for _, op := range termOperators {
if strings.HasPrefix(remaining, op) {
return op, term[i+len(op):], true
}
}
}
return "", "", false
}
270 changes: 270 additions & 0 deletions server/utils/list_options_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,270 @@
package utils

import (
"strconv"
"testing"
"time"

"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
)

func TestListOptionsMethods(t *testing.T) {
baseOptions := ListOptions{}

t.Run("WithLimit", func(t *testing.T) {
result := baseOptions.WithLimit(10)
require.Equal(t, 10, result.Limit)
})

t.Run("WithOffset", func(t *testing.T) {
result := baseOptions.WithOffset(5)
require.Equal(t, 5, result.Offset)
})

t.Run("WithShowRemainingItemCount", func(t *testing.T) {
result := baseOptions.WithShowRemainingItemCount(true)
require.True(t, result.ShowRemainingItemCount)
})

t.Run("WithMaxStartedAt", func(t *testing.T) {
now := time.Now()
result := baseOptions.WithMaxStartedAt(now)
require.Equal(t, now, result.MaxStartedAt)
})

t.Run("WithMinStartedAt", func(t *testing.T) {
now := time.Now()
result := baseOptions.WithMinStartedAt(now)
require.Equal(t, now, result.MinStartedAt)
})

t.Run("WithStartedAtAscending", func(t *testing.T) {
result := baseOptions.WithStartedAtAscending(true)
require.True(t, result.StartedAtAscending)
})
}

func TestBuildListOptions(t *testing.T) {
tests := []struct {
name string
options metav1.ListOptions
ns string
namePrefix string
expected ListOptions
expectedError error
}{
{
name: "Basic case",
options: metav1.ListOptions{
Continue: "5",
Limit: 10,
},
ns: "default",
namePrefix: "test-",
expected: ListOptions{
Namespace: "default",
NamePrefix: "test-",
Limit: 10,
Offset: 5,
},
},
{
name: "Invalid continue",
options: metav1.ListOptions{
Continue: "invalid",
},
expectedError: status.Error(codes.InvalidArgument, "listOptions.continue must be int"),
},
{
name: "Negative continue",
options: metav1.ListOptions{
Continue: "-1",
},
expectedError: status.Error(codes.InvalidArgument, "listOptions.continue must >= 0"),
},
{
name: "Field selectors",
options: metav1.ListOptions{
FieldSelector: "metadata.namespace=test,metadata.name=myname,spec.startedAt>2023-01-01T00:00:00Z,spec.startedAt<2023-12-31T23:59:59Z,ext.showRemainingItemCount=true",
},
expected: ListOptions{
Namespace: "test",
Name: "myname",
NameOperator: "=",
MinStartedAt: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC),
MaxStartedAt: time.Date(2023, 12, 31, 23, 59, 59, 0, time.UTC),
ShowRemainingItemCount: true,
},
},
{
name: "Invalid field selector",
options: metav1.ListOptions{
FieldSelector: "unsupported=value",
},
expectedError: status.Error(codes.InvalidArgument, "unsupported requirement unsupported=value"),
},
{
name: "Conflicting namespace in query param and field selector",
options: metav1.ListOptions{
FieldSelector: "metadata.namespace=test",
},
ns: "different-namespace",
expectedError: status.Errorf(codes.InvalidArgument,
"'namespace' query param (%q) and fieldselector 'metadata.namespace' (%q) are both specified and contradict each other",
"different-namespace", "test"),
},
{
name: "Unsupported metadata.name field selector",
options: metav1.ListOptions{
FieldSelector: "metadata.name:invalid",
},
expectedError: status.Errorf(codes.InvalidArgument,
"unsupported fieldselector 'metadata.name' metadata.name:invalid"),
},
{
name: "Invalid maxStartedAt< format in field selector",
options: metav1.ListOptions{
FieldSelector: "spec.startedAt<invalid-date-format",
},
expectedError: func() error {
_, err := time.Parse(time.RFC3339, "invalid-date-format")
return ToStatusError(err, codes.Internal)
}(),
},
{
name: "Invalid maxStartedAt> format in field selector",
options: metav1.ListOptions{
FieldSelector: "spec.startedAt>invalid-date-format",
},
expectedError: func() error {
_, err := time.Parse(time.RFC3339, "invalid-date-format")
return ToStatusError(err, codes.Internal)
}(),
},
{
name: "Invalid showRemainingItemCount in field selector",
options: metav1.ListOptions{
FieldSelector: "ext.showRemainingItemCount=invalid",
},
expectedError: func() error {
_, err := strconv.ParseBool("invalid")
return ToStatusError(err, codes.Internal)
}(),
},
{
name: "Label selector",
options: metav1.ListOptions{
LabelSelector: "app=myapp,env=prod,label==mylabel",
},
expected: ListOptions{
LabelRequirements: mustParseToRequirements(t, "app=myapp,env=prod,label==mylabel"),
},
},
{
name: "Invalid label selector",
options: metav1.ListOptions{
LabelSelector: "app=myapp,",
},
expectedError: status.Error(codes.InvalidArgument, "found '', expected: identifier after ','"),
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result, err := BuildListOptions(tt.options, tt.ns, tt.namePrefix)
if tt.expectedError != nil {
require.Error(t, err)
require.Equal(t, tt.expectedError.Error(), err.Error())
} else {
require.NoError(t, err)
require.Equal(t, tt.expected, result)
}
})
}
}

func TestSplitTerm(t *testing.T) {
tests := []struct {
name string
term string
wantOp string
wantRhs string
wantOk bool
}{
{
name: "Not equal operator",
term: "field!=value",
wantOp: "!=",
wantRhs: "value",
wantOk: true,
},
{
name: "Double equal operator",
term: "field==value",
wantOp: "==",
wantRhs: "value",
wantOk: true,
},
{
name: "Equal operator",
term: "field=value",
wantOp: "=",
wantRhs: "value",
wantOk: true,
},
{
name: "No operator",
term: "fieldvalue",
wantOp: "",
wantRhs: "",
wantOk: false,
},
{
name: "Invalid operator",
term: "field:value",
wantOp: "",
wantRhs: "",
wantOk: false,
},
{
name: "Operator at the end",
term: "field=",
wantOp: "=",
wantRhs: "",
wantOk: true,
},
{
name: "Multiple operators",
term: "field==value!=othervalue",
wantOp: "==",
wantRhs: "value!=othervalue",
wantOk: true,
},
{
name: "Operator in the middle",
term: "pre!=post=value",
wantOp: "!=",
wantRhs: "post=value",
wantOk: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
op, rhs, ok := splitTerm(tt.term)
require.Equal(t, tt.wantOp, op)
require.Equal(t, tt.wantRhs, rhs)
require.Equal(t, tt.wantOk, ok)
})
}
}

func mustParseToRequirements(t *testing.T, labelSelector string) labels.Requirements {
requirements, err := labels.ParseToRequirements(labelSelector)
require.NoError(t, err)
return requirements
}
Loading
Loading