Merge pull request #2903 from dlipovetsky/remove-warninghandleroptions

⚠️ Remove options.WarningHandler

pkg/client/client.go

@@ -50,28 +50,10 @@ type Options struct {
 	// Cache, if provided, is used to read objects from the cache.
 	Cache *CacheOptions
 
-	// WarningHandler is used to configure the warning handler responsible for
-	// surfacing and handling warnings messages sent by the API server.
-	WarningHandler WarningHandlerOptions
-
 	// DryRun instructs the client to only perform dry run requests.
 	DryRun *bool
 }
 
-// WarningHandlerOptions are options for configuring a
-// warning handler for the client which is responsible
-// for surfacing API Server warnings.
-type WarningHandlerOptions struct {
-	// SuppressWarnings decides if the warnings from the
-	// API server are suppressed or surfaced in the client.
-	SuppressWarnings bool
-	// AllowDuplicateLogs does not deduplicate the to-be
-	// logged surfaced warnings messages. See
-	// log.WarningHandlerOptions for considerations
-	// regarding deduplication
-	AllowDuplicateLogs bool
-}
-
 // CacheOptions are options for creating a cache-backed client.
 type CacheOptions struct {
 	// Reader is a cache-backed reader that will be used to read objects from the cache.
@@ -91,6 +73,12 @@ 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.
+// See [sigs.k8s.io/controller-runtime/pkg/log.KubeAPIWarningLogger] for
+// an example.
+//
 // 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 +112,14 @@ 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{
+				Deduplicate: true,
+			},
+		)
 	}
 
 	// 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,15 +231,19 @@ 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{}
-			cl, err := client.New(cfg, client.Options{
-				WarningHandler: client.WarningHandlerOptions{SuppressWarnings: false}, Cache: &client.CacheOptions{Reader: cache, DisableFor: []client.Object{&corev1.Namespace{}}},
-			})
+
+			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: "ws-disabled"}}
+			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())
@@ -257,45 +263,17 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
 			Expect(err).NotTo(HaveOccurred())
 			Expect(cl).NotTo(BeNil())
 
-			scanner := bufio.NewScanner(&log)
-			for scanner.Scan() {
-				line := scanner.Text()
+			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 in the client log")
-		})
-
-		It("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{}}},
-			})
-			Expect(err).NotTo(HaveOccurred())
-			Expect(cl).NotTo(BeNil())
-
-			tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ws-enabled"}}
-			tns, err = clientset.CoreV1().Namespaces().Create(ctx, tns, metav1.CreateOptions{})
-			Expect(err).NotTo(HaveOccurred())
-			Expect(tns).NotTo(BeNil())
-
-			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())
+			defer Fail("expected to find one API server warning logged the config.WarningHandler")
 
 			scanner := bufio.NewScanner(&log)
 			for scanner.Scan() {
@@ -308,7 +286,6 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
 					break
 				}
 			}
-			deleteNamespace(ctx, tns)
 		})
 	})
 

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,26 @@ func ExampleNew() {
 	}
 }
 
+func ExampleNew_suppress_warnings() {
+	cfg := config.GetConfigOrDie()
+	// Use a rest.WarningHandler that discards warning messages.
+	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.