Refactor kubernetes autodiscover to enable different resource based discovery #14738

vjsamuel · 2019-11-25T08:59:58Z

This PR tries to move around code in a way such that each resource has its own interface implementation and the autodiscover relies on a resource parameter to determine what the watcher to spin up and how to create hints. We also now add scope as a parameter to define if Beats is running either at a node level or at a cluster level.

metricbeat.autodiscover:
  providers:
    - type: kubernetes
      kube_config: ${HOME}/.kube/config
      resource: service
      hints.enabled: true

output.console.pretty: true

The above configuration would enable discovering service objects across the cluster.

metricbeat.autodiscover:
  providers:
    - type: kubernetes
      kube_config: ${HOME}/.kube/config
      resource: pod
      scope: cluster
      hints.enabled: true

The above configuration would allow doing pod discovery across the cluster.

closes #14044 and #10578

elasticmachine · 2019-11-25T09:00:00Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2019-11-25T09:00:00Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2019-11-25T09:00:02Z

libbeat/autodiscover/providers/kubernetes/pod.go

+	watcher kubernetes.Watcher
+}
+
+func NewPodEventer(uuid uuid.UUID, cfg *common.Config, client k8s.Interface, publish func(event bus.Event)) (Eventer, error) {


exported function NewPodEventer should have comment or be unexported

houndci-bot · 2019-11-25T09:00:02Z

libbeat/autodiscover/providers/kubernetes/kubernetes.go

 	"github.com/elastic/beats/libbeat/logp"
 )

 func init() {
 	autodiscover.Registry.AddProvider("kubernetes", AutodiscoverBuilder)
 }

+type Eventer interface {


exported type Eventer should have comment or be unexported

exekias · 2019-11-25T14:03:35Z

libbeat/autodiscover/providers/kubernetes/config.go

@@ -15,6 +15,23 @@
 // specific language governing permissions and limitations
 // under the License.

+// Licensed to Elasticsearch B.V. under one or more contributor


for some reason this was duplicated 🤔

exekias · 2019-11-25T14:49:05Z

so far this is looking good to me 👍 thank you for working on it!

houndci-bot · 2019-11-25T23:27:00Z

libbeat/autodiscover/providers/kubernetes/node.go

+	return ""
+}
+
+func (no *node) isNodeReady(node *kubernetes.Node) bool {


receiver name no should be consistent with previous receiver name n for node

houndci-bot · 2019-11-25T23:27:00Z

libbeat/autodiscover/providers/kubernetes/node.go

+	return ""
+}
+
+func (no *node) isNodeReady(node *kubernetes.Node) bool {


receiver name no should be consistent with previous receiver name n for node

exekias · 2019-11-27T09:10:25Z

libbeat/autodiscover/providers/kubernetes/config.go

+	// Needed when resource is a pod
+	Host string `config:"host"`
+	// Scope can be either host or cluster.
+	Scope    string `config:"scope"`


This would also solve #10578 🎉

can happen in a different PR but I think we should extend this setting to add_kubernetes_metadata processor. I'm unsure about the other (resource), probably not worth it until we see the need.

will create follow up PR

exekias · 2019-11-27T09:11:17Z

libbeat/autodiscover/providers/kubernetes/config.go

+	Host string `config:"host"`
+	// Scope can be either host or cluster.
+	Scope    string `config:"scope"`
+	Resource string `config:"resource"`


both scope and resource are new, could you update the docs?

updated docs.

exekias · 2019-11-27T09:14:24Z

libbeat/autodiscover/providers/kubernetes/service.go

+	logger  *logp.Logger
+	publish func(bus.Event)
+	watcher kubernetes.Watcher
+}


pod.go is fairly well tested, could you add some tests for service and node too?

added test cases for node and service.

exekias · 2019-11-27T09:15:43Z

libbeat/common/kubernetes/watcher.go

+			},
+		}
+
+		objType = "service"


