Skip to content
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.

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

✨ Surface API Server warnings in client #1468

Merged
merged 2 commits into from
Apr 13, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
42 changes: 42 additions & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"

"github.com/go-logr/logr"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand All @@ -31,15 +32,34 @@ import (
"k8s.io/client-go/rest"

"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/controller-runtime/pkg/log"
)

// 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 deuplication
AllowDuplicateLogs bool
}

// Options are creation options for a Client
type Options struct {
// Scheme, if provided, will be used to map go structs to GroupVersionKinds
Scheme *runtime.Scheme

// Mapper, if provided, will be used to map GroupVersionKinds to Resources
Mapper meta.RESTMapper

// Opts is used to configure the warning handler responsible for
// surfacing and handling warnings messages sent by the API server.
Opts WarningHandlerOptions
}

// New returns a new Client using the provided config and Options.
Expand All @@ -56,6 +76,27 @@ func New(config *rest.Config, options Options) (Client, error) {
if config == nil {
return nil, fmt.Errorf("must provide non-nil rest.Config to client.New")
}
var logger logr.Logger

if !options.Opts.SuppressWarnings {
// surface warnings
logger = log.Log.WithName("KubeAPIWarningLogger")
} else {
// suppress warnings
logger = log.NullLogger{}
}

// Set a WarningHandler, the default WarningHandler is log.WarningLogger with
// deduplication enabled. See log.WarningLoggerOptions for considerations regarding
// deduplication.
rest.SetDefaultWarningHandler(
MadhavJivrajani marked this conversation as resolved.
Show resolved Hide resolved
log.NewWarningLogger(
logger,
log.WarningLoggerOptions{
Deduplicate: !options.Opts.AllowDuplicateLogs,
},
),
)
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering a bit if the client is the best place to plug this in. Maybe pkg/cluster would be better? Or both?
Many ppl will use the cache-backed client and all its read operations will not have this enabled since they go through the cache.
cc @vincepri


// Init a scheme if none provided
if options.Scheme == nil {
Expand Down Expand Up @@ -110,6 +151,7 @@ var _ Client = &client{}

// client is a client.Client that reads and writes directly from/to an API server. It lazily initializes
// new clients at the time they are used, and caches the client.
// new clients at the time they are used, and caches the client.
MadhavJivrajani marked this conversation as resolved.
Show resolved Hide resolved
type client struct {
typedClient typedClient
unstructuredClient unstructuredClient
Expand Down
74 changes: 74 additions & 0 deletions pkg/log/warning_handler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
Copyright 2018 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package log

import (
"sync"

"github.com/go-logr/logr"
)

// WarningLoggerOptions controls the behavior of a rest.WarningHandler constructed using NewWarningLogger()
type WarningLoggerOptions struct {
// Deduplicate indicates a given warning message should only be written once.
// Setting this to true in a long-running process handling many warnings can
// result in increased memory use.
Deduplicate bool
}

// WarningLogger is a wrapper around DelegatingLogger that implements the
// rest.WarningHandler interface.
type WarningLogger struct {
// logger is used to log responses with the warning header
logger logr.Logger
// opts contain options controlling warning output
opts WarningLoggerOptions
// writtenLock gurads written
writtenLock sync.Mutex
// used to keep track of already logged messages
// and help in de-duplication.
written map[string]struct{}
}

// HandleWarningHeader handles logging for responses from API server that are
// warnings with code being 299 and uses a logr.Logger for it's logging purposes.
func (l *WarningLogger) HandleWarningHeader(code int, agent string, message string) {
if code != 299 || len(message) == 0 {
return
}

if l.opts.Deduplicate {
l.writtenLock.Lock()
defer l.writtenLock.Unlock()

if _, alreadyLogged := l.written[message]; alreadyLogged {
return
}
l.written[message] = struct{}{}
}
l.logger.Info(message)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
l.logger.Info(message)
l.logger.Warn(message)

Copy link
Contributor Author

@MadhavJivrajani MadhavJivrajani Apr 13, 2021

Choose a reason for hiding this comment

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

One clarification: if I understand correctly, the logr.Logger interface doesn't implement a Warn level and instead recommends specifying verbosity levels for Info logs itself as explained here, so maybe setting a verbosity level using V or using coloured output for the Info messages that come out of the API Warning Logger?

Copy link
Member

Choose a reason for hiding this comment

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

I see, lets keep it at Info then

}

// NewWarningLogger returns an implementation of rest.WarningHandler that logs warnings
// with code = 299 using a DelegatingLogger.
MadhavJivrajani marked this conversation as resolved.
Show resolved Hide resolved
func NewWarningLogger(l logr.Logger, opts WarningLoggerOptions) *WarningLogger {
MadhavJivrajani marked this conversation as resolved.
Show resolved Hide resolved
h := &WarningLogger{logger: l, opts: opts}
if opts.Deduplicate {
h.written = map[string]struct{}{}
}
return h
}