Add option INSERT_FIRST in EnvoyFilter.Patch.Operation

[gargnupur]

Add option INSERT_FIRST in EnvoyFilter.Patch.Operation. This is so that a filter can be added to the top of the list based on Match clause too.

[mandarjog]

@rshriram the use case here is we want to insert metadata exchange filter as the 1st filter in the filter chain.

We need to specify what happens when 2 configs match an insertion point and both want to be first

1. Reject config as invalid.
2. Specify insert order consistent with others in EnvoyFilter. This is problematic because if 2 filters really want to be insert_first then the use needs to reconcile the config beforehand. Two insert_first type configs are possible when using double tunneling.

[hzxuzhonghu]

We need to specify what happens when 2 configs match an insertion point and both want to be first

To keep it consistent with other configs, we should take precedence by creation time. In case it is influenced by a later misconfigured one.

[rshriram]

Why is https://github.com/istio/api/pull/1234/files#diff-fa79aef151053e8b9e7be36c58eeaf39R631-R633 not sufficient? @louiscryan did not want insert first. Instead he wanted to add this rider to INSERT_BEFORE (as written in the comment above). I dont mind adding it for clarity. And what about the other one that kuat had? the filter class so that we can intelligently order groups of filters without users having to specify the exact location?

[mandarjog]

@rshriram we can discuss the filter classes in the other PR.
We had missed the encapsulation/tunneling use case in the other API.
We need this update to the EnvoyFilter api because we cannot wait for the API redesign.
Tunneling requires that this filter be first. In order to use Insert_before to approximate insert_first you need to know other filters in the chain which may change.

[rshriram]

In order to use Insert_before to approximate insert_first you need to know other filters in the chain which may change.

This is what I was pointing out. If you omit the filter/subfilter match completely, then INSERT_BEFORE acts as an insert first in the filter list (in http or network as applicable). Is this semantic insufficient?

[mandarjog]

Aah ok, thanks for point it out. We will give it a try. @gargnupur can you try it out?

[rshriram]

out of curiosity, why do we need to keep the metadata filter in EnvoyFilter? why can't we program it from Pilot as part of the core filter stack?

[gargnupur]

@rshriram , @mandarjog : I saw that INSERT_BEFORE acts as INSERT_FIRST when there is no filter/subfilter match but there are 2 problems with it:

1. I think it is required to specify a match.. I get error if I don't
2. Based on this, filter will get added in all filter chains even when tcp proxy is not there. There can be scenarios where its can get added when there was no filter chain and we will get an error that a non terminal filter is added in the filter chain.
   (I am verifying this too)

[mandarjog]

Metadata filter does not have config so we can actually program it in pilot and add a meshconfig setting. I will evaluate that.

[gargnupur]

We get this error if a match is not specified:
"spec.configPatches.match" must validate one and only one schema (oneOf). Found none valid
spec.configPatches.match.listener in body is required
because it's a oneof..

[gargnupur]

Specifying empty listener match made it work. The only caveat is that filter gets added in http filter chains too. Although, should not be a problem as filter will get skipped for Http as agreed ALPN for http won't be istio-peer-exchange...
So, although this will work in our scenario but it would be nice to have more control on where exactly filter gets added based on some matches in filterchains..

[hzxuzhonghu]

So, although this will work in our scenario but it would be nice to have more control on where exactly filter gets added based on some matches in filterchains..

How would INSERT_FIRST solve this?

[gargnupur]

So, although this will work in our scenario but it would be nice to have more control on where exactly filter gets added based on some matches in filterchains..

How would INSERT_FIRST solve this?

@hzxuzhonghu : With INSERT_FIRST, we can have a match clause that says, add a filter on top of the filter chain list, if it finds tcp_proxy in the filter chain. So filter will get added in tcp chain and not http chain..

[hzxuzhonghu]

Another question: how will know whether it should be applied to tcp filter instead of http? Specify it in Match?

[gargnupur]

Another question: how will know whether it should be applied to tcp filter instead of http? Specify it in Match?

Yes.. we can specify match for tcp_proxy v/s http mangager in listener filter chain match

[gargnupur]

@rshriram : Thanks for the review!
@smawson , @linsun , @dcberg , @louiscryan , @geeknoid : Can one of you please take a look at this pr?