comment on exported function CreateL4EgressFilter should be of the form "CreateL4EgressFilter ..."

comment on exported function CreateL4EgressFilter should be of the form "CreateL4EgressFilter ..."

It might be worth noting that we expect perfect input in the function doc. We will, inevitably, call this with unsanitized addresses some day.

Do we also want to call ip.CoalesceCIDRs ?
Was the rational to add this function and types here because we need the auto-generated serialisation functions? It seems like a useful extension to the other package.

The rationale here is that #3835 will add some additional helper functions for CIDR / labels conversion, which will need this function. I didn't attempt to change any logic.

#1658 tracks making use of ip.CoalesceCIDRs.

Any chance you can make ingress bool a TrafficDirection type with two possible values? Calls to this function will then not have a mysterious boolean parament but we could also drop the ingress/egress wrappers.

Good point, I'll fix this up.

hrm, we have a TrafficDirection type, but it's specific to the BPF datapath at the moment. Are you suggesting introducing another such type for this layer? /cc @ianvernon

:/ I would prefer to reuse types if possible, but it's unlikely to be safe in this case. Perhaps it's out of scope since this is basically a bike-shed type thing on where such a thing should live and what it should be called.
Ultimately, my hope was to have a little less code. As it stands, it's readable since the Ingress/Egress is in the wrappers.

I took a stab at it, it removed about 20LoC and seems about as clear. Please take a look.

EDIT: Turns out I forgot to include the new file I added. It adds an extra 20 LoC, but that's mostly copyright header.

very keen! I might prefer the direction earlier in the parameter list but I'm not sure if a caller considers it the most important/global property of the filter (I do but I also asked you to expose the parameter directly so I am biased).

Putting TrafficDirection in the BPF datapath layer was a mistake on my part. IMO we can have it as its own little package? It'd make it much more portable and reusable.

This seems like a reasonable suggestion, but it's getting further and further from what I'm trying to achieve so I intend to just drop this for now.

Sure, that's fine.

Why is this deliberately distinct from policymap.TrafficDirection? Wouldn't it make more sense to put the type into its own package so it's more portable?

I thought about that, but then you're basically saying that the details of one datapath implementation should define the behaviour all the way up the userspace layers. Worst case, we do this and someone ends up thinking they can just extend the field later on or change its meaning and it breaks the datapath.

I'll drop that patch, it doesn't make a big difference and I'd rather focus on other things. Thanks for looking it over.

This looks like a functional change to me. Previously the incoming parameter was passed on. Now the possibly modified form is passed on.

I believe that this is the right behaviour. We should ensure that we are setting up the endpointselectors in the L4Filter, rather than relying on layers further down to treat the empty list of peerEndpoints as a wildcard selector.

/cc @ianvernon

Filter endpoints is only modified if the filter selects all endpoints. If it does, then this is just an optimization, which I don't see a problem with at face value.