libreswan: Present --dpddelay if --dpdaction is specified #2599
Conversation
|
🤖 Created branch: z_pr2599/ueno/wip/dpdaction |
|
Thank you @ueno for the PR. |
When a connection is added through `ipsec whack`, --dpdaction=hold needs --dpddelay to be specified; otherwise the next probes may be scheduled too frequently. Signed-off-by: Daiki Ueno <[email protected]>
Instead of specifying the absolute path of the "whack" command, it is recommended to invoke it through the "ipsec" wrapper. Signed-off-by: Daiki Ueno <[email protected]>
| @@ -460,7 +463,8 @@ func (i *libreswan) serverConnectToEndpoint(connectionName string, endpointInfo | |||
| "--id", remoteEndpointIdentifier, | |||
| "--host", "%any", | |||
| "--client", rightSubnet, | |||
| "--dpdaction=hold") | |||
| "--dpdaction=hold", | |||
| "--dpddelay", strconv.Itoa(dpdDelay)) | |||
There was a problem hiding this comment.
Thanks for this PR @ueno , overall code LGTM.
I have short Q: do we need to set also dpdtimeout in case dpddelay is set?
According to https://libreswan.org/man/ipsec.conf.5.html :
`dpddelay
Set the delay (in time units, defaults to seconds) between Dead Peer Detection (IKEv1 RFC 3706) or IKEv2 Liveness keepalives that are sent for this connection (default 0 seconds). Set to enable checking. If dpddelay is set, dpdtimeout also needs to be set.`
There was a problem hiding this comment.
According to Paul, dpdtimeout is only for IKEv1 and ignored if IKEv2 is used, so if you only use IKEv2 it shouldn't be necessary.
There was a problem hiding this comment.
correct. with ikev1 message ID is random, and you can send many messages to see if you get a response with the same msgid. For IKEv2, msgid is linear and has a window size. Libreswan uses windows size = 1, so you can only have one IKE message outstanding per IKE SA. So if you send a DPD probe (IKEv2 Informational exchange msg), then you cannot any other IKE message until you get a response. Therefor, not getting a response is fatal, and this is tied to the generic IKE message response timeouts and there is no seperate "DPD timeout"
|
What's the plan for taking this out of draft? Do we need further testing? |
| @@ -175,7 +176,7 @@ func retrieveActiveConnectionStats() (map[string]int, map[string]int, error) { | |||
| defer cancel() | |||
|
|
|||
| // Retrieve active tunnels from the daemon | |||
| cmd := exec.CommandContext(ctx, "/usr/libexec/ipsec/whack", "--trafficstatus") | |||
| cmd := exec.CommandContext(ctx, "/usr/sbin/ipsec", "whack", "--trafficstatus") | |||
There was a problem hiding this comment.
this is marginally better, but the best is to use "ipsec" without a path, so it works as long as it is in the path (/sbin or /usr/sbin or /usr/local/sbin or whatever compile time options are used). Using "ipsec " is supported specifically to avoid any hardcoded paths. if exec.CommandContext uses regular PATH handling, this is best run without the hardcoded path
There was a problem hiding this comment.
Yeah, I can modify the PR to use exec.LookPath("ipsec"), maybe somewhere in init.
| @@ -460,7 +463,8 @@ func (i *libreswan) serverConnectToEndpoint(connectionName string, endpointInfo | |||
| "--id", remoteEndpointIdentifier, | |||
| "--host", "%any", | |||
| "--client", rightSubnet, | |||
| "--dpdaction=hold") | |||
| "--dpdaction=hold", | |||
| "--dpddelay", strconv.Itoa(dpdDelay)) | |||
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@ueno we are approaching the end of the Submariner 0.16 dev cycle, and going to enter code freeze soon. Can you please take this out of draft? I am assuming this is something we want to merge. Thanks! |
|
All e2e deployments are failing while overriding the network-plugin-syncer component. |
|
🤖 Closed branches: [z_pr2599/ueno/wip/dpdaction] |
dfarrell07
added
backport
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
Release notes for submariner-io/submariner#2599 Signed-off-by: Sridhar Gaddam <[email protected]>
These couple of commits are based on the suggestion from @paulwouters some time ago. I'm marking this as RFC as I haven't tested it but just built with "make build".