[release-0.18] 🐛 Suppress API server warnings in the client (#2890)

* Allow client tests to inspect controller and client log messages

* Verify that client respects option to suppress warnings

* Correctly suppress API server warnings

* fixup! Verify that client respects option to suppress warnings

Always delete test namespace.

---------

Co-authored-by: Daniel Lipovetsky <[email protected]>

Author: k8s-infra-cherrypick-robot

pkg/client/client.go

@@ -124,19 +124,15 @@ func newClient(config *rest.Config, options Options) (*client, error) {
 		config.UserAgent = rest.DefaultKubernetesUserAgent()
 	}
 
-	if !options.WarningHandler.SuppressWarnings {
-		// surface warnings
-		logger := log.Log.WithName("KubeAPIWarningLogger")
-		// Set a WarningHandler, the default WarningHandler
-		// is log.KubeAPIWarningLogger with deduplication enabled.
-		// See log.KubeAPIWarningLoggerOptions for considerations
-		// regarding deduplication.
-		config.WarningHandler = log.NewKubeAPIWarningLogger(
-			logger,
-			log.KubeAPIWarningLoggerOptions{
-				Deduplicate: !options.WarningHandler.AllowDuplicateLogs,
-			},
-		)
+	// 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{}
 	}
 
 	// Use the rest HTTP client for the provided config if unset

pkg/client/client_suite_test.go

@@ -17,13 +17,16 @@ limitations under the License.
 package client_test
 
 import (
+	"bytes"
+	"io"
 	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/examples/crd/pkg"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
@@ -36,12 +39,24 @@ func TestClient(t *testing.T) {
 	RunSpecs(t, "Client Suite")
 }
 
-var testenv *envtest.Environment
-var cfg *rest.Config
-var clientset *kubernetes.Clientset
+var (
+	testenv   *envtest.Environment
+	cfg       *rest.Config
+	clientset *kubernetes.Clientset
+
+	// Used by tests to inspect controller and client log messages.
+	log bytes.Buffer
+)
 
 var _ = BeforeSuite(func() {
-	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
+	// Forwards logs to ginkgo output, and allows tests to inspect logs.
+	mw := io.MultiWriter(&log, GinkgoWriter)
+
+	// Use prefixes to help us tell the source of the log message.
+	// controller-runtime uses logf
+	logf.SetLogger(zap.New(zap.WriteTo(mw), zap.UseDevMode(true)).WithName("logf"))
+	// client-go logs uses klog
+	klog.SetLogger(zap.New(zap.WriteTo(mw), zap.UseDevMode(true)).WithName("klog"))
 
 	testenv = &envtest.Environment{CRDDirectoryPaths: []string{"./testdata"}}
 

pkg/client/client_test.go

@@ -17,11 +17,13 @@ limitations under the License.
 package client_test
 
 import (
+	"bufio"
 	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"reflect"
+	"strings"
 	"sync/atomic"
 	"time"
 
@@ -226,6 +228,90 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
 		Expect(client.IgnoreNotFound(err)).NotTo(HaveOccurred())
 	})
 
+	Describe("WarningHandler", func() {
+		It("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{}}},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cl).NotTo(BeNil())
+
+			tns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ws-disabled"}}
+			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())
+
+			scanner := bufio.NewScanner(&log)
+			for scanner.Scan() {
+				line := scanner.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())
+
+			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
+				}
+			}
+			deleteNamespace(ctx, tns)
+		})
+	})
+
 	Describe("New", func() {
 		It("should return a new Client", func() {
 			cl, err := client.New(cfg, client.Options{})