CVPN-1818 Inside Packet Codec #161
Open
+976
−47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Code coverage summary for 69482af: ✅ Region coverage 54% passes |
kp-max-li
force-pushed
the
cvpn-1818
branch
16 times, most recently
from
958b759 to
3de9228
Compare
kp-max-li
force-pushed
the
cvpn-1818
branch
6 times, most recently
from
aaf7e86 to
eb7088e
Compare
| // Ignore invalid inside packet | ||
| } | ||
| Err(err) => { | ||
| // Propagate fatal error up |
There was a problem hiding this comment.
This will drop all the remaining packets in the current vector.
We will need a metric to monitor this usage
kp-max-li
force-pushed
the
cvpn-1818
branch
2 times, most recently
from
27a2532 to
8231bb6
Compare
kp-max-li
force-pushed
the
cvpn-1818
branch
5 times, most recently
from
6e84723 to
e499d14
Compare
kp-max-li
force-pushed
the
cvpn-1818
branch
2 times, most recently
from
7d183f5 to
f5bc40a
Compare
kp-max-li
changed the title
kp-max-li
force-pushed
the
cvpn-1818
branch
3 times, most recently
from
b169417 to
2fe9714
Compare
kp-max-li
changed the title
kp-max-li
changed the title
kp-max-li
changed the title
Interface for encoding and decoding inside packets in lightway-core. Inside packets can be accumulated and encoded, which allows error correction code to be integrated to lightway-core.
These new types of wire frame works like Data and DataFrag. The sole purpose is to determines whether the Data payload is encoded or not.
Connection optionally holds an encoder and an decoder for inside packets. Encoder can either accumulates the inside packets and encode it before it is sent to WolfSSL, or skip the packet and Connection will directly pass it to WoflSSL. Decoder accumulates all encoded packets and decode it before it is sent to TUN.
Packet Codec is supplied by lw-server to lw-core. The packets in the encoder might have sit in the encoder for too long if inside packets are not frequently received. To avoid having a high packet delay, lw-server triggers a flush when the encoder is ready. There could be stale states in the decoder. The decoder's stale state is cleaned periodically. lw-server tracks the list of encoders and decoders for each connection. Instead of accesing the codec via locking the connection, codec is access directly instead. In this way, the number of locking to Connection is minimized.
Packet Codec is supplied by lw-client to lw-core. The packets in the encoder might have sit in the encoder for too long if inside packets are not frequently received. To avoid having a high packet delay, lw-server triggers a flush when the encoder is ready. There could be stale states in the decoder. The decoder's stale state is cleaned periodically.
By default, inside packet encoding is disabled. The client now can toggle encoding in both directions in the following work-flow: 1. Client sends a ToggleEncoding packet to the server 2. Server toggles encoding, and reply with the same packet 3. Client receives the packet and toggles encoding TODO: under heavy packet loss, the above packets could be lost. We may need to add a re-transmission mechanism to make this more robust.
Client now has two ways to toggle encoding: 1. -e flag in CLI: automatically sends a toggle encoding packet to the server when lightway-core's state changes to Connected. 2. send a signal to the toggle_encode_tx in main.rs to toggle encoding at runtime. The client may enable/disable in runtime without disrupting the connection. TODO: create an appropriate interface to send a toggle encoding signal via CLI?
When flushing the encoder, more than one packets are sent to WolfSSL. If any of the send operation returns a fatal error, all subsequent inside packets are dropped. Added a metric to track the number of such packets.
| /// | ||
| /// Returns a [`CodecStatus`]. | ||
| /// If the status is [`CodecStatus::ReadyToFlush`], encoded packets are ready to be retrieved. | ||
| /// If the status is [`CodecStatus::SkipPacket`], the packet should not be added to the encoder. |
There was a problem hiding this comment.
Does not this mean `the packet is skipped by the encoder and lightway can send it as normal Wire::Data packet.
| fn cleanup_stale_states(&mut self); | ||
| } | ||
|
|
||
| /// Indicates whether the [`PacketEncoder`] or [`PacketDecoder`] is ready to be flushed or not |
There was a problem hiding this comment.
Indicates the status of [PacketEncoder] or [PacketDecoder] after storing the current packet
| @@ -137,6 +138,9 @@ async fn main() -> Result<()> { | |||
| key_update_interval: config.key_update_interval.into(), | |||
| inside_plugins: Default::default(), | |||
| outside_plugins: Default::default(), | |||
| inside_pkt_codec: None, | |||
| pkt_encoder_flush_interval: Duration::from_secs_f64(0.0001), | |||
| pkt_decoder_clean_up_interval: Duration::from_secs_f64(0.5), | |||
There was a problem hiding this comment.
Can we make this values as cli args ?
| } else { | ||
| metrics::tun_rejected_packet_no_connection(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| tokio::spawn(pkt_encoder_flush( |
There was a problem hiding this comment.
This two tasks should be created only if config.inside_pkt_codec is not None, should not this ?
| @@ -97,6 +99,9 @@ async fn main() -> Result<()> { | |||
| server: config.server, | |||
| inside_plugins: Default::default(), | |||
| outside_plugins: Default::default(), | |||
| inside_pkt_codec: None, | |||
| pkt_encoder_flush_interval: Duration::from_secs_f64(0.0001), | |||
| pkt_decoder_clean_up_interval: Duration::from_secs_f64(0.5), | |||
There was a problem hiding this comment.
Same as server, better this be an cli config
| // encoder is not set. Awaits forever... | ||
| // Not returning immediately to avoid triggering an exit of the | ||
| // entire client program. | ||
| tracing::debug!("Pkt encoder flush task is freezed"); |
There was a problem hiding this comment.
FYI, tokio::select supports enabling the branch only on valid condition (valid packet codec)
You might use it to disable this task check instead of this.
|
|
||
| /// Signal for toggling inside packet encoding | ||
| #[educe(Debug(ignore))] | ||
| pub toggle_encoding_signal: tokio::sync::mpsc::Receiver<bool>, |
There was a problem hiding this comment.
We should not force apps to provide this value. Better to make this as option similar to inside_pkt_codec
There was a problem hiding this comment.
Or should we create a new PacketEncoderConfig and add all these variables inside it and make it as an option.
It reduces the burden for apps to skip this, if codec is not needed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added inside packet codec interface for Lightway UDP:
EncodedDataandEncodedDataFragToggleEncodingthat allows the client to enable encoding on both sides-eflag in CLI args to automatically send aToggleEncodingpacket to the server once lightway-core changes state toConnectedMotivation and Context
FEC code can be integrated to inside packets. Throughput and latency can be reduced under high packet loss network conditions.
How Has This Been Tested?
iperf3andping 8.8.8.8Integration with C servers
With the network namespace scripts, running the latest version of lightway C UDP server: Verified that sending wire frames with the new ids does not break the connection with the server.
Types of changes
Checklist:
main