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

health check: allow request header formatting for HTTP health check request #3882

Merged
merged 3 commits into from
Jul 26, 2018

Conversation

dio
Copy link
Member

@dio dio commented Jul 18, 2018

Description:
This PR enables the use of header formatting pattern for HTTP health check request.
And while at this, the hostname initialization is moved to the HttpActiveHealthCheckSession's constructor for consistency.

Risk Level: Low.
Testing: Unit test.
Docs Changes: Updated.
Release Notes: Added.

Fixes #3835

Signed-off-by: Dhi Aurrahman dio@rockybars.com

@dio dio changed the title health check: allow request header formatting for health check request health check: allow request header formatting for HTTP health check request Jul 18, 2018
@junr03 junr03 self-assigned this Jul 18, 2018
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@bplotnick
Copy link
Contributor

Thanks for doing this! I tried this branch out and it definitely fixed my issue (injecting upstream metadata)

Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks for this change.

Could you add a link from the HttpHealthCheck request_headers_to_add field to the Http Connection Manager custom request headers section and a note in version_history.rst? (The ref is <config_http_conn_man_headers_custom_request_headers>.)

@zuercher zuercher self-assigned this Jul 23, 2018
dio added 2 commits July 24, 2018 01:09
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Jul 23, 2018

@bplotnick thank you for verifying this!

@zuercher, thanks for the review. Updated the doc and release note in here: 75d1be9.

@dio
Copy link
Member Author

dio commented Jul 26, 2018

@zuercher is there anything else that I need to look at? Thanks

@zuercher
Copy link
Member

@alyssawilk do you have time to take a look?

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, thanks for fixing this!

@mattklein123 mattklein123 merged commit 42109e5 into envoyproxy:master Jul 26, 2018
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.

6 participants