[autodiscover] - Providing config option to disable Kubeadm config api requests #98
Conversation
gizas
requested review from
AndersonQ,
leehinman,
MichaelKatsoulis,
tetianakravchenko and
constanca-m
pierrehilbert
added
the
Team:Elastic-Agent-Data-Plane
gizas
added
the
Team:Cloudnative-Monitoring
kubernetes/metadata/metadata.go
Outdated
| if kubeadm { | ||
| return clusterInfo, fmt.Errorf("unable to get cluster identifiers from kubeadm-config because conf_kubeadm access has been disabled") | ||
| } |
There was a problem hiding this comment.
One question, will this be reported as an error in the logs? It seems to me if the user disabled that, it should not be reported as an error.
There was a problem hiding this comment.
Good point. I will change it to Debug
kubernetes/metadata/config.go
Outdated
| KubeConfig string `config:"kube_config"` | ||
|
|
||
| KubeConfig string `config:"kube_config"` | ||
| KubeAdm bool `config:"disable_kubeadm"` |
There was a problem hiding this comment.
| KubeAdm bool `config:"disable_kubeadm"` | |
| UseKubeAdm bool `config:"use_kubeadm"` |
I would use affirmative name instead, so true means enabled, false - disabled, not vice versa
imagine checking in code c.KubeAdm = false would give an impression that it is disabled, it introduce obscurity. WDYT?
default value should be changed in this case
There was a problem hiding this comment.
You are right, I was actually thinking about that. Reason that I chose not to is that the absence of setting the variable evaluates to false and it was quicker like that to implement the code. For eg. https://github.com/elastic/beats/blob/feef1138f809281ac0a70ec97be6ed782901c2ab/metricbeat/module/kubernetes/util/kubernetes.go#L1231 the absence here would mean that this will evaluate to false and all will work and wont have to initialise anything in beats
This would mean that in beats we will need to revert the logic.
In a nutshell,
So not setting use_kubeadm -> use_kubeadm: false in beats module but in here KubeAdm should be true
There was a problem hiding this comment.
@tetianakravchenko I repeated the tests with use_kubeadm
Final PR has the logic reverted , I mean that use_kubeadm is true by default
Users need to disable (use_kubeadm: false) to see no api calls
I have run tests with metricbeat:
Initial run is with metricbeat-8.15.0-SNAPSHOT: 170 api calls
Second run is with new build of metricbeat and not setting the use_kubeadm variable (so is true): 170 api calls
3rd run is with new build of metricbeat and setting use_kubeadm variable=false: 0 api calls
Reverting back returns the calls
There was a problem hiding this comment.
Thank you for reverting the logic and tests!
💚 Build Succeeded
History
|
|
(Note to self and anyone else watching this PR) @gizas is on PTO until the last week of August. |
Related changes needed for elastic/beats#40086