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.

Sign up for GitHub

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

Jump to bottom

Route-local fault filter config #3084

Merged
merged 45 commits into from
Apr 20, 2018
Merged

Route-local fault filter config #3084

merged 45 commits into from
Apr 20, 2018

Conversation

rshriram
Copy link
Member

@rshriram rshriram commented Apr 16, 2018 •
edited
Loading

Follow up from @rodaine's route-local config framework. Enabling per-route/per-vhost config for fault filters. (#3045).

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

Chris Roche and others added 20 commits April 11, 2018 15:11
Router: Route/VHost/W.Cluster local filter configuration
636e605 
Signed-off-by: Chris Roche <croche@lyft.com>
format
b2d0263 
Signed-off-by: Chris Roche <croche@lyft.com>
missed async client types
134734b 
Signed-off-by: Chris Roche <croche@lyft.com>
fix repo locations
0cc834f 
Signed-off-by: Chris Roche <croche@lyft.com>
s/RDS/Route/
35f37f7 
Signed-off-by: Chris Roche <croche@lyft.com>
remove unneccessary template var
4820c48 
Signed-off-by: Chris Roche <croche@lyft.com>
the nullptr likes to be called just nullptr
d76ad4d 
Signed-off-by: Chris Roche <croche@lyft.com>
checkpoint
6309d41 
Signed-off-by: Chris Roche <croche@lyft.com>
s/ASSERT/EXPECT/
9897138 
Signed-off-by: Chris Roche <croche@lyft.com>
dependency shuffle
118c728 
Signed-off-by: Chris Roche <croche@lyft.com>
format + convert test to use fixtures with dummy filter
711f59f 
Signed-off-by: Chris Roche <croche@lyft.com>
Merge branch 'route-local-config' of https://github.com/rodaine/envoy …
0aa06d6 
…into perfilterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rodaine
Merge branch 'master' into route-local-config
472f7ae
compilation fixes post repo reorg
aad91ea 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
converting Fault Filter to perfilterconfig
99039ab 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Merge branch 'master' of https://github.com/envoyproxy/envoy into per…
c3d6347 
…filterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Merge branch 'route-local-config' of https://github.com/rodaine/envoy …
fe20045 
…into perfilterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
comment nits
c2f915b 
Signed-off-by: Chris Roche <croche@lyft.com>
@rodaine
Merge branch 'master' into route-local-config
cbd19d4
Merge branch 'route-local-config' of https://github.com/rodaine/envoy …
c78fca0 
…into perfilterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@mattklein123
Copy link
Member

@rodaine can you own first review on this.

Merge branch 'master' of https://github.com/envoyproxy/envoy into per…
eb56fdd 
…filterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram rshriram changed the title [WIP] Route-local fault filter config Route-local fault filter config Apr 16, 2018
Shriram Rajagopalan added 6 commits April 16, 2018 17:15
well known names
298e226 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
remove unnecessary includes
73f2f66 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
missing includes
295f82a 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
format
0d5fe62 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Merge branch 'master' of https://github.com/envoyproxy/envoy into per…
6d1838f 
…filterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
moar compile fixes
c0c6f3a 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Member Author

done..

mattklein123
mattklein123 previously approved these changes Apr 18, 2018
View reviewed changes
@mattklein123
Copy link
Member

Holding on merging until we resolve #3128 (review). cc @ggreenway @rodaine

@rshriram rshriram mentioned this pull request Apr 18, 2018
Merged
Merge remote-tracking branch 'origin/master' into perfilterconfig
ca301f9 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@mattklein123
Copy link
Member

@rshriram can you merge master and fix this up? Then we can get this in.

rework based on new interface
739e572 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Shriram Rajagopalan added 3 commits April 19, 2018 21:22
format
70f13d0 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
fixes
ce73907 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
cleanup
897f78f 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Member Author

@mattklein123 updated

mattklein123
mattklein123 reviewed Apr 20, 2018
View reviewed changes
source/extensions/filters/http/fault/config.cc Outdated
envoy::config::filter::http::fault::v2::HTTPFault proto_config;
MessageUtil::jsonConvert(struct_config, proto_config);
Router::RouteSpecificFilterConfigConstSharedPtr rconfig;
rconfig.reset(new Fault::FaultSettings(proto_config));
Copy link
Member

Choose a reason for hiding this comment

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

nit: return std::make_shared<...>(...)

source/extensions/filters/http/fault/fault_filter.cc Outdated
// faults. In other words, runtime is supported only when faults are
// configured at the filter level.
if (callbacks_->route() && callbacks_->route()->routeEntry()) {
const std::string name = Extensions::HttpFilters::HttpFilterNames::get().FAULT;
Copy link
Member

Choose a reason for hiding this comment

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

nit: const std::string&

source/extensions/filters/http/fault/fault_filter.cc Outdated
const FaultSettings* per_route_settings =
route_entry->perFilterConfigTyped<FaultSettings>(name)
?: route_entry->virtualHost().perFilterConfigTyped<FaultSettings>(name);
config_->updateFaultSettings(per_route_settings);
Copy link
Member

Choose a reason for hiding this comment

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

Aren't you effectively updating a global shared pointer here? I don't think this is right? Can you make sure FaultFilterConfigSharedPtr is actually FaultFilterConfigConstSharedPtr to catch this?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack.. good catch..

Copy link
Member Author

Choose a reason for hiding this comment

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

now that I think about it, the reason I did this was because I wanted to avoid allocation in the request path. I couldn't create an identical config object for RouteSpecific.. because I don't have access to runtime/stats/scope etc in that API Call.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Then this is the same problem as Lua. We need to fix this now I guess and be able to pass the full FactoryContext into the factory function. Can you take a look at that?

Copy link
Member Author

Choose a reason for hiding this comment

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

ignore please. fixed it

Shriram Rajagopalan added 3 commits April 20, 2018 12:02
Merge branch 'master' of https://github.com/envoyproxy/envoy into per…
c095075 
…filterconfig

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
fixes
e58e958 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
format
46dbf9f 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
mattklein123
mattklein123 reviewed Apr 20, 2018
View reviewed changes
source/extensions/filters/http/fault/fault_filter.h Outdated
Runtime::Loader& runtime_;
FaultFilterStats stats_;
const std::string stats_prefix_;
Stats::Scope& scope_;
const FaultSettings settings_;
};

typedef std::shared_ptr<FaultFilterConfig> FaultFilterConfigSharedPtr;
Copy link
Member

@mattklein123 mattklein123 Apr 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can you change this typedef to std::shared_ptr<const FaultFilterConfig> FaultFilterConfigConstSharedPtr

mattklein123
mattklein123 reviewed Apr 20, 2018
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM other than small comment.

const faultfilterconfig
80a3aaa 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@mattklein123
Copy link
Member

@rshriram check OSX failure, it's real compiler error.

Shriram Rajagopalan added 2 commits April 20, 2018 14:26
const everything
9a15b9f 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
unconst
5982b5f 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Member Author

so, I cant really do const filterconfig.. because the filter config has stats as well, which get incremented whenever faults are injected..

alignment fix
1e585b6 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@mattklein123
Copy link
Member

@rshriram you should be able to fix with a mutable on the stats.

fix conditional assignment
5e893aa 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Member Author

Tests passing finally.

mattklein123
mattklein123 approved these changes Apr 20, 2018
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

nice

@mattklein123 mattklein123 merged commit cf1e2df into envoyproxy:master Apr 20, 2018
@rshriram rshriram mentioned this pull request Jun 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@rodaine rodaine rodaine approved these changes

Assignees

@rodaine rodaine

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rshriram @mattklein123 @rodaine