feat: Allow writing logs to custom file #2473

maxsmythe · 2022-12-20T03:20:59Z

Logging to stderr can make it hard to use logging agents in a manner described by
https://kubernetes.io/docs/concepts/cluster-administration/logging/#sidecar-container-with-logging-agent

Since we run in a distroless container, simply redirecting stderr is not an option. This PR adds the ability to more freely customize a deployment's logging architecture.

Signed-off-by: Max Smythe [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

Logging to stderr can make it hard to use logging agents in a manner described by https://kubernetes.io/docs/concepts/cluster-administration/logging/#sidecar-container-with-logging-agent Since we run in a distroless container, simply redirecting stderr is not an option. This PR adds the ability to more freely customize a deployment's logging architecture. Signed-off-by: Max Smythe <[email protected]>

codecov-commenter · 2022-12-20T03:27:10Z

Codecov Report

Base: 53.42% // Head: 53.19% // Decreases project coverage by -0.22% ⚠️

Coverage data is based on head (185ecb1) compared to base (88afaa4).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2473      +/-   ##
==========================================
- Coverage   53.42%   53.19%   -0.23%     
==========================================
  Files         115      116       +1     
  Lines       10196    10270      +74     
==========================================
+ Hits         5447     5463      +16     
- Misses       4334     4382      +48     
- Partials      415      425      +10

Flag	Coverage Δ
unittests	`53.19% <ø> (-0.23%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...onstrainttemplate/constrainttemplate_controller.go	`54.78% <0.00%> (-5.03%)`	⬇️
pkg/gator/reader/read_constraints.go	`12.50% <0.00%> (ø)`
cmd/gator/test/test.go	`33.78% <0.00%> (ø)`
pkg/audit/manager.go	`1.07% <0.00%> (+1.07%)`	⬆️
pkg/readiness/list.go	`91.17% <0.00%> (+11.76%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Max Smythe <[email protected]>

ritazh · 2022-12-21T02:29:13Z

main.go

@@ -92,6 +93,7 @@ var (
 )

 var (
+	logFile              = flag.String("log-file", "", "Log to file, if specified. Default is to log to stderr.")


Is this mutual exclusive such that you can only choose either or between logFile and stderr? What if someone wants both?

Good question. If the logging frameworks support it, we could extend this to allow the flag to be specified multiple times.

So as it's currently implemented, this is mutually exclusive right?

ritazh · 2022-12-21T02:29:55Z

main.go

-		setupLog.Error(err, "problem running manager")
-		hadError = true
+blockingLoop:
+	for i := 0; i < 2; i++ {


What is this looping 2x?

Just a precautionary measure to make sure it's not an infinite loop. If both functions return we should exit (in practice we always return when Manager.Start() exits, but it guards against future whoopsies)

could we add your explanation in as a comment?

added an explanation.

ritazh · 2022-12-21T02:32:28Z

cmd/build/helmify/replacements.go

+
+	"- HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_LOGFILE": `
+        {{- if .Values.controllerManager.logFile}}
+        - --log-file={{ .Values.controllerManager.logFile }}


Do we want to add this new flag to https://github.com/open-policy-agent/gatekeeper/blob/master/Makefile#L65 so we can test it as part of the end to end?

TBH I'm a little wary of having too many permutations being tested end-to-end, for both test-run-time and being too e2e-test-heavy reasons. IMO this shouldn't change the overall behavior enough to warrant a new e2e tests.

If we tested every permutation of flags, we'd have probably millions of independent e2e tests.

I will say that I ran G8r with this flag manually and it works, though

As discussed in the 1/4/2023 community call, let's leave this out of e2e as the impact surface is low and optional.

acpana

just some questions, otherwise LGTM

acpana · 2022-12-28T22:05:37Z

main.go

@@ -123,11 +125,15 @@ func init() {
 }

 func main() {
+	os.Exit(innerMain())


is this refactor just so we don't call os.Exit() in a bunch of places?

Correct, os.Exit() breaks defer, so defer close() would not reliably be called without this.

acpana · 2022-12-28T22:17:56Z

main.go

+		defer handle.Close()
+		logStream = handle


if there is an err above, the handle will be nil right? won't this cause problems when attempting to close?

It would return ErrInvalid, which would be swallowed silently:

https://cs.opensource.google/go/go/+/refs/tags/go1.19.4:src/os/file_posix.go;l=23

Because an error implies nothing was opened, presumably there is nothing to close.

However, a failure to open a logfile should probably be a fatal error, so updating to exit immediately on error.

acpana · 2022-12-28T22:53:25Z

main.go

 	sink := zapcore.AddSync(os.Stderr)
+	if dest != nil {


is there a minimal unit test we could set up to prove that writing to file works?

In short, no.

Given that these are all library functions, which presumably have their own unit tests, there is no need to duplicate work that is more properly owned by zapcore

If we wanted to test, e2e would be the appropriate venue, per my answer to Rita's comment, however, I don't think this is a permutation that warrants another e2e run.

acpana · 2022-12-28T22:58:46Z

main.go

-		setupLog.Error(err, "problem running manager")
-		hadError = true
+blockingLoop:
+	for i := 0; i < 2; i++ {


could we add your explanation in as a comment?

Signed-off-by: Max Smythe <[email protected]>

ritazh

LGTM

maxsmythe requested review from sozercan and ritazh December 20, 2022 03:20

Clean up os.Exits

c94ecfc

Signed-off-by: Max Smythe <[email protected]>

maxsmythe changed the title ~~Allow writing logs to custom file~~ feat: Allow writing logs to custom file Dec 20, 2022

Switch to clearer threading model

0f565cc

Signed-off-by: Max Smythe <[email protected]>

ritazh reviewed Dec 21, 2022

View reviewed changes

acpana reviewed Dec 28, 2022

View reviewed changes

Address comments

185ecb1

Signed-off-by: Max Smythe <[email protected]>

maxsmythe requested a review from ritazh January 4, 2023 01:58

ritazh approved these changes Jan 5, 2023

View reviewed changes

maxsmythe added 2 commits January 6, 2023 17:27

Merge branch 'master' into log-file-flag

b9c2d6f

Merge branch 'master' into log-file-flag

1145055

maxsmythe merged commit bb11f3e into open-policy-agent:master Jan 10, 2023

p-linnane mentioned this pull request Apr 14, 2023

gator 3.12.0 Homebrew/homebrew-core#128418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow writing logs to custom file #2473

feat: Allow writing logs to custom file #2473

maxsmythe commented Dec 20, 2022

codecov-commenter commented Dec 20, 2022 •

edited

Loading

ritazh Dec 21, 2022

maxsmythe Dec 22, 2022

ritazh Jan 5, 2023 •

edited

Loading

maxsmythe Jan 5, 2023

ritazh Dec 21, 2022

maxsmythe Dec 22, 2022

acpana Dec 28, 2022

maxsmythe Jan 4, 2023

ritazh Dec 21, 2022 •

edited

Loading

maxsmythe Dec 22, 2022

maxsmythe Dec 22, 2022 •

edited

Loading

ritazh Jan 5, 2023

acpana left a comment

acpana Dec 28, 2022

maxsmythe Jan 4, 2023

acpana Dec 28, 2022

maxsmythe Jan 4, 2023 •

edited

Loading

acpana Dec 28, 2022

maxsmythe Jan 4, 2023

maxsmythe Jan 4, 2023

acpana Dec 28, 2022

ritazh left a comment

feat: Allow writing logs to custom file #2473

feat: Allow writing logs to custom file #2473

Conversation

maxsmythe commented Dec 20, 2022

codecov-commenter commented Dec 20, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh Jan 5, 2023 • 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

ritazh Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acpana left a 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

maxsmythe Jan 4, 2023 • 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

ritazh left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 20, 2022 •

edited

Loading

ritazh Jan 5, 2023 •

edited

Loading

ritazh Dec 21, 2022 •

edited

Loading

maxsmythe Dec 22, 2022 •

edited

Loading

maxsmythe Jan 4, 2023 •

edited

Loading