feat: support Health Check in BackendTrafficPolicy #2244

lemonlinger · 2023-11-27T07:36:17Z

What type of PR is this?

What this PR does / why we need it:

This PR introduces active health checking to BackendTrafficPolicy, supporting 3 types of health checkers: HTTP, gRPC, and TCP.

Which issue(s) this PR fixes:

relates to #2126

codecov · 2023-11-27T07:40:08Z

Codecov Report

Attention: 136 lines in your changes are missing coverage. Please review.

Comparison is base (caf2ddb) 64.64% compared to head (06973fe) 64.60%.

Files	Patch %	Lines
internal/ir/zz_generated.deepcopy.go	0.00%	105 Missing and 1 partial ⚠️
internal/gatewayapi/backendtrafficpolicy.go	83.56%	8 Missing and 4 partials ⚠️
internal/ir/xds.go	90.72%	7 Missing and 2 partials ⚠️
internal/xds/translator/cluster.go	88.46%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2244      +/-   ##
==========================================
- Coverage   64.64%   64.60%   -0.04%     
==========================================
  Files         115      115              
  Lines       16844    17199     +355     
==========================================
+ Hits        10888    11112     +224     
- Misses       5260     5382     +122     
- Partials      696      705       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shawnh2

thanks @lemonlinger , it's better to add some e2e test and user facing doc in the following PR.

api/v1alpha1/healthcheck_types.go

internal/xds/translator/testdata/in/xds-ir/health-check.yaml

internal/xds/translator/cluster.go

internal/ir/xds.go

api/v1alpha1/healthcheck_types.go

arkodg · 2023-11-27T18:28:07Z

api/v1alpha1/healthcheck_types.go

+	// +kubebuilder:validation:Minimum=1
+	HealthyThreshold uint32 `json:"healthyThreshold"`
+
+	// HealthChecker defines the concrete health checker to do health checking.


we need a better name here, it currently looks like

healthCheck: healthChecker: .....

What about checker ?

healthCheck: checker:

Thoughts on endpoint ?

I feel a little bit confused. Any thoughts from others?

PTAL #1821 the healthChecking part as a reference

how about removing healthChecking and copying the fields directly into top level struct ?

How about this ?

healthChecking: # optional connectionTimeout: duration # optional healthyCheckInterval: duration # optional, default=5s unhealthyCheckInterval: duration, # optional, default=10s unhealthyThreshold: int # optional, default=3 healthyThreshold: int # optional, default=1 type: Enum (grpc/http) grpc: # required when type=grpc upstreamName: string # required authority: string # optional http: # required when type=http expectedStatuses: # required, minItems=1 - min: int # min=100, max=599 max: int # min=100, max=599 path: string # optional hostname: string # optional

yup looking good !

prefer "healthCheck" as the top level noun

lets only support tcp and http for now, and add support for grpc later

inline heathChecker to heathCheck and remove grpc checker.

done

api/v1alpha1/healthcheck_types.go

arkodg · 2023-11-27T18:30:07Z

api/v1alpha1/healthcheck_types.go

+	Method string `json:"method,omitempty" yaml:"method,omitempty"`
+	// ExpectedStatuses defines a list of HTTP response statuses considered healthy.
+	// +optional
+	ExpectedStatuses []Int64Range `json:"expectedStatuses,omitempty" yaml:"expectedStatuses,omitempty"`


range doesnt seem like the common case for me, prefer a list of ints e.g. []int{200, 201}
curious what others think

It seems to me that using range provides more clarity and understanding compared to an array of integers.

+1 for @lemonlinger, since an arrry of intergers will introduce length check, not a fun

this design is also corresponding to https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/v3/range.proto#envoy-v3-api-msg-type-v3-int64range

api/v1alpha1/healthcheck_types.go

Xunzhuo · 2023-11-30T02:15:38Z

/retest

lemonlinger · 2023-12-02T13:32:55Z

thanks @lemonlinger , it's better to add some e2e test and user facing doc in the following PR.

Sure, will do.

test/cel-validation/backendtrafficpolicy_test.go

api/v1alpha1/healthcheck_types.go

lemonlinger · 2023-12-04T11:20:37Z

/retest

lemonlinger · 2023-12-05T01:14:35Z

/retest

Xunzhuo

Great work @lemonlinger, will start reviewing this week.

lemonlinger · 2023-12-06T09:02:28Z

/retest

Xunzhuo · 2023-12-06T09:07:32Z

Hey @AliceProxy, would you like to take a review on this ?

zirain · 2023-12-06T11:26:11Z

/retest

lemonlinger · 2023-12-06T14:53:50Z

/retest

api/v1alpha1/healthcheck_types.go

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

lemonlinger · 2023-12-25T11:20:23Z

/retest

arkodg · 2024-01-04T01:17:32Z

@lemonlinger can you address the comments ?

lemonlinger · 2024-01-04T04:46:04Z

@lemonlinger can you address the comments ?

Sure thing. I'll get it done today.

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

arkodg

LGTM thanks !
can you also raise a separate GH issue for E2E and docs

lemonlinger · 2024-01-07T02:01:29Z

LGTM thanks !

can you also raise a separate GH issue for E2E and docs

Sure thing.

lemonlinger requested a review from a team as a code owner November 27, 2023 07:36

lemonlinger force-pushed the feature/health-check branch from 1fbb912 to 94cc6e7 Compare November 27, 2023 07:55

shawnh2 reviewed Nov 27, 2023

View reviewed changes

arkodg reviewed Nov 27, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Show resolved Hide resolved

arkodg reviewed Nov 27, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Outdated Show resolved Hide resolved

arkodg reviewed Nov 27, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Show resolved Hide resolved

arkodg reviewed Nov 27, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Show resolved Hide resolved

arkodg reviewed Nov 27, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Show resolved Hide resolved

lemonlinger force-pushed the feature/health-check branch 2 times, most recently from 2c7203a to bb329db Compare December 2, 2023 13:58

shawnh2 reviewed Dec 4, 2023

View reviewed changes

test/cel-validation/backendtrafficpolicy_test.go Outdated Show resolved Hide resolved

shawnh2 reviewed Dec 4, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Outdated Show resolved Hide resolved

Xunzhuo reviewed Dec 5, 2023

View reviewed changes

lemonlinger force-pushed the feature/health-check branch from 1b90fcc to 9c234ab Compare December 5, 2023 11:24

Xunzhuo added the road-to-ga label Dec 6, 2023

Xunzhuo added this to the v1.0.0-rc1 milestone Dec 6, 2023

lemonlinger force-pushed the feature/health-check branch from 9c234ab to ab72867 Compare December 6, 2023 08:43

Xunzhuo requested a review from Alice-Lilith December 6, 2023 09:07

lemonlinger force-pushed the feature/health-check branch from ab72867 to 5cec58b Compare December 6, 2023 10:52

github-actions bot temporarily deployed to pull request December 8, 2023 06:26 Inactive

arkodg reviewed Dec 19, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Show resolved Hide resolved

arkodg reviewed Dec 19, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Outdated Show resolved Hide resolved

arkodg reviewed Dec 19, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Outdated Show resolved Hide resolved

arkodg reviewed Dec 19, 2023

View reviewed changes

api/v1alpha1/healthcheck_types.go Outdated Show resolved Hide resolved

lemonlinger added 2 commits December 25, 2023 14:44

use an array of int to represent expected http statuses

8d21fb8

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

fix cel validation tests

18511d2

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

lemonlinger force-pushed the feature/health-check branch from f4e7087 to 18511d2 Compare December 25, 2023 07:46

lemonlinger added 2 commits December 25, 2023 16:00

deduplicate http statuses of the slice of ExpectedStatuses

314eb4a

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

Merge branch main into current branch

4a41b4c

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

lemonlinger force-pushed the feature/health-check branch 2 times, most recently from cffba8b to a431627 Compare December 25, 2023 10:17

fix lint complaints

7e4022e

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

lemonlinger force-pushed the feature/health-check branch from a431627 to 7e4022e Compare December 25, 2023 10:20

lemonlinger added 4 commits January 5, 2024 14:30

inline health checker into health check config

d33f24a

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

remove grpc health checker

0087e97

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

merge with main branch

cd48623

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

fix lint complaints

06973fe

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

arkodg approved these changes Jan 5, 2024

View reviewed changes

arkodg requested review from shawnh2, Xunzhuo and a team January 5, 2024 22:25

Xunzhuo approved these changes Jan 6, 2024

View reviewed changes

Xunzhuo merged commit fb67037 into envoyproxy:main Jan 6, 2024
20 checks passed

lemonlinger deleted the feature/health-check branch January 7, 2024 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support Health Check in BackendTrafficPolicy #2244

feat: support Health Check in BackendTrafficPolicy #2244

lemonlinger commented Nov 27, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading

shawnh2 left a comment

arkodg Nov 27, 2023

lemonlinger Nov 28, 2023

arkodg Nov 28, 2023

lemonlinger Nov 30, 2023

Xunzhuo Dec 6, 2023

arkodg Dec 19, 2023

lemonlinger Jan 4, 2024

arkodg Jan 4, 2024

lemonlinger Jan 5, 2024

arkodg Nov 27, 2023 •

edited

Loading

lemonlinger Nov 30, 2023

shawnh2 Dec 4, 2023 •

edited

Loading

Xunzhuo commented Nov 30, 2023

lemonlinger commented Dec 2, 2023 •

edited

Loading

lemonlinger commented Dec 4, 2023

lemonlinger commented Dec 5, 2023

Xunzhuo left a comment

lemonlinger commented Dec 6, 2023

Xunzhuo commented Dec 6, 2023

zirain commented Dec 6, 2023

lemonlinger commented Dec 6, 2023

lemonlinger commented Dec 25, 2023

arkodg commented Jan 4, 2024

lemonlinger commented Jan 4, 2024

arkodg left a comment

lemonlinger commented Jan 7, 2024

feat: support Health Check in BackendTrafficPolicy #2244

feat: support Health Check in BackendTrafficPolicy #2244

Conversation

lemonlinger commented Nov 27, 2023

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

shawnh2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnh2 Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Xunzhuo commented Nov 30, 2023

lemonlinger commented Dec 2, 2023 • edited Loading

lemonlinger commented Dec 4, 2023

lemonlinger commented Dec 5, 2023

Xunzhuo left a comment

Choose a reason for hiding this comment

lemonlinger commented Dec 6, 2023

Xunzhuo commented Dec 6, 2023

zirain commented Dec 6, 2023

lemonlinger commented Dec 6, 2023

lemonlinger commented Dec 25, 2023

arkodg commented Jan 4, 2024

lemonlinger commented Jan 4, 2024

arkodg left a comment

Choose a reason for hiding this comment

lemonlinger commented Jan 7, 2024

codecov bot commented Nov 27, 2023 •

edited

Loading

arkodg Nov 27, 2023 •

edited

Loading

shawnh2 Dec 4, 2023 •

edited

Loading

lemonlinger commented Dec 2, 2023 •

edited

Loading