Eventing: Request for ApiServerSource is able to filter on a resource's name with a trigger (not only the type).

[capri-xiyue]

Problem
We are going to move the sources api to v1beta1.
For ApiServerSource, there are some features requests like:
https://github.com/knative/eventing/issues/3295#issuecomment-641105306
https://github.com/knative/eventing/issues/3295#issuecomment-641106861

Persona:
Which persona is this feature for?

Exit Criteria
A measurable (binary) test that would indicate that the problem has been resolved.

Time Estimate (optional):
How many developer-days do you think this may take to resolve?

Additional context (optional)
Add any other context about the feature request here.

[capri-xiyue]

@rhuss
Maybe I have some misunderstanding about the new feature requests of filter on a resource's name. Just wanna clarify it:
Does it mean we will have spec like this:

apiVersion: sources.knative.dev/v1alpha1
kind: ApiServerSource
metadata:
  name: testnamespace
  namespace: default
spec:
  serviceAccountName: events-sa
  mode: Resource
  resources:
    - apiVersion: v1
      kind: Namespace
      fieldSelector: metadata.namespace=filternamespace  

And it supports send events for add, update, delete of v1 namespace with name filternamespace

Or does it mean we need to add some attributes into the event fired by ApiServerSource, currently the event looks like this:

☁️  cloudevents.Event
Validation: valid
Context Attributes,
  specversion: 1.0
  type: dev.knative.apiserver.resource.add
  source: https://10.0.0.1:443
  subject: /apis/v1/namespaces/default/events/apiserversource-testevents-337160f6-30e8-4a5b-ab55-7a61b5bpsg95.1622a0e145791b51
  id: 0efad937-d982-43f4-bf21-9a92fd534429
  time: 2020-07-17T19:28:27.443852277Z
  datacontenttype: application/json
Data,
  {
    "apiVersion": "v1",
    "count": 1,
    "eventTime": null,
    "firstTimestamp": "2020-07-17T19:28:27Z",
    "involvedObject": {
      "apiVersion": "v1",
      "fieldPath": "spec.containers{receive-adapter}",
      "kind": "Pod",
      "name": "apiserversource-testevents-337160f6-30e8-4a5b-ab55-7a61b5bpsg95",
      "namespace": "default",
      "resourceVersion": "13699162",
      "uid": "c6d1bf3d-6b4b-4f3c-a6b1-e0f069cf4038"
    },
    "kind": "Event",
    "lastTimestamp": "2020-07-17T19:28:27Z",
    "message": "Stopping container receive-adapter",
    "metadata": {
      "creationTimestamp": "2020-07-17T19:28:27Z",
      "name": "apiserversource-testevents-337160f6-30e8-4a5b-ab55-7a61b5bpsg95.1622a0e145791b51",
      "namespace": "default",
      "resourceVersion": "27656",
      "selfLink": "/api/v1/namespaces/default/events/apiserversource-testevents-337160f6-30e8-4a5b-ab55-7a61b5bpsg95.1622a0e145791b51",
      "uid": "c5ba281e-5e38-4fad-ab69-7fafe04b455a"
    },
    "reason": "Killing",
    "reportingComponent": "",
    "reportingInstance": "",
    "source": {
      "component": "kubelet",
      "host": "gke-xiyue-opensource-clu-default-pool-9691806d-05w6"
    },
    "type": "Normal"
  }

For the new feature request of filtering on a resource's name with a trigger, do you mean we add some CloudEvents attributes or extensions like resource.name so that trigger can filter it?

[grantr]

/assign danyinggu

[knative-prow-robot]

@grantr: GitHub didn't allow me to assign the following users: danyinggu.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign danyinggu

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[grantr]

/assign @danyinggu

[rhuss]

@capri-xiyue @danyinggu The uses case is to allow filtering on the resource's name in a trigger because a certain sink might be interested only on a specific resource, identified by name. This idea could be extended to copy over other meta-data from the resource watched to the attributes of the generated CEs. Labels would be a perfect fit so that a Trigger then can also filter on labels like a label-selector would do e.g. for a service. But also annotation could be added that way.

In order to configure the amount of data that is emitted as attributes I suggest to add a mode: kind of field to the ApiServerSource CRD's spec to allow to select which meta-data to copy over (name, labels and/or annotations). But I think this could be optional and we could just copy over name, labels and annotations to some CloudEvent attributes.

[lionelvillard]

This requirement makes sense because today Trigger does not support content-based filtering, is that right @rhuss ?

[nachocano]

It seems to me that @rhuss proposal can be done both at the data plane level and also control plane.

The problem of doing it at the data plane would be that we need to parse the event payload to extract the appropriate fields to be set as context attributes. Do folks think that is acceptable?
I haven't used ApiServerSource myself. Quick question, is the CE payload a k8s Event, right? I'm asking to understand the data schema, and what things we can extract from it in case we have to parse it.

On the control plane side, if we add a fieldSelector to the spec, when users create a CO, they can specify the metadata.name and namespace. Regarding labels, I think that is covered with the current label selector we have...
As for annotations, I think they are not-indexed, so we cannot properly do it here. It would still need to be at the data plane level?

@lionelvillard @n3wscott what do you think. We need to define this so that @danyinggu can take a stab at it.

[capri-xiyue]

It seems to me that @rhuss proposal can be done both at the data plane level and also control plane.

The problem of doing it at the data plane would be that we need to parse the event payload to extract the appropriate fields to be set as context attributes. Do folks think that is acceptable?
I haven't used ApiServerSource myself. Quick question, is the CE payload a k8s Event, right? I'm asking to understand the data schema, and what things we can extract from it in case we have to parse it.

On the control plane side, if we add a fieldSelector to the spec, when users create a CO, they can specify the metadata.name and namespace. Regarding labels, I think that is covered with the current label selector we have...
As for annotations, I think they are not-indexed, so we cannot properly do it here. It would still need to be at the data plane level?

@lionelvillard @n3wscott what do you think. We need to define this so that @danyinggu can take a stab at it.

@nachocano Here is the example event emitted by ApiServerSource

[danyinggu]

@nachocano I just tried more examples using the current APIServerSource. If we're listening to Kind:Pod, the CE will be like this

☁️  cloudevents.Event
Validation: valid
Context Attributes,
  specversion: 1.0
  type: dev.knative.apiserver.resource.update
  source: https://10.8.0.1:443
  subject: /apis/v1/namespaces/default/pods/busybox
  id: d09f9beb-7936-445f-b9ca-3b9dc34bc06f
  time: 2020-07-23T20:05:51.193940948Z
  datacontenttype: application/json
Extensions,
  knativearrivaltime: 2020-07-23T20:05:51.194372634Z
  knativehistory: default-kne-trigger-kn-channel.default.svc.cluster.local
