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.

Sign up for GitHub

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

Jump to bottom

Import NAT code #206

Merged
merged 6 commits into from
Feb 5, 2025
Merged

Import NAT code #206

merged 6 commits into from
Feb 5, 2025

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Feb 4, 2025

  • nat: Import NAT draft code
  • nat: Replace deprecated serde_yaml with serde_yml
  • nat: Fix warnings about dead code
  • nat: Use struct Vni instead of u32, for VNIs
  • nat: Make tests work

Not fully functional yet.

Imported from the separate, private repo githedgehog/tmp-nat. There is no attempt to complete or improve the existing code in this PR, only to merge it into the repo while keeping cargo build and cargo test functional.

Refer to individual commits for details.

Fixes: #176

@qmonnet qmonnet added the area/nat Related to Network Address Translation (NAT) label Feb 4, 2025
@qmonnet qmonnet force-pushed the pr/qmonnet/merge-nat branch 2 times, most recently from 004b816 to d5de0e0 Compare February 4, 2025 14:55
@qmonnet
Copy link
Member Author

qmonnet commented Feb 4, 2025

@daniel-noland The issue I face with CI here is that some crate (namely, regex-automata) is pulled twice, with different minor versions, and triggers an error in the cargo deny check. How would you recommend to solve this? We could skip the ban for this crate I guess, but is there a cleaner way to address the issue?

@daniel-noland
Copy link
Collaborator

@daniel-noland The issue I face with CI here is that some crate (namely, regex-automata) is pulled twice, with different minor versions, and triggers an error in the cargo deny check. How would you recommend to solve this? We could skip the ban for this crate I guess, but is there a cleaner way to address the issue?

Will pull this and look.

The resolver is normally verry good about this but it does happen. Can't say just yet

@daniel-noland
Copy link
Collaborator

daniel-noland commented Feb 4, 2025 •
edited
Loading

@qmonnet, I think I figured out a workable solution. Suggested commit pushed but do let me know if you object observe observe

Looks like I'm a victim of my own commits. I didn't test properly and we are using tracing-test. Stand by

@daniel-noland
Copy link
Collaborator

The root issue is here: tokio-rs/tracing#3033

We should add an exception in the deny list for the moment and then adjust once tracing bumps to matchers 0.2

@daniel-noland daniel-noland force-pushed the pr/qmonnet/merge-nat branch 2 times, most recently from 6dd233b to ac4865c Compare February 4, 2025 20:32
@qmonnet qmonnet marked this pull request as ready for review February 5, 2025 09:55
@qmonnet qmonnet requested a review from a team as a code owner February 5, 2025 09:55
@qmonnet qmonnet requested review from Fredi-raspall and sergeymatov and removed request for a team February 5, 2025 09:55
@qmonnet
Copy link
Member Author

qmonnet commented Feb 5, 2025

Thanks Daniel 🙏

sergeymatov
sergeymatov approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@sergeymatov sergeymatov left a comment

Choose a reason for hiding this comment

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

lgtm. Don't forget to fix my TODOs later :)

qmonnet and others added 6 commits February 5, 2025 11:19
@qmonnet @sergeymatov @daniel-noland
nat: Import NAT draft code
b92a7b1 
Warning: This code is not fully functional, yet. Also, tests don't pass.

This is an import from the private repository at githedgehog/tmp-nat.
Only the nat.rs file has been ported so far. Some YAML files used for
tests will be ported in subsequent commits, while some other source
files (vni.rs, lr_examples/) were not meant to be imported here and were
ignored. The Git history from the separate repository had limited
insight about the code evolution, and is not imported.

Some minor adjustments were required:

- I added the license and copyright headers;
- I removed the declarations of the vni and lr_examples modules;
- I temporarily reverted to the use of u32s instead of Vni-s;
- I removed the empty main() function from nat.rs, now a module.

The code is not complete yet, but this will allow us to:

- avoid managing a separate repository,
- make it public (no reason to keep it private),
- better hook it into the dataplane package,
- build it in CI to detect regressions (no workflow in the other repo).

Joint work with Sergey (who authored most of this code) and Daniel.

Co-authored-by: Sergey Matov <sergey.matov@githedgehog.com>
Co-authored-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet
nat: Replace deprecated serde_yaml with serde_yml
7be468e 
The latter is a fork, apparently maintained, of the former.

Link: https://github.com/sebastienrousseau/serde_yml
Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet
nat: Fix warnings about dead code
9d65043 
The struct GlobalContext and the pif_table attribute in struct Vpc are
not used at this time. We'll use them soon (or clean them up if
necessary), but in the meantime we prefer to suppress the related
warnings at build time.

Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet @daniel-noland
nat: Use struct Vni instead of u32, for VNIs
e483ec1 
We already have a representation of VNIs, let's use it rather than a u32
that doesn't match the real size (24-bit) of the IDs.

Co-authored-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet
nat: Make tests work
77c5d34 
Import YAML files used for tests in the NAT module. Fix path handling
for pointing to these YAML files in the code, and also fix dependencies
imports for running the tests.

Signed-off-by: Quentin Monnet <qmo@qmon.net>
@daniel-noland @qmonnet
deny.toml: Temporarily allow duplicated version of matchers crate
7e76f22 
Tracking issue tokio-rs/tracing#3033

Adjust deny.toml to address a minor version alignment issue in the
tracing ecosystem causing duplicated versions of the matchers crate to
be pulled.

[ Quentin: Rebased, edited commit log, and fixed merge conflict with
    commit 90a9056 ("Syn is no longer duplicated") ]

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet qmonnet force-pushed the pr/qmonnet/merge-nat branch from ac4865c to 7e76f22 Compare February 5, 2025 11:26
@qmonnet
Copy link
Member Author

qmonnet commented Feb 5, 2025

I rebased the PR to address the merge conflicts on deny.toml and Cargo.lock. No other change.

@qmonnet qmonnet enabled auto-merge February 5, 2025 11:30
@qmonnet qmonnet added this pull request to the merge queue Feb 5, 2025
Merged via the queue into main with commit 7e76f22 Feb 5, 2025
20 of 25 checks passed
@qmonnet qmonnet deleted the pr/qmonnet/merge-nat branch February 5, 2025 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergeymatov sergeymatov sergeymatov approved these changes

@Fredi-raspall Fredi-raspall Awaiting requested review from Fredi-raspall Fredi-raspall is a code owner automatically assigned from githedgehog/dataplane-reviewers

Assignees
No one assigned
Labels
area/nat Related to Network Address Translation (NAT)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Import NAT code into dataplane repo
3 participants
@qmonnet @daniel-noland @sergeymatov