-
Notifications
You must be signed in to change notification settings - Fork 360
T7343: IPsec add traffic-selector AFI handling for VTI interfaces #4446
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
base: current
Are you sure you want to change the base?
Conversation
- Support for 'ipv4', 'ipv6', and 'both' traffic-selectors modes for VTI interfaces - Automatically sets local_ts and remote_ts values based on the selected mode - Defaults to 'both' if not explicitly specified ``` set vpn ipsec site-to-site peer RIGHT vti bind 'vti1' set vpn ipsec site-to-site peer RIGHT vti traffic-selectors 'ipv4' ```
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, adds smoketest to verify explicit and default functionality.
@@ -1244,6 +1244,31 @@ | |||
<children> | |||
#include <include/ipsec/bind.xml.i> | |||
#include <include/ipsec/esp-group.xml.i> | |||
<leafNode name="traffic-selectors"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is not a multi node and it can be either ipv4, ipv6 or both I would use singular for the node name.
@dmbaturin what's your opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
traffic-selectors means many prefixes
Even if it set to IPv4 => 0.0.0.0/0, the remote site could send several prefixes. As and local site as well.
We probably extend it in the future to
x.x.x.x/x
x.x.y.y/x[gre]
this way we’ll have less migrations for naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking about multi-nodes...
Why not make this a multi-node with only ipv4
and ipv6
values? And activate both if the node is not presented in the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this a multi-node with only
ipv4
andipv6
values? And activate both if the node is not presented in the config?
It doesn't make sense now.
By default, without a node, it uses the default value both
If we agree, I'm ready to replace this with /multi
.
Just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this a multi-node with only ipv4 and ipv6 values? And activate both if the node is not presented in the config?
This is the wrong way. If this should be a multi node, a migrator would be needed to change CLI to add both ipv4 and ipv6 options and not silently setting them under the hood. It should be obvious to the user. I kind of like the idea of a multi node, but it requires additional effort for the migrator and can not be backported to 1.4 as the CLI is fixed.
Co-authored-by: Christian Breunig <[email protected]>
Co-authored-by: Christian Breunig <[email protected]>
CI integration 👍 passed! Details
|
Change summary
ipv4
,ipv6
, andboth
traffic-selectors modes for VTI interfacesboth
if not explicitly specifiedTypes of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Configure
traffic-selectors
for VTI, check the strongswan.confVyOS configuration:
check the strongswan.conf, expected IPv4 only traffic-slectrots:
Smoketest
Checklist: