From ea3f25f49b4db5203c4f9cebcb2cdeee5a75085d Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 18 Jan 2022 16:50:55 +0100 Subject: [PATCH] logs: add alpha+beta feature gates It is useful to have the ability to control whether alpha or beta features are enabled. We can group features under LoggingAlphaOptions and LoggingBetaOptions because the configuration is designed so that each feature individually must be enabled via its own option. Currently, the JSON format itself is beta (graduated in 1.23) but additional options for it were only added in 1.23 and thus are still alpha: $ go run ./staging/src/k8s.io/component-base/logs/example/cmd/logger.go --logging-format=json --log-json-split-stream --log-json-info-buffer-size 1M --feature-gates LoggingBetaOptions=false [format: Forbidden: Log format json is BETA and disabled, see LoggingBetaOptions feature, options.json.splitStream: Forbidden: Feature LoggingAlphaOptions is disabled, options.json.infoBufferSize: Forbidden: Feature LoggingAlphaOptions is disabled] $ go run ./staging/src/k8s.io/component-base/logs/example/cmd/logger.go --logging-format=json --log-json-split-stream --log-json-info-buffer-size 1M [options.json.splitStream: Forbidden: Feature LoggingAlphaOptions is disabled, options.json.infoBufferSize: Forbidden: Feature LoggingAlphaOptions is disabled] This is the same approach that was taken for CPUManagerPolicyAlphaOptions and CPUManagerPolicyBetaOptions. In order to test this without modifying the global feature gate in a test file, ValidateKubeletConfiguration must take a feature gate as argument. --- cmd/kubelet/app/options/options.go | 2 +- cmd/kubelet/app/server.go | 2 +- .../apis/config/validation/validation.go | 8 +-- .../apis/config/validation/validation_test.go | 6 +- .../logs/api/v1/features_test.go | 47 ++++++++++++++ .../logs/api/v1/kube_features.go | 25 +++++++- .../component-base/logs/api/v1/options.go | 52 +++++++++++++--- .../logs/api/v1/options_test.go | 1 - .../component-base/logs/api/v1/registry.go | 49 ++++++++++----- .../component-base/logs/api/v1/types.go | 17 ++++-- .../logs/api/v1/validate_test.go | 35 ++++++++++- .../k8s.io/component-base/logs/json/json.go | 5 ++ .../logs/json/register/register.go | 2 +- .../logs/json/register/register_test.go | 61 +++++++++++++++++-- 14 files changed, 268 insertions(+), 44 deletions(-) create mode 100644 staging/src/k8s.io/component-base/logs/api/v1/features_test.go diff --git a/cmd/kubelet/app/options/options.go b/cmd/kubelet/app/options/options.go index 12a16bdc340..6da44a38789 100644 --- a/cmd/kubelet/app/options/options.go +++ b/cmd/kubelet/app/options/options.go @@ -248,7 +248,7 @@ func NewKubeletServer() (*KubeletServer, error) { // ValidateKubeletServer validates configuration of KubeletServer and returns an error if the input configuration is invalid. func ValidateKubeletServer(s *KubeletServer) error { // please add any KubeletConfiguration validation to the kubeletconfigvalidation.ValidateKubeletConfiguration function - if err := kubeletconfigvalidation.ValidateKubeletConfiguration(&s.KubeletConfiguration); err != nil { + if err := kubeletconfigvalidation.ValidateKubeletConfiguration(&s.KubeletConfiguration, utilfeature.DefaultFeatureGate); err != nil { return err } if err := ValidateKubeletFlags(&s.KubeletFlags); err != nil { diff --git a/cmd/kubelet/app/server.go b/cmd/kubelet/app/server.go index 8aaaeb50c77..06704135881 100644 --- a/cmd/kubelet/app/server.go +++ b/cmd/kubelet/app/server.go @@ -222,7 +222,7 @@ HTTP server: The kubelet can also listen for HTTP and respond to a simple API // We always validate the local configuration (command line + config file). // This is the default "last-known-good" config for dynamic config, and must always remain valid. - if err := kubeletconfigvalidation.ValidateKubeletConfiguration(kubeletConfig); err != nil { + if err := kubeletconfigvalidation.ValidateKubeletConfiguration(kubeletConfig, utilfeature.DefaultFeatureGate); err != nil { return fmt.Errorf("failed to validate kubelet configuration, error: %w, path: %s", err, kubeletConfig) } diff --git a/pkg/kubelet/apis/config/validation/validation.go b/pkg/kubelet/apis/config/validation/validation.go index 08425d25486..7b88556b11a 100644 --- a/pkg/kubelet/apis/config/validation/validation.go +++ b/pkg/kubelet/apis/config/validation/validation.go @@ -24,7 +24,7 @@ import ( 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/featuregate" "k8s.io/component-base/metrics" "k8s.io/kubernetes/pkg/features" kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config" @@ -38,12 +38,12 @@ var ( ) // ValidateKubeletConfiguration validates `kc` and returns an error if it is invalid -func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error { +func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration, featureGate featuregate.FeatureGate) error { allErrors := []error{} - // Make a local copy of the global feature gates and combine it with the gates set by this configuration. + // Make a local copy of the feature gates and combine it with the gates set by this configuration. // This allows us to validate the config against the set of gates it will actually run against. - localFeatureGate := utilfeature.DefaultFeatureGate.DeepCopy() + localFeatureGate := featureGate.DeepCopy() if err := localFeatureGate.SetFromMap(kc.FeatureGates); err != nil { return err } diff --git a/pkg/kubelet/apis/config/validation/validation_test.go b/pkg/kubelet/apis/config/validation/validation_test.go index 08057cf4458..26c517d1442 100644 --- a/pkg/kubelet/apis/config/validation/validation_test.go +++ b/pkg/kubelet/apis/config/validation/validation_test.go @@ -23,6 +23,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" logsapi "k8s.io/component-base/logs/api/v1" kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config" "k8s.io/kubernetes/pkg/kubelet/apis/config/validation" @@ -75,6 +76,9 @@ var ( ) func TestValidateKubeletConfiguration(t *testing.T) { + featureGate := utilfeature.DefaultFeatureGate.DeepCopy() + logsapi.AddFeatureGates(featureGate) + cases := []struct { name string configure func(config *kubeletconfig.KubeletConfiguration) *kubeletconfig.KubeletConfiguration @@ -497,7 +501,7 @@ func TestValidateKubeletConfiguration(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - errs := validation.ValidateKubeletConfiguration(tc.configure(successConfig.DeepCopy())) + errs := validation.ValidateKubeletConfiguration(tc.configure(successConfig.DeepCopy()), featureGate) if len(tc.errMsg) == 0 { if errs != nil { diff --git a/staging/src/k8s.io/component-base/logs/api/v1/features_test.go b/staging/src/k8s.io/component-base/logs/api/v1/features_test.go new file mode 100644 index 00000000000..ec968c78721 --- /dev/null +++ b/staging/src/k8s.io/component-base/logs/api/v1/features_test.go @@ -0,0 +1,47 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + "k8s.io/component-base/featuregate" +) + +var ( + // pre-defined feature gates with the features from this package in a + // certain state (default, all enabled, all disabled). + defaultFeatureGate, enabledFeatureGate, disabledFeatureGate featuregate.FeatureGate +) + +func init() { + mutable := featuregate.NewFeatureGate() + if err := AddFeatureGates(mutable); err != nil { + panic(err) + } + defaultFeatureGate = mutable + enabled := mutable.DeepCopy() + disabled := mutable.DeepCopy() + for feature := range mutable.GetAll() { + if err := enabled.SetFromMap(map[string]bool{string(feature): true}); err != nil { + panic(err) + } + if err := disabled.SetFromMap(map[string]bool{string(feature): false}); err != nil { + panic(err) + } + } + enabledFeatureGate = enabled + disabledFeatureGate = disabled +} diff --git a/staging/src/k8s.io/component-base/logs/api/v1/kube_features.go b/staging/src/k8s.io/component-base/logs/api/v1/kube_features.go index e9feaff9cd6..19a95b3b526 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/kube_features.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/kube_features.go @@ -1,5 +1,5 @@ /* -Copyright 2021 The Kubernetes Authors. +Copyright 2022 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -33,11 +33,34 @@ const ( // contextualLoggingDefault must remain false while in alpha. It can // become true in beta. contextualLoggingDefault = false + + // Allow fine-tuning of experimental, alpha-quality logging options. + // + // Per https://groups.google.com/g/kubernetes-sig-architecture/c/Nxsc7pfe5rw/m/vF2djJh0BAAJ + // we want to avoid a proliferation of feature gates. This feature gate: + // - will guard *a group* of logging options whose quality level is alpha. + // - will never graduate to beta or stable. + LoggingAlphaOptions featuregate.Feature = "LoggingAlphaOptions" + + // Allow fine-tuning of experimental, beta-quality logging options. + // + // Per https://groups.google.com/g/kubernetes-sig-architecture/c/Nxsc7pfe5rw/m/vF2djJh0BAAJ + // we want to avoid a proliferation of feature gates. This feature gate: + // - will guard *a group* of logging options whose quality level is beta. + // - is thus *introduced* as beta + // - will never graduate to stable. + LoggingBetaOptions featuregate.Feature = "LoggingBetaOptions" + + // Stable logging options. Always enabled. + LoggingStableOptions featuregate.Feature = "LoggingStableOptions" ) func featureGates() map[featuregate.Feature]featuregate.FeatureSpec { return map[featuregate.Feature]featuregate.FeatureSpec{ ContextualLogging: {Default: contextualLoggingDefault, PreRelease: featuregate.Alpha}, + + LoggingAlphaOptions: {Default: false, PreRelease: featuregate.Alpha}, + LoggingBetaOptions: {Default: true, PreRelease: featuregate.Beta}, } } diff --git a/staging/src/k8s.io/component-base/logs/api/v1/options.go b/staging/src/k8s.io/component-base/logs/api/v1/options.go index 02b9cc4bc04..84cbdf97444 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/options.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/options.go @@ -92,8 +92,19 @@ func (c *LoggingConfiguration) Validate(featureGate featuregate.FeatureGate, fld } } } - if _, err := logRegistry.get(c.Format); err != nil { + format, err := logRegistry.get(c.Format) + if err != nil { errs = append(errs, field.Invalid(fldPath.Child("format"), c.Format, "Unsupported log format")) + } else if format != nil { + if format.feature != LoggingStableOptions { + enabled := featureGates()[format.feature].Default + if featureGate != nil { + enabled = featureGate.Enabled(format.feature) + } + if !enabled { + errs = append(errs, field.Forbidden(fldPath.Child("format"), fmt.Sprintf("Log format %s is disabled, see %s feature", c.Format, format.feature))) + } + } } // The type in our struct is uint32, but klog only accepts positive int32. @@ -116,10 +127,35 @@ func (c *LoggingConfiguration) Validate(featureGate featuregate.FeatureGate, fld } } - // Currently nothing to validate for c.Options. + errs = append(errs, c.validateFormatOptions(featureGate, fldPath.Child("options"))...) return errs } +func (c *LoggingConfiguration) validateFormatOptions(featureGate featuregate.FeatureGate, fldPath *field.Path) field.ErrorList { + errs := field.ErrorList{} + errs = append(errs, c.validateJSONOptions(featureGate, fldPath.Child("json"))...) + return errs +} + +func (c *LoggingConfiguration) validateJSONOptions(featureGate featuregate.FeatureGate, fldPath *field.Path) field.ErrorList { + errs := field.ErrorList{} + if gate := LoggingAlphaOptions; c.Options.JSON.SplitStream && !featureEnabled(featureGate, gate) { + errs = append(errs, field.Forbidden(fldPath.Child("splitStream"), fmt.Sprintf("Feature %s is disabled", gate))) + } + if gate := LoggingAlphaOptions; c.Options.JSON.InfoBufferSize.Value() != 0 && !featureEnabled(featureGate, gate) { + errs = append(errs, field.Forbidden(fldPath.Child("infoBufferSize"), fmt.Sprintf("Feature %s is disabled", gate))) + } + return errs +} + +func featureEnabled(featureGate featuregate.FeatureGate, feature featuregate.Feature) bool { + enabled := false + if featureGate != nil { + enabled = featureGate.Enabled(feature) + } + return enabled +} + func (c *LoggingConfiguration) apply(featureGate featuregate.FeatureGate) error { contextualLoggingEnabled := contextualLoggingDefault if featureGate != nil { @@ -127,11 +163,11 @@ func (c *LoggingConfiguration) apply(featureGate featuregate.FeatureGate) error } // if log format not exists, use nil loggr - factory, _ := logRegistry.get(c.Format) - if factory == nil { + format, _ := logRegistry.get(c.Format) + if format.factory == nil { klog.ClearLogger() } else { - log, flush := factory.Create(*c) + log, flush := format.factory.Create(*c) klog.SetLoggerWithOptions(log, klog.ContextualLogger(contextualLoggingEnabled), klog.FlushLogger(flush)) } if err := loggingFlags.Lookup("v").Value.Set(VerbosityLevelPflag(&c.Verbosity).String()); err != nil { @@ -151,7 +187,7 @@ func (c *LoggingConfiguration) AddFlags(fs *pflag.FlagSet) { // hyphens, even if currently no normalization function is set for the // flag set yet. unsupportedFlags := strings.Join(unsupportedLoggingFlagNames(cliflag.WordSepNormalizeFunc), ", ") - formats := fmt.Sprintf(`"%s"`, strings.Join(logRegistry.list(), `", "`)) + formats := logRegistry.list() 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() @@ -163,8 +199,8 @@ func (c *LoggingConfiguration) AddFlags(fs *pflag.FlagSet) { // JSON options. We only register them if "json" is a valid format. The // config file API however always has them. if _, err := logRegistry.get("json"); err == nil { - fs.BoolVar(&c.Options.JSON.SplitStream, "log-json-split-stream", false, "[Experimental] In JSON format, write error messages to stderr and info messages to stdout. The default is to write a single stream to stdout.") - fs.Var(&c.Options.JSON.InfoBufferSize, "log-json-info-buffer-size", "[Experimental] In JSON format with split output streams, the info messages can be buffered for a while to increase performance. The default value of zero bytes disables buffering. The size can be specified as number of bytes (512), multiples of 1000 (1K), multiples of 1024 (2Ki), or powers of those (3M, 4G, 5Mi, 6Gi).") + fs.BoolVar(&c.Options.JSON.SplitStream, "log-json-split-stream", false, "[Alpha] In JSON format, write error messages to stderr and info messages to stdout. The default is to write a single stream to stdout. Enable the LoggingAlphaOptions feature gate to use this.") + fs.Var(&c.Options.JSON.InfoBufferSize, "log-json-info-buffer-size", "[Alpha] In JSON format with split output streams, the info messages can be buffered for a while to increase performance. The default value of zero bytes disables buffering. The size can be specified as number of bytes (512), multiples of 1000 (1K), multiples of 1024 (2Ki), or powers of those (3M, 4G, 5Mi, 6Gi). Enable the LoggingAlphaOptions feature gate to use this.") } } diff --git a/staging/src/k8s.io/component-base/logs/api/v1/options_test.go b/staging/src/k8s.io/component-base/logs/api/v1/options_test.go index 2c2bbf15e2a..40f334df902 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/options_test.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/options_test.go @@ -55,7 +55,6 @@ func TestOptions(t *testing.T) { testcases := []struct { name string args []string - path *field.Path want *LoggingConfiguration errs field.ErrorList }{ diff --git a/staging/src/k8s.io/component-base/logs/api/v1/registry.go b/staging/src/k8s.io/component-base/logs/api/v1/registry.go index 14995035092..78bc8f8853f 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/registry.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/registry.go @@ -19,18 +19,26 @@ package v1 import ( "fmt" "sort" + "strings" "github.com/go-logr/logr" + + "k8s.io/component-base/featuregate" ) var logRegistry = newLogFormatRegistry() // logFormatRegistry stores factories for all supported logging formats. type logFormatRegistry struct { - registry map[string]LogFormatFactory + registry map[string]logFormat frozen bool } +type logFormat struct { + factory LogFormatFactory + feature featuregate.Feature +} + // LogFormatFactory provides support for a certain additional, // non-default log format. type LogFormatFactory interface { @@ -42,49 +50,58 @@ type LogFormatFactory interface { } // RegisterLogFormat registers support for a new logging format. This must be called -// before using any of the methods in LoggingConfiguration. -func RegisterLogFormat(name string, factory LogFormatFactory) error { - return logRegistry.register(name, factory) +// before using any of the methods in LoggingConfiguration. The feature must +// be one of those defined in this package (typically LoggingAlphaOptions, +// LoggingBetaOptions or LoggingStableOptions). +func RegisterLogFormat(name string, factory LogFormatFactory, feature featuregate.Feature) error { + return logRegistry.register(name, logFormat{factory, feature}) } func newLogFormatRegistry() *logFormatRegistry { registry := &logFormatRegistry{ - registry: make(map[string]LogFormatFactory), + registry: make(map[string]logFormat), frozen: false, } - registry.register("text", nil) + registry.register("text", logFormat{feature: LoggingStableOptions}) return registry } // register adds a new log format. It's an error to modify an existing one. -func (lfr *logFormatRegistry) register(name string, factory LogFormatFactory) error { +func (lfr *logFormatRegistry) register(name string, format logFormat) error { if lfr.frozen { return fmt.Errorf("log format registry is frozen, unable to register log format %s", name) } if _, ok := lfr.registry[name]; ok { return fmt.Errorf("log format: %s already exists", name) } - lfr.registry[name] = factory + if _, ok := featureGates()[format.feature]; !ok && format.feature != LoggingStableOptions { + return fmt.Errorf("log format %s: unsupported feature gate %s", name, format.feature) + } + lfr.registry[name] = format return nil } // get specified log format factory -func (lfr *logFormatRegistry) get(name string) (LogFormatFactory, error) { - re, ok := lfr.registry[name] +func (lfr *logFormatRegistry) get(name string) (*logFormat, error) { + format, ok := lfr.registry[name] if !ok { return nil, fmt.Errorf("log format: %s does not exists", name) } - return re, nil + return &format, nil } -// list names of registered log formats (sorted) -func (lfr *logFormatRegistry) list() []string { +// list names of registered log formats, including feature gates (sorted) +func (lfr *logFormatRegistry) list() string { formats := make([]string, 0, len(lfr.registry)) - for f := range lfr.registry { - formats = append(formats, f) + for name, format := range lfr.registry { + item := fmt.Sprintf(`"%s"`, name) + if format.feature != LoggingStableOptions { + item += fmt.Sprintf(" (gated by %s)", format.feature) + } + formats = append(formats, item) } sort.Strings(formats) - return formats + return strings.Join(formats, ", ") } // freeze prevents further modifications of the registered log formats. diff --git a/staging/src/k8s.io/component-base/logs/api/v1/types.go b/staging/src/k8s.io/component-base/logs/api/v1/types.go index 5f953932424..d1bf313643b 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/types.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/types.go @@ -31,6 +31,9 @@ const ( JSONLogFormat = "json" ) +// The alpha or beta level of structs is the highest stability level of any field +// inside it. Feature gates will get checked during LoggingConfiguration.ValidateAndApply. + // LoggingConfiguration contains logging options. type LoggingConfiguration struct { // Format Flag specifies the structure of log messages. @@ -48,26 +51,30 @@ type LoggingConfiguration struct { // VModule overrides the verbosity threshold for individual files. // Only supported for "text" log format. VModule VModuleConfiguration `json:"vmodule,omitempty"` - // [Experimental] Options holds additional parameters that are specific + // [Alpha] Options holds additional parameters that are specific // to the different logging formats. Only the options for the selected // format get used, but all of them get validated. + // Only available when the LoggingAlphaOptions feature gate is enabled. Options FormatOptions `json:"options,omitempty"` } // FormatOptions contains options for the different logging formats. type FormatOptions struct { - // [Experimental] JSON contains options for logging format "json". + // [Alpha] JSON contains options for logging format "json". + // Only available when the LoggingAlphaOptions feature gate is enabled. JSON JSONOptions `json:"json,omitempty"` } // JSONOptions contains options for logging format "json". type JSONOptions struct { - // [Experimental] SplitStream redirects error messages to stderr while + // [Alpha] SplitStream redirects error messages to stderr while // info messages go to stdout, with buffering. The default is to write - // both to stdout, without buffering. + // both to stdout, without buffering. Only available when + // the LoggingAlphaOptions feature gate is enabled. SplitStream bool `json:"splitStream,omitempty"` - // [Experimental] InfoBufferSize sets the size of the info stream when + // [Alpha] InfoBufferSize sets the size of the info stream when // using split streams. The default is zero, which disables buffering. + // Only available when the LoggingAlphaOptions feature gate is enabled. InfoBufferSize resource.QuantityValue `json:"infoBufferSize,omitempty"` } diff --git a/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go b/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go index 6cacb4e6d18..99c7f657cad 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go @@ -21,13 +21,28 @@ import ( "testing" "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/component-base/featuregate" ) func TestValidation(t *testing.T) { + jsonOptionsEnabled := LoggingConfiguration{ + Format: "text", + Options: FormatOptions{ + JSON: JSONOptions{ + SplitStream: true, + InfoBufferSize: resource.QuantityValue{ + Quantity: *resource.NewQuantity(1024, resource.DecimalSI), + }, + }, + }, + } + jsonErrors := `[options.json.splitStream: Forbidden: Feature LoggingAlphaOptions is disabled, options.json.infoBufferSize: Forbidden: Feature LoggingAlphaOptions is disabled]` testcases := map[string]struct { config LoggingConfiguration path *field.Path + featureGate featuregate.FeatureGate expectErrors string }{ "okay": { @@ -114,11 +129,29 @@ func TestValidation(t *testing.T) { }, expectErrors: `[format: Invalid value: "json": Unsupported log format, vmodule: Forbidden: Only supported for text log format]`, }, + "JSON used, default gates": { + config: jsonOptionsEnabled, + featureGate: defaultFeatureGate, + expectErrors: jsonErrors, + }, + "JSON used, disabled gates": { + config: jsonOptionsEnabled, + featureGate: disabledFeatureGate, + expectErrors: jsonErrors, + }, + "JSON used, enabled gates": { + config: jsonOptionsEnabled, + featureGate: enabledFeatureGate, + }, } for name, test := range testcases { t.Run(name, func(t *testing.T) { - err := test.config.Validate(nil, test.path) + featureGate := test.featureGate + if featureGate == nil { + featureGate = defaultFeatureGate + } + err := test.config.Validate(featureGate, test.path) if len(err) == 0 { if test.expectErrors != "" { t.Fatalf("did not get expected error(s): %s", test.expectErrors) diff --git a/staging/src/k8s.io/component-base/logs/json/json.go b/staging/src/k8s.io/component-base/logs/json/json.go index 2876c602cda..1ccd0f3de64 100644 --- a/staging/src/k8s.io/component-base/logs/json/json.go +++ b/staging/src/k8s.io/component-base/logs/json/json.go @@ -26,6 +26,7 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" + "k8s.io/component-base/featuregate" logsapi "k8s.io/component-base/logs/api/v1" ) @@ -86,6 +87,10 @@ type Factory struct{} var _ logsapi.LogFormatFactory = Factory{} +func (f Factory) Feature() featuregate.Feature { + return logsapi.LoggingBetaOptions +} + func (f Factory) Create(c logsapi.LoggingConfiguration) (logr.Logger, func()) { // We intentionally avoid all os.File.Sync calls. Output is unbuffered, // therefore we don't need to flush, and calling the underlying fsync diff --git a/staging/src/k8s.io/component-base/logs/json/register/register.go b/staging/src/k8s.io/component-base/logs/json/register/register.go index 06dea3d34b7..bf76425a2af 100644 --- a/staging/src/k8s.io/component-base/logs/json/register/register.go +++ b/staging/src/k8s.io/component-base/logs/json/register/register.go @@ -23,7 +23,7 @@ import ( func init() { // JSON format is optional klog format - if err := logsapi.RegisterLogFormat(logsapi.JSONLogFormat, json.Factory{}); err != nil { + if err := logsapi.RegisterLogFormat(logsapi.JSONLogFormat, json.Factory{}, logsapi.LoggingBetaOptions); err != nil { panic(err) } } diff --git a/staging/src/k8s.io/component-base/logs/json/register/register_test.go b/staging/src/k8s.io/component-base/logs/json/register/register_test.go index dae64494fdc..0b5398ae4a0 100644 --- a/staging/src/k8s.io/component-base/logs/json/register/register_test.go +++ b/staging/src/k8s.io/component-base/logs/json/register/register_test.go @@ -37,7 +37,7 @@ func TestJSONFlag(t *testing.T) { c.AddFlags(fs) fs.SetOutput(&output) fs.PrintDefaults() - wantSubstring := `Permitted formats: "json", "text".` + wantSubstring := `Permitted formats: "json" (gated by LoggingBetaOptions), "text".` if !assert.Contains(t, output.String(), wantSubstring) { t.Errorf("JSON logging format flag is not available. expect to contain %q, got %q", wantSubstring, output.String()) } @@ -46,13 +46,62 @@ func TestJSONFlag(t *testing.T) { func TestJSONFormatRegister(t *testing.T) { config := logsapi.NewLoggingConfiguration() klogr := klog.Background() + defaultGate := featuregate.NewFeatureGate() + err := logsapi.AddFeatureGates(defaultGate) + require.NoError(t, err) + allEnabled := defaultGate.DeepCopy() + allDisabled := defaultGate.DeepCopy() + for feature := range defaultGate.GetAll() { + if err := allEnabled.SetFromMap(map[string]bool{string(feature): true}); err != nil { + panic(err) + } + if err := allDisabled.SetFromMap(map[string]bool{string(feature): false}); err != nil { + panic(err) + } + } testcases := []struct { name string args []string contextualLogging bool + featureGate featuregate.FeatureGate want *logsapi.LoggingConfiguration errs field.ErrorList }{ + { + name: "JSON log format, default gates", + args: []string{"--logging-format=json"}, + want: func() *logsapi.LoggingConfiguration { + c := config.DeepCopy() + c.Format = logsapi.JSONLogFormat + return c + }(), + }, + { + name: "JSON log format, disabled gates", + args: []string{"--logging-format=json"}, + featureGate: allDisabled, + want: func() *logsapi.LoggingConfiguration { + c := config.DeepCopy() + c.Format = logsapi.JSONLogFormat + return c + }(), + errs: field.ErrorList{&field.Error{ + Type: "FieldValueForbidden", + Field: "format", + BadValue: "", + Detail: "Log format json is disabled, see LoggingBetaOptions feature", + }}, + }, + { + name: "JSON log format, enabled gates", + args: []string{"--logging-format=json"}, + featureGate: allEnabled, + want: func() *logsapi.LoggingConfiguration { + c := config.DeepCopy() + c.Format = logsapi.JSONLogFormat + return c + }(), + }, { name: "JSON log format", args: []string{"--logging-format=json"}, @@ -98,10 +147,14 @@ func TestJSONFormatRegister(t *testing.T) { if !assert.Equal(t, tc.want, c) { t.Errorf("Wrong Validate() result for %q. expect %v, got %v", tc.name, tc.want, c) } - featureGate := featuregate.NewFeatureGate() - logsapi.AddFeatureGates(featureGate) - err := featureGate.SetFromMap(map[string]bool{string(logsapi.ContextualLogging): tc.contextualLogging}) + featureGate := tc.featureGate + if featureGate == nil { + featureGate = defaultGate + } + mutable := featureGate.DeepCopy() + err := mutable.SetFromMap(map[string]bool{string(logsapi.ContextualLogging): tc.contextualLogging}) require.NoError(t, err) + featureGate = mutable errs := c.ValidateAndApply(featureGate) defer klog.ClearLogger() if !assert.ElementsMatch(t, tc.errs, errs) {