Skip to content

health check: add optional alternative health check port #597

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

Merged
mattklein123 merged 7 commits into from
Apr 3, 2018
Merged

health check: add optional alternative health check port #597

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d46994
Add optional alternative health check port
dio Apr 1, 2018
6023047
Review comment
dio Apr 2, 2018
88c37cd
Add health check config
dio Apr 3, 2018
b64c384
Re-index
dio Apr 3, 2018
71c5ac5
Re-index
dio Apr 3, 2018
0da7365
Rename
dio Apr 3, 2018
75662e3
Document non-zero
dio Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions envoy/api/v2/endpoint/endpoint.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,15 @@ option (gogoproto.equal_all) = true;

// Upstream host identifier.
message Endpoint {

@htuch htuch Apr 2, 2018

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.

Do we really need each endpoint to have its own alternative health check port, or can this just be specified at cluster level?

@dio dio Apr 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Some related discussions:

From above threads, I perceived that providing an alternative health check port per endpoint is preferred.

@mattklein123 mattklein123 Apr 2, 2018

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.

I think per-host is likely required due to potentially dynamic ports on the service side. In the cluster general situation it's more verbose, but I don't think we are optimizing for lack of verbosity here so I think it's OK?

@htuch htuch Apr 2, 2018

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.

SGTM

// The upstream host address.
core.Address address = 1;

// [#not-implemented-hide:] Optional alternative health check port.

@htuch htuch Apr 2, 2018

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.

How general is this? I'm wondering what other parameters might be configurable at the per-endpoint level, e.g. might we want to health check using TLS and a different cert, etc? Not really advocating to make this more complicated, but would like to make sure we have covered the potential endpoint health check specializations at least at the design level.

@dio dio Apr 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Probably this thread from @codesuki on #envoy-users could be related too?

@mattklein123 mattklein123 Apr 2, 2018

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.

+1 at least from the proto perspective. Can you maybe add a new message such as HealthCheckConfig (or something like that), and then put the port override in there? Then we can add other things in the future if needed.

From a doc perspective, we should make it clear that health checking must be configured at the cluster level for these options to take any effect, etc.

//
// By default the health check address port of an upstream host is the same as the host's serving

@htuch htuch Apr 2, 2018

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.

Presumably we're using uint32 rather than google.protobuf.UInt32Value because we can't possibly have a valid health check port of 0, since that doesn't make sense in TCP.

// address port. This provides an alternative health check port. If set it allows an upstream host
// to have different health check address port. The health checker should honor this.

@htuch htuch Apr 2, 2018

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 can drop the last sentence, seems superfluous.

uint32 health_check_port = 2;

@htuch htuch Apr 2, 2018

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.

I think we might eventually want something closer to

data-plane-api/envoy/api/v2/core/address.proto

Line 40 in f88047c

oneof port_specifier {
, but for now this is OK (we can always upgrade to a oneof without breaking wire compat).

}

// An Endpoint that Envoy can route traffic to.
Expand Down