feat: Replace network header types from bindings.rs to crate #147

maheshrayas · 2023-11-29T10:56:59Z

fixes: #99

linux-foundation-easycla · 2023-11-29T10:57:05Z

The committers listed above are authorized under a signed CLA.

✅ login: maheshrayas / name: Mahesh Rayas (fa24c88, dd19ccf, f9347f9, a7fd3f2)

k8s-ci-robot · 2023-11-29T10:57:08Z

Welcome @maheshrayas!

It looks like this is your first PR to kubernetes-sigs/blixt 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/blixt has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

shaneutt

It's super cool that you jumped in here to take care of this, thank you! Very glad to have your help with the project!

Code looks great, I have only a few minor comments.

/ok-to-test

/cc @astoycos for review

and /cc @vadorovsky just for awareness that we're going to start using your crate, thanks for creating and maintaining it! 🖖

dataplane/ebpf/Cargo.toml

dataplane/ebpf/src/egress/icmp.rs

dataplane/ebpf/src/main.rs

dataplane/ebpf/src/utils.rs

astoycos · 2023-11-29T15:26:24Z

Thanks @maheshrayas! I will also take a look today 👍

* run cargo fmt * Add the offset variable * remove the ETH_HDR_LEN and IP_HDR_LEN constant and use the constant defined in crate

shaneutt

Looks awesome!

/ok-to-test
/approve

Thank you so much for jumping in and taking care of this one. Approved, but I'll leave the final LGTM to @astoycos.

astoycos

Looking really good @maheshrayas Thanks for taking care of this 🥳

Some small nits/fix also please remove all the xtask machinery we have at https://github.com/kubernetes-sigs/blixt/blob/main/dataplane/xtask/src/main.rs#L29 concerning codegen!!

dataplane/ebpf/Cargo.toml

dataplane/ebpf/src/utils.rs

vadorovsky

Thanks for pinging me. I made a new release of network-types, so you don't need to use git https://crates.io/crates/network-types/0.0.5

Besides, I think this xtask subcommand can be removed now :)

https://github.com/kubernetes-sigs/blixt/blob/main/dataplane/xtask/src/codegen.rs

dataplane/ebpf/Cargo.toml

shaneutt · 2023-11-30T15:31:16Z

Thanks for pinging me. I made a new release of network-types, so you don't need to use git https://crates.io/crates/network-types/0.0.5

Besides, I think this xtask subcommand can be removed now :)

https://github.com/kubernetes-sigs/blixt/blob/main/dataplane/xtask/src/codegen.rs

ACK

Thank you! 🥳

* remove codegen functionality in xtask * remove unused imports * bump the network_type crate version to v0.0.5

maheshrayas · 2023-11-30T22:15:08Z

@astoycos thank you for reviewing. Resolved all.

astoycos

/lgtm
/approve

Thanks again @maheshrayas!

k8s-ci-robot · 2023-11-30T22:22:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, maheshrayas, shaneutt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [astoycos,shaneutt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

astoycos · 2023-11-30T22:38:16Z

oops @maheshrayas Can you remove that one extra commit 6fe3471

maheshrayas · 2023-11-30T22:42:10Z

oops @maheshrayas Can you remove that one extra commit 6fe3471

done

astoycos · 2023-11-30T23:03:47Z

/lgtm

feat: Replace network header types from bindings.rs to crate

fa24c88

fixes: kubernetes-sigs#99

k8s-ci-robot requested review from astoycos and robscott November 29, 2023 10:57

fix: delete commented code

dd19ccf

shaneutt requested changes Nov 29, 2023

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 29, 2023

kubernetes-sigs deleted a comment from k8s-ci-robot Nov 29, 2023

shaneutt modified the milestones: v1.0.0 - Reference Implementation, v0.3.0 - Gateway API Conformance Nov 29, 2023

astoycos self-assigned this Nov 29, 2023

fix: review comments

f9347f9

* run cargo fmt * Add the offset variable * remove the ETH_HDR_LEN and IP_HDR_LEN constant and use the constant defined in crate

shaneutt approved these changes Nov 30, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023

astoycos requested changes Nov 30, 2023

View reviewed changes

dataplane/ebpf/Cargo.toml Outdated Show resolved Hide resolved

dataplane/ebpf/src/utils.rs Outdated Show resolved Hide resolved

vadorovsky suggested changes Nov 30, 2023

View reviewed changes

dataplane/ebpf/Cargo.toml Outdated Show resolved Hide resolved

fix: review comments

a7fd3f2

* remove codegen functionality in xtask * remove unused imports * bump the network_type crate version to v0.0.5

astoycos approved these changes Nov 30, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

maheshrayas force-pushed the main branch from 6fe3471 to a7fd3f2 Compare November 30, 2023 22:41

astoycos added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

k8s-ci-robot merged commit dc1a070 into kubernetes-sigs:main Nov 30, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Replace network header types from bindings.rs to crate #147

feat: Replace network header types from bindings.rs to crate #147

maheshrayas commented Nov 29, 2023

linux-foundation-easycla bot commented Nov 29, 2023 •

edited

Loading

k8s-ci-robot commented Nov 29, 2023

shaneutt left a comment •

edited

Loading

astoycos commented Nov 29, 2023

shaneutt left a comment •

edited

Loading

astoycos left a comment

vadorovsky left a comment

shaneutt commented Nov 30, 2023

maheshrayas commented Nov 30, 2023

astoycos left a comment

k8s-ci-robot commented Nov 30, 2023

astoycos commented Nov 30, 2023

maheshrayas commented Nov 30, 2023

astoycos commented Nov 30, 2023

feat: Replace network header types from bindings.rs to crate #147

feat: Replace network header types from bindings.rs to crate #147

Conversation

maheshrayas commented Nov 29, 2023

linux-foundation-easycla bot commented Nov 29, 2023 • edited Loading

k8s-ci-robot commented Nov 29, 2023

shaneutt left a comment • edited Loading

Choose a reason for hiding this comment

astoycos commented Nov 29, 2023

shaneutt left a comment • edited Loading

Choose a reason for hiding this comment

astoycos left a comment

Choose a reason for hiding this comment

vadorovsky left a comment

Choose a reason for hiding this comment

shaneutt commented Nov 30, 2023

maheshrayas commented Nov 30, 2023

astoycos left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 30, 2023

astoycos commented Nov 30, 2023

maheshrayas commented Nov 30, 2023

astoycos commented Nov 30, 2023

linux-foundation-easycla bot commented Nov 29, 2023 •

edited

Loading

shaneutt left a comment •

edited

Loading

shaneutt left a comment •

edited

Loading