Add k8s metadata autodetection #13473

ChrsMark · 2019-09-03T14:25:16Z

This PR is a proposal for enabling autodetection mode for add_kubernetes_metadata.

Currently we configure add_cloud_metadata and add_host_metadata by default in all Beats.

add_cloud_metadata does autodetection, and it will only enrich events if the beat is running on a known public cloud.

It would be useful to have the same for Kubernetes, so if Beats is started in a scenario where Kubernetes is reachable with a default config, it automatically enables metadata for it. If Kubernetes is not reachable with a default config nothing will happen.

Part of: #13068
Depends on #13374

How to test it

With k8s available
a. Run metricbeat or filebeat inside a k8s cluster and without setting kube_config.
b. Verify that processor is being automatically enabled. (Note that it will create a client in an in_cluster mode.)
With k8s unavailable
a. Run metricbeat or filebeat outside of a k8s cluster and without setting kube_config.
b. Verify that processor is not being automatically enabled.

docs: https://www.elastic.co/guide/en/beats/filebeat/master/add-kubernetes-metadata.html#add-kubernetes-metadata

odacremolbap

Hey @ChrsMark I haven't executed it (for now), so consider my comments code only.

It would be nice to an update on docs to explain how the autodetection works, maybe in this PR or at a follow up.

libbeat/processors/add_kubernetes_metadata/config.go

libbeat/processors/add_kubernetes_metadata/kubernetes.go

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark · 2019-09-10T07:19:39Z

Hey @ChrsMark I haven't executed it (for now), so consider my comments code only.

It would be nice to an update on docs to explain how the autodetection works, maybe in this PR or at a follow up.

Hey @odacremolbap thank you for reviewing. I think all your comments have been addressed. I would love to hear back if you notice anything else.

As far as the docs' update is concerned, if it is needed I would go with a follow up PR. @jsoriano WDYT about his?

Signed-off-by: chrismark <chrismarkou92@gmail.com>

libbeat/processors/add_kubernetes_metadata/config.go

libbeat/common/kubernetes/util.go

Signed-off-by: chrismark <chrismarkou92@gmail.com>

odacremolbap

LGTM
Let's wait for CI outcome

@jsoriano do you want to take a look?

jsoriano

I see that you have added some code to handle in-cluster configuration vs. explicitly specified kubeconfig, I am worried that this can cause some behavioural inconsistencies with other kubernetes features in Beats.
I think this code shouldn't be needed here. If it is needed then this is probably also required in other places and it should be added to some common place so all kubernetes clients are initialized consistently.

jsoriano · 2019-09-11T10:52:27Z

libbeat/processors/add_kubernetes_metadata/config.go

+		return true
+	}
+	return false
+}


We should be consistent with the logic to configure kubernetes clients along all our kubernetes features.

I think we shouldn't need to add special handling here to get the config path from the environment variables, or to get in-cluster configuration. If we need to add this logic for some reason, we should consider adding it in a common places, so logic is kept consistent along all kubernetes features.

Why did you need to add this logic?

We may need to revisit this conversation.

Actually, the field InCluster can be removed.

Regarding the searching of environmental variables or kubeconfig in $HOME directory:
If we are not in an "incluster" mode then the autodetection will just fail if users have not set kube_config in their configuration cause it will not be able to create a client etc. So auto-detection will be actually available only in "Incluster" deployments or we should require from the user to set the kubeconfig.

In addition this code has do only with add_kubernetes_metadata processor, so is it possible to affect other features too (really not sure about that, just asking).

So what we should do? I'm open on any proposal :)

Actually, the field InCluster can be removed.

👍

Regarding the searching of environmental variables or kubeconfig in $HOME directory:
If we are not in an "incluster" mode then the autodetection will just fail if users have not set kube_config in their configuration cause it will not be able to create a client etc. So auto-detection will be actually available only in "Incluster" deployments or we should require from the user to set the kubeconfig.

