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

EventSource "Resource" label selector doesn't support "exists" operator (and possibly others) #2775

Closed
azazel75 opened this issue Aug 26, 2023 · 3 comments · Fixed by #2795
Closed
Labels
bug Something isn't working

Comments

@azazel75
Copy link

Describe the bug
I've added the following EventSource to get events when the Workflow instances are completed:

apiVersion: argoproj.io/v1alpha1
kind: EventSource
metadata:
  name: notification
spec:
  template:
    serviceAccountName: viewer
    container:
      env:
        - name: DEBUG_LOG
          value: "true"
  resource:
    completed-workflows:
      namespace: devops
      group: argoproj.io
      version: v1alpha1
      resource: workflows
      eventTypes:
        - UPDATE
      filter:
        afterStart: true
        labels:
          - key: workflows.argoproj.io/completed
            operation: "=="
            value: "true"
          - key: workflows.argoproj.io/phase
            operation: exists

but the event source containers crashes during startup, this the relevant log:

2023-08-26T14:33:06.147Z	ERROR	argo-events.eventsource	eventsources/eventing.go:564	Failed to start listening eventsource	{"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "completed-workflows", "error": "failed after retries: failed to create the label selector for the event source completed-workflows, values: Invalid value: []string{\"\"}: values set must be empty for exists and does not exist"}

I believe the error is where the requirements are created: there it always initializes a string value

To Reproduce
Add the already mentioned eventsource with kubectl apply -f eventsource.yaml

Expected behavior
To not crash and filter updates that only ave that label

Environment (please complete the following information):

  • Kubernetes: 1.26.4
  • Argo: 3.4.8
  • Argo Events: 1.7.6

Message from the maintainers:

If you wish to see this enhancement implemented please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

@azazel75 azazel75 added the bug Something isn't working label Aug 26, 2023
@gokulav137
Copy link
Contributor

gokulav137 commented Sep 10, 2023

If we check the documentation for selectors here https://github.com/argoproj/argo-events/blob/master/api/event-source.md#selector. It says "Supported operations like ==, !=, <=, >= etc"

But if we use a "<=" operator we will get an error.
operator: Unsupported value: "<=": supported values: "in", "notin", "=", "==", "!=", "gt", "lt", "exists", "!"

Argo uses NewRequirements under the hood: https://pkg.go.dev/k8s.io/[email protected]/pkg/labels#NewRequirement
Which has following constraints that is throwing the error

// NewRequirement is the constructor for a Requirement.
// If any of these rules is violated, an error is returned:
// (1) The operator can only be In, NotIn, Equals, DoubleEquals, Gt, Lt, NotEquals, Exists, or DoesNotExist.
// (2) If the operator is In or NotIn, the values set must be non-empty.
// (3) If the operator is Equals, DoubleEquals, or NotEquals, the values set must contain one value.
// (4) If the operator is Exists or DoesNotExist, the value set must be empty.
// (5) If the operator is Gt or Lt, the values set must contain only one value, which will be interpreted as an integer.
// (6) The key is invalid due to its length, or sequence
//     of characters. See validateLabelKey for more details.
//
// The empty string is a valid value in the input values set.
// Returned error, if not nil, is guaranteed to be an aggregated field.ErrorList

operators : In, NotIn etc are defined here: https://pkg.go.dev/k8s.io/[email protected]/pkg/selection#Operator

Argo creates a requirement using the following line

req, err := labels.NewRequirement(sel.Key, op, []string{sel.Value})

So for both:

- key: workflows.argoproj.io/phase
  operation: exists
- key: workflows.argoproj.io/phase
  operation: exists
  value: ""

a list [""] is passed to NewRequirements where it is expecting an empty list [] for operators exist and !. This is throwing the error in the issue.
Invalid value: []string{""}: values set must be empty for exists and does not exist

Also only one value is used to create the requirement, this means in and notin are the same as == and !=

Possible fixes

For fixing exist and !:

  • pass an empty list for exists and ! when value is "" to add support for these operators

For fixing <= and >= error:

  • update the documentation to specify the correct operators gt and lt
    OR
  • map "<= value" to "lt value+1" and ">= value" to "gt value-1" (as value for gt and lt is restricted to int). This would provide backward compatibility is what I am thinking if required.

For in and notin:

  • maybe we can look into using a separator to split the input string for in and notin to specify different values. "," can be used as value is currently constrained to be alphanumeric.
    OR
  • maybe reject in and notin as its kinda confusing, as it does the same thing as == and !=?
    OR
  • Change the Selector type to something similar to LabelSelectorRequirement defined as in k8s here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1226
    It would make life easier than trying to convert between the two.
    But changing value's type from string to []string could lead to backward comparability issues

I would like to contribute to fix this issue.

@azazel75
Copy link
Author

Then propose a pull request

@gokulav137
Copy link
Contributor

Before changes

2023-09-11T09:13:00.623Z        INFO    argo-events.eventsource resource/start.go:108   configuring label selectors if filters are selected...  {"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "example"}
2023-09-11T09:13:00.623Z        ERROR   argo-events.eventsource eventsources/eventing.go:586    Failed to start listening eventsource   {"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "example", "error": "failed after retries: failed to create the label selector for the event source example, values: Invalid value: []string{\"\"}: values set must be empty for exists and does not exist"}

After changes

2023-09-11T09:21:21.184Z        INFO    argo-events.eventsource resource/start.go:108   configuring label selectors if filters are selected...  {"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "example"}
2023-09-11T09:21:21.184Z        INFO    argo-events.eventsource resource/start.go:121   setting up informer factory...  {"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "example"}
2023-09-11T09:21:21.184Z        INFO    argo-events.eventsource resource/start.go:234   running informer...     {"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "example"}
2023-09-11T09:21:21.184Z        INFO    argo-events.eventsource resource/start.go:174   listening to resource events... {"eventSourceName": "notification", "eventSourceType": "resource", "eventName": "example"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants