Consistently handle filters with empty string as value on optional attributes and extensions

[pierDipi]

Describe the bug

// TriggerFilterAttributes is a map of context attribute names to values for
// filtering by equality. Only exact matches will pass the filter. You can use the value ''
// to indicate all strings match.

Our implementation:

eventing/pkg/eventfilter/attributes/filter.go

Lines 37 to 74 in daa085d

	func (attrs attributesFilter) Filter(ctx context.Context, event cloudevents.Event) eventfilter.FilterResult {
	// Set standard context attributes. The attributes available may not be
	// exactly the same as the attributes defined in the current version of the
	// CloudEvents spec.
	ce := map[string]interface{}{
	"specversion": event.SpecVersion(),
	"type": event.Type(),
	"source": event.Source(),
	"subject": event.Subject(),
	"id": event.ID(),
	"time": event.Time().String(),
	"schemaurl": event.DataSchema(),
	"datacontenttype": event.DataContentType(),
	"datamediatype": event.DataMediaType(),
	// TODO: use data_base64 when SDK supports it.
	"datacontentencoding": event.DeprecatedDataContentEncoding(),
	}
	ext := event.Extensions()
	for k, v := range ext {
	ce[k] = v
	}
	for k, v := range attrs {
	var value interface{}
	value, ok := ce[k]
	// If the attribute does not exist in the event, return false.
	if !ok {
	logging.FromContext(ctx).Debug("Attribute not found", zap.String("attribute", k))
	return eventfilter.FailFilter
	}
	// If the attribute is not set to any and is different than the one from the event, return false.
	if v != eventingv1beta1.TriggerAnyFilter && v != value {
	logging.FromContext(ctx).Debug("Attribute had non-matching value", zap.String("attribute", k), zap.String("filter", v), zap.Any("received", value))
	return eventfilter.FailFilter
	}
	}
	return eventfilter.PassFilter
	}

---

Given a filter like:

subject: ""

and an event like:

# ... all required attributes

the filter passes even though there is no subject.

---

Given a filter like:

myextension: ""

and an event like:

# ... all required attributes

the filter doesn't pass because there is no extension myextension.

Additional context

• discussion Add filterEventByAttributes tests #3647

[zhongduo]

IMO empty string should have the same meaning as not having the key, which means all strings.

[pierDipi]

IMO empty string should have the same meaning as not having the key, which means all strings.

This is what we're doing in Kafka broker. so +1.
However, another understandable opinion that came up during a meeting was that it seems wonky since it's equivalent to no filter for that attribute.

@knative/eventing-wg-leads, please, share your opinions.

[grantr]

+1 to the behavior suggested by @zhongduo and implemented in the Kafka broker. It is unfortunate that there's no way to match a message missing an attribute, but perhaps that should be handled by a different filter type specifically for matching attribute presence.

[vaikas]

Hm, I'm not sure. I actually think if we change it to mean there's no event, then it's not possible to say to have a filter that gets all the events that have a particular extension regardless of what it is. I'm actually thinking we should possibly change the Subject = ''
to only match if there's a subject on the event.

Otherwise it seems goofy because then:
missing myextension => all events that do not have myextension
myextension = "" all events that have and do not have myextension
myextension = "myexactmatch" only with that particular exact myexactmatch.

So, I would flip it actually around and say that subject behaviour is broken and that should be changed.

[vaikas]

@grantr @pierDipi @zhongduo Thoughts on ☝️ seemingly unpopular opinion :)

[pierDipi]

@vaikas it makes a lot of sense to me your opinion, and since it increases the filter's expressivity, I like it. 👍

[n3wscott]

We have clarified this in the CloudEvents spec and the ruling might apply here: an empty string is not allowed, and a nil value is equivalent to not set.