Avoid panic applying TProxy Envoy extensions #17537

zalimeni · 2023-05-31T23:14:48Z

When UpstreamEnvoyExtender was introduced, some code was left duplicated between it and BasicEnvoyExtender. One path in that code panics when a TProxy listener patch is attempted due to no upstream data in RuntimeConfig matching the local service (which would only happen in rare cases).

Instead, we can remove the special handling of upstream VIPs from BasicEnvoyExtender entirely, greatly simplifying the listener filter patch code and avoiding the panic. UpstreamEnvoyExtender, which needs this code to function, is modified to ensure a panic does not occur.

This also fixes a second regression in which the Lua extension was not applied to TProxy outbound listeners.

Description

This fixes two regressions caused by #17415.

Testing & Reproduction steps

I've hand-tested this fix in addition to adding tests to guard against the regressions.

PR Checklist

updated test coverage
external facing docs updated - N/A
appropriate backport labels added
not a security concern

zalimeni · 2023-05-31T23:27:08Z

envoyextensions/extensioncommon/basic_envoy_extender.go

-	switch config.Kind {
-	case api.ServiceKindTerminatingGateway:
-		return b.patchTerminatingGatewayListenerFilterChains(config, l, nameOrSNI)
-	case api.ServiceKindConnectProxy:
-		return b.patchConnectProxyListenerFilterChains(config, l, nameOrSNI)
-	}
-	return l, nil
-}
-
-func (b *BasicEnvoyExtender) patchTerminatingGatewayListenerFilterChains(config *RuntimeConfig, l *envoy_listener_v3.Listener, nameOrSNI string) (*envoy_listener_v3.Listener, error) {
-	var resultErr error
-	for idx, filterChain := range l.FilterChains {
-		if patchedFilterChain, err := b.patchFilterChain(config, filterChain, l); err == nil {
-			l.FilterChains[idx] = patchedFilterChain
-		} else {
-			resultErr = multierror.Append(resultErr, fmt.Errorf("error patching filter chain of terminating gateway listener %q: %w", nameOrSNI, err))
-		}
-	}
-
-	return l, resultErr
-}
-
-func (b *BasicEnvoyExtender) patchConnectProxyListenerFilterChains(config *RuntimeConfig, l *envoy_listener_v3.Listener, nameOrSNI string) (*envoy_listener_v3.Listener, error) {
-	if IsOutboundTProxyListener(l) {
-		patchedListener, err := b.patchTProxyListenerFilterChains(config, l)
-		if err == nil {
-			return patchedListener, nil
-		} else {
-			return l, fmt.Errorf("error patching filter chain of TProxy listener %q: %w", nameOrSNI, err)
-		}
-	} else {
-
-		patchedListener, err := b.patchNonTProxyConnectProxyListenerFilterChains(config, l)
-		if err == nil {
-			return patchedListener, nil
-		} else {
-			return l, fmt.Errorf("error patching filter chain of connect proxy listener %q: %w", nameOrSNI, err)
-		}
-	}
-}


All the filter chain patching code turned out to be identical except for the removed check for VIPs below (which is not necessary for the BasicEnvoyExtender, because UpstreamEnvoyExtender now handles extensions - Lambda and Validate - configured via an upstream of the local proxy).

zalimeni · 2023-05-31T23:28:36Z

envoyextensions/extensioncommon/basic_envoy_extender.go

-func (b *BasicEnvoyExtender) patchTProxyListenerFilterChains(config *RuntimeConfig, l *envoy_listener_v3.Listener) (*envoy_listener_v3.Listener, error) {
-	var resultErr error
-
-	vip := config.Upstreams[config.ServiceName].VIP


This line was the source of the panic. We wouldn't expect config.Upstreams[config.ServiceName] to exist except if the local service has itself configured as an upstream; regardless, it's not necessary here.

Removing this check fixes the second regression in which TProxy filters were not patched by BasicEnvoyExtender (Lua).

zalimeni · 2023-05-31T23:29:05Z

envoyextensions/extensioncommon/upstream_envoy_extender.go

+	upstream := config.Upstreams[config.ServiceName]
+	if upstream == nil {
+		return l, false, nil
+	}
+	vip := upstream.VIP


Guards against the same bug without removing the VIP check, which is necessary in this case.

zalimeni · 2023-05-31T23:48:03Z

agent/proxycfg/testing_tproxy.go

@@ -728,7 +728,7 @@ func TestConfigSnapshotTransparentProxyDestination(t testing.T) *ConfigSnapshot
 	})
 }

