diff --git a/persist/sqldb/selector.go b/persist/sqldb/selector.go index 5e2b9cbb53ca..511988b319f3 100644 --- a/persist/sqldb/selector.go +++ b/persist/sqldb/selector.go @@ -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)) diff --git a/persist/sqldb/workflow_archive.go b/persist/sqldb/workflow_archive.go index c18c2017b369..a44208381855 100644 --- a/persist/sqldb/workflow_archive.go +++ b/persist/sqldb/workflow_archive.go @@ -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 + "%"} @@ -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 { diff --git a/server/utils/list_options.go b/server/utils/list_options.go index 69a03456cbd5..68078f10bc98 100644 --- a/server/utils/list_options.go +++ b/server/utils/list_options.go @@ -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 + MinStartedAt, MaxStartedAt time.Time + LabelRequirements labels.Requirements + Limit, Offset int + ShowRemainingItemCount bool + StartedAtAscending bool } func (l ListOptions) WithLimit(limit int) ListOptions { @@ -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 @@ -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 { @@ -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, @@ -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 +} diff --git a/server/utils/list_options_test.go b/server/utils/list_options_test.go new file mode 100644 index 000000000000..96a36b06823b --- /dev/null +++ b/server/utils/list_options_test.go @@ -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 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 +} diff --git a/server/workflowarchive/archived_workflow_server_test.go b/server/workflowarchive/archived_workflow_server_test.go index 3f66a5919c09..344dda39c6be 100644 --- a/server/workflowarchive/archived_workflow_server_test.go +++ b/server/workflowarchive/archived_workflow_server_test.go @@ -61,13 +61,13 @@ func Test_archivedWorkflowServer(t *testing.T) { createdTime := metav1.Time{Time: time.Now().UTC()} finishedTime := metav1.Time{Time: createdTime.Add(time.Second * 2)} repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "", NamePrefix: "", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}}, nil) - repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NamePrefix: "", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}}, nil) + repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NameOperator: "=", NamePrefix: "", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}}, nil) repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}}, nil) - repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}}, nil) - repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0, ShowRemainingItemCount: true}).Return(wfv1.Workflows{{}}, nil) + repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NameOperator: "=", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}}, nil) + repo.On("ListWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NameOperator: "=", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0, ShowRemainingItemCount: true}).Return(wfv1.Workflows{{}}, nil) repo.On("ListWorkflows", sutils.ListOptions{Namespace: "user-ns", Name: "", NamePrefix: "", MinStartedAt: time.Time{}, MaxStartedAt: time.Time{}, Limit: 2, Offset: 0}).Return(wfv1.Workflows{{}, {}}, nil) - repo.On("CountWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(int64(5), nil) - repo.On("CountWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0, ShowRemainingItemCount: true}).Return(int64(5), nil) + repo.On("CountWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NameOperator: "=", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0}).Return(int64(5), nil) + repo.On("CountWorkflows", sutils.ListOptions{Namespace: "", Name: "my-name", NameOperator: "=", NamePrefix: "my-", MinStartedAt: minStartAt, MaxStartedAt: maxStartAt, Limit: 2, Offset: 0, ShowRemainingItemCount: true}).Return(int64(5), nil) repo.On("GetWorkflow", "", "", "").Return(nil, nil) repo.On("GetWorkflow", "my-uid", "", "").Return(&wfv1.Workflow{ ObjectMeta: metav1.ObjectMeta{Name: "my-name"}, @@ -92,7 +92,8 @@ func Test_archivedWorkflowServer(t *testing.T) { FinishedAt: finishedTime, Nodes: map[string]wfv1.NodeStatus{ "failed-node": {Name: "failed-node", StartedAt: createdTime, FinishedAt: finishedTime, Phase: wfv1.NodeFailed, Message: "failed"}, - "succeeded-node": {Name: "succeeded-node", StartedAt: createdTime, FinishedAt: finishedTime, Phase: wfv1.NodeSucceeded, Message: "succeeded"}}, + "succeeded-node": {Name: "succeeded-node", StartedAt: createdTime, FinishedAt: finishedTime, Phase: wfv1.NodeSucceeded, Message: "succeeded"}, + }, }, }, nil) repo.On("GetWorkflow", "resubmit-uid", "", "").Return(&wfv1.Workflow{ @@ -186,7 +187,6 @@ func Test_archivedWorkflowServer(t *testing.T) { // pass namespace as field selector and query parameter both, where they don't match _, err = w.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{Namespace: "user-ns", ListOptions: &metav1.ListOptions{Limit: 1, FieldSelector: "metadata.namespace=other-ns"}}) assert.Equal(t, err, status.Error(codes.InvalidArgument, "'namespace' query param (\"user-ns\") and fieldselector 'metadata.namespace' (\"other-ns\") are both specified and contradict each other")) - }) t.Run("GetArchivedWorkflow", func(t *testing.T) { allowed = false