should the watcher refuse to start if scope != cluster? We should at least explain this in the docs if not

updated the docs for the same. the watcher needs some refactor so that we can address some of these concerns and also allow it to do additional enrichment

exekias · 2019-11-28T09:02:09Z

jenkins test this please

exekias

This is looking great! I left a few minor comments, thank you for adding docs and tests!!

exekias · 2019-11-28T09:05:26Z

libbeat/autodiscover/providers/kubernetes/config.go

+	// Check if host is being defined and change it to node instead.
+	if c.Node == "" && c.Host != "" {
+		c.Node = c.Host
+		logp.L().Warnf("`host` will be deprecated in 8.0. use `node` instead")


❤️ Could you use cfgwarn.Deprecate deprecate here? Also, it would be nice to update our manifests to avoid this warning to new users: https://github.com/elastic/beats/tree/master/deploy/kubernetes

We should make this change also in add_kubernetes_metadata

will add this as part of follow up PR to add scope setting

exekias · 2019-11-28T09:11:45Z

libbeat/autodiscover/providers/kubernetes/config.go

+		logp.L().Warnf("`host` will be deprecated in 8.0. use `node` instead")
+	}
+
+	// Check if resource is either node or pod. If yes then default the scope to "host" if not provided.


s/host/node/?

exekias · 2019-11-28T09:11:56Z

libbeat/autodiscover/providers/kubernetes/config.go

