Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
hcm: add match_upstream to SchemeHeaderTransformation #34099
hcm: add match_upstream to SchemeHeaderTransformation #34099
Changes from 1 commit
d419b59
0a542fb
07ccff2
92dd21e
16d82f8
5ea3820
9325d99
695663e
0e5e248
0a80882
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so there's a difference in what we pass in here
I think we need to decide if we want to set scheme based on "is upstream ssl" or "is upstream secure"
e.g. ALTS is secure but not TLS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the golang grpc and alts source code, it seems like this should only be set when the upstream is using TLS and not ATLS. The RFC also seems to couple it with TLS.
Given the number of transport sockets, I'm thinking of adding a new virtual method to the transport socket to allow each socket to select which scheme it would like to set. Let me know if you have any opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to keep it simple and just check if it has an SSL context (TLS) to set the scheme to https, otherwise use http.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level comment:
it seems that the relevant part of the test is essentially:
EXPECT_TRUE(filter->callbacks_->streamInfo().shouldSchemeMatchUpstream());
Can this test be refactored to minimize the non-relevant parts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cut as much of the test as I'm familiar with. Will need to dive deeper to see if any other cuts can be made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If this is not being overwritten, consider converting to const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need this to be non-const to override in the
PassMatchUpstreamSchemeHintToStreamInfo
test I wrote.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to subclass? Can't you just put the expect_call in your test body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed new class and moved code to router.cc where all the tests that use
RouterTest
live.