Skip to content

healthcheck: Allow setting host header value of HTTP health checker#2591

Merged
htuch merged 8 commits intoenvoyproxy:masterfrom
dio:add-host-http-healthcheck
Feb 13, 2018
Merged

healthcheck: Allow setting host header value of HTTP health checker#2591
htuch merged 8 commits intoenvoyproxy:masterfrom
dio:add-host-http-healthcheck

Conversation

@dio
Copy link
Member

@dio dio commented Feb 12, 2018

title: healthcheck: Allow setting host header value of HTTP health checker

Description:
This patch enables setting host header value as mentioned in HttpHealthCheck.host.

Risk Level: Low

Testing:
unit and manual testing

Docs Changes: envoyproxy/data-plane-api#493

Release Notes:
Enable setting host header value for HTTP HC

Fixes: #2450

API Changes:
envoyproxy/data-plane-api#493

dio added 5 commits February 13, 2018 05:54
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.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.

Very nice, thanks! Question about v1 support.

const std::string hc_type = json_health_check.getString("type");
if (hc_type == "http") {
health_check.mutable_http_health_check()->set_path(json_health_check.getString("path"));
if (json_health_check.hasObject("host")) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need v1 support? If so we need to add v1 docs. Otherwise I would just drop it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed the v1 support in the following commit 5937131

}

void setupServiceValidationWithCustomHostValueHC(const std::string& host) {
std::string json = fmt::format(R"EOF(
Copy link
Member

Choose a reason for hiding this comment

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

If we drop v1 support, let's make this a v2 YAML config for testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I have it in v2 YAML on b3f2c8f. Thanks for reviewing @mattklein123!

dio added 3 commits February 13, 2018 07:52
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks.

}

TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithCustomHostValue) {
std::string host = "www.envoyproxy.io";
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

@htuch htuch merged commit 6f8a368 into envoyproxy:master Feb 13, 2018
@dio dio mentioned this pull request Feb 14, 2018
htuch pushed a commit that referenced this pull request Feb 15, 2018
Fix some nit for PR #2591.

Risk Level: Low

Testing:
unit

Docs Changes:
N/A

Release Notes:
N/A
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* expose service port

* fix?

* fix??

* fix

* last fix

* typo

* port

* hard code

* use server port

* last fix

* remove spaces
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: Add support for PAC proxies. Fixes envoyproxy/envoy-mobile#2531.
Risk Level: Low, the change should be additive and it's guarded with an engine builder flag (`enableProxying(true/false)` that's disabled by default.
Testing: Manual testing for PAC proxies, integrations tests for other types of proxies.
Docs Changes: N/A
Release Notes: WIP

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: Add support for PAC proxies. Fixes envoyproxy/envoy-mobile#2531.
Risk Level: Low, the change should be additive and it's guarded with an engine builder flag (`enableProxying(true/false)` that's disabled by default.
Testing: Manual testing for PAC proxies, integrations tests for other types of proxies.
Docs Changes: N/A
Release Notes: WIP

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.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.

3 participants