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.

Sign up for GitHub

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

[VC-35738] Use klog and logr logger instead of log in the agent package #609

Merged
merged 1 commit into from
Nov 12, 2024
Merged

[VC-35738] Use klog and logr logger instead of log in the agent package #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cmd

import (
"context"
"fmt"
"os"
"strings"
Expand Down Expand Up @@ -45,8 +46,9 @@ func init() {
// will be logged and the process will exit with status 1.
func Execute() {
logs.AddFlags(rootCmd.PersistentFlags())
ctx := klog.NewContext(context.Background(), klog.Background())
Copy link
Member Author

Choose a reason for hiding this comment

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

I was unsure whether calling klog.Background() here, before logs.Initialize() has been called in the PersistentPreRunE function (above); but it seems to work fine.
For example here is an example of the --logging-format=json flag having the desired effect:

$ ./preflight agent  --one-shot --output-path=/dev/null --api-token=foo --install-namespace=default --logging-format=json 2>&1 | jl
[2024-11-07 15:54:38] Starting [caller=agent/run.go:58 commit= logger=Run version=development]
[2024-11-07 15:54:38] Using the Jetstack Secure API Token auth mode since --api-token was specified. [caller=agent/config.go:395 logger=Run]
[2024-11-07 15:54:38] ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in Jetstack Secure API Token mode. [caller=agent/config.go:479 logger=Run]
[2024-11-07 15:54:38] Healthz endpoints enabled [addr=:8081 caller=agent/run.go:116 logger=Run.APIServer path=/healthz]
[2024-11-07 15:54:38] Readyz endpoints enabled [addr=:8081 caller=agent/run.go:120 logger=Run.APIServer path=/readyz]
[2024-11-07 15:54:38] Error messages will not show in the pod's events because the POD_NAME environment variable is empty [caller=agent/run.go:262 logger=Run]
[2024-11-07 15:54:38] Starting DataGatherer [caller=agent/run.go:179 logger=Run]
[2024-11-07 15:54:38] Starting DataGatherer [caller=agent/run.go:179 logger=Run]
[2024-11-07 15:54:38] Data saved to local file [caller=agent/run.go:315 outputPath=/dev/null]

var exitCode int
if err := rootCmd.Execute(); err != nil {
if err := rootCmd.ExecuteContext(ctx); err != nil {
exitCode = 1
klog.ErrorS(err, "Exiting due to error", "exit-code", exitCode)
}
Expand Down
40 changes: 22 additions & 18 deletions pkg/agent/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ package agent
import (
"fmt"
"io"
"log"
Copy link
Member Author

Choose a reason for hiding this comment

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

And to remove the use of the log module in this package.

"net/url"
"os"
"time"

"github.com/go-logr/logr"
"github.com/hashicorp/go-multierror"
"github.com/pkg/errors"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -355,32 +355,35 @@ type CombinedConfig struct {
// The error returned may be a multierror.Error. Use multierror.Prefix(err,
// "context:") rather than fmt.Errorf("context: %w", err) when wrapping the
// error.
func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags) (CombinedConfig, client.Client, error) {
func ValidateAndCombineConfig(log logr.Logger, cfg Config, flags AgentCmdFlags) (CombinedConfig, client.Client, error) {
res := CombinedConfig{}
var errs error

{
var mode AuthMode
var (
mode AuthMode
reason string
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided not to make these log messages structured, instead I just log the original message at Info level,
to avoid too many changes in the associated tests.
I moved the message to a reason var to make the diff a bit easier to read and makes it easier to turn these into structured logs in the future, e.g.

log.Info("Auth mode selected", "mode", mode, "reason", reason")

switch {
case flags.VenafiCloudMode && flags.CredentialsPath != "":
mode = VenafiCloudKeypair
log.Printf("Using the %s auth mode since --venafi-cloud and --credentials-path were specified.", mode)
reason = fmt.Sprintf("Using the %s auth mode since --venafi-cloud and --credentials-path were specified.", mode)
case flags.ClientID != "" && flags.PrivateKeyPath != "":
mode = VenafiCloudKeypair
log.Printf("Using the %s auth mode since --client-id and --private-key-path were specified.", mode)
reason = fmt.Sprintf("Using the %s auth mode since --client-id and --private-key-path were specified.", mode)
case flags.ClientID != "":
return CombinedConfig{}, nil, fmt.Errorf("if --client-id is specified, --private-key-path must also be specified")
case flags.PrivateKeyPath != "":
return CombinedConfig{}, nil, fmt.Errorf("--private-key-path is specified, --client-id must also be specified")
case flags.VenConnName != "":
mode = VenafiCloudVenafiConnection
log.Printf("Using the %s auth mode since --venafi-connection was specified.", mode)
reason = fmt.Sprintf("Using the %s auth mode since --venafi-connection was specified.", mode)
case flags.APIToken != "":
mode = JetstackSecureAPIToken
log.Printf("Using the %s auth mode since --api-token was specified.", mode)
reason = fmt.Sprintf("Using the %s auth mode since --api-token was specified.", mode)
case !flags.VenafiCloudMode && flags.CredentialsPath != "":
mode = JetstackSecureOAuth
log.Printf("Using the %s auth mode since --credentials-file was specified without --venafi-cloud.", mode)
reason = fmt.Sprintf("Using the %s auth mode since --credentials-file was specified without --venafi-cloud.", mode)
default:
return CombinedConfig{}, nil, fmt.Errorf("no auth mode specified. You can use one of four auth modes:\n" +
" - Use (--venafi-cloud with --credentials-file) or (--client-id with --private-key-path) to use the " + string(VenafiCloudKeypair) + " mode.\n" +
Expand All @@ -389,6 +392,7 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
" - Use --api-token if you want to use the " + string(JetstackSecureAPIToken) + " mode.\n")
}
res.AuthMode = mode
log.Info(reason)
}

// Validation and defaulting of `server` and the deprecated `endpoint.path`.
Expand All @@ -403,10 +407,10 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
case hasServerField && hasEndpointField:
// The `server` field takes precedence over the deprecated
// `endpoint` field.
log.Printf("The `server` and `endpoint` fields are both set in the config; using the `server` field.")
log.Info("The `server` and `endpoint` fields are both set in the config; using the `server` field.")
server = cfg.Server
case !hasServerField && hasEndpointField:
log.Printf("Using deprecated Endpoint configuration. User Server instead.")
log.Info("Using deprecated Endpoint configuration. User Server instead.")
if cfg.Endpoint.Protocol == "" && cfg.Server == "" {
cfg.Endpoint.Protocol = "http"
}
Expand All @@ -424,7 +428,7 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
errs = multierror.Append(errs, fmt.Errorf("server %q is not a valid URL", server))
}
if res.AuthMode == VenafiCloudVenafiConnection && server != "" {
log.Printf("ignoring the server field specified in the config file. In %s mode, this field is not needed.", VenafiCloudVenafiConnection)
log.Info(fmt.Sprintf("ignoring the server field specified in the config file. In %s mode, this field is not needed.", VenafiCloudVenafiConnection))
server = ""
}
res.Server = server
Expand Down Expand Up @@ -454,7 +458,7 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
// change this value with the new --venafi-connection flag, and this
// field is simply ignored.
if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploadPath != "" {
log.Printf(`ignoring the venafi-cloud.upload_path field in the config file. In %s mode, this field is not needed.`, res.AuthMode)
log.Info(fmt.Sprintf(`ignoring the venafi-cloud.upload_path field in the config file. In %s mode, this field is not needed.`, res.AuthMode))
}
uploadPath = ""
}
Expand All @@ -472,7 +476,7 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
// https://venafi.atlassian.net/browse/VC-35385 is done.
{
if cfg.VenafiCloud != nil && cfg.VenafiCloud.UploaderID != "" {
log.Printf(`ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in %s mode.`, res.AuthMode)
log.Info(fmt.Sprintf(`ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in %s mode.`, res.AuthMode))
}
}

Expand Down Expand Up @@ -524,13 +528,13 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
case flags.Period == 0 && cfg.Period == 0:
errs = multierror.Append(errs, fmt.Errorf("period must be set using --period or -p, or using the 'period' field in the config file"))
case flags.Period == 0 && cfg.Period > 0:
log.Printf("Using period from config %s", cfg.Period)
log.Info("Using period from config", "period", cfg.Period)
period = cfg.Period
case flags.Period > 0 && cfg.Period == 0:
period = flags.Period
case flags.Period > 0 && cfg.Period > 0:
// The flag takes precedence.
log.Printf("Both the 'period' field and --period are set. Using the value provided with --period.")
log.Info("Both the 'period' field and --period are set. Using the value provided with --period.")
period = flags.Period
}
res.Period = period
Expand Down Expand Up @@ -599,7 +603,7 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags)
// The error returned may be a multierror.Error. Use multierror.Prefix(err,
// "context:") rather than fmt.Errorf("context: %w", err) when wrapping the
// error.
func validateCredsAndCreateClient(log *log.Logger, flagCredentialsPath, flagClientID, flagPrivateKeyPath, flagAPIToken string, cfg CombinedConfig) (client.Client, error) {
func validateCredsAndCreateClient(log logr.Logger, flagCredentialsPath, flagClientID, flagPrivateKeyPath, flagAPIToken string, cfg CombinedConfig) (client.Client, error) {
var errs error

var preflightClient client.Client
Expand Down Expand Up @@ -719,7 +723,7 @@ func ValidateDataGatherers(dataGatherers []DataGatherer) error {

// The error returned may be a multierror.Error. Instead of adding context to
// the error with fmt.Errorf("%w", err), use multierror.Prefix(err, "context").
func createCredentialClient(log *log.Logger, credentials client.Credentials, cfg CombinedConfig, agentMetadata *api.AgentMetadata) (client.Client, error) {
func createCredentialClient(log logr.Logger, credentials client.Credentials, cfg CombinedConfig, agentMetadata *api.AgentMetadata) (client.Client, error) {
switch creds := credentials.(type) {
case *client.VenafiSvcAccountCredentials:
// The uploader ID isn't actually used in the backend, let's use an
Expand All @@ -730,7 +734,7 @@ func createCredentialClient(log *log.Logger, credentials client.Credentials, cfg
if cfg.AuthMode == VenafiCloudKeypair {
// We don't do this for the VenafiCloudVenafiConnection mode because
// the upload_path field is ignored in that mode.
log.Println("Loading upload_path from \"venafi-cloud\" configuration.")
log.Info("Loading upload_path from \"venafi-cloud\" configuration.")
uploadPath = cfg.UploadPath
}
return client.NewVenafiCloudClient(agentMetadata, creds, cfg.Server, uploaderID, uploadPath, cfg.DisableCompression)
Expand Down
33 changes: 18 additions & 15 deletions pkg/agent/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ import (
"context"
"fmt"
"io"
"log"
"net/http"
"os"
"testing"
"time"

"github.com/go-logr/logr"
"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/klog/v2/ktesting"

"github.com/jetstack/preflight/pkg/client"
"github.com/jetstack/preflight/pkg/testutil"
Expand Down Expand Up @@ -86,7 +87,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) {

t.Run("--period flag takes precedence over period field in config, shows warning", func(t *testing.T) {
t.Setenv("POD_NAMESPACE", "venafi")
log, gotLogs := recordLogs()
log, gotLogs := recordLogs(t)
got, _, err := ValidateAndCombineConfig(log,
withConfig(testutil.Undent(`
server: https://api.venafi.eu
Expand All @@ -97,8 +98,8 @@ func Test_ValidateAndCombineConfig(t *testing.T) {
withCmdLineFlags("--period", "99m", "--credentials-file", fakeCredsPath))
require.NoError(t, err)
assert.Equal(t, testutil.Undent(`
Using the Jetstack Secure OAuth auth mode since --credentials-file was specified without --venafi-cloud.
Both the 'period' field and --period are set. Using the value provided with --period.
INFO Using the Jetstack Secure OAuth auth mode since --credentials-file was specified without --venafi-cloud.
INFO Both the 'period' field and --period are set. Using the value provided with --period.
Comment on lines +101 to +102
Copy link
Member

@maelvls maelvls Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this output not matching the klog text format and uses a custom format? I'd prefer reviewing the logs as seen by the users

Copy link
Member Author

Choose a reason for hiding this comment

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

Because these tests are using the ktesting test logger which has a buffer containing simplified output format for easier testing and matching.
The log format of the preflight command is tested elsewhere.
I'll leave this as-is.

`), gotLogs.String())
assert.Equal(t, 99*time.Minute, got.Period)
})
Expand Down Expand Up @@ -573,7 +574,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) {
t.Run("venafi-cloud-workload-identity-auth: warning about server, venafi-cloud.uploader_id, and venafi-cloud.upload_path being skipped", func(t *testing.T) {
t.Setenv("POD_NAMESPACE", "venafi")
t.Setenv("KUBECONFIG", withFile(t, fakeKubeconfig))
log, gotLogs := recordLogs()
log, gotLogs := recordLogs(t)
got, gotCl, err := ValidateAndCombineConfig(log,
withConfig(testutil.Undent(`
server: https://api.venafi.eu
Expand All @@ -587,11 +588,11 @@ func Test_ValidateAndCombineConfig(t *testing.T) {
)
require.NoError(t, err)
assert.Equal(t, testutil.Undent(`
Using the Venafi Cloud VenafiConnection auth mode since --venafi-connection was specified.
ignoring the server field specified in the config file. In Venafi Cloud VenafiConnection mode, this field is not needed.
ignoring the venafi-cloud.upload_path field in the config file. In Venafi Cloud VenafiConnection mode, this field is not needed.
ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in Venafi Cloud VenafiConnection mode.
Using period from config 1h0m0s
INFO Using the Venafi Cloud VenafiConnection auth mode since --venafi-connection was specified.
INFO ignoring the server field specified in the config file. In Venafi Cloud VenafiConnection mode, this field is not needed.
INFO ignoring the venafi-cloud.upload_path field in the config file. In Venafi Cloud VenafiConnection mode, this field is not needed.
INFO ignoring the venafi-cloud.uploader_id field in the config file. This field is not needed in Venafi Cloud VenafiConnection mode.
INFO Using period from config period="1h0m0s"
`), gotLogs.String())
assert.Equal(t, VenafiCloudVenafiConnection, got.AuthMode)
assert.IsType(t, &client.VenConnClient{}, gotCl)
Expand Down Expand Up @@ -994,13 +995,15 @@ func withFile(t testing.TB, content string) string {
return f.Name()
}

func recordLogs() (*log.Logger, *bytes.Buffer) {
b := bytes.Buffer{}
return log.New(&b, "", 0), &b
func recordLogs(t *testing.T) (logr.Logger, ktesting.Buffer) {
log := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true)))
testingLogger, ok := log.GetSink().(ktesting.Underlier)
require.True(t, ok)
return log, testingLogger.GetBuffer()
}

func discardLogs() *log.Logger {
return log.New(io.Discard, "", 0)
func discardLogs() logr.Logger {
return logr.Discard()
}

// Shortcut for ParseConfig.
Expand Down
Loading