I think that failing if in-cluster configuration is not available and kubeconfig is not set would be perfectly fine, and it would be consistent with the rest of kubernetes features.

Said that, I see that needing to set kubeconfig in all Kubernetes features can be cumbersome, we could try to explore the use of KUBECONFIG env variable as an alternative to set it everywhere. But if something needs to be done for that I would do it in the common helpers, and in a separate PR.

In addition this code has do only with add_kubernetes_metadata processor, so is it possible to affect other features too (really not sure about that, just asking).

So what we should do? I'm open on any proposal :)

I'd say to:

Keep it failing if in-cluster configuration is not available and kubeconfig is not set. In any case the most common case for Kubernetes is to have in-cluster configuration working.

Consider leveraging KUBECONFIG for all our Kubernetes features, but as a separate issue/PR.

jsoriano · 2019-09-11T10:54:59Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

+func isKubernetesAvailable(client k8s.Interface) bool {
+	server, err := client.Discovery().ServerVersion()
+	if err != nil {
+		logp.Err("%v: could not detect kubernetes env: %v", "add_kubernetes_metadata", err)


This shouldn't be logged as error, as this is going to happen on any non-kubernetes deployment using the default configuration.

jsoriano · 2019-09-11T11:00:19Z

libbeat/processors/add_kubernetes_metadata/config.go

 func defaultKubernetesAnnotatorConfig() kubeAnnotatorConfig {
 	return kubeAnnotatorConfig{
+		KubeConfig:      getSystemKubeConfig(),


I think that in other places we are assuming that if no kubeconfig is specified, then in cluster configuration is wanted. We should keep this behaviour also here.

I think it falls under the same doubts of #13473 (comment)

jsoriano · 2019-09-11T11:02:43Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

+		} else {
+			logp.Err("%v: could not create kubernetes client using config: %v", "add_kubernetes_metadata", config.KubeConfig)
+		}
+		processor.kubernetesAvailable = false


Nit. Set kubernetesAvailable to false already when initializing the processor (or just don't set it as is the zero value), and only set it to true when all checks have been done.

Also here it is not needed to set processor.kubernetesAvailable = false.

Suggested change

processor.kubernetesAvailable = false

jsoriano · 2019-09-11T11:04:03Z

libbeat/processors/add_kubernetes_metadata/kubernetes_test.go

+				"original": "fields",
+			},
+		},
+	})


Why initializing the cache if kubernetes is not available?

Maybe it is not required. It is just a left-over from copying the above test. :)

Ok, try to remove it then 🙂

libbeat/processors/add_kubernetes_metadata/config.go

