Skip to content

Conversation

@ejona86
Copy link
Member

@ejona86 ejona86 commented Jul 21, 2021

This is to match Envoy's handling. See also
#250 for the http filter change.

This is to match Envoy's handling. See also
grpc#250 for the http filter change.
@ejona86 ejona86 requested review from dfawley and markdroth July 21, 2021 18:36
@markdroth markdroth changed the title A36: Require HCM to be the terminal network filter A36 update: Require HCM to be the terminal network filter Jul 21, 2021
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @zasweq

@ejona86
Copy link
Member Author

ejona86 commented Jul 21, 2021

CC @yashykt, @YifeiZhuang

@ejona86 ejona86 merged commit 2d1fc6d into grpc:master Jul 21, 2021
@ejona86 ejona86 deleted the terminal-filters branch July 21, 2021 22:42
@YifeiZhuang
Copy link
Member

CC @yashykt, @YifeiZhuang

@ejona86 thank you.
To make sure I understand things correctly and completely, we do will do these in java, right?

  1. add isTerminalFilter() API in Filter interface and their implementations.
  2. add isTerminal == isLast validation is XdsClient upon receiving listener update response, for both client and server httpFilter configs.
  3. remove LameFilter entirely.
  4. change to only allow one HCM in each server listener filterChain in XdsClient parsing validation logic.

@ejona86
Copy link
Member Author

ejona86 commented Jul 22, 2021

add isTerminalFilter() API in Filter interface and their implementations.

I don't think we'll add the API isTerminalFilter(). From what I saw, it is easiest to just hard-code the check for Router in validation of XdsClient. But yes, the other parts are what I was thinking.

Note that the HCM validation logic is already hard-coded; it seems with this change we don't even need a loop any more, but whatever ¯_(ツ)_/¯

@YifeiZhuang
Copy link
Member

I don't think we'll add the API isTerminalFilter(). From what I saw, it is easiest to just hard-code the check for Router in validation of XdsClient. But yes, the other parts are what I was thinking.

Got it. Just it might be a conflict for the coming soon server validation because java share the same validation code. But this is trivial. As long as we know that "Router is not the last" is invalid ¯_(ツ)_/¯

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants