-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Allign k8s configuration settings #29908
Conversation
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Pinging @elastic/integrations (Team:Integrations) |
This pull request does not have a backport label. Could you fix it @ChrsMark? 🙏
NOTE: |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
/package |
Note to myself: add changelog entry before merging |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@@ -29,7 +29,7 @@ import ( | |||
type kubeAnnotatorConfig struct { | |||
KubeConfig string `config:"kube_config"` | |||
KubeClientOptions kubernetes.KubeClientOptions `config:"kube_client_options"` | |||
Host string `config:"host"` | |||
Node string `config:"node"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it first be deprecated instead of just dropping old setting name (the same as for autodiscover)? should it be considered a breaking change?
not sure what is the process here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it should be done like this the same time we did for autodiscover and processor. However we can claim that the change goes all the way with the k8s features and ship it under 8.1 as a breaking change which already had a notice. In any case this is was not set in any default manifests or configs so I don't forsee any huge impact.
@@ -51,12 +51,13 @@ type kubernetesConfig struct { | |||
KubeConfig string `config:"kube_config"` | |||
KubeClientOptions kubernetes.KubeClientOptions `config:"kube_client_options"` | |||
|
|||
Host string `config:"host"` | |||
Node string `config:"node"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same question here
@@ -262,14 +263,15 @@ func getResourceMetadataWatchers(config *kubernetesConfig, resource kubernetes.R | |||
|
|||
options := kubernetes.WatchOptions{ | |||
SyncTimeout: config.SyncPeriod, | |||
Namespace: config.Namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be added a check if config.Namespace is defined?
smth like here -
beats/libbeat/autodiscover/providers/kubernetes/pod.go
Lines 98 to 100 in fb2f53c
if config.Namespace != "" { | |
options.Namespace = config.Namespace | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it really matters but I'm adding the check anyways. I would expect an empty value to have no impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting why this check is added only for options
that are used for nodeWatcher
, but not for watcher
(in autodiscover -
beats/libbeat/autodiscover/providers/kubernetes/pod.go
Lines 98 to 100 in fb2f53c
if config.Namespace != "" { | |
options.Namespace = config.Namespace | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try to remove it and see what happens.
# If kube_config is not set, KUBECONFIG environment variable will be checked | ||
# and if not present it will fall back to InCluster | ||
#kube_config: ~/.kube/config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this setting is already defined in file - line 41, should be removed then
/package |
@tetianakravchenko I think we are fine to remove the check for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What does this PR do?
This PR adds
namespace
and moveshost
tonode
settings under thekubernetes
module.Why is it important?
In order to have alignment between the metadata configurations.
Related issues
Closes #29132