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

feat(test): e2e integration test for new helpers #4354

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

rscampos
Copy link
Collaborator

@rscampos rscampos commented Oct 16, 2024

fix: #4346

1. Explain what the PR does

f457c2b feat(test): e2e integration test for new helpers

2. Explain how to test it

  1. Duplicate the http.go signature file.
  2. Update the event to net_packet_http_request or net_packet_http_response.
  3. Use the appropriate helper: GetProtoHTTPRequestByName or GetProtoHTTPResponseByName.

3. Other comments

@NDStrahilevitz
Copy link
Collaborator

@rscampos Maybe use these helpers in the existing signatures if possible? For example add themto the instrumentation tests. Then we can merge.

@rscampos rscampos force-pushed the 4346_net_packet_http_request branch 2 times, most recently from b85e0fb to bedbced Compare October 18, 2024 20:05
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

LGTM, although we should probably use something similar to the flow events in DNS instead of these events (which are meant for legacy compatibility iirc).

go.mod Outdated
@@ -43,6 +43,8 @@ require (
sigs.k8s.io/controller-runtime v0.18.2
)

replace github.com/aquasecurity/tracee/signatures/helpers => ./signatures/helpers/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove and update through types PR

@NDStrahilevitz
Copy link
Collaborator

@rscampos Merge at will once you do the types PR (which you can merge without +1 as far as i'm concerned)

@rscampos rscampos force-pushed the 4346_net_packet_http_request branch from 5a60f6f to f457c2b Compare December 3, 2024 22:15
@rscampos rscampos changed the title feat(helpers): net_packet_http request/response feat(test): e2e integration test for new helpers Dec 4, 2024
@rscampos
Copy link
Collaborator Author

rscampos commented Dec 4, 2024

/fast-forward

This comment was marked as outdated.

@rscampos
Copy link
Collaborator Author

rscampos commented Dec 4, 2024

/fast-forward

This comment was marked as duplicate.

@geyslan
Copy link
Member

geyslan commented Dec 4, 2024

/fast-forward

Copy link

github-actions bot commented Dec 4, 2024

Triggered from #4354 (comment) by @​geyslan.

Trying to fast forward main (8f63209) to 4346_net_packet_http_request (f457c2b).

Target branch (main):

commit 8f63209c1dc5ecd7e08fce2ebaf2eb3f802068a3 (HEAD -> main, origin/main, origin/HEAD)
Author: Gregório G. <geyslan@gmail.com>
Date:   Tue Dec 3 17:25:38 2024 -0300

    fix(tests): possible out of range in integration (#4305)
    
    * fix(tests): possible out of range in integration
    
    ExpectAllInOrderSequentially might try to access an index out of range
    depending on the number of events that are being checked.
    
    More about in issue #4255.
    
    * fix(tests): use ExpectAtLeastOneForEach
    
    Two test cases were using ExpectAllInOrderSequentially helper function
    and passing by luck, since they emit more events than expected only
    beyond the expected events boundary.
    
    For that cases we should use ExpectAtLeastOneForEach helper function
    instead.

Pull request (4346_net_packet_http_request):

commit f457c2bf6cffc49a0ed9c04d3d35a842f6802fe7 (pull_request/4346_net_packet_http_request)
Author: Raphael Campos <raphaelcampos.rp@gmail.com>
Date:   Fri Oct 18 08:35:53 2024 -0500

    feat(test): e2e integration test for new helpers

Fast forwarding main (8f63209) to 4346_net_packet_http_request (f457c2b).

$ git push origin f457c2bf6cffc49a0ed9c04d3d35a842f6802fe7:main
To https://github.com/aquasecurity/tracee.git
 ! [remote rejected]   f457c2bf6cffc49a0ed9c04d3d35a842f6802fe7 -> main (refusing to allow a GitHub App to create or update workflow `.github/workflows/pr.yaml` without `workflows` permission)
error: failed to push some refs to 'https://github.com/aquasecurity/tracee.git'

@geyslan
Copy link
Member

geyslan commented Dec 4, 2024

Or, you're touching workflows. That's why. In this case you need to use the conventional Github button.

@rscampos rscampos merged commit 23044a1 into aquasecurity:main Dec 4, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetProtoHTTPByName for net_packet_http_request
3 participants