[NET-4703] Prevent partial application of Envoy extensions by zalimeni · Pull Request #18068 · hashicorp/consul

zalimeni · 2023-07-10T22:43:57Z

Ensure that non-required extensions do not change xDS resources before exiting on failure by cloning proto messages prior to applying each extension.

Also avoid unnecessary cloning by moving CanApply checks up a layer and making them before application is attempted.

Description

Today, it's theoretically possible that a non-required extension could fail partway through making updates. If that extension is not configured as Required, we will not short-circuit xDS updates; the error will be logged and swallowed, allowing any other extensions to continue applying before updating xDS resources.

By proactively cloning resources that are subject to extensions, s.t. we do not pass any data by reference that is intended to be modified atomically, we can prevent the unintentional partial application of extensions regardless of their Required status.

Testing & Reproduction steps

Added tests that use a faulty extension to make changes, then fail, and verify that those changes are not retained
Added tests for newly promoted CanApply check to ensure we skip application when extensions should not be applied
Added tests for new Clone method of IndexedResources

PR Checklist

updated test coverage
external facing docs updated
appropriate backport labels added
not a security concern

zalimeni · 2023-07-11T14:43:02Z

agent/xds/delta_test.go

~ Given how huge delta_test.go is, I think it might be worth considering moving some of these recent tests to delta_envoy_extender_test.go, which is currently just a set of helpers to aid goldens in their respective test files.

Following suit with Test_validateAndApplyEnvoyExtension_Validations today to minimize change.

zalimeni · 2023-07-24T17:56:46Z

envoyextensions/extensioncommon/runtime_config.go

These changes were necessary to avoid nil pointer panics when calling CanApply on extensions that use these helpers, which aren't guaranteed to have data outside their use case (IsSourcedFromUpstream).

zalimeni · 2023-07-24T18:28:20Z

cc @mkeeler in case you'd like to follow along since you raised this bug

cthain

This looks great. Nice work on this fix, the test coverage is awesome 👍

Left a few minor, non-blocking comments.

cthain · 2023-07-25T14:58:43Z

agent/xds/delta.go

