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.

Sign up for GitHub

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

fix: Eventsource resourse label selector operators not working #2795

Merged
merged 8 commits into from
Sep 12, 2023
Merged

fix: Eventsource resourse label selector operators not working #2795

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ccfda47
Merge pull request #2 from argoproj/master
gokulav137 Sep 9, 2023
17b419e
Modify LabelReq to process Exists, DoesNotExist, In and NotIn operato…
gokulav137 Sep 11, 2023
681973d
Add tests for LabelReq
gokulav137 Sep 11, 2023
b30df9c
Fix documentation for Resource Filte and Selector
gokulav137 Sep 11, 2023
3d13501
Run make codegen
gokulav137 Sep 11, 2023
fb6c87f
Merge branch 'master' into fix-label-selector-exist-and-doesnot-exist
gokulav137 Sep 11, 2023
e29d940
Refactor values creation logic using var and else if blocks
gokulav137 Sep 12, 2023
3030191
fix linting error - refactor if else chain to switch
gokulav137 Sep 12, 2023
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
8 changes: 6 additions & 2 deletions api/event-source.html
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 5 additions & 2 deletions api/event-source.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions api/jsonschema/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2573,7 +2573,7 @@
"type": "array"
},
"labels": {
"description": "Labels provide listing options to K8s API to watch resource/s. Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/label-selectors/ for more info.",
"description": "Labels provide listing options to K8s API to watch resource/s. Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/label-selectors/ for more info. Unlike K8s field selector, multiple values are passed as comma separated values instead of list of values. Eg: value: value1,value2. Same as K8s label selector, operator \"=\", \"==\", \"!=\", \"exists\", \"!\", \"notin\", \"in\", \"gt\" and \"lt\" are supported",
"items": {
"$ref": "#/definitions/io.argoproj.eventsource.v1alpha1.Selector"
},
Expand Down Expand Up @@ -2759,7 +2759,7 @@
"type": "string"
},
"operation": {
"description": "Supported operations like ==, !=, \u003c=, \u003e= etc. Defaults to ==. Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors for more info.",
"description": "Supported operations like ==, != etc. Defaults to ==. Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors for more info.",
"type": "string"
},
"value": {
Expand Down
4 changes: 2 additions & 2 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 8 additions & 1 deletion eventsources/sources/resource/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,14 @@ func LabelReq(sel v1alpha1.Selector) (*labels.Requirement, error) {
if sel.Operation != "" {
op = selection.Operator(sel.Operation)
}
req, err := labels.NewRequirement(sel.Key, op, []string{sel.Value})
values := []string{sel.Value}
Copy link
Member

Choose a reason for hiding this comment

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

Start with var values = make([]string, 0), and use if else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense will make the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used switch and var []string instead of if else block due to linting error.

if (op == selection.Exists || op == selection.DoesNotExist) && sel.Value == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Return an error when sel.Value != ""?

Copy link
Contributor Author

@gokulav137 gokulav137 Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If operator is exists or doesnotexists, an error will be thrown from labels.NewRequirement if values is not empty.

Invalid value: []string{""}: values set must be empty for exists and does not exist

Should I add an error from our end? @whynowy

Copy link
Member

Choose a reason for hiding this comment

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

This is good, thanks!

values = []string{}
}
if op == selection.In || op == selection.NotIn {
values = strings.Split(sel.Value, ",")
}
req, err := labels.NewRequirement(sel.Key, op, values)
if err != nil {
return nil, err
}
Expand Down
173 changes: 173 additions & 0 deletions eventsources/sources/resource/start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ package resource
import (
"context"
"encoding/json"
"fmt"
"testing"
"time"

"github.com/smartystreets/goconvey/convey"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/client-go/kubernetes/fake"

"github.com/argoproj/argo-events/common/logging"
Expand Down Expand Up @@ -105,3 +107,174 @@ func TestFilter(t *testing.T) {
convey.So(pass, convey.ShouldBeTrue)
})
}

func TestLabelSelector(t *testing.T) {
// Test equality operators =, == and in
for _, op := range []string{"==", "=", "in"} {
t.Run(fmt.Sprintf("Test operator %v", op), func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: op,
Value: "1",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"key": "1"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"key": "2"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
}
// Test inequality operators != and notin
for _, op := range []string{"!=", "notin"} {
t.Run(fmt.Sprintf("Test operator %v", op), func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: op,
Value: "1",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"key": "2"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"key": "1"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
}
// Test greater than operator
t.Run("Test operator gt", func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: "gt",
Value: "1",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"key": "2"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"key": "1"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
// Test lower than operator
t.Run("Test operator lt", func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: "lt",
Value: "2",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"key": "1"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"key": "2"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
// Test exists operator
t.Run("Test operator exists", func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: "exists",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"key": "something"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"notkey": "something"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
// Test doesnot exist operator
t.Run("Test operator !", func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: "!",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"notkey": "something"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"key": "something"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
// Test default operator
t.Run("Test default operator", func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: "",
Value: "something",
}})
if err != nil {
t.Fatal(err)
}
validL := &labels.Set{"key": "something"}
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
invalidL := &labels.Set{"key": "not something"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
// Test invalid operators <= and >=
for _, op := range []string{"<=", ">="} {
t.Run(fmt.Sprintf("Invalid operator %v", op), func(t *testing.T) {
_, err := LabelSelector([]v1alpha1.Selector{{
Key: "workflows.argoproj.io/phase",
Operation: op,
Value: "1",
}})
if err == nil {
t.Errorf("Invalid operator should throw error")
}
})
}
// Test comma separated values for in
t.Run("Comma separated values", func(t *testing.T) {
r, err := LabelSelector([]v1alpha1.Selector{{
Key: "key",
Operation: "in",
Value: "a,b,",
}})
if err != nil {
t.Fatal("valid value threw error, value %w", err)
}
for _, validL := range []labels.Set{{"key": "a"}, {"key": "b"}, {"key": ""}} {
if !r.Matches(validL) {
t.Errorf("didnot match %v", validL)
}
}
invalidL := &labels.Set{"key": "c"}
if r.Matches(invalidL) {
t.Errorf("matched %v", invalidL)
}
})
}
6 changes: 5 additions & 1 deletion pkg/apis/eventsource/v1alpha1/generated.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/apis/eventsource/v1alpha1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion pkg/apis/eventsource/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,10 @@ type ResourceFilter struct {
Prefix string `json:"prefix,omitempty" protobuf:"bytes,1,opt,name=prefix"`
// Labels provide listing options to K8s API to watch resource/s.
// Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/label-selectors/ for more info.
// Unlike K8s field selector, multiple values are passed as comma separated values instead of list of values.
// Eg: value: value1,value2.
// Same as K8s label selector, operator "=", "==", "!=", "exists", "!", "notin", "in", "gt" and "lt"
// are supported
// +optional
Labels []Selector `json:"labels,omitempty" protobuf:"bytes,2,rep,name=labels"`
// Fields provide field filters similar to K8s field selector
Expand All @@ -371,7 +375,7 @@ type ResourceFilter struct {
type Selector struct {
// Key name
Key string `json:"key" protobuf:"bytes,1,opt,name=key"`
// Supported operations like ==, !=, <=, >= etc.
// Supported operations like ==, != etc.
// Defaults to ==.
// Refer https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors for more info.
// +optional
Expand Down