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

Client creation sets a global warning handler by default #1928

Closed
ash2k opened this issue Jun 9, 2022 · 2 comments · Fixed by #1944
Closed

Client creation sets a global warning handler by default #1928

ash2k opened this issue Jun 9, 2022 · 2 comments · Fixed by #1944

Comments

@ash2k
Copy link
Member

ash2k commented Jun 9, 2022

#1468 introduced warning handling. Unfortunately, the code makes an assumption that it's the place in a program that has the authority to set global warning handlers. It's simply not true.

rest.SetDefaultWarningHandler(
log.NewKubeAPIWarningLogger(
logger,
log.KubeAPIWarningLoggerOptions{
Deduplicate: !options.Opts.AllowDuplicateLogs,
},
),
)

The code above shouldn't mess with globals, it should be done by the program's author in main() or something like that. The above code should only set warning handler on the provided config. And even that is questionable - if the caller wants, they can just configure that by themself, right? In the config they are providing and/or via the mentioned global. controller-runtime simply doesn't need to do anything, this code should be removed.

In my concrete situation I set warning handler in main() and it's overwritten by controller-runtime somewhere deep in the call stack.

@ash2k
Copy link
Member Author

ash2k commented Jun 9, 2022

It's clearly documented with examples how to use warning handlers in the introductory blog post https://kubernetes.io/blog/2020/09/03/warnings/#customize-client-handling. The blog was even linked in the original issue (#1150), so really not clear why this code was introduced.

@alvaroaleman
Copy link
Member

alvaroaleman commented Jun 14, 2022

@ash2k feel free to create a PR that copies the restCfg prior to setting that handler You can disable setting this in the client opts. Copying the config in the client creation and only setting it in the config would also make sense.

@alvaroaleman alvaroaleman changed the title Libraries should not modify globals as they do not and cannot see the big picture Client creation globally sets a warning handler by default Jun 14, 2022
@alvaroaleman alvaroaleman changed the title Client creation globally sets a warning handler by default Client creation sets a global warning handler by default Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants