Skip to content

🐛 Suppress API server warnings in the client #2887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 29, 2024
Merged

🐛 Suppress API server warnings in the client #2887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b09cb19
Allow client tests to inspect controller and client log messages
dlipovetsky Jul 23, 2024
a7baa23
Verify that client respects option to suppress warnings
dlipovetsky Jul 23, 2024
16d25f1
Correctly suppress API server warnings
dlipovetsky Jul 24, 2024
3750cad
fixup! Verify that client respects option to suppress warnings
dlipovetsky Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about keeping config.WarningHandler if it is not nil?

Copy link
Contributor Author

@dlipovetsky dlipovetsky Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I considered this, and actually prefer it.

I thought it might be seen as a backward-incompatible change, so I did not include it in this PR.

However, thinking about it now, I realize that, by default, config.WarningHandler is nil. So, in the default case, we would continue to modify warning handler, but we would also respect the warning handler the user chose.

The downside is that, in some cases. we respect config.WarningHandler, but ignore options.WarningHandler. For example, if the user says:

config.WarningHandler = rest.NoWarnings{}
options.WarningHandler = WarningHandlerOptions{SuppressWarnings: false}

Then we will suppress warnings, even though the user appears to ask us to surface warnings.

Copy link
Contributor Author

@dlipovetsky dlipovetsky Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note, I thought it might be easier to remove options.WarningHandler altogether, and ask the user to define config.WarningHandler. However, removing that might annoy users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about that, the more I like it. How about: If you define config.WarningHandler, we'll respect it. If you don't , we'll use KubeAPIWarningLogger configured some default way.

If you don't like its default configuration, you can always choose to instantiate KubeAPIWarningLogger with a custom configuration, and yourself, and assign it to config.WarningHandler.

Copy link
Contributor Author

@dlipovetsky dlipovetsky Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same approach we take with user agent. That is, we respect the user's configuration, or we use a value that we choose:

controller-runtime/pkg/client/client.go

Lines 123 to 125 in 16d25f1

if config.UserAgent == "" {
config.UserAgent = rest.DefaultKubernetesUserAgent()
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main goal is to be able to configure a custom warning handler. E.g. for Cluster API it could be useful to only drop certain warnings (the finalizer one, in controllers).

I'm not entirely sure if we should deprecate the WarningHandler field or not

Copy link
Member

@sbueringer sbueringer Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also allow folks to configure a WarningHandler in the WarningHandlerOptions (that has a higher priority than the other two fields)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess this depends overall if we think we should:

  • just find a way that user can configure a WarningHandler
  • also respect what we get from the rest config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #2887 (comment) I demonstrated that we cannot meet both goals at the same time.

Working from the "principle of least suprise," I think it's better to deprecate WarningHandlerOptions, and ask the user to use the rest config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me! Let's see what others say on the new PR

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
Expand Down
23 changes: 19 additions & 4 deletions pkg/client/client_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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"}}

Expand Down
86 changes: 86 additions & 0 deletions pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ limitations under the License.
package client_test

import (
"bufio"
"context"
"encoding/json"
"errors"
"fmt"
"reflect"
"strings"
"sync/atomic"
"time"

Expand Down Expand Up @@ -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{})
Expand Down