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

new(cmd,internal/utils,pkg/driver): use correct engine.kind config key #357

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Nov 24, 2023

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area library
/area cli
/area tests

What this PR does / why we need it:

Moreover, added a new ReplaceTextInFile utils, and added tests for it and ReplaceLineInFile.

Which issue(s) this PR fixes:

Refs #327

Fixes #

Special notes for your reviewer:

return err
}
if err = checkFalcoRunsWithDrivers(cfg.Engine.Kind); err != nil {
return err
Copy link
Contributor Author

@FedeDP FedeDP Nov 24, 2023

Choose a reason for hiding this comment

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

I am not sure whether we should error out in this case.
ConfigMap loop does just continue to the next configmap found, if any.

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 we might not want to give an error in this case but just print a warning?
Indeed, we might want to add a -f,--force option to force the update.

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 added the changes in the latest commit.
Example output when running fine and when running with the warning:

sudo ./falcoctl driver config --host-root "." --type ebpf
2023-11-24 12:20:02 INFO  Running falcoctl driver config driver host root: . driver type: ebpf

sudo ./falcoctl driver config --host-root "." --type ebpf
2023-11-24 12:20:09 INFO  Running falcoctl driver config driver host root: . driver type: ebpf
2023-11-24 12:20:09 WARN  Avoid updating Falco configuration config: etc/falco/falco.yaml reason: engine.kind is not driver driven: gvisor

@@ -28,7 +28,7 @@ import (
)

// TypeBpf is the string for the bpf driver type.
const TypeBpf = "bpf"
const TypeBpf = "ebpf"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New name. See falcosecurity/falco#2413

@@ -23,7 +23,7 @@ import (
)

// TypeModernBpf is the string for the bpf driver type.
const TypeModernBpf = "modern-bpf"
const TypeModernBpf = "modern_ebpf"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New name, see falcosecurity/falco#2413

…key.

Moreover, added a new ReplaceTextInFile utils, and added tests for it and ReplaceLineInFile.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…a non-driver driven kind.

Instead, print a warning.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/use_correct_engine_kind branch from 7bc0860 to 635e873 Compare November 24, 2023 11:27
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 24, 2023

Rebased on main.

Copy link
Member

@alacuku alacuku left a comment

Choose a reason for hiding this comment

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

Hey @FedeDP, left some comments.

cmd/driver/config/config.go Show resolved Hide resolved
Comment on lines +189 to +194
type engineCfg struct {
Kind string `yaml:"kind"`
}
type falcoCfg struct {
Engine engineCfg `yaml:"engine"`
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we please declare these new types outside the function?

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 they are good there since they are only needed to unmarshal the Falco engine.kind configuration key; no need to expose them elsewhere in the package.

Comment on lines +195 to +202
yamlFile, err := os.ReadFile(filepath.Clean(falcoCfgFile))
if err != nil {
return err
}
cfg := falcoCfg{}
if err = yaml.Unmarshal(yamlFile, &cfg); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if there is a better way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just manually parse the file line by line, but i think this is the best way to achieve it actually.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Co-authored-by: Aldo Lacuku <aldo@lacuku.eu>
@poiana
Copy link
Contributor

poiana commented Nov 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alacuku, 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 merged commit d762a9c into main Nov 27, 2023
12 checks passed
@poiana poiana deleted the new/use_correct_engine_kind branch November 27, 2023 10:33
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 28, 2023

/milestone v0.7.0

@poiana poiana added this to the v0.7.0 milestone Nov 28, 2023
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.

4 participants