Skip to content

Commit b6309f7

Browse files
pohlypohly
committed
client-go/test: warning handler with contextual logging
The default handler now uses contextual logging. Instead of warnings.go:106] warning 1 it now logs the caller of client-go and uses structured, contextual logging main.go:100] "Warning" message="warning 1" Users of client-go have the choice whether the handler that they provide uses the traditional API (no API break!) or contextual logging.
1 parent bd55d18 commit b6309f7

File tree

8 files changed

+343
-61
lines changed

8 files changed

+343
-61
lines changed

‎staging/src/k8s.io/client-go/rest/client.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ type RESTClient struct {
101101

102102
// warningHandler is shared among all requests created by this client.
103103
// If not set, defaultWarningHandler is used.
104-
warningHandler WarningHandler
104+
warningHandler WarningHandlerWithContext
105105

106106
// Set specific behavior of the client. If not set http.DefaultClient will be used.
107107
Client *http.Client

‎staging/src/k8s.io/client-go/rest/config.go

Lines changed: 56 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,23 @@ type Config struct {
129129
RateLimiter flowcontrol.RateLimiter
130130

131131
// WarningHandler handles warnings in server responses.
132-
// If not set, the default warning handler is used.
133-
// See documentation for SetDefaultWarningHandler() for details.
132+
// If this and WarningHandlerWithContext are not set, the
133+
// default warning handler is used. If both are set,
134+
// WarningHandlerWithContext is used.
135+
//
136+
// See documentation for [SetDefaultWarningHandler] for details.
137+
//
138+
//logcheck:context // WarningHandlerWithContext should be used instead of WarningHandler in code which supports contextual logging.
134139
WarningHandler WarningHandler
135140

141+
// WarningHandlerWithContext handles warnings in server responses.
142+
// If this and WarningHandler are not set, the
143+
// default warning handler is used. If both are set,
144+
// WarningHandlerWithContext is used.
145+
//
146+
// See documentation for [SetDefaultWarningHandler] for details.
147+
WarningHandlerWithContext WarningHandlerWithContext
148+
136149
// The maximum length of time to wait before giving up on a server request. A value of zero means no timeout.
137150
Timeout time.Duration
138151

@@ -381,12 +394,27 @@ func RESTClientForConfigAndClient(config *Config, httpClient *http.Client) (*RES
381394
}
382395

383396
restClient, err := NewRESTClient(baseURL, versionedAPIPath, clientContent, rateLimiter, httpClient)
384-
if err == nil && config.WarningHandler != nil {
385-
restClient.warningHandler = config.WarningHandler
386-
}
397+
maybeSetWarningHandler(restClient, config.WarningHandler, config.WarningHandlerWithContext)
387398
return restClient, err
388399
}
389400

401+
// maybeSetWarningHandler sets the handlerWithContext if non-nil,
402+
// otherwise the handler with a wrapper if non-nil,
403+
// and does nothing if both are nil.
404+
//
405+
// May be called for a nil client.
406+
func maybeSetWarningHandler(c *RESTClient, handler WarningHandler, handlerWithContext WarningHandlerWithContext) {
407+
if c == nil {
408+
return
409+
}
410+
switch {
411+
case handlerWithContext != nil:
412+
c.warningHandler = handlerWithContext
413+
case handler != nil:
414+
c.warningHandler = warningLoggerNopContext{l: handler}
415+
}
416+
}
417+
390418
// UnversionedRESTClientFor is the same as RESTClientFor, except that it allows
391419
// the config.Version to be empty.
392420
func UnversionedRESTClientFor(config *Config) (*RESTClient, error) {
@@ -448,9 +476,7 @@ func UnversionedRESTClientForConfigAndClient(config *Config, httpClient *http.Cl
448476
}
449477

450478
restClient, err := NewRESTClient(baseURL, versionedAPIPath, clientContent, rateLimiter, httpClient)
451-
if err == nil && config.WarningHandler != nil {
452-
restClient.warningHandler = config.WarningHandler
453-
}
479+
maybeSetWarningHandler(restClient, config.WarningHandler, config.WarningHandlerWithContext)
454480
return restClient, err
455481
}
456482

@@ -616,15 +642,16 @@ func AnonymousClientConfig(config *Config) *Config {
616642
CAData: config.TLSClientConfig.CAData,
617643
NextProtos: config.TLSClientConfig.NextProtos,
618644
},
619-
RateLimiter: config.RateLimiter,
620-
WarningHandler: config.WarningHandler,
621-
UserAgent: config.UserAgent,
622-
DisableCompression: config.DisableCompression,
623-
QPS: config.QPS,
624-
Burst: config.Burst,
625-
Timeout: config.Timeout,
626-
Dial: config.Dial,
627-
Proxy: config.Proxy,
645+
RateLimiter: config.RateLimiter,
646+
WarningHandler: config.WarningHandler,
647+
WarningHandlerWithContext: config.WarningHandlerWithContext,
648+
UserAgent: config.UserAgent,
649+
DisableCompression: config.DisableCompression,
650+
QPS: config.QPS,
651+
Burst: config.Burst,
652+
Timeout: config.Timeout,
653+
Dial: config.Dial,
654+
Proxy: config.Proxy,
628655
}
629656
}
630657

@@ -658,17 +685,18 @@ func CopyConfig(config *Config) *Config {
658685
CAData: config.TLSClientConfig.CAData,
659686
NextProtos: config.TLSClientConfig.NextProtos,
660687
},
661-
UserAgent: config.UserAgent,
662-
DisableCompression: config.DisableCompression,
663-
Transport: config.Transport,
664-
WrapTransport: config.WrapTransport,
665-
QPS: config.QPS,
666-
Burst: config.Burst,
667-
RateLimiter: config.RateLimiter,
668-
WarningHandler: config.WarningHandler,
669-
Timeout: config.Timeout,
670-
Dial: config.Dial,
671-
Proxy: config.Proxy,
688+
UserAgent: config.UserAgent,
689+
DisableCompression: config.DisableCompression,
690+
Transport: config.Transport,
691+
WrapTransport: config.WrapTransport,
692+
QPS: config.QPS,
693+
Burst: config.Burst,
694+
RateLimiter: config.RateLimiter,
695+
WarningHandler: config.WarningHandler,
696+
WarningHandlerWithContext: config.WarningHandlerWithContext,
697+
Timeout: config.Timeout,
698+
Dial: config.Dial,
699+
Proxy: config.Proxy,
672700
}
673701
if config.ExecProvider != nil && config.ExecProvider.Config != nil {
674702
c.ExecProvider.Config = config.ExecProvider.Config.DeepCopyObject()

‎staging/src/k8s.io/client-go/rest/config_test.go

Lines changed: 76 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import (
4141
"github.com/google/go-cmp/cmp"
4242
fuzz "github.com/google/gofuzz"
4343
"github.com/stretchr/testify/assert"
44+
"github.com/stretchr/testify/require"
4445
)
4546

4647
func TestIsConfigTransportTLS(t *testing.T) {
@@ -266,6 +267,19 @@ type fakeWarningHandler struct{}
266267

267268
func (f fakeWarningHandler) HandleWarningHeader(code int, agent string, message string) {}
268269

270+
type fakeWarningHandlerWithLogging struct {
271+
messages []string
272+
}
273+
274+
func (f *fakeWarningHandlerWithLogging) HandleWarningHeader(code int, agent string, message string) {
275+
f.messages = append(f.messages, message)
276+
}
277+
278+
type fakeWarningHandlerWithContext struct{}
279+
280+
func (f fakeWarningHandlerWithContext) HandleWarningHeaderWithContext(ctx context.Context, code int, agent string, message string) {
281+
}
282+
269283
type fakeNegotiatedSerializer struct{}
270284

271285
func (n *fakeNegotiatedSerializer) SupportedMediaTypes() []runtime.SerializerInfo {
@@ -330,6 +344,9 @@ func TestAnonymousAuthConfig(t *testing.T) {
330344
func(h *WarningHandler, f fuzz.Continue) {
331345
*h = &fakeWarningHandler{}
332346
},
347+
func(h *WarningHandlerWithContext, f fuzz.Continue) {
348+
*h = &fakeWarningHandlerWithContext{}
349+
},
333350
// Authentication does not require fuzzer
334351
func(r *AuthProviderConfigPersister, f fuzz.Continue) {},
335352
func(r *clientcmdapi.AuthProviderConfig, f fuzz.Continue) {
@@ -428,6 +445,9 @@ func TestCopyConfig(t *testing.T) {
428445
func(h *WarningHandler, f fuzz.Continue) {
429446
*h = &fakeWarningHandler{}
430447
},
448+
func(h *WarningHandlerWithContext, f fuzz.Continue) {
449+
*h = &fakeWarningHandlerWithContext{}
450+
},
431451
func(r *AuthProviderConfigPersister, f fuzz.Continue) {
432452
*r = fakeAuthProviderConfigPersister{}
433453
},
@@ -619,25 +639,69 @@ func TestConfigSprint(t *testing.T) {
619639
KeyData: []byte("fake key"),
620640
NextProtos: []string{"h2", "http/1.1"},
621641
},
622-
UserAgent: "gobot",
623-
Transport: &fakeRoundTripper{},
624-
WrapTransport: fakeWrapperFunc,
625-
QPS: 1,
626-
Burst: 2,
627-
RateLimiter: &fakeLimiter{},
628-
WarningHandler: fakeWarningHandler{},
629-
Timeout: 3 * time.Second,
630-
Dial: fakeDialFunc,
631-
Proxy: fakeProxyFunc,
642+
UserAgent: "gobot",
643+
Transport: &fakeRoundTripper{},
644+
WrapTransport: fakeWrapperFunc,
645+
QPS: 1,
646+
Burst: 2,
647+
RateLimiter: &fakeLimiter{},
648+
WarningHandler: fakeWarningHandler{},
649+
WarningHandlerWithContext: fakeWarningHandlerWithContext{},
650+
Timeout: 3 * time.Second,
651+
Dial: fakeDialFunc,
652+
Proxy: fakeProxyFunc,
632653
}
633654
want := fmt.Sprintf(
634-
`&rest.Config{Host:"localhost:8080", APIPath:"v1", ContentConfig:rest.ContentConfig{AcceptContentTypes:"application/json", ContentType:"application/json", GroupVersion:(*schema.GroupVersion)(nil), NegotiatedSerializer:runtime.NegotiatedSerializer(nil)}, Username:"gopher", Password:"--- REDACTED ---", BearerToken:"--- REDACTED ---", BearerTokenFile:"", Impersonate:rest.ImpersonationConfig{UserName:"gopher2", UID:"uid123", Groups:[]string(nil), Extra:map[string][]string(nil)}, AuthProvider:api.AuthProviderConfig{Name: "gopher", Config: map[string]string{--- REDACTED ---}}, AuthConfigPersister:rest.AuthProviderConfigPersister(--- REDACTED ---), ExecProvider:api.ExecConfig{Command: "sudo", Args: []string{"--- REDACTED ---"}, Env: []ExecEnvVar{--- REDACTED ---}, APIVersion: "", ProvideClusterInfo: true, Config: runtime.Object(--- REDACTED ---), StdinUnavailable: false}, TLSClientConfig:rest.sanitizedTLSClientConfig{Insecure:false, ServerName:"", CertFile:"a.crt", KeyFile:"a.key", CAFile:"", CertData:[]uint8{0x2d, 0x2d, 0x2d, 0x20, 0x54, 0x52, 0x55, 0x4e, 0x43, 0x41, 0x54, 0x45, 0x44, 0x20, 0x2d, 0x2d, 0x2d}, KeyData:[]uint8{0x2d, 0x2d, 0x2d, 0x20, 0x52, 0x45, 0x44, 0x41, 0x43, 0x54, 0x45, 0x44, 0x20, 0x2d, 0x2d, 0x2d}, CAData:[]uint8(nil), NextProtos:[]string{"h2", "http/1.1"}}, UserAgent:"gobot", DisableCompression:false, Transport:(*rest.fakeRoundTripper)(%p), WrapTransport:(transport.WrapperFunc)(%p), QPS:1, Burst:2, RateLimiter:(*rest.fakeLimiter)(%p), WarningHandler:rest.fakeWarningHandler{}, Timeout:3000000000, Dial:(func(context.Context, string, string) (net.Conn, error))(%p), Proxy:(func(*http.Request) (*url.URL, error))(%p)}`,
655+
`&rest.Config{Host:"localhost:8080", APIPath:"v1", ContentConfig:rest.ContentConfig{AcceptContentTypes:"application/json", ContentType:"application/json", GroupVersion:(*schema.GroupVersion)(nil), NegotiatedSerializer:runtime.NegotiatedSerializer(nil)}, Username:"gopher", Password:"--- REDACTED ---", BearerToken:"--- REDACTED ---", BearerTokenFile:"", Impersonate:rest.ImpersonationConfig{UserName:"gopher2", UID:"uid123", Groups:[]string(nil), Extra:map[string][]string(nil)}, AuthProvider:api.AuthProviderConfig{Name: "gopher", Config: map[string]string{--- REDACTED ---}}, AuthConfigPersister:rest.AuthProviderConfigPersister(--- REDACTED ---), ExecProvider:api.ExecConfig{Command: "sudo", Args: []string{"--- REDACTED ---"}, Env: []ExecEnvVar{--- REDACTED ---}, APIVersion: "", ProvideClusterInfo: true, Config: runtime.Object(--- REDACTED ---), StdinUnavailable: false}, TLSClientConfig:rest.sanitizedTLSClientConfig{Insecure:false, ServerName:"", CertFile:"a.crt", KeyFile:"a.key", CAFile:"", CertData:[]uint8{0x2d, 0x2d, 0x2d, 0x20, 0x54, 0x52, 0x55, 0x4e, 0x43, 0x41, 0x54, 0x45, 0x44, 0x20, 0x2d, 0x2d, 0x2d}, KeyData:[]uint8{0x2d, 0x2d, 0x2d, 0x20, 0x52, 0x45, 0x44, 0x41, 0x43, 0x54, 0x45, 0x44, 0x20, 0x2d, 0x2d, 0x2d}, CAData:[]uint8(nil), NextProtos:[]string{"h2", "http/1.1"}}, UserAgent:"gobot", DisableCompression:false, Transport:(*rest.fakeRoundTripper)(%p), WrapTransport:(transport.WrapperFunc)(%p), QPS:1, Burst:2, RateLimiter:(*rest.fakeLimiter)(%p), WarningHandler:rest.fakeWarningHandler{}, WarningHandlerWithContext:rest.fakeWarningHandlerWithContext{}, Timeout:3000000000, Dial:(func(context.Context, string, string) (net.Conn, error))(%p), Proxy:(func(*http.Request) (*url.URL, error))(%p)}`,
635656
c.Transport, fakeWrapperFunc, c.RateLimiter, fakeDialFunc, fakeProxyFunc,
636657
)
637658

638659
for _, f := range []string{"%s", "%v", "%+v", "%#v"} {
639660
if got := fmt.Sprintf(f, c); want != got {
640-
t.Errorf("fmt.Sprintf(%q, c)\ngot: %q\nwant: %q", f, got, want)
661+
t.Errorf("fmt.Sprintf(%q, c)\ngot: %q\nwant: %q\ndiff: %s", f, got, want, cmp.Diff(want, got))
641662
}
642663
}
643664
}
665+
666+
func TestConfigWarningHandler(t *testing.T) {
667+
config := &Config{}
668+
config.GroupVersion = &schema.GroupVersion{}
669+
config.NegotiatedSerializer = &fakeNegotiatedSerializer{}
670+
handlerNoContext := &fakeWarningHandler{}
671+
handlerWithContext := &fakeWarningHandlerWithContext{}
672+
673+
t.Run("none", func(t *testing.T) {
674+
client, err := RESTClientForConfigAndClient(config, nil)
675+
require.NoError(t, err)
676+
assert.Nil(t, client.warningHandler)
677+
})
678+
679+
t.Run("no-context", func(t *testing.T) {
680+
config := CopyConfig(config)
681+
handler := &fakeWarningHandlerWithLogging{}
682+
config.WarningHandler = handler
683+
client, err := RESTClientForConfigAndClient(config, nil)
684+
require.NoError(t, err)
685+
client.warningHandler.HandleWarningHeaderWithContext(context.Background(), 0, "", "message")
686+
assert.Equal(t, []string{"message"}, handler.messages)
687+
688+
})
689+
690+
t.Run("with-context", func(t *testing.T) {
691+
config := CopyConfig(config)
692+
config.WarningHandlerWithContext = handlerWithContext
693+
client, err := RESTClientForConfigAndClient(config, nil)
694+
require.NoError(t, err)
695+
assert.Equal(t, handlerWithContext, client.warningHandler)
696+
})
697+
698+
t.Run("both", func(t *testing.T) {
699+
config := CopyConfig(config)
700+
config.WarningHandler = handlerNoContext
701+
config.WarningHandlerWithContext = handlerWithContext
702+
client, err := RESTClientForConfigAndClient(config, nil)
703+
require.NoError(t, err)
704+
assert.NotNil(t, client.warningHandler)
705+
assert.Equal(t, handlerWithContext, client.warningHandler)
706+
})
707+
}

‎staging/src/k8s.io/client-go/rest/exec_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,9 @@ func TestConfigToExecClusterRoundtrip(t *testing.T) {
242242
func(h *WarningHandler, f fuzz.Continue) {
243243
*h = &fakeWarningHandler{}
244244
},
245+
func(h *WarningHandlerWithContext, f fuzz.Continue) {
246+
*h = &fakeWarningHandlerWithContext{}
247+
},
245248
// Authentication does not require fuzzer
246249
func(r *AuthProviderConfigPersister, f fuzz.Continue) {},
247250
func(r *clientcmdapi.AuthProviderConfig, f fuzz.Continue) {
@@ -289,6 +292,7 @@ func TestConfigToExecClusterRoundtrip(t *testing.T) {
289292
expected.Burst = 0
290293
expected.RateLimiter = nil
291294
expected.WarningHandler = nil
295+
expected.WarningHandlerWithContext = nil
292296
expected.Timeout = 0
293297
expected.Dial = nil
294298

‎staging/src/k8s.io/client-go/rest/request.go

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ type Request struct {
103103
contentConfig ClientContentConfig
104104
contentTypeNotSet bool
105105

106-
warningHandler WarningHandler
106+
warningHandler WarningHandlerWithContext
107107

108108
rateLimiter flowcontrol.RateLimiter
109109
backoff BackoffManager
@@ -271,8 +271,21 @@ func (r *Request) BackOff(manager BackoffManager) *Request {
271271
}
272272

273273
// WarningHandler sets the handler this client uses when warning headers are encountered.
274-
// If set to nil, this client will use the default warning handler (see SetDefaultWarningHandler).
274+
// If set to nil, this client will use the default warning handler (see [SetDefaultWarningHandler]).
275+
//
276+
//logcheck:context // WarningHandlerWithContext should be used instead of WarningHandler in code which supports contextual logging.
275277
func (r *Request) WarningHandler(handler WarningHandler) *Request {
278+
if handler == nil {
279+
r.warningHandler = nil
280+
return r
281+
}
282+
r.warningHandler = warningLoggerNopContext{l: handler}
283+
return r
284+
}
285+
286+
// WarningHandlerWithContext sets the handler this client uses when warning headers are encountered.
287+
// If set to nil, this client will use the default warning handler (see [SetDefaultWarningHandlerWithContext]).
288+
func (r *Request) WarningHandlerWithContext(handler WarningHandlerWithContext) *Request {
276289
r.warningHandler = handler
277290
return r
278291
}
@@ -776,7 +789,7 @@ func (r *Request) watchInternal(ctx context.Context) (watch.Interface, runtime.D
776789
resp, err := client.Do(req)
777790
retry.After(ctx, r, resp, err)
778791
if err == nil && resp.StatusCode == http.StatusOK {
779-
return r.newStreamWatcher(resp)
792+
return r.newStreamWatcher(ctx, resp)
780793
}
781794

782795
done, transformErr := func() (bool, error) {
@@ -969,7 +982,7 @@ func (r *Request) handleWatchList(ctx context.Context, w watch.Interface, negoti
969982
}
970983
}
971984

972-
func (r *Request) newStreamWatcher(resp *http.Response) (watch.Interface, runtime.Decoder, error) {
985+
func (r *Request) newStreamWatcher(ctx context.Context, resp *http.Response) (watch.Interface, runtime.Decoder, error) {
973986
contentType := resp.Header.Get("Content-Type")
974987
mediaType, params, err := mime.ParseMediaType(contentType)
975988
if err != nil {
@@ -980,7 +993,7 @@ func (r *Request) newStreamWatcher(resp *http.Response) (watch.Interface, runtim
980993
return nil, nil, err
981994
}
982995

983-
handleWarnings(resp.Header, r.warningHandler)
996+
handleWarnings(ctx, resp.Header, r.warningHandler)
984997

985998
frameReader := framer.NewFrameReader(resp.Body)
986999
watchEventDecoder := streaming.NewDecoder(frameReader, streamingSerializer)
@@ -1067,7 +1080,7 @@ func (r *Request) Stream(ctx context.Context) (io.ReadCloser, error) {
10671080

10681081
switch {
10691082
case (resp.StatusCode >= 200) && (resp.StatusCode < 300):
1070-
handleWarnings(resp.Header, r.warningHandler)
1083+
handleWarnings(ctx, resp.Header, r.warningHandler)
10711084
return resp.Body, nil
10721085

10731086
default:
@@ -1365,7 +1378,7 @@ func (r *Request) transformResponse(ctx context.Context, resp *http.Response, re
13651378
body: body,
13661379
contentType: contentType,
13671380
statusCode: resp.StatusCode,
1368-
warnings: handleWarnings(resp.Header, r.warningHandler),
1381+
warnings: handleWarnings(ctx, resp.Header, r.warningHandler),
13691382
}
13701383
}
13711384
}
@@ -1384,7 +1397,7 @@ func (r *Request) transformResponse(ctx context.Context, resp *http.Response, re
13841397
statusCode: resp.StatusCode,
13851398
decoder: decoder,
13861399
err: err,
1387-
warnings: handleWarnings(resp.Header, r.warningHandler),
1400+
warnings: handleWarnings(ctx, resp.Header, r.warningHandler),
13881401
}
13891402
}
13901403

@@ -1393,7 +1406,7 @@ func (r *Request) transformResponse(ctx context.Context, resp *http.Response, re
13931406
contentType: contentType,
13941407
statusCode: resp.StatusCode,
13951408
decoder: decoder,
1396-
warnings: handleWarnings(resp.Header, r.warningHandler),
1409+
warnings: handleWarnings(ctx, resp.Header, r.warningHandler),
13971410
}
13981411
}
13991412

0 commit comments

Comments
 (0)