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.

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

migrate health check filter to v2 api #2047

Merged
merged 71 commits into from
Nov 10, 2017

Conversation

rshriram
Copy link
Member

title: migrate health check filter to v2 api

Description: add functionality to populate health check filter directly from protos
Risk Level: Low
Testing: unit tests, and existing health check filter tests remain untouched.

Shriram Rajagopalan added 30 commits October 2, 2017 17:07
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
fix
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
fix
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Shriram Rajagopalan added 13 commits November 10, 2017 01:02
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
@mattklein123
Copy link
Member

Let's wait for the other PR to merge and then please rebase this on master.

Shriram Rajagopalan added 2 commits November 10, 2017 12:19
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
@mattklein123
Copy link
Member

please squash rebase on origin/master

Shriram Rajagopalan added 2 commits November 10, 2017 13:53
…ck_v2

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
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, small nit

envoy::api::v2::filter::http::HealthCheck& health_check) {
json_health_check.validateSchema(Json::Schema::HEALTH_CHECK_HTTP_FILTER_SCHEMA);

health_check.mutable_pass_through_mode()->set_value(
Copy link
Member

Choose a reason for hiding this comment

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

JSON_UTIL_SET_BOOL

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>
bool pass_through_mode = config.getBoolean("pass_through_mode");
int64_t cache_time_ms = config.getInteger("cache_time_ms", 0);
std::string hc_endpoint = config.getString("endpoint");
bool pass_through_mode = health_check.pass_through_mode().value();
Copy link
Member

Choose a reason for hiding this comment

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

sorry as long as you are in here can you make these all const?

Copy link
Member

Choose a reason for hiding this comment

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

That's fine we can just merge this and can fix in a follow up.

@rshriram
Copy link
Member Author

Oh yeah good point. I am off from today. I can fix these in a follow up PR when I return later next week or if you feel this is critical, let this stay and I will get to it mid next week.

@mattklein123 mattklein123 merged commit e48d422 into envoyproxy:master Nov 10, 2017
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* Bump enoy version to pickup json access log fix

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

* Add sha256sum and note about how to retrieve the sum

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
When building the jni dylib for android, we previously stripped all
debug info to decrease the artifact size. With this change we now
produce the same stripped binary as before, but before stripping it we
create a dump of the debug info suitable for crash reporting.

This is made overly difficult for a few reasons:

1. Bazel doesn't support fission for Android bazelbuild/bazel#14765
2. Extra outputs from rules are not propagated up the dependency tree,
   so just building `android_dist` at the top level, isn't enough to get
   the extra outputs built as well
3. Building the library manually alongside the android artifact on the
   command line results in 2 separate builds, one for android as a
   transitive dependency of `android_dist` and one for the host
   platform

This change avoids #1 fission for now, but the same approach could be used
once that change makes its way to a bazel release.

This change fixes #2 by using a separate output group that can be depended
on by the genrule that writes to dist while avoiding #3 because the custom
rule producing these uses the android transition.

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
When building the jni dylib for android, we previously stripped all
debug info to decrease the artifact size. With this change we now
produce the same stripped binary as before, but before stripping it we
create a dump of the debug info suitable for crash reporting.

This is made overly difficult for a few reasons:

1. Bazel doesn't support fission for Android bazelbuild/bazel#14765
2. Extra outputs from rules are not propagated up the dependency tree,
   so just building `android_dist` at the top level, isn't enough to get
   the extra outputs built as well
3. Building the library manually alongside the android artifact on the
   command line results in 2 separate builds, one for android as a
   transitive dependency of `android_dist` and one for the host
   platform

This change avoids #1 fission for now, but the same approach could be used
once that change makes its way to a bazel release.

This change fixes #2 by using a separate output group that can be depended
on by the genrule that writes to dist while avoiding #3 because the custom
rule producing these uses the android transition.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.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