Skip to content
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

0.2.0 alpha #138

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

0.2.0 alpha #138

wants to merge 8 commits into from

Conversation

drunkirishcoder
Copy link
Contributor

@drunkirishcoder drunkirishcoder commented Mar 29, 2021

Description

Finally here. Long waited divorce from Tokio pre-alpha as the underlying runtime. Along the way we also wanted to re-envision how we process packets. Major breaking changes,

  • goodbye Tokio (closing Upgrade the task scheduler to stable Tokio #75), hello Smol. this actually doesn't impact the end user much.
  • brand new runtime with a simpler set of API! all FFI code has been rewritten from the ground up too.
  • no more combinators. that was an idea carried over from NetBricks and that concept no longer align with how we handle types.
  • same example apps but they really work! no more replaying pcap files, they now interact with live traffic in our vagrant.

a couple things still missing

  • metrics! need to put metrics back.
  • revisit the packet ownership around parse and peek.

Type of change

  • Breaking change
  • Documentation update

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #138 (1232a4c) into master (63b7155) will increase coverage by 7.60%.
The diff coverage is 55.59%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   68.69%   76.29%   +7.60%     
==========================================
  Files          66       50      -16     
  Lines        5836     5050     -786     
==========================================
- Hits         4009     3853     -156     
+ Misses       1827     1197     -630     
Impacted Files Coverage Δ
core/src/packets/arp.rs 63.31% <ø> (-0.37%) ⬇️
core/src/packets/ethernet.rs 79.86% <0.00%> (ø)
core/src/packets/icmp/v4/echo_reply.rs 70.66% <ø> (ø)
core/src/packets/icmp/v4/echo_request.rs 80.00% <ø> (ø)
core/src/packets/icmp/v4/mod.rs 80.73% <ø> (ø)
core/src/packets/icmp/v4/redirect.rs 75.82% <ø> (ø)
core/src/packets/icmp/v4/time_exceeded.rs 74.07% <ø> (ø)
core/src/packets/icmp/v6/echo_reply.rs 70.66% <ø> (ø)
core/src/packets/icmp/v6/echo_request.rs 70.66% <ø> (ø)
core/src/packets/icmp/v6/mod.rs 77.96% <ø> (ø)
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zeeshanlakhani zeeshanlakhani self-requested a review March 29, 2021 23:24
@drunkirishcoder drunkirishcoder marked this pull request as draft March 29, 2021 23:25
@drunkirishcoder drunkirishcoder linked an issue Mar 30, 2021 that may be closed by this pull request
@drunkirishcoder drunkirishcoder marked this pull request as ready for review April 7, 2021 15:46
@JakkuSakura
Copy link
Contributor

I have operation not supported running any of the examples.

@drunkirishcoder
Copy link
Contributor Author

drunkirishcoder commented Apr 10, 2021

are you running the examples in our vagrant environment? they only work inside vagrant+docker setup because they have to assume a certain network topology with specific device names. if you are using vagrant, can you post the program output? I will take a look.

@JakkuSakura
Copy link
Contributor

JakkuSakura commented Apr 10, 2021 via email

@drunkirishcoder
Copy link
Contributor Author

drunkirishcoder commented Apr 10, 2021

At this time, we are only going to support the 0.2.0 examples running in our controlled environment. To make them work and interact with live packets, we have to make a lot of assumptions about their device name, IP address, MAC address, subnet etc. basically a lot goes into controlling how the packets are routed to make the examples work.

but if you set the log level to DEBUG and share the output, I can take a look and maybe identify what "operation not supported" is referring to. that error could suggest you are potentially encountering some device capability limitation? I'm not sure. but I don't have a lot info to go on.

@JakkuSakura
Copy link
Contributor

Maybe my environment was not clean enough. After rebooting, all examples are runnable.

Copy link
Member

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

Still going through stuff and testing on baremetal, but starting through the process.

bench/combinators.rs Show resolved Hide resolved
}
}

// delete later?
Copy link
Member

Choose a reason for hiding this comment

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

@drunkirishcoder just not something we're using currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm honestly don't remember why I commented this, been almost a year. lol. I guess I had to do something during migrating code from v0.1 to v0.2? I will see what happens if I delete this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed comment. it is being used. might clean up in the following work around mbuf clones.

core/src/testils/mod.rs Show resolved Hide resolved
core/src/runtime/pcap_dump.rs Show resolved Hide resolved

/// Makes NonNull even easier to use with the downside that it hides the
/// unsafeness of the underlying pointer access.
pub(crate) struct EasyPtr<T>(NonNull<T>);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a better alternative to investigate here?

Copy link
Contributor Author

@drunkirishcoder drunkirishcoder Jan 10, 2022

Choose a reason for hiding this comment

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

probably not? this is completely internal to capsule ffi. it just cuts down on how many unsafe {} we have to sprinkle through out the code base and the use is completely limited to the ffi layer. for example, all the packet headers are held as NonNull, not EasyPtr. if you see any usage outside the ffi layer, call it out.

Copy link
Member

Choose a reason for hiding this comment

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

Will double check

core/src/ffi/pcap.rs Outdated Show resolved Hide resolved
core/src/packets/size_of.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
core/Cargo.toml Outdated
libc = "0.2"
metrics-core = { version = "0.5", optional = true }
metrics-runtime = { version = "0.13", optional = true, default-features = false }
metrics = "0.14"
Copy link
Member

Choose a reason for hiding this comment

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

@drunkirishcoder should we remove for now (until we return metrics separately)?

Copy link
Member

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

so a few minor updates to make, but think we're ready to bring this in.

Will finish running this on the baremetal linux box as well.

@zeeshanlakhani
Copy link
Member

@drunkirishcoder also maybe do a deps update and see if we can fix the security issue related to chrono/time.

@drunkirishcoder drunkirishcoder force-pushed the djin/0.2.0-alpha branch 2 times, most recently from 8e818aa to 74ab2b5 Compare February 26, 2022 03:48
@drunkirishcoder
Copy link
Contributor Author

sigh

getting blocked by an indirect dependency. not sure which lib is pulling it in, but will need to update the sandbox to the latest stable rust. will do it when I find time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade the task scheduler to stable Tokio
3 participants