🐛 Suppress API server warnings in the client #2887

dlipovetsky · 2024-07-23T22:55:50Z

This adds:

Tests that verify warning suppression in the client. Note that one of the test fails, confirming the bug. These tests are meant to help us validate the fix, and avoid future regression.
Ensures, by configuring the warning handler in client-go, that API server warnings are in fact suppressed.

Test failure

> Enter [BeforeEach] Client - /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:153 @ 07/24/24 03:22:08.098
< Exit [BeforeEach] Client - /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:153 @ 07/24/24 03:22:08.098 (0s)
> Enter [It] should not log warnings when warning suppression is enabled - /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:273 @ 07/24/24 03:22:08.098
2024-07-24T03:22:08Z	INFO	klog	unknown field "spec"
2024-07-24T03:22:08Z	INFO	klog	unknown field "status"
[FAILED] expected to find zero API server warnings in the client log
In [It] at: /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:312 @ 07/24/24 03:22:08.127
< Exit [It] should not log warnings when warning suppression is enabled - /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:273 @ 07/24/24 03:22:08.127 (29ms)
> Enter [AfterEach] Client - /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:210 @ 07/24/24 03:22:08.127
< Exit [AfterEach] Client - /home/prow/go/src/sigs.k8s.io/controller-runtime/pkg/client/client_test.go:210 @ 07/24/24 03:22:08.138 (11ms)

Fixes #2886

k8s-ci-robot · 2024-07-23T22:55:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dlipovetsky · 2024-07-24T03:17:13Z

/test all

I want to demonstrate that one of the new tests fails, as expected.

Please see https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/2887/pull-controller-runtime-test/1815949340502396928

sbueringer · 2024-07-25T16:53:30Z

pkg/client/client.go

-			},
-		)
+	// By default, we de-duplicate and surface warnings.
+	config.WarningHandler = log.NewKubeAPIWarningLogger(


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

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.

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.

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.

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()

}

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

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

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

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.

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

pkg/client/client_test.go

sbueringer · 2024-07-26T13:21:15Z

@dlipovetsky

I agree with

I think we can merge this PR, because it preserves existing behavior (which does not respect config.WarningHandler by default).

Let's address the nit here (#2887 (comment)) then we can look for a more complete solution in the next PR

@alvaroaleman @vincepri WDYT?

Always delete test namespace.

sbueringer · 2024-07-29T17:04:03Z

/lgtm
/approve

k8s-ci-robot · 2024-07-29T17:04:09Z

LGTM label has been added.

Git tree hash: e100f449b2494aed104fc0e316ef991e05d426a7

k8s-ci-robot · 2024-07-29T17:04:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlipovetsky, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2024-07-29T17:04:20Z

/cherry-pick release-0.18

k8s-infra-cherrypick-robot · 2024-07-29T17:04:22Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-0.18 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.18

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-infra-cherrypick-robot · 2024-07-29T17:07:54Z

@sbueringer: new pull request created: #2890

In response to this:

/cherry-pick release-0.18

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dlipovetsky · 2024-07-29T20:51:30Z

Thanks a lot for backporting!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 23, 2024

k8s-ci-robot requested review from alvaroaleman and FillZpp July 23, 2024 22:56

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 23, 2024

dlipovetsky changed the title ~~🐛 Tests to verify warning suppression in the client~~ 🐛 Suppress API server warnings in the client Jul 24, 2024

dlipovetsky marked this pull request as ready for review July 24, 2024 04:13

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2024

k8s-ci-robot requested a review from inteon July 24, 2024 04:13

dlipovetsky added 3 commits July 23, 2024 21:17

Allow client tests to inspect controller and client log messages

b09cb19

Verify that client respects option to suppress warnings

a7baa23

Correctly suppress API server warnings

16d25f1

dlipovetsky force-pushed the fix-warning-handler branch from 9f18c25 to 16d25f1 Compare July 24, 2024 04:17

sbueringer reviewed Jul 25, 2024

View reviewed changes

sbueringer reviewed Jul 26, 2024

View reviewed changes

pkg/client/client_test.go Outdated Show resolved Hide resolved

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 26, 2024

fixup! Verify that client respects option to suppress warnings

3750cad

Always delete test namespace.

k8s-ci-robot assigned sbueringer Jul 29, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2024

k8s-ci-robot merged commit 89bb86e into kubernetes-sigs:main Jul 29, 2024
9 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Jul 29, 2024

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

Merged

This was referenced Jul 30, 2024

Provide an option in the clusterctl library to suppress API server warnings kubernetes-sigs/cluster-api#10932

Closed

⚠️ Deprecate client.Options.WarningHandler #2896

Closed

⚠️ Remove options.WarningHandler #2903

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Suppress API server warnings in the client #2887

🐛 Suppress API server warnings in the client #2887

dlipovetsky commented Jul 23, 2024 •

edited

Loading

k8s-ci-robot commented Jul 23, 2024

dlipovetsky commented Jul 24, 2024 •

edited

Loading

sbueringer Jul 25, 2024

dlipovetsky Jul 25, 2024 •

edited

Loading

dlipovetsky Jul 25, 2024 •

edited

Loading

dlipovetsky Jul 25, 2024

dlipovetsky Jul 25, 2024 •

edited

Loading

sbueringer Jul 26, 2024

sbueringer Jul 26, 2024 •

edited

Loading

sbueringer Jul 26, 2024

dlipovetsky Jul 26, 2024

sbueringer Jul 29, 2024

sbueringer commented Jul 26, 2024

sbueringer commented Jul 29, 2024

k8s-ci-robot commented Jul 29, 2024

k8s-ci-robot commented Jul 29, 2024

sbueringer commented Jul 29, 2024

k8s-infra-cherrypick-robot commented Jul 29, 2024

k8s-infra-cherrypick-robot commented Jul 29, 2024

dlipovetsky commented Jul 29, 2024

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

🐛 Suppress API server warnings in the client #2887

🐛 Suppress API server warnings in the client #2887

Conversation

dlipovetsky commented Jul 23, 2024 • edited Loading

k8s-ci-robot commented Jul 23, 2024

dlipovetsky commented Jul 24, 2024 • edited Loading

sbueringer Jul 25, 2024

Choose a reason for hiding this comment

dlipovetsky Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

dlipovetsky Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

dlipovetsky Jul 25, 2024

Choose a reason for hiding this comment

dlipovetsky Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 26, 2024

Choose a reason for hiding this comment

sbueringer Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 26, 2024

Choose a reason for hiding this comment

dlipovetsky Jul 26, 2024

Choose a reason for hiding this comment

sbueringer Jul 29, 2024

Choose a reason for hiding this comment

sbueringer commented Jul 26, 2024

sbueringer commented Jul 29, 2024

k8s-ci-robot commented Jul 29, 2024

k8s-ci-robot commented Jul 29, 2024

sbueringer commented Jul 29, 2024

k8s-infra-cherrypick-robot commented Jul 29, 2024

k8s-infra-cherrypick-robot commented Jul 29, 2024

dlipovetsky commented Jul 29, 2024

dlipovetsky commented Jul 23, 2024 •

edited

Loading

dlipovetsky commented Jul 24, 2024 •

edited

Loading

dlipovetsky Jul 25, 2024 •

edited

Loading

dlipovetsky Jul 25, 2024 •

edited

Loading

dlipovetsky Jul 25, 2024 •

edited

Loading

sbueringer Jul 26, 2024 •

edited

Loading