+			r = resources
+			// Only report an error if the extension should have been applied.
+			if canApply {
+				e = fmt.Errorf("unexpected panic: %v", e)


I'm glad you guard against the case of faulty extension logic causing a panic! WDYT about making the error message here a bit more verbose? Also, I think we want to wrap err in the new error that we return.

Suggested change

e = fmt.Errorf("unexpected panic: %v", e)

e = fmt.Errorf("attempt to apply Envoy extension %q caused an unexpected panic: %w", runtimeConfig.EnvoyExtension.Name, err)

I don't see a test for the panic case. It feels like a case we should probably unit test for.

Good call on both 👍🏻 I'll follow up on these.

Had to stick w/ %v due to type uncertainty of err after recover() (TIL), but updated message. Also added tests around panic.

agent/xds/delta.go

agent/xds/delta_test.go

zalimeni · 2023-07-25T18:39:24Z

Thank you for the thorough review @cthain ! Will get these comments addressed before merging, agree w/ all of them.

Ensure that non-required extensions do not change xDS resources before exiting on failure by cloning proto messages prior to applying each extension. To support this change, also move `CanApply` checks up a layer and make them prior to attempting extension application, s.t. we avoid unnecessary copies where extensions can't be applied. Last, ensure that we do not allow panics from `CanApply` or `Extend` checks to escape the attempted extension application.

zalimeni · 2023-07-27T22:25:02Z

agent/xds/delta.go

+			r = resources
+			// Only report an error if the extension should have been applied.
+			if canApply {
+				e = fmt.Errorf("unexpected panic: %v", e)


Had to stick w/ %v due to type uncertainty of err after recover() (TIL), but updated message. Also added tests around panic.

zalimeni · 2023-07-27T22:25:33Z

agent/xds/delta.go

+			e = fmt.Errorf("attempt to apply Envoy extension %q caused an unexpected panic: %v",
+				runtimeConfig.EnvoyExtension.Name, err)


When writing tests for the panic handling, I decided it was best to err on the side of explicit errors if CanApply fails entirely, so I removed the previous conditional around this logging.

zalimeni · 2023-07-27T22:26:02Z

agent/xds/delta_test.go

+	}
+}
+
+func Test_applyEnvoyExtension_HandlesPanics(t *testing.T) {


New panic test here

cthain

🚀

zalimeni added pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. labels Jul 10, 2023

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

zalimeni force-pushed the zalimeni/net-4703-prevent-envoy-extension-partial-application branch 8 times, most recently from 2edb5d4 to 3b4ecf2 Compare July 11, 2023 14:58

zalimeni changed the title ~~Prevent partial application of Envoy extensions~~ [NET-4703] Prevent partial application of Envoy extensions Jul 11, 2023

zalimeni force-pushed the zalimeni/net-4703-prevent-envoy-extension-partial-application branch 6 times, most recently from 13aead3 to 301fbaf Compare July 24, 2023 17:57

zalimeni commented Jul 24, 2023

View reviewed changes

zalimeni force-pushed the zalimeni/net-4703-prevent-envoy-extension-partial-application branch from 301fbaf to 7402d06 Compare July 24, 2023 18:00

zalimeni marked this pull request as ready for review July 24, 2023 18:25

zalimeni requested review from cthain and hashi-derek July 24, 2023 18:27

cthain approved these changes Jul 25, 2023

View reviewed changes

hashi-derek approved these changes Jul 26, 2023

View reviewed changes

zalimeni force-pushed the zalimeni/net-4703-prevent-envoy-extension-partial-application branch from 7402d06 to a920c71 Compare July 27, 2023 22:23

zalimeni commented Jul 27, 2023

View reviewed changes

zalimeni requested a review from cthain July 27, 2023 22:26

cthain approved these changes Jul 31, 2023

View reviewed changes

zalimeni merged commit b1b05f0 into main Jul 31, 2023

zalimeni deleted the zalimeni/net-4703-prevent-envoy-extension-partial-application branch July 31, 2023 19:24

hc-github-team-consul-core mentioned this pull request Jul 31, 2023

Backport of [NET-4703] Prevent partial application of Envoy extensions into release/1.16.x #18332

Merged

4 tasks

chenrui333 mentioned this pull request Aug 9, 2023

consul 1.16.1 Homebrew/homebrew-core#139040

Merged

asheshvidyut added a commit that referenced this pull request Sep 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NET-4703] Prevent partial application of Envoy extensions#18068

[NET-4703] Prevent partial application of Envoy extensions#18068
zalimeni merged 1 commit intomainfrom
zalimeni/net-4703-prevent-envoy-extension-partial-application

zalimeni commented Jul 10, 2023 •

edited

Loading

Uh oh!

zalimeni Jul 11, 2023

Uh oh!

zalimeni Jul 24, 2023

Uh oh!

zalimeni commented Jul 24, 2023

Uh oh!

cthain left a comment

Uh oh!

cthain Jul 25, 2023

Uh oh!

cthain Jul 25, 2023

Uh oh!

zalimeni Jul 25, 2023

Uh oh!

zalimeni Jul 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zalimeni commented Jul 25, 2023

Uh oh!

zalimeni Jul 27, 2023

Uh oh!

zalimeni Jul 27, 2023

Uh oh!

zalimeni Jul 27, 2023

Uh oh!

cthain left a comment

Uh oh!

	e = fmt.Errorf("unexpected panic: %v", e)
	e = fmt.Errorf("attempt to apply Envoy extension %q caused an unexpected panic: %w", runtimeConfig.EnvoyExtension.Name, err)

		e = fmt.Errorf("attempt to apply Envoy extension %q caused an unexpected panic: %v",
		runtimeConfig.EnvoyExtension.Name, err)

Conversation

zalimeni commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing & Reproduction steps

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zalimeni commented Jul 24, 2023

Uh oh!

cthain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zalimeni commented Jul 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthain left a comment

Choose a reason for hiding this comment

Uh oh!

zalimeni commented Jul 10, 2023 •

edited

Loading