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

Added two apis for adding and removing given ebpf programs #76

Merged
merged 5 commits into from
Aug 17, 2022

Conversation

Atul-source
Copy link
Contributor

@Atul-source Atul-source commented Jun 15, 2022

Signed-off-by: Atul [email protected]

@Atul-source
Copy link
Contributor Author

Atul-source commented Jun 15, 2022

This PR is adding two granular apis

  1. add : add given eBPF programs on given node.
  2. delete: delete given eBPF programs on given node.

issue #57

dthaler
dthaler previously approved these changes Jun 22, 2022
@Atul-source
Copy link
Contributor Author

It doesn’t make any sense to test all the functions in this PR because they are eventually calling other functions which are already tested.

@Atul-source
Copy link
Contributor Author

Atul-source commented Jun 23, 2022

when api payload files merged in l3af-arch repository then docs link will work

pull request : l3af-project/l3af-arch#41

return fmt.Errorf("failed to chain XDP BPF programs: %w", err)
}
log.Info().Msgf("Push Back and Start XDP program : %s seq_id : %d", bpfProg.Name, bpfProg.SeqID)
if err := c.PushBackAndStartBPF(bpfProg, ifaceName, models.XDPIngressType); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if err := c.PushBackAndStartBPF(bpfProg, ifaceName, models.XDPIngressType); err != nil {
if err := c.PushBackAndStartEbpfProgram(bpfProg, ifaceName, models.XDPIngressType); err != nil {

it doesn't start ebpf per se, it starts an ebpf program, if I understand correctly, hence the suggested rename

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this we can do it in a separate PR because we need to do lot of changes

@Atul-source Atul-source changed the title Added to apis for adding and removing given ebpf programs Added two apis for adding and removing given ebpf programs Jul 13, 2022
@jniesz jniesz requested a review from charleskbliu0 July 26, 2022 22:21
@jniesz jniesz requested a review from kanika-mago August 2, 2022 16:40
sanfern
sanfern previously approved these changes Aug 3, 2022
Copy link
Contributor

@sanfern sanfern left a comment

Choose a reason for hiding this comment

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

LGTM

@jniesz
Copy link
Contributor

jniesz commented Aug 10, 2022

I only have one pending unresolved issue above on this and will approve when that is fixed. Please squash all the commits down to a single commit prior to merging, once everything is settled.


defer func(mesg *string, statusCode *int) {
w.WriteHeader(*statusCode)
_, err := w.Write([]byte(*mesg))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we cannot write, should we continue to limp along? I don't quite understand what would happen here after it logs the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just return from function because defer functions will execute in the end of parent function

Copy link
Collaborator

@chaitanyalala chaitanyalala left a comment

Choose a reason for hiding this comment

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

I added a bunch of comments. Please check

Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the support for chaining disabled. I suggested a few minor changes for your updates.

sanfern
sanfern previously approved these changes Aug 14, 2022
jniesz
jniesz previously approved these changes Aug 15, 2022
Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

LGTM with the changes. The only request I have is to squash into a single commit prior to merging.

Signed-off-by: Atul-source <[email protected]>
sanfern
sanfern previously approved these changes Aug 16, 2022
jniesz
jniesz previously approved these changes Aug 16, 2022
return nil
}

switch direction {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we want l3afd to be able to work with arbitrary prog types in the future, I think it will be necessary in the future, and "direction" will be a poor label at that point.

Signed-off-by: Atul-source <[email protected]>
@Atul-source Atul-source dismissed stale reviews from jniesz and sanfern via 49c7042 August 16, 2022 16:36
Signed-off-by: Atul-source <[email protected]>
jniesz
jniesz previously approved these changes Aug 17, 2022
Copy link
Contributor

@jniesz jniesz left a comment

Choose a reason for hiding this comment

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

LGTM

sanfern
sanfern previously approved these changes Aug 17, 2022
Fix name

Co-authored-by: Dave Thaler <[email protected]>
@jniesz jniesz dismissed stale reviews from sanfern and themself via afa1534 August 17, 2022 16:14
fix name

Co-authored-by: Dave Thaler <[email protected]>
@jniesz jniesz merged commit 67ecddb into l3af-project:main Aug 17, 2022
@Atul-source Atul-source deleted the api branch August 18, 2022 06:57
@jniesz jniesz added the feat New Feature label Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants