Consolidate Filter Trait to two functions about quilkin HOT 8 CLOSED

This sounds reasonable to me.

Not sure if I like the filter names local_receive_and_send_to_endpoint and endpoint_receive_and_send_to_local, but I feel like they are clear on what is happening at each point. If anyone has alternatives, would love to hear them.

If we can say that the proxy receives data upstream and sends response back downstream, we should be able to call them onReceive and onSend?

from quilkin.

My concern with onReceive and onSend (or similar), is that without memorising and internalising the flow, it could be easy to think:

"Wait, am I receiving from the local port, or the endpoint?" or "am I sending from the local port or the endpoint?" and then have to look at the docs (probably several times 😄 )

I had similar thoughts about maybe doing local and endpoint (or onLocal)... but I'm concerned the same confusion may still exist 🤔

from quilkin.

Another suggestion is onSendUpstream and onSendDownstream that has the direction implied in the name, is this clearer do you think? We would also be able to use direction = upstream | downstream globally for filter configurations that need a direction like the rate limiter

from quilkin.

I'm definitely on board with the on_ nomenclature - I like that it has the event feel.

how would on_local_receive and on_endpoint_receive work for you? It is basically what kicks off the event - a packet is received at either end. Then where it goes is up to you.

I'm just trying to limit the amount of terms a developer has to internalise. We already have "local" and "endpoint" - if we add more things (upstream/downstream), it's yet another thing to memorise.

WDYT?

I started doing some of the refactoring work on this, since it touches a lot of things, and it sounds like we're now just discussing naming.

from quilkin.

My reservation is mostly about how we'll be using endpoint to mean something specific in this case and that we'll need to clarify it in other places it since any party with an address is an endpoint normally. I was thinking upstream/downstream could replace local/endpoint since it's already a known terminology in networking and won't need explanation - and it could for example be akward to say direction: endpoint in rate limiter config.
This isn't a big deal we can also leave as-is with local_receive_and_send_to_endpoint

from quilkin.

Checking terminology (and piggybacking on some conversations re: xDS as well)
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/endpoint/v3/endpoint_components.proto

Endpoint is an existing term, but they also they use the term upstream to define an endpoint as well.

So now I'm coming around again 😄 to upstream/downstream terminology (especially if we take on xDS terminology, which is more complex than just local/endpoint)

I would probably argue on_downstream_receive and on_upstream_receive rather then send, just because the event is fired on packet being received, rather than sent.

@luna-duclos do you have thoughts, coming off the back of xDS conversations?

from quilkin.

on_downstream_receive/on_upstream_receive works as well for me.

from quilkin.

I BELIEVE WE HAVE CONSENSUS 🎉

from quilkin.