new(cmd,pkg,docs,docgen,validate): switched from `slog` go library to rich-text falcoctl log library #333

FedeDP · 2024-04-11T08:56:50Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area cmd
/area pkg
/area docs

What this PR does / why we need it:

To better integrate falcoctl and driverkit, we needed to:

avoid using a global logger in driverkit (we always used default slog instance)
possibly use same logger in both library

Since falcoctl log library is much richer and better, i ported driverkit to use it; now each executor takes a logger parameter, that makes much easier and seamless the ingreation with falcoctl.
The result is wonderful:
driverkit.webm

While i was at it, i also:

updated docs (never updated since the introduction of local builder
ported driverkit help/usage message printing to follow same conventions as falcoctl (ie: print them less often basically :D)

TODO:

fix tests :)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(cmd,pkg,docs,docgen,validate): switched from `slog` go library to rich-text falcoctl log library

FedeDP · 2024-04-11T08:58:02Z

cmd/completion.go

@@ -46,12 +47,12 @@ func validateArgs() cobra.PositionalArgs {
 		if len(args) == 0 {
 			return nil
 		}
-		return cobra.ExactValidArgs(1)(c, args)
+		return cobra.MatchAll(cobra.ExactArgs(1), cobra.OnlyValidArgs)(c, args)


This avoids using a deprecated method.

FedeDP · 2024-04-11T08:58:13Z

cmd/completion.go

 	}
 }

 // NewCompletionCmd ...
-func NewCompletionCmd() *cobra.Command {
+func NewCompletionCmd(_ *ConfigOptions, _ *RootOptions, _ *pflag.FlagSet) *cobra.Command {


Same signature as other commands.

cmd/config_options.go

FedeDP · 2024-04-11T08:59:39Z

cmd/config_options.go

-		errArr := []error{}
-		for _, e := range errors {
+		var errs validator.ValidationErrors
+		errors.As(err, &errs)


Use errors.As as suggested by ide.
This kind of cleanup is done multiple times.

FedeDP · 2024-04-11T09:00:23Z

cmd/config_options.go

 }

 // NewConfigOptions creates an instance of ConfigOptions.
-func NewConfigOptions() *ConfigOptions {
-	o := &ConfigOptions{}
+func NewConfigOptions() (*ConfigOptions, error) {


When creating a configoptions, default to a INFO level logger.

FedeDP · 2024-04-11T09:01:27Z

pkg/driverbuilder/builder/amazonlinux.go

@@ -212,7 +211,6 @@ func buildMirror(a amazonBuilder, r string, kv kernelrelease.KernelRelease) (str
 	}

 	mirror := fmt.Sprintf("%s/%s", baseURL, "mirror.list")
-	slog.With("url", mirror, "version", r).Debug("looking for repo...")


I don't think we actually needed these ones; passing a printer down there would be a nightmare :)
Also, i think distro builders should not log anything.

FedeDP · 2024-04-11T09:03:05Z

pkg/driverbuilder/builder/builders.go

@@ -415,7 +421,6 @@ func GetResolvingURLs(urls []string) ([]string, error) {
 		}
 		if res.StatusCode == http.StatusOK {
 			results = append(results, u)
-			slog.With("url", u).Debug("kernel header url found")


Again, passing the printer down here would be a bit troublesome.
I preferred to just kill this debug log (and adding a log in KernelDownloadScript: https://github.com/falcosecurity/driverkit/pull/333/files#diff-16a66a3bc20a8ec693f64ed821148f71efa386a7e942444444eb591512bf129bR173)

FedeDP · 2024-04-11T09:03:30Z

pkg/driverbuilder/builder/builders.go

 	}
 	base, err := url.Parse(uu.Host)
 	if err != nil {
-		slog.Error(err.Error())
-		os.Exit(1)
+		panic(err)


Panic instead of erroring out. Again, this was done to avoid passing the printer down here.

FedeDP · 2024-04-11T09:06:11Z

pkg/driverbuilder/docker.go

@@ -63,26 +63,27 @@ func (bp *DockerBuildProcessor) String() string {
 	return DockerBuildProcessorName
 }

-func mustCheckArchUseQemu(ctx context.Context, b *builder.Build, cli *client.Client) {
+func (bp *DockerBuildProcessor) mustCheckArchUseQemu(ctx context.Context, b *builder.Build, cli *client.Client) {


Added the receiver since we need to access bp.Printer. I could've just used b.Printer instead, it would be the same, but i preferred this way since this method is tied to docker build processor anyway.

FedeDP · 2024-04-11T09:07:05Z

cmd/docker.go

-			slog.With("processor", c.Name()).Info("driver building, it will take a few seconds")
-			if !configOptions.DryRun {
-				b := rootOpts.ToBuild()
+		RunE: func(c *cobra.Command, args []string) error {


Another small change: return an error from executors instead of dealing with os.Exit(1) ourselves.

FedeDP · 2024-04-11T09:08:23Z

cmd/docker.go

+				// Since we use a spinner, cache log data to a bytesbuffer;
+				// we will later print it once we stop the spinner.
+				var buf bytes.Buffer
+				b := rootOpts.ToBuild(configOpts.Printer.WithWriter(&buf))


This is a small trick: since when using a Spinner we cannot print anything to screen (otherwise that would cause a spinner refresh that changes its title and looks ugly), we store to Build a printer that prints to a bytes buffer; then we defer a function that prints everything after the spinner was stopped.

FedeDP · 2024-04-23T08:57:42Z

Needs a rebase! Going to rebase asap!

… use rich-text falcoctl log library. Signed-off-by: Federico Di Pierro <[email protected]>

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

Moreover, moved cmd output to more strictly follow `falcoctl` one, with regards to printing usage/helper messages. Signed-off-by: Federico Di Pierro <[email protected]>

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

EXONER4TED

/approve

poiana · 2024-04-23T13:00:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [EXONER4TED,FedeDP]

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

poiana added kind/feature New feature or request do-not-merge/work-in-progress dco-signoff: yes area/cmd area/pkg area/docs labels Apr 11, 2024

poiana requested review from leodido and Lowaiz April 11, 2024 08:56

poiana added approved size/XXL labels Apr 11, 2024

FedeDP commented Apr 11, 2024

View reviewed changes

cmd/config_options.go Show resolved Hide resolved

FedeDP commented Apr 11, 2024

View reviewed changes

FedeDP force-pushed the new/falcoctl_printer branch 2 times, most recently from c1d2c62 to 40efc36 Compare April 11, 2024 12:19

FedeDP changed the title ~~wip: new(cmd,pkg,docs,docgen,validate): switched from slog go library to rich-text falcoctl log library~~ new(cmd,pkg,docs,docgen,validate): switched from slog go library to rich-text falcoctl log library Apr 11, 2024

poiana removed the do-not-merge/work-in-progress label Apr 11, 2024

FedeDP added 4 commits April 23, 2024 10:58

new(cmd,pkg,docs,docgen,validate): switched from slog go library to…

2701ea5

… use rich-text falcoctl log library. Signed-off-by: Federico Di Pierro <[email protected]>

chore(cmd,pkg): small improvements and initial test fixes.

8c6a73b

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

chore(cmd,pkg): allow cli test to disable styling.

589ad27

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

chore(cmd,docs): updated docs and fixed cmd related tests.

1b25d9c

Moreover, moved cmd output to more strictly follow `falcoctl` one, with regards to printing usage/helper messages. Signed-off-by: Federico Di Pierro <[email protected]>

chore(go.sum): bumped docker library.

66e37f7

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

FedeDP force-pushed the new/falcoctl_printer branch from 40efc36 to 66e37f7 Compare April 23, 2024 09:00

EXONER4TED approved these changes Apr 23, 2024

View reviewed changes

poiana assigned EXONER4TED Apr 23, 2024

poiana added the lgtm label Apr 23, 2024

poiana merged commit 00c7e79 into master Apr 23, 2024
8 checks passed

poiana deleted the new/falcoctl_printer branch April 23, 2024 13:01

FedeDP mentioned this pull request May 8, 2024

new(pkg): use driverkit local build processor instead of implementing drivers build falcosecurity/falcoctl#356

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(cmd,pkg,docs,docgen,validate): switched from `slog` go library to rich-text falcoctl log library #333

new(cmd,pkg,docs,docgen,validate): switched from `slog` go library to rich-text falcoctl log library #333

FedeDP commented Apr 11, 2024 •

edited

Loading

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP Apr 11, 2024

FedeDP commented Apr 23, 2024

EXONER4TED left a comment

poiana commented Apr 23, 2024

new(cmd,pkg,docs,docgen,validate): switched from slog go library to rich-text falcoctl log library #333

new(cmd,pkg,docs,docgen,validate): switched from slog go library to rich-text falcoctl log library #333

Conversation

FedeDP commented Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP commented Apr 23, 2024

EXONER4TED left a comment

Choose a reason for hiding this comment

poiana commented Apr 23, 2024

new(cmd,pkg,docs,docgen,validate): switched from `slog` go library to rich-text falcoctl log library #333

new(cmd,pkg,docs,docgen,validate): switched from `slog` go library to rich-text falcoctl log library #333

FedeDP commented Apr 11, 2024 •

edited

Loading