Skip to content

Expose Virtual Host in Route Entry#314

Merged
ccaraman merged 3 commits intomasterfrom
vh-rl
Jan 5, 2017
Merged

Expose Virtual Host in Route Entry#314
ccaraman merged 3 commits intomasterfrom
vh-rl

Conversation

@ccaraman
Copy link
Copy Markdown
Contributor

@ccaraman ccaraman commented Jan 5, 2017

This expose the virtual host that owns the route entry. The virtual host is needed within http/filter/ratelimit.cc to be able to apply ratelimits at the virtual host layer. The next PR will expose RateLimitPolicy on VirtualHost.

This PR also enhances test coverage on RateLimitPolicyEntry and simplifies the test code within router_ratelimit_test.cc

@ccaraman
Copy link
Copy Markdown
Contributor Author

ccaraman commented Jan 5, 2017

@lyft/network-team

VirtualHostImpl(const Json::Object& virtual_host, Runtime::Loader& runtime,
Upstream::ClusterManager& cm);

// Router::VirtualHost
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: interface implementation definitions go below the other random methods.

public:
// Router::VirtualHost
const std::string& name() const override { return name_; }
std::string name_{"fake_vhost"};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: newline before this line

TEST(BadRateLimitConfiguration, MissingActions) {
std::string json = R"EOF(
{
"virtual_hosts": [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You still need some test that tests that the main config properly loads an embedded rate limit. Do you still have at least one test that loads a full config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is below. The tests are under the RateLimitConfiguration class.

Copy link
Copy Markdown
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 after nits

@ccaraman ccaraman merged commit 8337bb1 into master Jan 5, 2017
@ccaraman ccaraman deleted the vh-rl branch January 5, 2017 17:44
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
* Basic integration test

* Addressed comments
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
Automatic merge from submit-queue.

[DO NOT MERGE] Auto PR to update dependencies of mixerclient

This PR will be merged automatically once checks are successful.
```release-note
none
```
lizan pushed a commit to lizan/envoy that referenced this pull request Nov 25, 2019
Fixes envoyproxy#161.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
mathetake pushed a commit that referenced this pull request Mar 3, 2026
**Commit Message**

Upgrading to docusaurus to version 3.7.0 and related dependencies.

Signed-off-by: Erica Hughberg <erica.sundberg.90@gmail.com>
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.

2 participants