From 5da30e54b3898ae21e8ec4d03c190abc18370a76 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 9 Dec 2024 13:00:47 +0100 Subject: [PATCH] client-go features: ignore contextual logging The client-go feature gates implementation logs information about feature states at V(1). Changing that would imply changing the Enabled method, which is very intrusive because there are many callers which are not expected to log and thus don't have access to a contextual logger. The code is not active in Kubernetes components, those use the clientAdapter to make client-go use the normal feature gate implementation, which doesn't log anything. Therefore the code doesn't get changed and only annotated so that logcheck won't complain. Kubernetes-commit: e47b186e6ba3cbc9c732409ef9037e883ea80da1 --- features/envvar.go | 15 +++++++++++---- features/features.go | 6 ++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/features/envvar.go b/features/envvar.go index 8c3f887dc..7ac21b383 100644 --- a/features/envvar.go +++ b/features/envvar.go @@ -141,6 +141,13 @@ func (f *envVarFeatureGates) Set(featureName Feature, featureValue bool) error { // read from the corresponding environmental variable. func (f *envVarFeatureGates) getEnabledMapFromEnvVar() map[Feature]bool { f.readEnvVarsOnce.Do(func() { + // This code does not really support contextual logging. Making it do so has huge + // implications for several call chains because the Enabled call then needs + // a `*WithLogger` variant. This does not matter in Kubernetes itself because + // all Kubernetes components replace the feature gate implementation used + // by client-go, but it might matter elsewhere. + logger := klog.Background() + featureGatesState := map[Feature]bool{} for feature, featureSpec := range f.known { featureState, featureStateSet := os.LookupEnv(fmt.Sprintf("KUBE_FEATURE_%s", feature)) @@ -150,10 +157,10 @@ func (f *envVarFeatureGates) getEnabledMapFromEnvVar() map[Feature]bool { boolVal, boolErr := strconv.ParseBool(featureState) switch { case boolErr != nil: - utilruntime.HandleError(fmt.Errorf("cannot set feature gate %q to %q, due to %v", feature, featureState, boolErr)) + utilruntime.HandleErrorWithLogger(logger, boolErr, "Could not set feature gate", "feature", feature, "desiredState", featureState) case featureSpec.LockToDefault: if boolVal != featureSpec.Default { - utilruntime.HandleError(fmt.Errorf("cannot set feature gate %q to %q, feature is locked to %v", feature, featureState, featureSpec.Default)) + utilruntime.HandleErrorWithLogger(logger, nil, "Could not set feature gate, feature is locked", "feature", feature, "desiredState", featureState, "lockedState", featureSpec.Default) break } featureGatesState[feature] = featureSpec.Default @@ -166,10 +173,10 @@ func (f *envVarFeatureGates) getEnabledMapFromEnvVar() map[Feature]bool { for feature, featureSpec := range f.known { if featureState, ok := featureGatesState[feature]; ok { - klog.V(1).InfoS("Feature gate updated state", "feature", feature, "enabled", featureState) + logger.V(1).Info("Feature gate updated state", "feature", feature, "enabled", featureState) continue } - klog.V(1).InfoS("Feature gate default state", "feature", feature, "enabled", featureSpec.Default) + logger.V(1).Info("Feature gate default state", "feature", feature, "enabled", featureSpec.Default) } }) return f.enabledViaEnvVar.Load().(map[Feature]bool) diff --git a/features/features.go b/features/features.go index cabb7468d..f21511dca 100644 --- a/features/features.go +++ b/features/features.go @@ -17,7 +17,7 @@ limitations under the License. package features import ( - "errors" + "context" "sync/atomic" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -127,7 +127,9 @@ func AddVersionedFeaturesToExistingFeatureGates(registry VersionedRegistry) erro // clientgofeaturegate.ReplaceFeatureGates(utilfeature.DefaultMutableFeatureGate) func ReplaceFeatureGates(newFeatureGates Gates) { if replaceFeatureGatesWithWarningIndicator(newFeatureGates) { - utilruntime.HandleError(errors.New("the default feature gates implementation has already been used and now it's being overwritten. This might lead to unexpected behaviour. Check your initialization order")) + // TODO (?): A new API would be needed where callers pass in a context or logger. + // Probably not worth it. + utilruntime.HandleErrorWithContext(context.TODO(), nil, "The default feature gates implementation has already been used and now it's being overwritten. This might lead to unexpected behaviour. Check your initialization order.") } }