Skip to content
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.

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

Skip filter chain created by permissive mtls #20406

Merged
merged 7 commits into from
Jan 31, 2024

Conversation

kisunji
Copy link
Contributor

@kisunji kisunji commented Jan 30, 2024

Backports:
1.17.x
1.16.x

Description

Customers transitioning to service mesh using mTLS permissive mode and transparent proxy have run into an issue when using some envoy extensions like local rate-limiter because permissive mode adds a new filter chain to the public_listener: https://github.com/hashicorp/consul/blob/v1.17.2/agent/xds/listeners.go#L1436-L1445

Most of our envoy extensions assume one filter chain containing an HTTPConnectionManager but that assumption is broken in permissive mode.

We have yet to decide on how the permissive filter chain should interact with envoy extensions but until then, it is safer to explicitly skip it (meaning envoy extensions will not apply to insecure traffic to services) so that xDS does not error upon being unable to find an HTTPConnectionManager.

Testing & Reproduction steps

Manual testing was done on enterprise:

  1. Started a cluster with TProxy and Permissive mTLS enabled
  2. Added ratelimit service config entry
  3. Observed xDS errors:
[warning] envoy.config(14) DeltaAggregatedResources gRPC config stream to consul-dataplane closed: 3, failed to patch xDS resources in │
│  the "builtin/rate-limit" extension: 1 error occurred:%0A%09* error patching listener filter chain "public_listener:10.15.2.251:20000": error patching filters: failed to insert top l │
│ evel local_ratelimit filter: failed to get HTTP connection manager%0A%0A

After this patch, the errors were suppressed and the ratelimit filter applied correctly to the non-permissive filter chain.

PR Checklist

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

@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Jan 30, 2024
@kisunji kisunji added backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. backport/1.18 labels Jan 30, 2024
@kisunji kisunji requested review from a team, johnlanda and roncodingenthusiast and removed request for a team January 30, 2024 21:00
@jkirschner-hashicorp
Copy link
Contributor

We should update the permissive mode docs to list which functionality is not supported with permissive mode (e.g., rate limiting, all the Envoy extensions that can be invoked directly).

That can happen in a separate PR if needed.

@kisunji kisunji force-pushed the kisunji/fix-permissive-envoy-ext branch from 81ee5b8 to e9a628e Compare January 31, 2024 15:52
@@ -296,6 +296,27 @@ func (b *BasicEnvoyExtender) patchSupportedListenerFilterChains(config *RuntimeC
func (b *BasicEnvoyExtender) patchListenerFilterChains(config *RuntimeConfig, l *envoy_listener_v3.Listener, nameOrSNI string) (*envoy_listener_v3.Listener, error) {
var resultErr error

// Special case for Permissive mTLS, which adds a filter chain
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried working with a sentinel name for the filter chain but ran into issues with v2 resource tests since v2 proxystate Routers don't have names.

Instead I added this special case code block to suppress errors as long as one filter chain gets patched successfully.

@kisunji kisunji force-pushed the kisunji/fix-permissive-envoy-ext branch from e9a628e to d002f4d Compare January 31, 2024 18:44
@kisunji kisunji force-pushed the kisunji/fix-permissive-envoy-ext branch from d002f4d to 4e54636 Compare January 31, 2024 19:08
@kisunji kisunji merged commit b6f10bc into main Jan 31, 2024
91 checks passed
@kisunji kisunji deleted the kisunji/fix-permissive-envoy-ext branch January 31, 2024 21:39
@hc-github-team-consul-core
Copy link
Collaborator

@hc-github-team-consul-core
Copy link
Collaborator

@kisunji, a backport is missing for this PR [20406] for versions [1.16,1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

kisunji pushed a commit that referenced this pull request Feb 1, 2024
@hc-github-team-consul-core
Copy link
Collaborator

@kisunji, a backport is missing for this PR [20406] for versions [1.16,1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

4 similar comments
@hc-github-team-consul-core
Copy link
Collaborator

@kisunji, a backport is missing for this PR [20406] for versions [1.16,1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

@kisunji, a backport is missing for this PR [20406] for versions [1.16,1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

@kisunji, a backport is missing for this PR [20406] for versions [1.16,1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

@kisunji, a backport is missing for this PR [20406] for versions [1.16,1.17] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants