feat(ingress-gateway): support outlier detection of upstream service for ingress gateway #15614

huikang · 2022-11-30T16:07:40Z

Description

Support outlier detection of upstream service for ingress gateway.
Consul has supported adding outlier detection to envoy proxy for the connect-proxy services. This PR adds the similar function to ingress gateway proxy.

Example:

Kind = "ingress-gateway"
Name = "ingress-gateway"
Defaults {
  PassiveHealthCheck {
    max_failures = 10   // defaults for all upstream services
  }
Listeners = [
 {
   Port = 18080
   Services = [
     {
        Name = "backend"
        PassiveHealthCheck {
          max_failures = 20   // override the defaults
        }
     },
   ]
 },
}

generated envoy config

      "outlier_detection": {
       "consecutive_5xx": 20,
      },

Testing & Reproduction steps

An example is added to the integration test

Links

Fix #15401

Please be mindful not to leak any customer or confidential information. HashiCorp employees may want to use our internal URL shortener to obfuscate links.

PR Checklist

updated test coverage
external facing docs updated
not a security concern

trujillo-adam

Added some suggestions for the docs

website/content/docs/connect/config-entries/ingress-gateway.mdx

eculver

Looks good! We should probably let someone from the core team chime in too just for good measure, but all good from my perspective.

Since this is technically a new feature, do we want to backport this?

huikang · 2022-12-02T15:00:15Z

Since this is technically a new feature, do we want to backport this?

@eculver , thanks for your comments. I think we have several patch releases (e.g., 1.14.2) that contains new features for envoy proxy. So I think backport should be fine.

erichaberkorn · 2022-12-09T14:21:37Z

agent/xds/clusters.go

+			outlierDetection.Consecutive_5Xx = &wrappers.UInt32Value{Value: svc.PassiveHealthCheck.MaxFailures}
+		}
+
+		// if svc.PassiveHealthCheck.EnforcingConsecutive5xx != nil && *svc.PassiveHealthCheck.EnforcingConsecutive5xx != 0 {


Can we delete this?

erichaberkorn · 2022-12-09T14:27:16Z

agent/xds/clusters.go

+	outlierDetection := ToOutlierDetection(cfgSnap.IngressGateway.Defaults.PassiveHealthCheck)
+
+	// override the default outlier detection value
+	if svc.PassiveHealthCheck != nil {


I wonder if it would simplify things to either update ToOutlierDetection or make a new function that takes a variable number of *structs.PassiveHealthChecks and peeredService. If we do that, I believe we can share this logic everywhere in this file.

Done. Add an override parameter to ToOutlierDetection.

erichaberkorn · 2022-12-09T14:33:07Z

agent/xds/clusters.go

+		outlierDetection.MaxEjectionPercent = &wrappers.UInt32Value{Value: c.OutlierDetection.MaxEjectionPercent.Value}
+	}
+
+	c.OutlierDetection = outlierDetection


I think always setting outlier detection will end up being a breaking change and is causing golden tests like this one to change. Is that intended?

If so, we should make this a breaking change in the changelog and we can't backport it.

After second thought, I think adding empty outlier_detection make it consistent with other places, like connect-proxy, mesh-gateway

consul/agent/xds/testdata/clusters/connect-proxy-with-tls-outgoing-cipher-suites.latest.golden

Lines 21 to 24 in 4a32070

"outlierDetection": {

},