-func TestConfigSnapshotTransparentProxyDestinationHTTP(t testing.T) *ConfigSnapshot {
+func TestConfigSnapshotTransparentProxyDestinationHTTP(t testing.T, nsFn func(ns *structs.NodeService)) *ConfigSnapshot {


This allows us to configure the Lua extension for the local service, which will target TProxy upstreams.

zalimeni · 2023-05-31T23:49:45Z

...ata/builtin_extension/listeners/lua-outbound-applies-to-local-upstreams-tproxy.latest.golden

+                    "name": "envoy.filters.http.lua",
+                    "typedConfig": {
+                      "@type": "type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua",
+                      "inlineCode": "\nfunction envoy_on_request(request_handle)\n  request_handle:headers():add(\"test\", \"test\")\nend"
+                    }


Patched HTTP upstream 1/2

zalimeni · 2023-05-31T23:49:55Z

...ata/builtin_extension/listeners/lua-outbound-applies-to-local-upstreams-tproxy.latest.golden

+                    "name": "envoy.filters.http.lua",
+                    "typedConfig": {
+                      "@type": "type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua",
+                      "inlineCode": "\nfunction envoy_on_request(request_handle)\n  request_handle:headers():add(\"test\", \"test\")\nend"
+                    }


Patched HTTP upstream 2/2

hashi-derek

This looks fine to me if you're certain that the removed code in the basic envoy extender wasn't needed for anything.

cthain

Great work. I'm super glad that you caught this.

When UpstreamEnvoyExtender was introduced, some code was left duplicated between it and BasicEnvoyExtender. One path in that code panics when a TProxy listener patch is attempted due to no upstream data in RuntimeConfig matching the local service (which would only happen in rare cases). Instead, we can remove the special handling of upstream VIPs from BasicEnvoyExtender entirely, greatly simplifying the listener filter patch code and avoiding the panic. UpstreamEnvoyExtender, which needs this code to function, is modified to ensure a panic does not occur. This also fixes a second regression in which the Lua extension was not applied to TProxy outbound listeners.

zalimeni · 2023-06-01T16:06:49Z

envoyextensions/extensioncommon/basic_envoy_extender.go

-func (b *BasicEnvoyExtender) patchListenerFilterChains(config *RuntimeConfig, l *envoy_listener_v3.Listener, nameOrSNI string) (*envoy_listener_v3.Listener, error) {
+func (b *BasicEnvoyExtender) patchSupportedListenerFilterChains(config *RuntimeConfig, l *envoy_listener_v3.Listener, nameOrSNI string) (*envoy_listener_v3.Listener, error) {
 	switch config.Kind {
-	case api.ServiceKindTerminatingGateway:
-		return b.patchTerminatingGatewayListenerFilterChains(config, l, nameOrSNI)
-	case api.ServiceKindConnectProxy:
-		return b.patchConnectProxyListenerFilterChains(config, l, nameOrSNI)
+	case api.ServiceKindTerminatingGateway, api.ServiceKindConnectProxy:
+		return b.patchListenerFilterChains(config, l, nameOrSNI)


After double checking the original change that introduced this switch and the matching one above, I restored it to align with that and reduce changes here (I'd originally thought it was more copy-pasta). This could prevent a future bug if we were to support more proxy types for extensions in general without aligning that support with listeners specifically.

No other changes since the last review. I've also re-tested by hand w/ this change.

github-actions bot added the theme/envoy/xds Related to Envoy support label May 31, 2023

zalimeni added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. labels May 31, 2023

zalimeni force-pushed the zalimeni/net-3900-fix-tproxy-extension-panic branch from f8b8034 to 8c30455 Compare May 31, 2023 23:45

zalimeni commented May 31, 2023

View reviewed changes

zalimeni requested review from cthain and hashi-derek May 31, 2023 23:57

zalimeni marked this pull request as ready for review May 31, 2023 23:58

zalimeni mentioned this pull request Jun 1, 2023

Manual backport 1.15.x of Avoid panic applying TProxy Envoy extensions #17539

Merged

hashi-derek approved these changes Jun 1, 2023

View reviewed changes

cthain approved these changes Jun 1, 2023

View reviewed changes

zalimeni force-pushed the zalimeni/net-3900-fix-tproxy-extension-panic branch from 8c30455 to c66703f Compare June 1, 2023 15:59

zalimeni commented Jun 1, 2023

View reviewed changes

hashi-derek approved these changes Jun 1, 2023

View reviewed changes

zalimeni merged commit ad03a5d into main Jun 1, 2023

zalimeni deleted the zalimeni/net-3900-fix-tproxy-extension-panic branch June 1, 2023 17:04

hc-github-team-consul-core mentioned this pull request Jun 1, 2023

Backport of Avoid panic applying TProxy Envoy extensions into release/1.15.x #17542

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid panic applying TProxy Envoy extensions #17537

Avoid panic applying TProxy Envoy extensions #17537

zalimeni commented May 31, 2023 •

edited

Loading

zalimeni May 31, 2023

zalimeni May 31, 2023 •

edited

Loading

zalimeni May 31, 2023

zalimeni May 31, 2023

zalimeni May 31, 2023

zalimeni May 31, 2023

hashi-derek left a comment

cthain left a comment

zalimeni Jun 1, 2023 •

edited

Loading

Avoid panic applying TProxy Envoy extensions #17537

Avoid panic applying TProxy Envoy extensions #17537

Conversation

zalimeni commented May 31, 2023 • edited Loading

Description

Testing & Reproduction steps

PR Checklist

zalimeni May 31, 2023

Choose a reason for hiding this comment

zalimeni May 31, 2023 • edited Loading

Choose a reason for hiding this comment

zalimeni May 31, 2023

Choose a reason for hiding this comment

zalimeni May 31, 2023

Choose a reason for hiding this comment

zalimeni May 31, 2023

Choose a reason for hiding this comment

zalimeni May 31, 2023

Choose a reason for hiding this comment

hashi-derek left a comment

Choose a reason for hiding this comment

cthain left a comment

Choose a reason for hiding this comment

zalimeni Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

zalimeni commented May 31, 2023 •

edited

Loading

zalimeni May 31, 2023 •

edited

Loading

zalimeni Jun 1, 2023 •

edited

Loading