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

WarningHandler cannot be configured when applying #1586

Open
wangli1030 opened this issue Apr 17, 2024 · 13 comments
Open

WarningHandler cannot be configured when applying #1586

wangli1030 opened this issue Apr 17, 2024 · 13 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@wangli1030
Copy link

wangli1030 commented Apr 17, 2024

What happened:
In Argo project, kubectl client is used to apply the resources and WarningHandler need to be configured in order to expose warning when applying the resources. However, I think WarningHandler cannot be configured when using kubctl client.

What you expected to happen:
WarningHandler should be configured when using kubectl client.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:
Customized WarningHandler will be configured in ApplyOptions and
in this line, it generate the objects but it does not contains the WarningHandler in the ApplyOptions. And the client in generated object is used to communicate with API server in this line, this line and this line. So it means, whatever the user configure the WarningHandler in ApplyOptions, it is not used.

Environment:

  • Kubernetes client and server versions (use kubectl version):
  • Cloud provider or hardware configuration:
  • OS (e.g: cat /etc/os-release):
@wangli1030 wangli1030 added the kind/bug Categorizes issue or PR as related to a bug. label Apr 17, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 17, 2024
@ardaguclu
Copy link
Member

Thanks for raising this issue. Is it possible to give the link of the WarningHandler to clarify which one we are talking about?. Furthermore, how did you customize it?.

@wangli1030
Copy link
Author

Hi @ardaguclu, thank you for your reply and sorry I did not make myself clear. The WarningHandler I am referring is in the client-go. And if it is not set, the default WarningHandler will be used which logs the warnings. The customize I mean is to set NewWarningWriter() which outputs warnings to the provided writer. Hope it makes sense.

@ah8ad3
Copy link
Member

ah8ad3 commented Apr 22, 2024

Thanks for raising the issue.
I'm not sure if this is a bug, did you read this article? https://kubernetes.io/blog/2020/09/03/warnings/ .
Maybe you can propose a feature to maybe customize the behavior based on user input.

@brianpursley
Copy link
Member

@wangli1030 are you thinking something like this?
kubernetes/kubernetes#124489

Regardless, it seems wrong that NewWarningWriter is exported, but its return type, warningWriter is not:
https://github.com/kubernetes/kubernetes/blob/a9593d634c6a053848413e600dadbf974627515f/staging/src/k8s.io/client-go/rest/warnings.go#L94

@wangli1030
Copy link
Author

wangli1030 commented Apr 24, 2024

Hi @ah8ad3 and @brianpursley, thank you for your reply and I am sorry if I did not make myself clear. In rest.config, warningHandler could be config and a dynamicClient could be generated by this config. And, In kubectl side, dynamicClient is one of the fields in ApplyOptions, which will be used for Run command. So the problem is, a warningWriter has been set as warningHandler in rest.config and used to generate the dynamicClient, and the client has been passed in applyoptions and calling the Run function. But based on my debug and research as I described previously, the warningWriter is not used when actually communicate with kubeAPI so I cannot use the warning message. Also, @brianpursley I did not look very deep on the code, not too sure the private warningWriter is the root cause for this. Hope it makes sense and look forward to your reply. Thank you.

@mpuckett159
Copy link
Contributor

I think that this issue should be accepted but I'm having some trouble wrapping my head around it. I think a PR (or even just a branch I can diff against master with) demonstrating the desired outcome would help clarify this for me so we can decide to accept this or not, and what SIG this should belong to. At this point I think this might be API machinery and not CLI.

@wangli1030
Copy link
Author

wangli1030 commented Apr 25, 2024

Hi @mpuckett159, thank you for looking at it. To be honest, currently I do not know the root cause and how to fix it. The simple way to describe this issue is that I believe this is the real place to apply the resources but it does not use the warningHandler/warningWriter which exists in DynamicClient in the o *ApplyOptions. Hope it makes sense.

@ardaguclu
Copy link
Member

@wangli1030 I investigated this today deeply and I think, I've figured out what is happening.

First of all, I'm not sure in what way are you consuming modules in k8s, are you using kubectl programmatically or are you directly relying on client-go's functionality. Why am I asking this because in order to set the warningHandler, it is crucial to know that. Default warning handler in kubectl is set in here https://github.com/kubernetes/kubernetes/pull/124489/files#diff-7e82ada63a2e6664b803ac3ccdb84759ac97338ce807df63be2f81e3c9e0ba9cL311 (example from the PR belongs to @brianpursley )

The problem is apply command in kubectl does only use DynamicClient for pruning purposes not for actual apply. Actual apply is performed by each resource's client https://github.com/kubernetes/kubernetes/blob/06679402e75d001d54770c9ec67cacbf28794009/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L565. That is the reason custom warning handler in DynamicClient is not being used by apply command.

If you are using kubectl functions, I believe that you can customize your specific warning handler by using this function https://github.com/kubernetes/kubernetes/blob/62d379fa5abd4f109b1f1dfe2112feff03c569b4/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go#L326

@wangli1030
Copy link
Author

wangli1030 commented May 9, 2024

Hi @ardaguclu really appreciate your time for digging into this. In ArgoCD, I believe it does not directly use client-go or kubctl as CLI, but it relies on kubectl ApplyOptions as dependency package to apply the resources. I have tried to use rest.SetDefaultWarningHandler to set global WarningHandler but it does not work well for concurrent goroutine since only one warning handler will be used for all clients and all warning message will go to one place. Therefore I would like to suggest to pass in customized warningHandler when creating each resource's client if needed. In this way, warning message for each resource will go to designated warningHandler(buffer).

@ardaguclu
Copy link
Member

I don't think we can offer such granularity per object. But instead of modifying the global default warning handler, I think you can inject your custom warning handler with https://github.com/kubernetes/kubernetes/blob/ebaf49dbd709a94e0c44aa75b4c32b50898a05e3/staging/src/k8s.io/client-go/rest/config.go#L128. Once you set your custom warning handler in config. It will eventually be set to restclient and will be used.

@wangli1030
Copy link
Author

wangli1030 commented May 9, 2024

Hi @ardaguclu, could you show me where I could inject config/warningHandler for resource client when using kubectl ApplyOptions? Thank you.

@ardaguclu
Copy link
Member

ah, now we are on the same page :). Because apply command does not use RestClient to set custom warning handler, instead it uses builder which does not support configuring custom warning handler.

/triage accepted
/remove-kind bug
/kind feature
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. kind/bug Categorizes issue or PR as related to a bug. labels May 9, 2024
@wangli1030
Copy link
Author

Thanks again @ardaguclu for looking at it. Hope it could be supported soon. Also please let me know if there is anything I could work on. I am more than happy to contribute to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

6 participants