From ee9d998d6e0c89bcf2b39fc011bfbc916060d451 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. --- staging/src/k8s.io/client-go/features/envvar.go | 15 +++++++++++---- staging/src/k8s.io/client-go/features/features.go | 6 ++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/client-go/features/envvar.go b/staging/src/k8s.io/client-go/features/envvar.go index 8c3f887dc42..7ac21b383e3 100644 --- a/staging/src/k8s.io/client-go/features/envvar.go +++ b/staging/src/k8s.io/client-go/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/staging/src/k8s.io/client-go/features/features.go b/staging/src/k8s.io/client-go/features/features.go index 5ccdcc55f6c..29bd7863c08 100644 --- a/staging/src/k8s.io/client-go/features/features.go +++ b/staging/src/k8s.io/client-go/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" @@ -103,7 +103,9 @@ func AddFeaturesToExistingFeatureGates(registry Registry) error { // 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.") } }