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

Add customizable metadata fetching params #1667

Merged

Conversation

zuc
Copy link
Member

@zuc zuc commented Jun 5, 2021

Signed-off-by: Michele Zuccala [email protected]

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

Not sure about the appropriate /area label here, could you please advise?

What this PR does / why we need it:

This PR adds support for customizable metadata fetching params that are going to be introduced with falcosecurity/libs#40.

Which issue(s) this PR fixes:

Special notes for your reviewer:

WIP because I'm planning to set the correct libs version and checksum once falcosecurity/libs#40 gets merged.

For now it's set to the feature branch name on https://github.com/falcosecurity/libs in order for this PR to be easily tested.

Does this PR introduce a user-facing change?:

new(userspace/falco): add customizable metadata fetching params

@poiana
Copy link
Contributor

poiana commented Jun 5, 2021

Welcome @zuc! It looks like this is your first PR to falcosecurity/falco 🎉

@poiana poiana added the size/S label Jun 5, 2021
@leogr leogr added this to the 0.30.0 milestone Jun 17, 2021
@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from d012472 to 9028dc6 Compare July 8, 2021 09:55
@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from 9028dc6 to f204065 Compare July 8, 2021 09:59
@zuc
Copy link
Member Author

zuc commented Jul 8, 2021

Note for the reviewer(s): I chose to keep m_metadata_download_max_mb expressed in megabytes for user-friendliness reasons, and to convert the value in bytes only when calling sinsp. Happy to discuss if you have a different point of view here!

@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from f204065 to 9db557e Compare July 8, 2021 15:42
@zuc zuc changed the title wip: add customizable metadata fetching params Add customizable metadata fetching params Jul 8, 2021
@zuc zuc marked this pull request as ready for review July 8, 2021 15:47
@zuc
Copy link
Member Author

zuc commented Jul 8, 2021

Since falcosecurity/libs#40 got merged, this is now ready for review.
Also, driver version has been bumped to falcosecurity/libs@c415d57.

leodido
leodido previously requested changes Jul 8, 2021
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

I believe that checking the config values (coming from the user) against limits (especially upper ones in this case) is necessary.

Example: a user configuring Falco to have max_mb equal to eg. 20000.

Since we know that this approach of fetching the container orchestrator metadata has an impact on memory (and Falco performances), I think it's sane to provide safe boundaries for them.

@poiana poiana added size/M and removed size/S labels Jul 9, 2021
@zuc
Copy link
Member Author

zuc commented Jul 9, 2021

@leodido I added an upper boundary for the maximum download size, and a check to force frequency seconds to be higher than 0.

I don't think it would make much sense to cap the remaining params/boundaries given their meaning, but happy to discuss.

@zuc zuc dismissed stale reviews from leogr and mstemm via 2e640e6 September 7, 2021 10:51
@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from 6ee3025 to 2e640e6 Compare September 7, 2021 10:51
@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from 2e640e6 to b54ca63 Compare September 7, 2021 10:55
@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from b54ca63 to 24c7532 Compare September 7, 2021 11:02
@poiana poiana added size/S and removed size/M labels Sep 7, 2021
@zuc zuc requested review from mstemm and leogr September 7, 2021 11:02
@zuc zuc force-pushed the feature/customizable-metadata-download-params branch from 24c7532 to ae4e1f1 Compare September 17, 2021 14:35
@leogr leogr dismissed leodido’s stale review September 17, 2021 14:54

Dismissing this because your suggestion was accepted and the fix was committed more than two months ago

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.

/approve

@poiana poiana added the lgtm label Sep 20, 2021
@poiana
Copy link
Contributor

poiana commented Sep 20, 2021

LGTM label has been added.

Git tree hash: 4eed899dc909ede675e884898a16c386c212cd58

@poiana
Copy link
Contributor

poiana commented Sep 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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 merged commit a684bec into falcosecurity:master Sep 20, 2021
@zuc zuc deleted the feature/customizable-metadata-download-params branch September 20, 2021 15:36
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.

5 participants