kubectl icon indicating copy to clipboard operation
kubectl copied to clipboard

WarningHandler cannot be configured when applying

Open wangli1030 opened this issue 3 months ago • 14 comments

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 avatar Apr 17 '24 10:04 wangli1030

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?.

ardaguclu avatar Apr 18 '24 11:04 ardaguclu

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.

wangli1030 avatar Apr 18 '24 12:04 wangli1030

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.

ah8ad3 avatar Apr 22 '24 08:04 ah8ad3

@wangli1030 are you thinking something like this? https://github.com/kubernetes/kubernetes/pull/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

brianpursley avatar Apr 23 '24 18:04 brianpursley

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.

wangli1030 avatar Apr 24 '24 06:04 wangli1030

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.

mpuckett159 avatar Apr 24 '24 18:04 mpuckett159

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.

wangli1030 avatar Apr 25 '24 06:04 wangli1030

@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

ardaguclu avatar May 08 '24 15:05 ardaguclu

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

wangli1030 avatar May 09 '24 07:05 wangli1030

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.

ardaguclu avatar May 09 '24 08:05 ardaguclu

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

wangli1030 avatar May 09 '24 10:05 wangli1030

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

ardaguclu avatar May 09 '24 11:05 ardaguclu

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.

wangli1030 avatar May 09 '24 13:05 wangli1030