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

fix(driver,userspace/libsinsp): use new PPME_CONTAINER_JSON_2_ events with large payload #118

Merged
merged 1 commit into from
Nov 3, 2021
Merged

fix(driver,userspace/libsinsp): use new PPME_CONTAINER_JSON_2_ events with large payload #118

merged 1 commit into from
Nov 3, 2021

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Nov 3, 2021 

Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Mark Stemm <[email protected]>

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area libsinsp

What this PR does / why we need it:

Deny new scap files with PPME_CONTAINER_JSON_2_ events from be opened by old falco; moreover, new falco is now correctly able to open old PPME_CONTAINER_JSON_ events.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@FedeDP @mstemm
fix(driver,userspace/libsinsp): use new PPME_CONTAINER_JSON_2_ events…
75851c8 
… with large payload; leave old ones untouched to avoid breaking backward compatibility.

This way, new scap files with PPME_CONTAINER_JSON_2_ events cannot  be open by old falco; moreover, new falco can correctly open old PPME_CONTAINER_JSON_ events.

Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Mark Stemm <[email protected]>
@poiana
Copy link
Contributor

poiana commented Nov 3, 2021

Hi @FedeDP. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@poiana poiana requested review from gnosek and leogr November 3, 2021 09:07
@poiana poiana added the size/S label Nov 3, 2021
@leogr
Copy link
Member

leogr commented Nov 3, 2021

/ok-to-test

leogr
leogr approved these changes Nov 3, 2021
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM
/approve

@poiana poiana added the lgtm label Nov 3, 2021
@poiana
Copy link
Contributor

poiana commented Nov 3, 2021

LGTM label has been added.

Git tree hash: 1ba222e6566b43cf0a6f0bb73a5c0737fb6fad7b

@poiana
Copy link
Contributor

poiana commented Nov 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, leogr

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:

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

@poiana poiana added the approved label Nov 3, 2021
mstemm
mstemm approved these changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

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

I tested with falco and tests that involved container json events in trace files are now passing. I also grepped for CONTAINER_JSON and you covered all the instances I found, so lgtm!

@poiana poiana merged commit f7877aa into falcosecurity:master Nov 3, 2021
@FedeDP FedeDP deleted the ppme_container_json_2 branch November 3, 2021 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstemm mstemm mstemm approved these changes

@leogr leogr leogr approved these changes

@gnosek gnosek Awaiting requested review from gnosek

Assignees

@leogr leogr

@mstemm mstemm

Labels
approved area/libsinsp dco-signoff: yes kind/bug Something isn't working lgtm ok-to-test release-note-none size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FedeDP @poiana @leogr @mstemm