"commonLbConfig": {

consul/agent/xds/testdata/clusters/mesh-gateway-with-exported-peered-services-http-with-router.latest.golden

Lines 17 to 19 in 4a32070

"outlierDetection": {

}

So I added a note of this breaking change to the changelog.

Since this is going to be a breaking change, could you remove the backport labels from this PR?

Sure, backport labels are removed.

erichaberkorn · 2022-12-09T14:35:24Z

agent/xds/clusters_test.go

 						entry.Listeners[0].Services[0].MaxConnections = 4096
 						entry.Listeners[0].Services[0].MaxPendingRequests = 2048
+						entry.Listeners[0].Services[0].PassiveHealthCheck = &structs.PassiveHealthCheck{


It might be safer to add the tests for outlier detection to new cases so it's easier to ensure the existing cases don't change.

Updated. Thanks.

…for ingress gateway

huikang

@erichaberkorn , thanks for your review. PR is updated. Please take another look.

huikang · 2022-12-09T15:33:35Z

agent/xds/clusters_test.go

 						entry.Listeners[0].Services[0].MaxConnections = 4096
 						entry.Listeners[0].Services[0].MaxPendingRequests = 2048
+						entry.Listeners[0].Services[0].PassiveHealthCheck = &structs.PassiveHealthCheck{


Updated. Thanks.

huikang · 2022-12-09T18:41:51Z

agent/xds/clusters.go

+	outlierDetection := ToOutlierDetection(cfgSnap.IngressGateway.Defaults.PassiveHealthCheck)
+
+	// override the default outlier detection value
+	if svc.PassiveHealthCheck != nil {


Done. Add an override parameter to ToOutlierDetection.

huikang · 2022-12-09T18:49:39Z

agent/xds/clusters.go

+		outlierDetection.MaxEjectionPercent = &wrappers.UInt32Value{Value: c.OutlierDetection.MaxEjectionPercent.Value}
+	}
+
+	c.OutlierDetection = outlierDetection


After second thought, I think adding empty outlier_detection make it consistent with other places, like connect-proxy, mesh-gateway

consul/agent/xds/testdata/clusters/connect-proxy-with-tls-outgoing-cipher-suites.latest.golden

Lines 21 to 24 in 4a32070

"outlierDetection": {

},

"commonLbConfig": {

consul/agent/xds/testdata/clusters/mesh-gateway-with-exported-peered-services-http-with-router.latest.golden

Lines 17 to 19 in 4a32070

"outlierDetection": {

}

So I added a note of this breaking change to the changelog.

erichaberkorn · 2022-12-09T20:11:49Z

proto/pbconfigentry/config_entry.proto

@@ -544,9 +546,9 @@ message UpstreamLimits {
 message PassiveHealthCheck {
  // mog: func-to=structs.DurationFromProto func-from=structs.DurationToProto
  google.protobuf.Duration Interval = 1;
-  uint32 MaxFailures = 2;
+  optional uint32 MaxFailures = 2;


Why did these become optional? https://github.com/hashicorp/consul/pull/15614/files#diff-9d2e9c8b34664ef85c83b6c6f4ec7f939cce7a64f41c267e5db248c1f95b0571R549-R551

Do you know if this is a breaking change?

IMO, this is not a breaking change.

Optional makes mog know if MaxFailures is defined or not. Otherwise, unspecified MaxFailures will become "0", which is a wrong value. In the PR, I added a test for unspecified EnforcingConsecutive5xx, which is the same type as MaxFailures, to verify the default value is "null" for envoy.

https://github.com/hashicorp/consul/pull/15614/files#diff-a44e2ceb5ced90fcc9be4d9518bb62626d62438751d9cc67286de9df15d15b54R72

@erichaberkorn , there is a test that asserts the maxFailure in upstream config

consul/test/integration/connect/envoy/case-upstream-config/verify.bats

Lines 49 to 52 in 4a32070

[ "$(echo $CLUSTER_CONFIG | jq --raw-output '.outlier_detection.consecutive_5xx')" = "4" ]

[ "$(echo $CLUSTER_CONFIG | jq --raw-output '.outlier_detection.interval')" = "22s" ]

}

Another test is added to verify unspecified MaxFailure in upstreams.config.passive_health_check

30af125

I'm talking about a protobuf compatibility breaking change. I'm pretty sure we would need to introduce a completely different field on the protobuf and deprecate the current field to get this working. Then we could remove the current field in a follow-up release.

@erichaberkorn , reverted the change to message PassiveHealthCheck by adding the constraint of EnforcingConsecutive5xx > 0. WDYT?

…nfig

erichaberkorn · 2022-12-12T14:33:26Z

agent/xds/clusters.go

+	outlierDetection := ToOutlierDetection(cfgSnap.IngressGateway.Defaults.PassiveHealthCheck, override)
+
+	// Specail handling for failover peering service, which has set MaxEjectionPercent
+	if c.OutlierDetection != nil && c.OutlierDetection.MaxEjectionPercent != nil {


Sorry for yet another comment. I just noticed that we aren't setting panic threshold here similar to other clusters. For the sake of consistency, I'm guessing we want to set it like we do on other cluster types (because panic threshold interacts with outlier detection).

Sorry, I am little confused since this has been set for the upstream clusters of the ingress gateway in

consul/agent/xds/clusters.go

Line 1147 in cd53120

func (s *ResourceGenerator) makeUpstreamClustersForDiscoveryChain(

, which is called in making upstream clusters for the ingress-gateway service

consul/agent/xds/clusters.go

Lines 789 to 796 in cd53120

upstreamClusters, err := s.makeUpstreamClustersForDiscoveryChain(

uid,

&u,

chain,

cfgSnap,

false,

)

if err != nil {

erichaberkorn · 2022-12-12T14:40:28Z

agent/xds/config.go

+		}
+
+		// NOTE: EnforcingConsecutive5xx must be great than 0
+		if p.EnforcingConsecutive5xx != nil && *p.EnforcingConsecutive5xx > 0 {


I believe this ends up getting rid of the ability for users to set EnforcingConsecutive5xx to zero which is a breaking change for other proxy types.

Added some logic to handle the legacy scenarios. Please take another look. Thanks.

agent/xds/config.go

Co-authored-by: Eric Haberkorn <erichaberkorn@gmail.com>

…for ingress gateway (#15614) * feat(ingress-gateway): support outlier detection of upstream service for ingress gateway * changelog Co-authored-by: Eric Haberkorn <erichaberkorn@gmail.com>

shweshi · 2023-01-05T12:28:20Z

website/content/docs/connect/config-entries/ingress-gateway.mdx

+            'Passive health checks remove hosts from the upstream cluster that are unreachable or that return errors.',
+          children: [
+            {
+              name: 'interval',


@huikang This should be Interval and not interval. Same for the Line 1243.

shweshi · 2023-01-05T12:28:58Z

website/content/docs/connect/config-entries/ingress-gateway.mdx

+                "The time in nanosecond between checks. Each check will cause hosts which have exceeded `max_failures` to be removed from the load balancer, and any hosts which have passed their ejection time to be returned to the load balancer. If not specified, it uses the default value. For example, 10s for Envoy proxy.",
+            },
+            {
+              name: 'max_failures',


@huikang The name should be MaxFailures. Same for the Line 1247.

shweshi · 2023-01-05T12:48:43Z

website/content/docs/connect/config-entries/ingress-gateway.mdx

+                'The number of consecutive failures that cause a host to be removed from the upstream cluster. If not specified, Consul uses the proxy\'s default value. For example, `5` for Envoy proxy.',
+            },
+            {
+              name: 'enforcing_consecutive_5xx',


@huikang The name field should be EnforcingConsecutive5xx. Same for the Line 1251.

shweshi · 2023-01-05T12:59:15Z

@huikang Any idea in from which consul version this feature will be available. Since the backport to 1.14 and 1.13 has been removed.

huikang requested a review from a team as a code owner November 30, 2022 16:07

github-actions bot added theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support type/docs Documentation needs to be created/updated/clarified labels Nov 30, 2022

vercel bot deployed to Preview – consul November 30, 2022 16:10 View deployment

huikang added backport/1.14 labels Nov 30, 2022

trujillo-adam reviewed Nov 30, 2022

View reviewed changes

huikang force-pushed the gh-15401-passive-health-check-ingress branch 3 times, most recently from 124790c to a381e17 Compare December 1, 2022 17:05

vercel bot had a problem deploying to Preview – consul December 1, 2022 17:10 Failure

vercel bot deployed to Preview – consul December 1, 2022 17:28 View deployment

eculver approved these changes Dec 1, 2022

View reviewed changes

huikang assigned david-yu and unassigned david-yu Dec 2, 2022

huikang requested review from david-yu, hashi-derek and erichaberkorn December 2, 2022 15:00

erichaberkorn reviewed Dec 9, 2022

View reviewed changes

huikang added 9 commits December 9, 2022 10:30

feat(ingress-gateway): support outlier detection of upstream service …

7a05fa5

…for ingress gateway

changelog

0a16c61

fix test

cc19ed1

make deep-copy

6916600

update testdata

bb72e92

reword the description

714d42a

fix

3018c7a

fix the pointer issue

555bdcc

fix

0b48fb8

create new test cases

4d13b4e

huikang force-pushed the gh-15401-passive-health-check-ingress branch from b34a3f2 to 4d13b4e Compare December 9, 2022 15:30

huikang added 2 commits December 9, 2022 13:41

refactor ToOutlierDetection

ec444a9

add changelog

bcfa5e6

huikang commented Dec 9, 2022

View reviewed changes

huikang requested a review from erichaberkorn December 9, 2022 18:51

huikang removed backport/1.13 labels Dec 9, 2022

erichaberkorn reviewed Dec 9, 2022

View reviewed changes

huikang added 2 commits December 9, 2022 16:23

Add test to verify unspecified MaxFailure isn't broken in upstream co…

30af125

…nfig

revert change to proto

ec19956

huikang force-pushed the gh-15401-passive-health-check-ingress branch from ab1a015 to ec19956 Compare December 9, 2022 23:46

erichaberkorn reviewed Dec 12, 2022

View reviewed changes

Allow connect and mesh gateway to have 0 for EnforcingConsecutive5xx

5d6d9a4

huikang force-pushed the gh-15401-passive-health-check-ingress branch from b695296 to 5d6d9a4 Compare December 12, 2022 20:07

huikang added backport/1.14 and removed backport/1.14 labels Dec 12, 2022

erichaberkorn approved these changes Dec 13, 2022

View reviewed changes

agent/xds/config.go Outdated Show resolved Hide resolved

agent/xds/config.go Outdated Show resolved Hide resolved

Apply suggestions from code review

bf0408a

Co-authored-by: Eric Haberkorn <erichaberkorn@gmail.com>

huikang merged commit 04bf24c into main Dec 13, 2022

huikang deleted the gh-15401-passive-health-check-ingress branch December 13, 2022 16:51

shweshi reviewed Jan 5, 2023

View reviewed changes

shweshi mentioned this pull request Jan 19, 2023

Update config-entry ingress-gateway.mdx doc with correct fields for Passive Health Check. #15905

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingress-gateway): support outlier detection of upstream service for ingress gateway #15614

feat(ingress-gateway): support outlier detection of upstream service for ingress gateway #15614

huikang commented Nov 30, 2022 •

edited

Loading

trujillo-adam left a comment

eculver left a comment

huikang commented Dec 2, 2022

erichaberkorn Dec 9, 2022

huikang Dec 9, 2022

erichaberkorn Dec 9, 2022

huikang Dec 9, 2022

erichaberkorn Dec 9, 2022

huikang Dec 9, 2022

erichaberkorn Dec 9, 2022

huikang Dec 9, 2022

erichaberkorn Dec 9, 2022

huikang Dec 9, 2022

huikang left a comment

huikang Dec 9, 2022

huikang Dec 9, 2022

huikang Dec 9, 2022

erichaberkorn Dec 9, 2022

huikang Dec 9, 2022

huikang Dec 9, 2022

huikang Dec 9, 2022

erichaberkorn Dec 9, 2022

huikang Dec 10, 2022

erichaberkorn Dec 12, 2022

huikang Dec 12, 2022

erichaberkorn Dec 12, 2022

huikang Dec 12, 2022

shweshi Jan 5, 2023 •

edited

Loading

shweshi Jan 5, 2023 •

edited

Loading

shweshi Jan 5, 2023

shweshi commented Jan 5, 2023


	[ "$(echo $CLUSTER_CONFIG \| jq --raw-output '.outlier_detection.consecutive_5xx')" = "4" ]
	[ "$(echo $CLUSTER_CONFIG \| jq --raw-output '.outlier_detection.interval')" = "22s" ]
	}

	upstreamClusters, err := s.makeUpstreamClustersForDiscoveryChain(
	uid,
	&u,
	chain,
	cfgSnap,
	false,
	)
	if err != nil {

feat(ingress-gateway): support outlier detection of upstream service for ingress gateway #15614

feat(ingress-gateway): support outlier detection of upstream service for ingress gateway #15614

Conversation

huikang commented Nov 30, 2022 • edited Loading

Description

Testing & Reproduction steps

Links

PR Checklist

trujillo-adam left a comment

Choose a reason for hiding this comment

eculver left a comment

Choose a reason for hiding this comment

huikang commented Dec 2, 2022

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

huikang 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

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

shweshi Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

shweshi Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shweshi commented Jan 5, 2023

huikang commented Nov 30, 2022 •

edited

Loading

shweshi Jan 5, 2023 •

edited

Loading

shweshi Jan 5, 2023 •

edited

Loading