Skip to content

tcp filter: Allow referencing dynamic clusters in v1 config#2707

Merged
htuch merged 1 commit intoenvoyproxy:masterfrom
snowp:remove-cluster-check
Mar 5, 2018
Merged

tcp filter: Allow referencing dynamic clusters in v1 config#2707
htuch merged 1 commit intoenvoyproxy:masterfrom
snowp:remove-cluster-check

Conversation

@snowp
Copy link
Copy Markdown
Contributor

@snowp snowp commented Mar 3, 2018

This removes the validation code that ensures that any cluster referenced in the tcp filter config is known when the config is applied. Removing this allows users to specify a cluster that is not known at the time but will be made available later on through CDS.

Risk Level: Low
This is already possible to do with the v2 config (as the validation was not applied in that case), so the behavior should already be battle tested. This does open up the possibility for people misconfiguring their static config (e.g. a typo when trying to reference a static cluster), although it will not break existing configs.

Testing:
None: the code path that this enables is already exercised when using the v2 config, so the existing test cases should cover this

Fixes issue #2075

cc @ggreenway

This removes the check ensuring that the cluster referenced in the tcp
filter is known. This allows users to reference a cluster that will be
made available later through CDS.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
@htuch htuch assigned zuercher, ggreenway and htuch and unassigned zuercher Mar 5, 2018
@htuch
Copy link
Copy Markdown
Member

htuch commented Mar 5, 2018

@ggreenway are there any implications here that are non-obvious?

@snowp I assume you really need this behavior? In general, we're pushing back on v1 enhancements but will still take them when there is a concrete need.

Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

I don't think there's any hidden issues here; v2 already supports this behavior.

The only issue is if somebody is depending on this behavior for static config validation, but they'll have no path forward in v2 if they are anyways, so I think this is fine.

@htuch htuch merged commit 642a195 into envoyproxy:master Mar 5, 2018
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* feat(edges): support configurable batch size

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

* address review comments

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

* address review comments

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
…roxy/envoy/pull/24187/files (#2707)

Fix typo in starting_envoy.rst
Backport of https://github.com/envoyproxy/envoy/pull/24187/files

Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
…roxy/envoy/pull/24187/files (#2707)

Fix typo in starting_envoy.rst
Backport of https://github.com/envoyproxy/envoy/pull/24187/files

Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants