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

fix(scripts): force falco-driver-loader script to try to compile the driver anyway even on unsupported platforms #2219

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Sep 22, 2022

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

What this PR does / why we need it:

Currently, when falco-driver-loader is not able to fetch OS_NAME from multiple files under /etc, like /etc/os-release, it exit with an error.
I think that the best approach is trying to compile the requested driver in any case; consider that TARGET_ID is only really used to concatenate the correct name for the driver on the s3 bucket.
So, in case we are not able to fetch any TARGET_ID, just fallback at disabling ENABLE_DOWNLOAD and go on to compile the driver.
This is mostly useful when using HOST_ROOT but the /etc/ folder is not a shared volume under it.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

…driver anyway even on unsupported platforms.

Signed-off-by: Federico Di Pierro <[email protected]>
jasondellaluce
jasondellaluce previously approved these changes Sep 22, 2022
Copy link
Contributor

@jasondellaluce jasondellaluce left a comment

Choose a reason for hiding this comment

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

LGTM!

@poiana
Copy link
Contributor

poiana commented Sep 22, 2022

LGTM label has been added.

Git tree hash: 5a50819d5b1d1d97967a00532f64a7ec3999cac8

leogr
leogr previously approved these changes Sep 26, 2022
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.

Just found two nits, otherwise LGTM.

Putting
/hold
in case you want to apply my suggestions

res=$?
if [ $res != 0 ]; then
>&2 echo "Detected an unsupported target system, please get in touch with the Falco community. Trying to compile anyway."
ENABLE_DOWNLOAD=
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENABLE_DOWNLOAD=
ENABLE_DOWNLOAD="no"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehy Leo! It would be great if this worked :D Unfortunately, in the script we check if it is not emtpy:

if [ -n "$ENABLE_DOWNLOAD" ]; then

I wanted to make the patch with the smallest diff possible, therefore i avoided to touch it.

get_target_id
res=$?
if [ $res != 0 ]; then
>&2 echo "Detected an unsupported target system, please get in touch with the Falco community. Trying to compile anyway."
Copy link
Member

Choose a reason for hiding this comment

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

Trying to compile anyway is true only if ENABLE_COMPILE is yes.

Perhaps it is worth printing a different message if both compile&download are disabled, and give up immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could be a great idea; but i didn't want to embed more logic inside the script :)
If this works for you, i'd leave it as is; otherwise i can make the small patch.

All in all, i am hoping for falco-driver-loader to be kicked out by falcoctl sooner or later!

@leogr
Copy link
Member

leogr commented Sep 26, 2022

/milestone 0.33.0

@poiana poiana added this to the 0.33.0 milestone Sep 26, 2022
…target distro could not be fetched.

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

Co-authored-by: Leonardo Grasso <[email protected]>
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 👍

@poiana poiana added the lgtm label Sep 27, 2022
@poiana
Copy link
Contributor

poiana commented Sep 27, 2022

LGTM label has been added.

Git tree hash: 554d3d4d92f388432ec27092586ad907aa477ef3

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 27, 2022

/milestone 0.34.0

@poiana poiana modified the milestones: 0.33.0, 0.34.0 Sep 27, 2022
@jasondellaluce
Copy link
Contributor

/hold until Falco 0.33.0 is released

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 25, 2022

/unhold

Copy link
Contributor

@jasondellaluce jasondellaluce 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
Copy link
Contributor

poiana commented Nov 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, jasondellaluce, 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:
  • OWNERS [FedeDP,jasondellaluce,leogr]

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 136eacc into master Nov 2, 2022
@poiana poiana deleted the fix/falco-driver-loader_unsupported_platform branch November 2, 2022 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants