reverse_tunnels: add validation in the network filter #41271
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
agrawroh
force-pushed
the
feat-rev-net-val-2
branch
2 times, most recently
from
8863154 to
a4b5d36
Compare
agrawroh
force-pushed
the
feat-rev-net-val-2
branch
2 times, most recently
from
9b4f13b to
df3cde7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds validation capabilities to the Reverse Tunnel network filter to validate incoming Node ID and Cluster ID values during reverse connection handshake. The implementation uses configurable format strings that can leverage Envoy's command operators (like Filter State, SNI, Certificate SAN, etc.) for flexible validation rules.
- Adds
Validationmessage to the proto config with node_id_format and cluster_id_format fields - Implements validation logic in the filter that compares extracted headers against expected values
- Adds optional dynamic metadata emission for validation results and debugging
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/envoy/extensions/filters/network/reverse_tunnel/v3/reverse_tunnel.proto | Defines new Validation message with format strings for node/cluster ID validation |
| source/extensions/filters/network/reverse_tunnel/reverse_tunnel_filter.h | Adds validation methods and formatter storage to config class |
| source/extensions/filters/network/reverse_tunnel/reverse_tunnel_filter.cc | Implements validation logic using formatters and metadata emission |
| source/extensions/filters/network/reverse_tunnel/config.cc | Updates factory to use new config creation method with error handling |
| source/extensions/filters/network/reverse_tunnel/BUILD | Adds formatter library dependencies |
| test/extensions/filters/network/reverse_tunnel/integration_test.cc | Comprehensive integration tests for validation scenarios |
| test/extensions/filters/network/reverse_tunnel/filter_unit_test.cc | Updates unit tests to use new config creation pattern |
| test/extensions/filters/network/reverse_tunnel/config_test.cc | Tests configuration parsing for validation scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
source/extensions/filters/network/reverse_tunnel/reverse_tunnel_filter.cc
Outdated
Show resolved
Hide resolved
agrawroh
force-pushed
the
feat-rev-net-val-2
branch
3 times, most recently
from
c2112cb to
f0d14bc
Compare
Signed-off-by: Rohit Agrawal <[email protected]>
agrawroh
force-pushed
the
feat-rev-net-val-2
branch
from
f0d14bc to
73cf122
Compare
| } | ||
|
|
||
| // Test validation passes when formatter returns empty and actual value is empty. | ||
| TEST_P(ReverseTunnelFilterIntegrationTest, ValidationWithBothValuesMatching) { |
There was a problem hiding this comment.
@agrawroh could we add a test where we validate with values set using the filter state:
`%FILTER_STATE(key)%'
Since that would be the primary used case of this validator. We won't have a case where the exact node_id_format and cluster_id_format will be set in the filter, because there will be only one instance of the filter running on upstream envoy, accepting reverse tunnels from multiple downstream nodes. There isn't a way for the filter to perform an exact match, but it can validate using values from the filter state or using regex.
Signed-off-by: Rohit Agrawal <[email protected]>
|
|
||
| // Namespace for emitted dynamic metadata when ``emit_dynamic_metadata`` is ``true``. | ||
| // If not specified, defaults to ``envoy.filters.network.reverse_tunnel``. | ||
| string dynamic_metadata_namespace = 4 [(validate.rules).string = {max_len: 255}]; |
There was a problem hiding this comment.
Out of curiosity, why do we need a max len of 255 here?
| // Reverse Tunnel Network Filter :ref:`configuration overview <config_network_filters_reverse_tunnel>`. | ||
| // [#extension: envoy.filters.network.reverse_tunnel] | ||
|
|
||
| // Validation configuration for reverse tunnel identifiers. |
There was a problem hiding this comment.
thanks for the great comments!
Description
This PR adds a
validationto the Reverse Tunnel filter which could be used to do validations on the incoming Node ID and Cluster ID values in the reverse connection handshake. It's possible to use Filter State, SNI, Certificate SAN, etc. to do these validations by configuring the formatter. It's also possible to do validations on all or some of the inputs.Commit Message: reverse_tunnels: add validation in the network filter
Additional Description: Adds
validationto perform validations on the incoming Node ID and Cluster ID from reverse connection handshake.Risk Level: Low
Testing: Added Unit + Integration Tests
Docs Changes: N/A
Release Notes: N/A