Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add listener message validation based on semantic #15372

Open
lambdai opened this issue Mar 8, 2021 · 8 comments
Open

add listener message validation based on semantic #15372

lambdai opened this issue Mar 8, 2021 · 8 comments

Comments

@lambdai
Copy link
Contributor

lambdai commented Mar 8, 2021

We have tcp listener, udp listener, api listener in the same Listener message ,
and I am adding another internal listener.

These listeners have their own feature set. e.g. connection_balance_config should not be used by udp listener, filter_chains should only be used by tcp listener (and quic ?), not all sock options are for udp, vice versa.

We don't have adequate validation for the above implicit rules for now.

It's probably good to warn the user upon receiving listener config, starting from "some fields are contradicting or ignored" and eventually reject inconsistent config.

WDYT?

@lambdai lambdai added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Mar 8, 2021
@mattklein123
Copy link
Member

Really the entire listener API is broken and in a perfect world needs to get redone, into essentially:

  1. Common fields for all listener types
  2. oneof of each actual listener type

If you want to start working towards this that would be great. cc @envoyproxy/api-shepherds

@mattklein123 mattklein123 added area/configuration area/listener help wanted Needs help! and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Mar 8, 2021
@htuch
Copy link
Member

htuch commented Mar 9, 2021

+1, this is one of those places that being able to make a more substantial refactor at proto level would be great, but I think we have to live with what we can in the constraints of v3.

@lambdai
Copy link
Contributor Author

lambdai commented Mar 9, 2021

I am adding more data point (or tech debt 😄 ) to this issue is #15376

+1, this is one of those places that being able to make a more substantial refactor at proto level would be great, but I think we have to live with what we can in the constraints of v3.

That being said a validator is a low-hanging fruit under api v3. And the validator might have value in v4 if the proto annotation doesn't provide completeness.

@mattklein123
Copy link
Member

IMO we don't need to wait until v4 (since at this point v4 is in the too far in the future / never category in my brain). We can start by adding a oneof for the new listener type and then we can reorganize via deprecations with the minor version proposal and get there over a few years. I will comment on the other PR.

@soulxu
Copy link
Member

soulxu commented May 20, 2021

@lambdai do you need any volunteers on this? I'm looking for task :)

@lambdai
Copy link
Contributor Author

lambdai commented May 21, 2021

@lambdai do you need any volunteers on this? I'm looking for task :)

Thank you! Go for it!

@daixiang0
Copy link
Member

@soulxu do you still work for this?

@soulxu
Copy link
Member

soulxu commented Jun 28, 2021

@daixiang0 yea, I'm working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants