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 scoped RDS routing into HCM #7762
[hcm] Add scoped RDS routing into HCM #7762
Changes from 104 commits
6a3f13b
cc806d7
bb55a44
8d7b4da
8bcddce
b1d0d5c
1007ee0
184ff2f
fe6b643
fc98e56
951bd28
75718d9
d84839d
27ff0a3
36a6e72
46e23f2
74038f4
ff7ccdb
06bd690
cf38720
5cad3ee
50e8fb5
ac65ba2
cb5324f
5c8a546
4a1fb01
fdd0a74
467a6a1
7dea1a0
c354a1e
2b5f572
9dd4214
5d2f8a2
86c455e
60dd433
6c1a672
c31de28
abfe558
69df754
dad2d59
6b6a0ad
964de34
5f357bd
dd3b97d
e796b09
6699279
75c8f49
fb28731
1bd43be
e43d8eb
f074e9d
7cbb6ae
fb7867b
ee09a5b
46ad9af
47a417d
e328d46
9c1f616
ae2ac31
b1acd70
d08bdb2
eb65b89
07bdfee
40b214e
dcea85c
4e57308
710c7e8
53081b2
2593bfa
2dd6714
cef747d
bb0cdb8
fa6ecb1
efaa891
bf4dc23
bbae81c
d847a24
e329f49
c4719d8
56e2876
d3a4dde
d26d9b7
9f37e93
14dd4c7
8addd25
ef07569
facf46d
aa4949a
3f2de6d
d20590c
c971cf3
bbb7918
121a4a7
ae88649
2386e93
6f67b78
3a39c68
0c3b2ed
fde988b
e111c0c
ea0ec01
97c1496
3af2d3b
430c450
4a0ef9a
e2dbe75
ab0eabb
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.
Need to be alpha sorted, otherwise LGTM.
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.
done.
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.
drive by follow up comment: I'm not crazy about including admin here and then special casing it with dynamic casts. Is it not possible for this logical to be external to the construction of the HCM and then specified correctly either by admin or by the normal filter?
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.
we've discussed it offline, I will make a cleanup/refactor PR to move the dynamic_cast outside of conn_manager_impl.
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.
Can't we rely on the
refreshCAchedRoute()
at the end of this method to perform the snap?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.
Probably not: there is header cleansing logic around line 771 that depends on snapped route config, so latching the route-config here is required.
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.
Fair, but please document this dependency in a comment here. Can you move this closer to where we use it also? I generally dislike action at a distance.
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.
done.
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.
I think it also makes sense to config guard this behavior via bootstrap config.
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.
I think you mean in http_connnection_manager.proto? it's a one-of there already. so it's kinda guarded there.
The Server::Admin impl is just a special HCM impl which doesn't need any routing.