Merge pull request #102431 from serathius/loggingconfig

Use LoggingConfig within LogOptions

cmd/kubelet/app/options/options.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	cliflag "k8s.io/component-base/cli/flag"
+	"k8s.io/component-base/logs"
 	"k8s.io/kubelet/config/v1beta1"
 	kubeletapis "k8s.io/kubelet/pkg/apis"
 	"k8s.io/kubernetes/pkg/apis/core"
@@ -533,9 +534,7 @@ func AddKubeletConfigFlags(mainfs *pflag.FlagSet, c *kubeletconfig.KubeletConfig
 	fs.StringSliceVar(&c.EnforceNodeAllocatable, "enforce-node-allocatable", c.EnforceNodeAllocatable, "A comma separated list of levels of node allocatable enforcement to be enforced by kubelet. Acceptable options are 'none', 'pods', 'system-reserved', and 'kube-reserved'. If the latter two options are specified, '--system-reserved-cgroup' and '--kube-reserved-cgroup' must also be set, respectively. If 'none' is specified, no additional options should be set. See https://kubernetes.io/docs/tasks/administer-cluster/reserve-compute-resources/ for more details.")
 	fs.StringVar(&c.SystemReservedCgroup, "system-reserved-cgroup", c.SystemReservedCgroup, "Absolute name of the top level cgroup that is used to manage non-kubernetes components for which compute resources were reserved via '--system-reserved' flag. Ex. '/system-reserved'. [default='']")
 	fs.StringVar(&c.KubeReservedCgroup, "kube-reserved-cgroup", c.KubeReservedCgroup, "Absolute name of the top level cgroup that is used to manage kubernetes components for which compute resources were reserved via '--kube-reserved' flag. Ex. '/kube-reserved'. [default='']")
-	fs.StringVar(&c.Logging.Format, "logging-format", c.Logging.Format, "Sets the log format. Permitted formats: \"text\", \"json\". \nNon-default formats don't honor these flags: --add-dir-header, --alsologtostderr, --log-backtrace-at, --log-dir, --log-file, --log-file-max-size, --logtostderr, --skip-headers, --skip-log-headers, --stderrthreshold, --log-flush-frequency. \nNon-default choices are currently alpha and subject to change without warning.")
-	fs.BoolVar(&c.Logging.Sanitization, "experimental-logging-sanitization", c.Logging.Sanitization, `[Experimental] When enabled prevents logging of fields tagged as sensitive (passwords, keys, tokens).
-Runtime log sanitization may introduce significant computation overhead and therefore should not be enabled in production.`)
+	logs.BindLoggingFlags(&c.Logging, fs)
 
 	// Graduated experimental flags, kept for backward compatibility
 	fs.BoolVar(&c.KernelMemcgNotification, "experimental-kernel-memcg-notification", c.KernelMemcgNotification, "Use kernelMemcgNotification configuration, this flag will be removed in 1.23.")

cmd/kubelet/app/server.go

@@ -434,9 +434,7 @@ func UnsecuredDependencies(s *options.KubeletServer, featureGate featuregate.Fea
 // Otherwise, the caller is assumed to have set up the Dependencies object and a default one will
 // not be generated.
 func Run(ctx context.Context, s *options.KubeletServer, kubeDeps *kubelet.Dependencies, featureGate featuregate.FeatureGate) error {
-	logOption := logs.NewOptions()
-	logOption.LogFormat = s.Logging.Format
-	logOption.LogSanitization = s.Logging.Sanitization
+	logOption := &logs.Options{Config: s.Logging}
 	logOption.Apply()
 	// To help debugging, immediately log version
 	klog.InfoS("Kubelet version", "kubeletVersion", version.Get())

pkg/kubelet/apis/config/validation/validation.go

@@ -23,6 +23,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/component-base/logs"
 	"k8s.io/component-base/metrics"
@@ -200,11 +201,8 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error
 	}
 	allErrors = append(allErrors, metrics.ValidateShowHiddenMetricsVersion(kc.ShowHiddenMetricsForVersion)...)
 
-	logOption := logs.NewOptions()
-	if kc.Logging.Format != "" {
-		logOption.LogFormat = kc.Logging.Format
+	if errs := logs.ValidateLoggingConfiguration(&kc.Logging, field.NewPath("logging")); len(errs) > 0 {
+		allErrors = append(allErrors, errs.ToAggregate().Errors()...)
 	}
-	allErrors = append(allErrors, logOption.Validate()...)
-
 	return utilerrors.NewAggregate(allErrors)
 }

pkg/kubelet/apis/config/validation/validation_test.go

@@ -62,6 +62,9 @@ func TestValidateKubeletConfiguration(t *testing.T) {
 			"CustomCPUCFSQuotaPeriod": true,
 			"GracefulNodeShutdown":    true,
 		},
+		Logging: componentbaseconfig.LoggingConfiguration{
+			Format: "text",
+		},
 	}
 	if allErrors := ValidateKubeletConfiguration(successCase1); allErrors != nil {
 		t.Errorf("expect no errors, got %v", allErrors)
@@ -102,6 +105,9 @@ func TestValidateKubeletConfiguration(t *testing.T) {
 		FeatureGates: map[string]bool{
 			"CustomCPUCFSQuotaPeriod": true,
 		},
+		Logging: componentbaseconfig.LoggingConfiguration{
+			Format: "text",
+		},
 	}
 	if allErrors := ValidateKubeletConfiguration(successCase2); allErrors != nil {
 		t.Errorf("expect no errors, got %v", allErrors)
@@ -178,8 +184,11 @@ func TestValidateKubeletConfiguration(t *testing.T) {
 		CPUCFSQuotaPeriod:               metav1.Duration{Duration: 100 * time.Millisecond},
 		ShutdownGracePeriod:             metav1.Duration{Duration: 30 * time.Second},
 		ShutdownGracePeriodCriticalPods: metav1.Duration{Duration: 60 * time.Second},
+		Logging: componentbaseconfig.LoggingConfiguration{
+			Format: "",
+		},
 	}
-	const numErrsErrorCase1 = 28
+	const numErrsErrorCase1 = 29
 	if allErrors := ValidateKubeletConfiguration(errorCase1); len(allErrors.(utilerrors.Aggregate).Errors()) != numErrsErrorCase1 {
 		t.Errorf("expect %d errors, got %v", numErrsErrorCase1, len(allErrors.(utilerrors.Aggregate).Errors()))
 	}
@@ -220,6 +229,9 @@ func TestValidateKubeletConfiguration(t *testing.T) {
 			"CustomCPUCFSQuotaPeriod": true,
 			"GracefulNodeShutdown":    true,
 		},
+		Logging: componentbaseconfig.LoggingConfiguration{
+			Format: "text",
+		},
 	}
 	const numErrsErrorCase2 = 3
 	if allErrors := ValidateKubeletConfiguration(errorCase2); len(allErrors.(utilerrors.Aggregate).Errors()) != numErrsErrorCase2 {

staging/src/k8s.io/component-base/logs/config.go

@@ -24,6 +24,7 @@ import (
 	"github.com/spf13/pflag"
 	"k8s.io/klog/v2"
 
+	"k8s.io/component-base/config"
 	json "k8s.io/component-base/logs/json"
 )
 
@@ -49,19 +50,17 @@ var supportedLogsFlags = map[string]struct{}{
 }
 
 // BindLoggingFlags binds the Options struct fields to a flagset
-func BindLoggingFlags(o *Options, fs *pflag.FlagSet) {
+func BindLoggingFlags(c *config.LoggingConfiguration, fs *pflag.FlagSet) {
 	normalizeFunc := func(name string) string {
 		f := fs.GetNormalizeFunc()
 		return string(f(fs, name))
 	}
-
 	unsupportedFlags := fmt.Sprintf("--%s", strings.Join(UnsupportedLoggingFlags(normalizeFunc), ", --"))
 	formats := fmt.Sprintf(`"%s"`, strings.Join(LogRegistry.List(), `", "`))
-	fs.StringVar(&o.LogFormat, "logging-format", DefaultLogFormat, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags))
-
+	fs.StringVar(&c.Format, "logging-format", c.Format, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags))
 	// No new log formats should be added after generation is of flag options
 	LogRegistry.Freeze()
-	fs.BoolVar(&o.LogSanitization, "experimental-logging-sanitization", o.LogSanitization, `[Experimental] When enabled prevents logging of fields tagged as sensitive (passwords, keys, tokens).
+	fs.BoolVar(&c.Sanitization, "experimental-logging-sanitization", c.Sanitization, `[Experimental] When enabled prevents logging of fields tagged as sensitive (passwords, keys, tokens).
 Runtime log sanitization may introduce significant computation overhead and therefore should not be enabled in production.`)
 }
 

staging/src/k8s.io/component-base/logs/options.go

@@ -17,49 +17,50 @@ limitations under the License.
 package logs
 
 import (
-	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
 
 	"k8s.io/klog/v2"
 
+	"k8s.io/component-base/config"
+	"k8s.io/component-base/config/v1alpha1"
 	"k8s.io/component-base/logs/sanitization"
 )
 
 // Options has klog format parameters
 type Options struct {
-	LogFormat       string
-	LogSanitization bool
+	Config config.LoggingConfiguration
 }
 
 // NewOptions return new klog options
 func NewOptions() *Options {
-	return &Options{
-		LogFormat: DefaultLogFormat,
-	}
+	c := v1alpha1.LoggingConfiguration{}
+	v1alpha1.RecommendedLoggingConfiguration(&c)
+	o := &Options{}
+	v1alpha1.Convert_v1alpha1_LoggingConfiguration_To_config_LoggingConfiguration(&c, &o.Config, nil)
+	return o
 }
 
 // Validate verifies if any unsupported flag is set
 // for non-default logging format
 func (o *Options) Validate() []error {
-	return ValidateLoggingConfiguration(o)
+	errs := ValidateLoggingConfiguration(&o.Config, nil)
+	if len(errs) != 0 {
+		return errs.ToAggregate().Errors()
+	}
+	return nil
 }
 
 // AddFlags add logging-format flag
 func (o *Options) AddFlags(fs *pflag.FlagSet) {
-	BindLoggingFlags(o, fs)
+	BindLoggingFlags(&o.Config, fs)
 }
 
 // Apply set klog logger from LogFormat type
 func (o *Options) Apply() {
 	// if log format not exists, use nil loggr
-	loggr, _ := o.Get()
+	loggr, _ := LogRegistry.Get(o.Config.Format)
 	klog.SetLogger(loggr)
-	if o.LogSanitization {
+	if o.Config.Sanitization {
 		klog.SetLogFilter(&sanitization.SanitizingFilter{})
 	}
 }
-
-// Get logger with LogFormat field
-func (o *Options) Get() (logr.Logger, error) {
-	return LogRegistry.Get(o.LogFormat)
-}

staging/src/k8s.io/component-base/logs/options_test.go

@@ -18,11 +18,13 @@ package logs
 
 import (
 	"bytes"
-	"fmt"
 	"testing"
 
 	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/component-base/config"
 )
 
 func TestFlags(t *testing.T) {
@@ -48,7 +50,7 @@ func TestOptions(t *testing.T) {
 		name string
 		args []string
 		want *Options
-		errs []error
+		errs field.ErrorList
 	}{
 		{
 			name: "Default log format",
@@ -63,24 +65,35 @@ func TestOptions(t *testing.T) {
 			name: "JSON log format",
 			args: []string{"--logging-format=json"},
 			want: &Options{
-				LogFormat: JSONLogFormat,
+				Config: config.LoggingConfiguration{
+					Format: JSONLogFormat,
+				},
 			},
 		},
 		{
 			name: "log sanitization",
 			args: []string{"--experimental-logging-sanitization"},
 			want: &Options{
-				LogFormat:       DefaultLogFormat,
-				LogSanitization: true,
+				Config: config.LoggingConfiguration{
+					Format:       DefaultLogFormat,
+					Sanitization: true,
+				},
 			},
 		},
 		{
 			name: "Unsupported log format",
 			args: []string{"--logging-format=test"},
 			want: &Options{
-				LogFormat: "test",
+				Config: config.LoggingConfiguration{
+					Format: "test",
+				},
 			},
-			errs: []error{fmt.Errorf("unsupported log format: test")},
+			errs: field.ErrorList{&field.Error{
+				Type:     "FieldValueInvalid",
+				Field:    "format",
+				BadValue: "test",
+				Detail:   "Unsupported log format",
+			}},
 		},
 	}
 
@@ -95,7 +108,8 @@ func TestOptions(t *testing.T) {
 			}
 			errs := o.Validate()
 			if !assert.ElementsMatch(t, tc.errs, errs) {
-				t.Errorf("Wrong Validate() result for %q. expect %v, got %v", tc.name, tc.errs, errs)
+				t.Errorf("Wrong Validate() result for %q.\n expect:\t%+v\n got:\t%+v", tc.name, tc.errs, errs)
+
 			}
 		})
 	}

staging/src/k8s.io/component-base/logs/validate.go

@@ -22,20 +22,23 @@ import (
 	"strings"
 
 	"github.com/spf13/pflag"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/component-base/config"
 )
 
-func ValidateLoggingConfiguration(o *Options) []error {
-	errs := []error{}
-	if o.LogFormat != DefaultLogFormat {
+func ValidateLoggingConfiguration(c *config.LoggingConfiguration, fldPath *field.Path) field.ErrorList {
+	errs := field.ErrorList{}
+	if c.Format != DefaultLogFormat {
 		allFlags := UnsupportedLoggingFlags(hyphensToUnderscores)
 		for _, fname := range allFlags {
 			if flagIsSet(fname, hyphensToUnderscores) {
-				errs = append(errs, fmt.Errorf("non-default logging format doesn't honor flag: %s", fname))
+				errs = append(errs, field.Invalid(fldPath.Child("format"), c.Format, fmt.Sprintf("Non-default format doesn't honor flag: %s", fname)))
 			}
 		}
 	}
-	if _, err := o.Get(); err != nil {
-		errs = append(errs, fmt.Errorf("unsupported log format: %s", o.LogFormat))
+	if _, err := LogRegistry.Get(c.Format); err != nil {
+		errs = append(errs, field.Invalid(fldPath.Child("format"), c.Format, "Unsupported log format"))
 	}
 	return errs
 }