Deprecate options.WarningHandler

- Deprecation and future removal does not change the default behavior,
  which is to de-duplicate and surface warnings.
- Uses config.WarningHandler to override default behavior. Describes
  this in the client.New godoc, adds an example test to demonstrate it,
  and a unit test to verify it.

Author: dlipovetsky

pkg/client/client.go

@@ -52,6 +52,11 @@ type Options struct {
 
 	// WarningHandler is used to configure the warning handler responsible for
 	// surfacing and handling warnings messages sent by the API server.
+	//
+	// Deprecated: This is deprecated and will be removed in a future release.
+	// Deprecation and future removal does not change the default behavior, which is to
+	// de-duplicate and surface warnings. For custom behavior, pass config.WarningHandler
+	// to New().
 	WarningHandler WarningHandlerOptions
 
 	// DryRun instructs the client to only perform dry run requests.
@@ -61,6 +66,8 @@ type Options struct {
 // WarningHandlerOptions are options for configuring a
 // warning handler for the client which is responsible
 // for surfacing API Server warnings.
+//
+// Deprecated: This is deprecated and will be removed in a future release.
 type WarningHandlerOptions struct {
 	// SuppressWarnings decides if the warnings from the
 	// API server are suppressed or surfaced in the client.
@@ -91,6 +98,11 @@ type NewClientFunc func(config *rest.Config, options Options) (Client, error)
 
 // New returns a new Client using the provided config and Options.
 //
+// By default, the client surfaces warnings returned by the server. To
+// suppress warnings, set config.WarningHandler = rest.NoWarnings{}. To
+// define custom behavior, implement the rest.WarningHandler interface.
+// For consistent log formatting, use pkg/log.Log in the implementation.
+//
 // The client's read behavior is determined by Options.Cache.
 // If either Options.Cache or Options.Cache.Reader is nil,
 // the client reads directly from the API server.
@@ -124,15 +136,19 @@ func newClient(config *rest.Config, options Options) (*client, error) {
 		config.UserAgent = rest.DefaultKubernetesUserAgent()
 	}
 
-	// By default, we de-duplicate and surface warnings.
-	config.WarningHandler = log.NewKubeAPIWarningLogger(
-		log.Log.WithName("KubeAPIWarningLogger"),
-		log.KubeAPIWarningLoggerOptions{
-			Deduplicate: !options.WarningHandler.AllowDuplicateLogs,
-		},
-	)
-	if options.WarningHandler.SuppressWarnings {
-		config.WarningHandler = rest.NoWarnings{}
+	if config.WarningHandler == nil {
+		// By default, we de-duplicate and surface warnings.
+		config.WarningHandler = log.NewKubeAPIWarningLogger(
+			log.Log.WithName("KubeAPIWarningLogger"),
+			log.KubeAPIWarningLoggerOptions{
+				// When we remove WarningHandlerOptions, the below will always be set to true.
+				Deduplicate: !options.WarningHandler.AllowDuplicateLogs,
+			},
+		)
+		// When we remove WarningHandlerOptions, we will remove the below condition.
+		if options.WarningHandler.SuppressWarnings {
+			config.WarningHandler = rest.NoWarnings{}
+		}
 	}
 
 	// Use the rest HTTP client for the provided config if unset

pkg/client/client_test.go

@@ -18,6 +18,7 @@ package client_test
 
 import (
 	"bufio"
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -43,6 +44,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/rest"
 	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/controller-runtime/examples/crd/pkg"
@@ -229,7 +231,64 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
 	})
 
 	Describe("WarningHandler", func() {
-		It("should log warnings when warning suppression is disabled", func() {
+		It("should log warnings with config.WarningHandler, if one is defined", func() {
+			cache := &fakeReader{}
+
+			testCfg := rest.CopyConfig(cfg)
+
+			var testLog bytes.Buffer
+			testCfg.WarningHandler = rest.NewWarningWriter(&testLog, rest.WarningWriterOptions{})
+
+			cl, err := client.New(testCfg, client.Options{Cache: &client.CacheOptions{Reader: cache, DisableFor: []client.Object{&corev1.Namespace{}}}})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cl).NotTo(BeNil())
+
+			tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "wh-defined"}}
+			tns, err = clientset.CoreV1().Namespaces().Create(ctx, tns, metav1.CreateOptions{})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(tns).NotTo(BeNil())
+			defer deleteNamespace(ctx, tns)
+
+			toCreate := &pkg.ChaosPod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "example",
+					Namespace: tns.Name,
+				},
+				// The ChaosPod CRD does not define Status, so the field is unknown to the API server,
+				// but field validation is not strict by default, so the API server returns a warning,
+				// and we need a warning to check whether suppression works.
+				Status: pkg.ChaosPodStatus{},
+			}
+			err = cl.Create(ctx, toCreate)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cl).NotTo(BeNil())
+
+			scannerTestLog := bufio.NewScanner(&testLog)
+			for scannerTestLog.Scan() {
+				line := scannerTestLog.Text()
+				if strings.Contains(
+					line,
+					"unknown field \"status\"",
+				) {
+					return
+				}
+			}
+			defer Fail("expected to find one API server warning logged the config.WarningHandler")
+
+			scanner := bufio.NewScanner(&log)
+			for scanner.Scan() {
+				line := scanner.Text()
+				if strings.Contains(
+					line,
+					"unknown field \"status\"",
+				) {
+					defer Fail("expected to find zero API server warnings in the client log")
+					break
+				}
+			}
+		})
+
+		It("(deprecated behavior) should log warnings when warning suppression is disabled", func() {
 			cache := &fakeReader{}
 			cl, err := client.New(cfg, client.Options{
 				WarningHandler: client.WarningHandlerOptions{SuppressWarnings: false}, Cache: &client.CacheOptions{Reader: cache, DisableFor: []client.Object{&corev1.Namespace{}}},
@@ -270,7 +329,7 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
 			defer Fail("expected to find one API server warning in the client log")
 		})
 
-		It("should not log warnings when warning suppression is enabled", func() {
+		It("(deprecated behavior) should not log warnings when warning suppression is enabled", func() {
 			cache := &fakeReader{}
 			cl, err := client.New(cfg, client.Options{
 				WarningHandler: client.WarningHandlerOptions{SuppressWarnings: true}, Cache: &client.CacheOptions{Reader: cache, DisableFor: []client.Object{&corev1.Namespace{}}},

pkg/client/example_test.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
+	"k8s.io/client-go/rest"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
@@ -56,6 +57,24 @@ func ExampleNew() {
 	}
 }
 
+func ExampleNew_suppress_warnings() {
+	cfg := config.GetConfigOrDie()
+	cfg.WarningHandler = rest.NoWarnings{}
+	cl, err := client.New(cfg, client.Options{})
+	if err != nil {
+		fmt.Println("failed to create client")
+		os.Exit(1)
+	}
+
+	podList := &corev1.PodList{}
+
+	err = cl.List(context.Background(), podList, client.InNamespace("default"))
+	if err != nil {
+		fmt.Printf("failed to list pods in namespace default: %v\n", err)
+		os.Exit(1)
+	}
+}
+
 // This example shows how to use the client with typed and unstructured objects to retrieve an object.
 func ExampleClient_get() {
 	// Using a typed object.