Data,
  {
    "apiVersion": "v1",
    "kind": "Pod",
    "metadata": {
      "annotations": {
        "kubernetes.io/limit-ranger": "LimitRanger plugin set: cpu request for container busybox"
      },
      "creationTimestamp": "2020-07-23T20:05:49Z",
      "labels": {
        "run": "busybox"
      },
      "name": "busybox",
      "namespace": "default",
      "resourceVersion": "446148",
      "selfLink": "/api/v1/namespaces/default/pods/busybox",
      "uid": "2fdf9e97-2f53-40c6-9f05-4e9354a255a9"
    },
    "spec": {
      "containers": [
        {
          "args": [
            "ls"
          ],
          "image": "busybox",
          "imagePullPolicy": "Always",
          "name": "busybox",
          "resources": {
            "requests": {
              "cpu": "100m"
            }
          },
          "terminationMessagePath": "/dev/termination-log",
          "terminationMessagePolicy": "File",
          "volumeMounts": [
            {
              "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount",
              "name": "default-token-rg7lv",
              "readOnly": true
            }
          ]
        }
      ],

Seems like the InvolvedObject is not in the data payload but metadata is here. It has annotation, name and labels in the metadata though. Maybe it depends on the type the source is listening to.

[capri-xiyue]

I think this can be done with both control plane and data plane.
If we do that with control plane, we can use https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/ in spec. The advantage of that is we will only fire events which matches the criteria which is more efficient and it doesn't depend on the trigger. In addition, it will support all filters that Field Selectors supports which is more flexible.

If we do that with data plane, I think we will need to parse the Data of event. If user just want to listen to events of match specific filter like specific resource's name, they will need to use trigger. And with this, the filter may be not so flexible since it can only filter the data we pass in CE extensions.

I think whether we want to do that in data plane or control plane depends on whether the user has the user case that they want the ApiServerSource to only emit events which matches the criteria like the resources' name and what kind of filters that user needs

[danyinggu]

@rhuss @lionelvillard @nachocano I checked the code base a bit. I think it would be more efficient for us to change the control plane to add a field selector. Roland mentions users may want to filter by label in Trigger, currently, there's already a labelSelector field in Spec, which means that users can already set it up in source to listen to some specific labels to ease the effort of Trigger. Thus, we can do the similar thing of fieldSelector as labelSelector does.

There are 2 concerns if we want to add the fieldSelector to spec though:

1. If we only want to add it in v1beta1 but not in previous versions, how to handle the deprecation policy that "API objects must be able to round-trip between API versions in a given release without information loss"?
2. The fieldSelector has options to filter by namespace. However, in our current logic (please correct me if I'm wrong), if the resource is namespacable, the source will only listen to the given resources that are in the same namespace that the sources is created at. Thus, if a user creates a source in namespace=A but define the fieldSelector be metadata.namespace=B which is a different namespace, there will be no event emitted. So it seems like the namespace fieldSelector is not that useful in our case.
   However, The metadata.name fieldSelector satisfies our use case that the source can directly listen to the resource with some name.

[rhuss]

Maybe we can go for a hybrid approach ? I think the name of a resource should be copied of to a CE attribute in the data plane, as this is probably one of the main use case (no numbers though): A generic source and 1..N triggers selecting on different names.

I can live with label selectors in the control plane as this is directly supported by the watch endpoint, so very efficient.
For annotations its a bit different, as there is no index on annotations so the source controller has to watch on every object and would then filter out on the control-plane provided annotation selector in the dataplane. This is not much different then just to copy over the annotations as CE attributes which delegates the the distribution to the broker and allows for a more flexibleeventing setup (e.g. triggers with different annotations selectors, served by the same source)

But even for labels, moving labelSelectors to the control plane means you are targeting more for a 1:1 relationship between source and trigger, whether putting them into the CE attributes allows for a 1:N relationship between sources and triggers (I think this was the initial idea of filters in triggers, wasn't it ?)

So if there are not technical limitations, I would always opt for a data-plane filter, as it move the responsibilities to the trigger-creator, not the source-creator (who might not be the same personas)

[grantr]

But I think this could be optional and we could just copy over name, labels and annotations to some CloudEvent attributes.

In general I am :+1: to adding extension attributes to the event by default if they might be useful for routing and filtering purposes. Adding separate object name, namespace, and resource name (as in triggers.eventing.knative.dev) attributes to each event is a minimal change that enables the basic use cases mentioned above.

Copying maps like label and annotations into attributes safely will be tricky while still conforming to the CloudEvents 1.0 spec because CloudEvents 1.0 restricts attribute keys to only the characters a-z and 0-9.

[danyinggu]

Sync with @rhuss offline, we agreed that we can start with copying the resource's name to the CE attribute.

We will discuss later on about labels and annotation.

[rhuss]

👍 also for adding the namespace and full resource kind along with resource's name to the attributes as these are scalar values and we have full controll over the attribute name (e.g. name, namespace and resource).

wrt/ annotations and labels, as discussed with @danyinggu I think adding labels might be possible by filtering all characters that are not allowed (e.g. a label name my-label would become mylabel). For annotation this would be much more difficult as the allowed characterset is much larger than for labels and compressed names would not be easy to understand and use in filters.

[lionelvillard]

filtering all characters that are not allowed

This is not great UX. Too many rules to remember. Let's not forget we are doing this just because of the Trigger shortcomings. I would rather focus on fixing Trigger (the last issue on this topic is this one: https://github.com/knative/eventing/issues/3359)

[danyinggu]

@rhuss When we discussed offline, we agreed to copy name as an extension first, but not mentioned about namespace and kind. Do you need these two to be copied as extension attributes as well? If so, I can add these two. Also, I'm confused about the name and resource you are given in the comment, I think the only name that we can extract from the object is the resource's name.

After our discussion and based on @lionelvillard's comment, I also think filtering all characters that are not allowed is not that helpful and the user creates the Trigger have to remember the rules correspondingly to change the label to make the filter work correctly, which is not that convenient. What do you think? @nachocano @grantr

[nachocano]

@rhuss When we discussed offline, we agreed to copy name as an extension first, but not mentioned about namespace and kind. Do you need these two to be copied as extension attributes as well? If so, I can add these two. Also, I'm confused about the name and resource you are given in the comment, I think the only name that we can extract from the object is the resource's name.

I'm fine with adding namespace and kind as well. Let's do it.

After our discussion and based on @lionelvillard's comment, I also think filtering all characters that are not allowed is not that helpful and the user creates the Trigger have to remember the rules correspondingly to change the label to make the filter work correctly, which is not that convenient. What do you think? @nachocano @grantr

I'd rather not add a modified version of the label names so that they comply with CE naming restrictions.

[grantr]

Also, I'm confused about the name and resource you are given in the comment

There is a difference between kind and resource in the Kubernetes API: kind is the singular class name (ApiServerSource) while resource is the API associated with that kind (apiserversources). An explanation of the difference is available in the kubebuilder docs. In some cases, a kind may be associated with multiple resources (the Scale kind), but for CRDs currently there is a 1:1 relationship between kind and resource.

An example of both kind and resource that will be familiar to most kubernetes users is creating an object with kubectl. The yaml definition of an object uses its kind, e.g. Service, but the kubectl get command uses its resource, e.g. services.

Regardless of whether we use kind or resource in the attribute, the value should probably be fully qualified by API group so there's a distinction between Service (or services) and Service.serving.knative.dev (or services.serving.knative.dev).

After our discussion and based on @lionelvillard's comment, I also think filtering all characters that are not allowed is not that helpful and the user creates the Trigger have to remember the rules correspondingly to change the label to make the filter work correctly, which is not that convenient. What do you think? @nachocano @grantr

I agree. There are too many ambiguous edge cases for this to be a consistently useful feature. A possible workaround is to use a label selector and add custom attributes to events via the ceOverrides field.

Let's not forget we are doing this just because of the Trigger shortcomings.

The issue is not entirely due to the Trigger IMO. The CloudEvent spec for attribute keys is insufficiently expressive. It doesn't even permit a control character such as - to represent the elision of invalid characters. There are workarounds that the Trigger can use for richer filtering, but they will be Trigger-specific extensions to the standard.

[lionelvillard]

Let's not forget we are doing this just because of the Trigger shortcomings.

The issue is not entirely due to the Trigger IMO. The CloudEvent spec for attribute keys is insufficiently expressive. ...

By shortcomings I mean Trigger does not support content-based filtering.

[nachocano]

Talked about this in the Sources WG. We added name, namespace, and kind as extension attributes.
Decided not to add labels nor annotations for the reasons Grant mentioned above.
I'm closing this one, please reopen if you have different thoughts and we can discuss further.