jsoriano · 2019-09-11T11:10:40Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

 	client, err := kubernetes.GetKubernetesClient(config.KubeConfig)
 	if err != nil {
-		return nil, err
+		if config.InCluster {


config.InCluster could be replaced by kubernetes.IsInCluster(config.KubeConfig) as used in other places. Or at least it should be consistent with the result of this call.

jsoriano · 2019-09-11T11:16:28Z

As far as the docs' update is concerned, if it is needed I would go with a follow up PR. @jsoriano WDYT about his?

Ok to do docs changes in a follow up
We may also need to review docs of in_cluster setting, I think that this is being ignored after #13051

ChrsMark · 2019-09-11T11:59:05Z

So @jsoriano the big deal is about if we decide to have auto enablement of add_kubernetes_metadata to be able to search on its own for kubeconfig file (in $HOME path or in env) or request from the user to specify that. WDYT?

cc: @odacremolbap

Signed-off-by: chrismark <chrismarkou92@gmail.com>

jsoriano

LGTM, I have added some extra minor comments, please address them before merging.

jsoriano · 2019-09-11T14:11:35Z

CHANGELOG.next.asciidoc

@@ -248,6 +248,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Add support for RFC3339 time zone offsets in JSON output. {pull}13227[13227]
 - Add autodetection mode for add_docker_metadata and enable it by default in included configuration files{pull}13374[13374]
 - Added `monitoring.cluster_uuid` setting to associate Beat data with specified ES cluster in Stack Monitoring UI. {pull}13182[13182]
+- Add autodetection mode for add_kubernetes_metadata and enable it by default in included configuration files{pull}13473[13473]


Nit.

Suggested change

- Add autodetection mode for add_kubernetes_metadata and enable it by default in included configuration files{pull}13473[13473]

- Add autodetection mode for add_kubernetes_metadata and enable it by default in included configuration files. {pull}13473[13473]

jsoriano · 2019-09-11T14:12:08Z

libbeat/common/kubernetes/util.go

@@ -36,6 +36,7 @@ const defaultNode = "localhost"
 // in cluster configuration based on the secrets mounted in the Pod. If kubeConfig is passed,
 // it parses the config file to get the config required to build a client.
 func GetKubernetesClient(kubeconfig string) (kubernetes.Interface, error) {
+


Nit. Remove this added line.

Suggested change

jsoriano · 2019-09-11T14:13:09Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

+		} else {
+			logp.Err("%v: could not create kubernetes client using config: %v", "add_kubernetes_metadata", config.KubeConfig)
+		}
+		processor.kubernetesAvailable = false


Also here it is not needed to set processor.kubernetesAvailable = false.

Suggested change

processor.kubernetesAvailable = false

jsoriano · 2019-09-11T14:14:45Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

+		if kubernetes.IsInCluster(config.KubeConfig) {
+			logp.Err("%v: could not create kubernetes client using in_cluster config", "add_kubernetes_metadata")
+		} else {
+			logp.Err("%v: could not create kubernetes client using config: %v", "add_kubernetes_metadata", config.KubeConfig)


Also these ones should be logged at debug level, because they are going to be logged in all non-kubernetes deployments.

jsoriano · 2019-09-11T14:23:06Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

@@ -26,17 +26,20 @@ import (
 	"github.com/elastic/beats/libbeat/common/kubernetes"
 	"github.com/elastic/beats/libbeat/logp"
 	"github.com/elastic/beats/libbeat/processors"
+
+	k8s "k8s.io/client-go/kubernetes"


This import should be placed before beats imports.

"fmt" "time" k8s "k8s.io/client-go/kubernetes" "github.com/elastic/beats/libbeat/beat" "github.com/elastic/beats/libbeat/common" "github.com/elastic/beats/libbeat/common/kubernetes" "github.com/elastic/beats/libbeat/logp" "github.com/elastic/beats/libbeat/processors"

And I wonder if we should use another alias, k8s is basically the same as kubernetes, maybe k8sclient... Not sure.

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark requested review from a team as code owners September 3, 2019 14:25

ChrsMark self-assigned this Sep 3, 2019

ChrsMark added Team:Integrations Label for the Integrations team containers Related to containers use case [zube]: In Review review test-plan Add this PR to be manual test plan labels Sep 3, 2019

ChrsMark mentioned this pull request Sep 3, 2019

Automatically enable add_docker_metadata or add_kubernetes_metadata #13068

Closed

ChrsMark force-pushed the add_k8s_metadata_autodetection branch from b11d4ec to fe6f4ca Compare September 4, 2019 09:28

ChrsMark requested a review from a team September 4, 2019 15:17

ChrsMark force-pushed the add_k8s_metadata_autodetection branch 2 times, most recently from 92f46a5 to 25e0373 Compare September 6, 2019 07:08

odacremolbap suggested changes Sep 9, 2019

View reviewed changes

ChrsMark added 2 commits September 9, 2019 13:43

Add k8s conn check

4fd9d44

Signed-off-by: chrismark <chrismarkou92@gmail.com>

Add tests and changelog

894f5b0

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark added [zube]: In Progress and removed [zube]: In Review labels Sep 9, 2019

andresrc added in progress Pull request is currently in progress. and removed review labels Sep 9, 2019

ChrsMark force-pushed the add_k8s_metadata_autodetection branch from 25e0373 to 7b89d95 Compare September 10, 2019 07:15

Improve k8s environment detection

2a9577f

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark force-pushed the add_k8s_metadata_autodetection branch from 7b89d95 to 2a9577f Compare September 10, 2019 07:54

odacremolbap self-requested a review September 10, 2019 08:13

odacremolbap suggested changes Sep 10, 2019

View reviewed changes

houndci-bot reviewed Sep 10, 2019

View reviewed changes

libbeat/common/kubernetes/util.go Outdated Show resolved Hide resolved

houndci-bot reviewed Sep 10, 2019

View reviewed changes

libbeat/common/kubernetes/util.go Outdated Show resolved Hide resolved

Improve detection and add InClusterConfig check

cbd16eb

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark force-pushed the add_k8s_metadata_autodetection branch from 4c4898e to cbd16eb Compare September 10, 2019 10:43

Change logging level

965eff1

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark requested a review from odacremolbap September 11, 2019 09:56

odacremolbap approved these changes Sep 11, 2019

View reviewed changes

jsoriano requested changes Sep 11, 2019

View reviewed changes

Finalizing autodection checks

442f348

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark requested a review from jsoriano September 11, 2019 13:16

jsoriano reviewed Sep 11, 2019

View reviewed changes

Minor improvements

9cc29e5

Signed-off-by: chrismark <chrismarkou92@gmail.com>

ChrsMark requested a review from jsoriano September 12, 2019 07:20

jsoriano approved these changes Sep 12, 2019

View reviewed changes

jsoriano added v7.5.0 [zube]: In Review review and removed [zube]: In Progress in progress Pull request is currently in progress. labels Sep 12, 2019

ChrsMark merged commit cec8311 into elastic:master Sep 12, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Sep 12, 2019

andrewkroh mentioned this pull request Sep 16, 2019

[Winlogbeat] Fails to start with default config having #13705

Closed

andresrc removed the [zube]: Done label Sep 17, 2019

ChrsMark added the [zube]: (7.5) Planned for release label Oct 2, 2019

andresrc added the needs testing notes label Oct 10, 2019

ChrsMark removed the needs testing notes label Oct 11, 2019

urso added the release-highlight label Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add k8s metadata autodetection #13473

Add k8s metadata autodetection #13473

ChrsMark commented Sep 3, 2019 •

edited

Loading

odacremolbap left a comment

ChrsMark commented Sep 10, 2019

odacremolbap left a comment

jsoriano left a comment •

edited

Loading

jsoriano Sep 11, 2019

ChrsMark Sep 11, 2019

jsoriano Sep 11, 2019

ChrsMark Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

ChrsMark Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

ChrsMark Sep 11, 2019

jsoriano Sep 11, 2019

ChrsMark Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano commented Sep 11, 2019

ChrsMark commented Sep 11, 2019

jsoriano left a comment

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

jsoriano Sep 11, 2019

	- Add autodetection mode for add_kubernetes_metadata and enable it by default in included configuration files{pull}13473[13473]
	- Add autodetection mode for add_kubernetes_metadata and enable it by default in included configuration files. {pull}13473[13473]

Add k8s metadata autodetection #13473

Add k8s metadata autodetection #13473

Conversation

ChrsMark commented Sep 3, 2019 • edited Loading

How to test it

odacremolbap left a comment

Choose a reason for hiding this comment

ChrsMark commented Sep 10, 2019

odacremolbap left a comment

Choose a reason for hiding this comment

jsoriano left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Sep 11, 2019

ChrsMark commented Sep 11, 2019

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrsMark commented Sep 3, 2019 •

edited

Loading

jsoriano left a comment •

edited

Loading