traffic simulation: add false positive support

[kailun-qin]

traffic simulation: add false positive support

Envoy is able to handle multiple configured filter chains w/ SNI wildcards,
which the current XDS traffic simulation flow considers as an error and
fails with a NACK.

This patch:

• extends the XDS traffic simulation flow with a false positive check support
  for multiple filter chain matched error on server_names;
• adds a new test for gateway using single virtual service with multiple TLS match blocks;
• removes confusing comments related to filter chain generation with SNI
  match specified in TLS match blocks.

Signed-off-by: Kailun Qin kailun.qin@intel.com

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[x] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Pull Request Attributes

Please check any characteristics that apply to this pull request.

[ ] Does not have any changes that may affect Istio users.

[istio-testing]

Hi @kailun-qin. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[howardjohn]

Sorry, I am not quite sure I understand this false positive stuff. If the logic for the filter chain matching is wrong, why not just modify the logic in L488-L498 that is doing the matching?

[kailun-qin]

Sorry, I am not quite sure I understand this false positive stuff. If the logic for the filter chain matching is wrong, why not just modify the logic in L488-L498 that is doing the matching?

There are several reasons:

1. The current filter() function returns all matching filter chains regardless of how we change the match() logic. This is different from Envoy's behavior on finding filter chains for wildcarded server names, which will anyway pick up the most specific one. I.e., if we'd like to simulate completely this behavior, we need a separate filter() specifically for server names.
2. Even w/ 1) done, we'll lose the intention of checking multiple filter chains matched for ALL the filters (which we tried to do on the completion of all filters w/ https://github.com/istio/istio/blob/master/pilot/pkg/simulation/traffic.go#L518). Since the filter() for server names will output one single chain matched after 1), it'll impact the successive levels of simulation as well.
3. In this simulation, we make assumptions that all filters can be added successfully and check multi filter chain on doing matching (though some may already make Envoy NACK on adding). We should/would like to cover both error in adding and matching.

Let me know if this explains a bit more. Or any other better way? Thanks!

[howardjohn]

Thank you! I will take another look

[howardjohn]

Your comment mentioned checking for overlapping FC when we create the objects instead of during match, but I don't see anything do that. is that intended? I think we could improve validateFilterChainMatch

[howardjohn]

Maybe it would be more clear to add the overlapping check there, then modify the filter() function to return the best match instead of all matches? I think that would mirror envoy logic better.

We may actually have false matches today. For example, if I have

- match: sni: *.com, transport: tls
- match: sni: example.com, transport: plain

and I send a request that is sni: example.com, transport: tls. Current logic would match FC 0; envoy would amtch neither I think

[kailun-qin]

Your comment mentioned checking for overlapping FC when we create the objects instead of during match, but I don't see anything do that. is that intended? I think we could improve validateFilterChainMatch

No, it is not included in this patchset. I did think that it should go that way but I was intended not to break too much on the current logic for this first attempt.
I agree that we could improve validateFilterChainMatch. What's in my mind is roughly similar to what you've suggested below. :)

[kailun-qin]

Maybe it would be more clear to add the overlapping check there, then modify the filter() function to return the best match instead of all matches? I think that would mirror envoy logic better.

We may actually have false matches today. For example, if I have

- match: sni: *.com, transport: tls
- match: sni: example.com, transport: plain

and I send a request that is sni: example.com, transport: tls. Current logic would match FC 0; envoy would amtch neither I think

Thanks a lot for the hints! I'll give a try for this approach.

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2021-01-28. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.