+
+	default:
+		if c.Scope == "node" {
+			logp.L().Warnf("can not set scope to `host` when using resource %s. resetting scope to `cluster`", c.Resource)


s/host/node/?

exekias · 2019-11-28T09:12:21Z

libbeat/autodiscover/providers/kubernetes/config_test.go

@@ -53,6 +53,22 @@ func TestConfigWithCustomBuilders(t *testing.T) {
 	assert.NotNil(t, err)
 }

+func TestConfigWithIncorrectScope(t *testing.T) {
+	cfg := common.MapStr{
+		"scope":         "host",


s/host/node/

exekias · 2019-11-28T09:13:02Z

libbeat/autodiscover/providers/kubernetes/node.go

+
+	// Ensure that host is set correctly whenever the scope is set to "host". Make sure that host is empty
+	// when cluster scope is enforced.
+	if config.Scope == "host" {


both description and here, should host be node?

btw I understand that setting node scope here means that we only watch for events in our node. right?

correct. it should be node. it will pin the discovery to that given node.

exekias · 2019-11-28T09:16:59Z

libbeat/docs/shared-autodiscover.asciidoc

+`resource`:: (Optional) Select the resource to do discovery on. Currently supported
+  Kubernetes resources are `pod`, `service` and `node`. If not configured `resource`
+  defaults to `pod`.
+`scope`:: (Optional) Specify at what level autodiscovery needs to be done at. `scope` can


Suggested change

`scope`:: (Optional) Specify at what level autodiscovery needs to be done at. `scope` can

`scope`:: (Optional) Specify at what level autodiscover needs to be done at. `scope` can

exekias · 2019-11-28T09:18:08Z

libbeat/docs/shared-autodiscover.asciidoc

+  the specified node. `cluster` scope allows cluster wide discovery. `pod` and `node` are
+  only resources that can be discovered at `node` scope.


only pod and node can be discovered at node scope.

exekias · 2019-11-28T09:20:19Z

libbeat/autodiscover/providers/kubernetes/node.go

+		config.Host = kubernetes.DiscoverKubernetesNode(config.Host, kubernetes.IsInCluster(config.KubeConfig), client)
+	} else {
+		config.Host = ""
+	}
+
+	logger.Debugf("Initializing a new Kubernetes watcher using host: %v", config.Host)
+
+	watcher, err := kubernetes.NewWatcher(client, &kubernetes.Node{}, kubernetes.WatchOptions{
+		SyncTimeout: config.SyncPeriod,
+		Node:        config.Host,


I think all these should use config.Node, right? same happens in other eventers. Maybe we can rename Host to HostDeprecated to avoid its usage?

exekias · 2019-11-28T09:27:22Z

could you please update the PR description? also in_cluster was removed when we migrated to client-go. You can drop it in the examples

exekias

Could you please add a changelog?

ChrsMark

Thanks for addressing the last comments!

…iscovery (elastic#14738) (cherry picked from commit 1a029e5)

…iscovery (#14738) (#14924) (cherry picked from commit 1a029e5)

ChrsMark · 2020-01-10T08:47:48Z

Manual Testing

We should check manually combinations for the following configuration options:

resource:: (Optional) Select the resource to do discovery on. Currently supported
Kubernetes resources are pod, service and node. If not configured resource
defaults to pod.
scope:: (Optional) Specify at what level autodiscover needs to be done at. scope can
either take node or cluster as values. node scope allows discovery of resources in
the specified node. cluster scope allows cluster wide discovery. Only pod and node resources can be discovered at node scope.

Here are two examples mentioned in the description of the PR:

metricbeat.autodiscover:
  providers:
    - type: kubernetes
      kube_config: ${HOME}/.kube/config
      resource: service
      hints.enabled: true

output.console.pretty: true
The above configuration would enable discovering service objects across the cluster.
metricbeat.autodiscover:
  providers:
    - type: kubernetes
      kube_config: ${HOME}/.kube/config
      resource: pod
      scope: cluster
      hints.enabled: true
The above configuration would allow doing pod discovery across the cluster.

A mix of combinations to test:

pod, node (check that a new pod can be discovered within the node)
pod, cluster (check that a new pod can be discovered along the cluster)
node, cluster (check that a new node can be discovered along the cluster)
node, node (check that the node can be discovered within the same node)
service, cluster (check that new services can be discovered in cluster scope)
service, node (check how this unsupported combination warns the user)

@exekias and @vjsamuel feel free to add/mention anything I might missed

exekias · 2020-01-10T09:50:11Z

Thank you for contributing @vjsamuel!!

odacremolbap · 2020-01-24T16:38:41Z

I couldn't get the node resource to work, probably because of this but I can't tell for certain.

All other resources worked

nerddelphi · 2020-02-20T20:27:19Z

I guess this change broke Autodiscover in Filebeat, as I state it here #16464

@odacremolbap can you check it? Thank you!

houndci-bot reviewed Nov 25, 2019

View reviewed changes

vjsamuel force-pushed the refactor_k8s_autodiscover branch from e55a7a1 to 9023690 Compare November 25, 2019 09:02

exekias added autodiscovery enhancement Team:Integrations Label for the Integrations team labels Nov 25, 2019

andresrc added the [zube]: In Progress label Nov 25, 2019

exekias reviewed Nov 25, 2019

View reviewed changes

houndci-bot reviewed Nov 25, 2019

View reviewed changes

vjsamuel force-pushed the refactor_k8s_autodiscover branch from 473bf23 to 64465ae Compare November 25, 2019 23:28

vjsamuel marked this pull request as ready for review November 25, 2019 23:29

exekias reviewed Nov 27, 2019

View reviewed changes

vjsamuel force-pushed the refactor_k8s_autodiscover branch 2 times, most recently from bfe75a5 to 3e03b63 Compare November 27, 2019 21:11

exekias suggested changes Nov 28, 2019

View reviewed changes

exekias self-assigned this Nov 28, 2019

exekias added [zube]: In Review and removed [zube]: In Progress labels Nov 28, 2019

vjsamuel force-pushed the refactor_k8s_autodiscover branch 2 times, most recently from abecaa4 to ec50931 Compare December 2, 2019 06:08

exekias approved these changes Dec 2, 2019

View reviewed changes

vjsamuel mentioned this pull request Dec 3, 2019

Refactor metadata generator to support adding metadata across resources #14875

Merged

ChrsMark approved these changes Dec 3, 2019

View reviewed changes

ChrsMark added the v7.6.0 label Dec 3, 2019

ChrsMark merged commit 1a029e5 into elastic:master Dec 3, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Dec 3, 2019

ChrsMark pushed a commit to ChrsMark/beats that referenced this pull request Dec 3, 2019

Refactor kubernetes autodiscover to enable different resource based d…

3019a05

…iscovery (elastic#14738) (cherry picked from commit 1a029e5)

ChrsMark mentioned this pull request Dec 3, 2019

Cherry-pick #14738 to 7.x: Refactor kubernetes autodiscover to enable different resource based discovery #14924

Merged

ChrsMark added a commit that referenced this pull request Dec 4, 2019

Refactor kubernetes autodiscover to enable different resource based d…

a9b22cd

…iscovery (#14738) (#14924) (cherry picked from commit 1a029e5)

andresrc removed the [zube]: Done label Dec 5, 2019

This was referenced Dec 6, 2019

Update Kubernetes Autodiscover docs with the new resources #14975

Closed

Update Kubernetes Autodiscover docs with the new resources #15055

Merged

ChrsMark mentioned this pull request Dec 16, 2019

Cherry-pick #15055 to 7.x: Update Kubernetes Autodiscover docs with the new resources #15116

Merged

andresrc added the needs testing notes label Jan 8, 2020

exekias added the needs_backport PR is waiting to be backported to other branches. label Jan 10, 2020

exekias removed the needs_backport PR is waiting to be backported to other branches. label Jan 10, 2020

exekias mentioned this pull request Jan 14, 2020

Add option for global kubernetes autodiscover #10578

Closed

andresrc assigned odacremolbap and unassigned exekias Jan 14, 2020

odacremolbap mentioned this pull request Jan 29, 2020

Kubernetes: node autodiscover, add InternalIP #15930

Merged

odacremolbap added the test-plan-ok This PR passed manual testing label Jan 30, 2020

This was referenced Jan 31, 2020

Cherry-pick #15930 to 7.x: Kubernetes: node autodiscover, add InternalIP #15989

Merged

Kubernetes autodiscover: add/restructure variables from resources #16059

Closed

vjsamuel deleted the refactor_k8s_autodiscover branch April 7, 2020 04:40

david-kow mentioned this pull request Jun 9, 2020

Using host instead of node in the config elastic/cloud-on-k8s#3206

Merged

jsoriano mentioned this pull request Jun 30, 2020

Proposal: Cache metadata to local disk/db #19511

Closed

	`scope`:: (Optional) Specify at what level autodiscovery needs to be done at. `scope` can
	`scope`:: (Optional) Specify at what level autodiscover needs to be done at. `scope` can

		the specified node. `cluster` scope allows cluster wide discovery. `pod` and `node` are
		only resources that can be discovered at `node` scope.

Refactor kubernetes autodiscover to enable different resource based discovery #14738

Refactor kubernetes autodiscover to enable different resource based discovery #14738

Conversation

vjsamuel commented Nov 25, 2019 • edited by zube bot Loading

elasticmachine commented Nov 25, 2019

elasticmachine commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Nov 25, 2019

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

exekias commented Nov 28, 2019

exekias 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

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

exekias commented Nov 28, 2019 • edited Loading

exekias left a comment

Choose a reason for hiding this comment

ChrsMark left a comment

Choose a reason for hiding this comment

ChrsMark commented Jan 10, 2020

Manual Testing

exekias commented Jan 10, 2020 • edited Loading

odacremolbap commented Jan 24, 2020

nerddelphi commented Feb 20, 2020

vjsamuel commented Nov 25, 2019 •

edited by zube bot

Loading

exekias commented Nov 28, 2019 •

edited

Loading

exekias commented Jan 10, 2020 •

edited

Loading