[Metricbeat autodiscover][Provider Kubernetes] Add condition to node/namespace watchers #37181

constanca-m · 2023-11-22T13:10:47Z

Proposed commit message

WHAT & WHY: the problem is explained in detail in [Metricbeat Autodiscover] Provider Kubernetes always creates watcher for namespaces and nodes #37179.

To disable the watchers we need to do both of these:

Disable hints:

hints.enabled: true

Disable resource metadata:

add_resource_metadata:
  namespace.enabled: false

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Follow the instructions in this file.

Make sure that at least namespace / node resources permissions are not granted cluster wide.
Run metricbeat without this permission and you should no longer see any error.

Related issues

Relates to [Metricbeat Autodiscover] Provider Kubernetes always creates watcher for namespaces and nodes #37179

Logs

The error

W1122 12:17:27.683394      23 reflector.go:324] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: failed to list *v1.Namespace: namespaces is forbidden: User "system:serviceaccount:kube-system:metricbeat" cannot list resource "namespaces" in API group "" at the cluster scope

is no longer present in the logs.

ChrsMark

Let's ensure that node and namespace settings are preserved with the patch.

ref: https://www.elastic.co/guide/en/beats/metricbeat/current/configuration-autodiscover.html#_kubernetes.

libbeat/autodiscover/providers/kubernetes/pod.go

gizas · 2023-11-22T13:41:42Z

Please add a new entry changelog for this PR.

Also this PR needs to be backported to 8.11 and 7.17 as it is a bug

constanca-m · 2023-11-22T13:45:12Z

Also this PR needs to be backported to 8.11 and 7.17 as it is a bug

The backports are there.

I will add an entry in changelog

elasticmachine · 2023-11-22T13:57:53Z

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-22T13:11:05.530+0000
Duration: 47 min 33 sec

Test stats 🧪

Test	Results
Failed	2
Passed	21723
Skipped	1582
Total	23307

Test errors

Expand to view the tests failures

`Build&Test / libbeat-unitTest / [empty] – TEST-go-unit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/PR-37181-1-ab3df46e-de8f-4ed2-be7d-d6815a8d393b/src/github.com/elastic/beats/build/libbeat/build/TEST-go-unit.xml was length 0

`Build&Test / libbeat-goIntegTest / [empty] – TEST-go-integration.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/PR-37181-1-f5919e51-b99e-4342-9f33-d79361e78ce5/src/github.com/elastic/beats/build/libbeat/build/TEST-go-integration.xml was length 0

Steps errors

Expand to view the steps failures

`Error signal`

Took 0 min 0 sec . View more details here
Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

CHANGELOG-developer.next.asciidoc

Co-authored-by: Chris Mark <[email protected]>

elasticmachine · 2023-11-22T15:28:21Z

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-22T13:47:44.893+0000
Duration: 100 min 22 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28258
Skipped	2015
Total	30273

Steps errors

Expand to view the steps failures

`Error signal`

Took 0 min 0 sec . View more details here
Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-11-22T17:34:09Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-22T15:20:48.444+0000
Duration: 133 min 5 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28692
Skipped	2015
Total	30707

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

libbeat/autodiscover/providers/kubernetes/service.go

ChrsMark · 2023-11-22T19:02:01Z

libbeat/autodiscover/providers/kubernetes/pod.go

 		updater := kubernetes.NewNodePodUpdater(p.unlockedUpdate, watcher.Store(), &p.crossUpdate)
 		nodeWatcher.AddEventHandler(updater)
 	}

-	if namespaceWatcher != nil && (config.Hints.Enabled() || metaConf.Namespace.Enabled()) {
+	if namespaceWatcher != nil && config.Hints.Enabled() {


Now that I see this line again I realize that metaConf.Namespace/Node was not checked in order to initialize the watchers above because these Watchers are used for Hint's based autodiscovery anyways no matter what the add_resource_metadata defines.

Let's take for example the namespaceWatcher:
We want to trigger the updater even if metaConf.Namespace.Enabled()!=true in case of having Hints enabled. Some details can be found at #25117.

So I believe we need to rethink of this patch more carefully. I see 2 options here:

We initialize the watchers if config.Hints.Enabled() || metaConf.Namespace.Enabled() anyways in order to have the hints to work as expected.

Introduce new settings to disable the Watchers only for users that actually don't have permissions to watch on Namespaces and/or Nodes.

The point is that we cannot couple Hint's based autodiscovery functionality with the add_resource_metadata setting. That's why we had this || in this if statement here.

Most probably that's why we fetch the namespace Annotations at

beats/libbeat/autodiscover/providers/kubernetes/pod.go

Line 330 in 8effe24

namespaceAnnotations := kubernetes.PodNamespaceAnnotations(pod, p.namespaceWatcher)

using the Watcher.

A real use case would be the following:

a) the users have disabled the namespace metadata enrichment with add_resource_metadata.namespace.enabled: false
b) but they want to have hints' based autodiscovery based on Namespace's annotations (see https://www.elastic.co/guide/en/beats/metricbeat/current/configuration-autodiscover-hints.html#_namespace_defaults)

The hints' events won't be complete because we won't have Namespace annotations at

beats/libbeat/autodiscover/providers/kubernetes/pod.go

Line 399 in 8effe24

kubemeta["namespace_annotations"] = namespaceAnnotations

.

I might miss sth here but my point is that we need to revisit this seeing the big picture and then decide accordingly. As we can see there are lot's of different pieces affected here.

We initialize the watchers if config.Hints.Enabled() || metaConf.Namespace.Enabled() anyways in order to have the hints to work as expected.

I like this option, because this way we can still stop the watchers with hints.enabled: false. The main problem as it is now is that we have no way to stop them.
We would have to update the documentation for Autodiscover with this, as we never mention the option hints (only at this page).

Introduce new settings to disable the Watchers only for users that actually don't have permissions to watch on Namespaces and/or Nodes.

Maybe the user would have way too many options when we can already use the ones available.

A real use case would be the following:

Thanks, it is clear 👍

We initialize the watchers if config.Hints.Enabled() || metaConf.Namespace.Enabled() anyways in order to have the hints to work as expected.

I commited new changes so now it works like this.

Also somehow related: #34717

ChrsMark · 2023-11-22T19:26:08Z

The PR's description mention the following:

Make sure that at least namespace / node resources permissions are not granted cluster wide.
Run metricbeat without this permission and you should no longer see any error.

However for changes on codebase that affect multiple features we can cannot just rely on verifying if a bug or specific thing is fixed/works. From the conversation it's obvious that proper e2e testing is missing here in order to catch possible regressions like the one mentioned at #37181 (comment).

@gizas @bturquet that is sth I suggest we should take into account some time soon.

elasticmachine · 2023-11-23T07:24:34Z

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-23T07:14:44.185+0000
Duration: 10 min 44 sec

Steps errors

Expand to view the steps failures

`Error signal`

Took 0 min 0 sec . View more details here
Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

ChrsMark · 2023-11-23T07:25:07Z

libbeat/autodiscover/providers/kubernetes/pod.go

 	// in order to be able to retrieve 2nd layer Owner metadata like in case of:
 	// Deployment -> Replicaset -> Pod
 	// CronJob -> job -> Pod
 	if metaConf.Deployment {
-		replicaSetWatcher, err = kubernetes.NewNamedWatcher("resource_metadata_enricher_rs", client, &kubernetes.ReplicaSet{}, kubernetes.WatchOptions{
+		options = kubernetes.WatchOptions{


Minor suggestion since we touch this codebase: Would it make sense to use the same Namespace scope setting with the one used for the Pod watcher?

If we watch for Pods on specific Namespace then their parent Deployments and CronJobs will be on the same Namespace as well.

constanca-m · 2023-11-23T08:04:13Z

From the conversation it's obvious that proper e2e testing is missing here in order to catch possible regressions like the one mentioned at #37181 (comment).

I could implement some tests with all the options (hints enabled, add resource metadata enabled or none) to check if the watchers exist or not. I don't think much else could be done on this part of the code. What do you think? Is it worth to check if it is nil or do we leave the test files as they are @ChrsMark ?

Edit: I added the test to check the conditions.

elasticmachine · 2023-11-23T08:50:13Z

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-23T07:19:30.327+0000
Duration: 90 min 29 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28260
Skipped	2015
Total	30275

Steps errors

Expand to view the steps failures

`Error signal`

Took 0 min 0 sec . View more details here
Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-11-23T10:55:57Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Duration: 132 min 16 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

constanca-m · 2023-11-30T09:31:10Z

I am closing this PR for now based on the discussion held yesterday. It seems the watchers issue is more complex, and this implementation needs to be changed.

Add condition to node/namespace watchers.

510a67a

constanca-m added backport-v7.11.0 Automated backport with mergify Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team backport-v8.11.0 Automated backport with mergify labels Nov 22, 2023

constanca-m self-assigned this Nov 22, 2023

constanca-m requested a review from a team as a code owner November 22, 2023 13:10

constanca-m requested review from ycombinator and rdner November 22, 2023 13:10

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 22, 2023

constanca-m requested review from a team, ChrsMark and tetianakravchenko and removed request for a team November 22, 2023 13:11

constanca-m mentioned this pull request Nov 22, 2023

[Metricbeat Autodiscover] Provider Kubernetes always creates watcher for namespaces and nodes #37179

Closed

3 tasks

ChrsMark requested changes Nov 22, 2023

View reviewed changes

libbeat/autodiscover/providers/kubernetes/pod.go Show resolved Hide resolved

ChrsMark requested review from gizas and MichaelKatsoulis November 22, 2023 13:36

Add CHANGELOG-developer.next.asciidoc entry

0a75329

ChrsMark reviewed Nov 22, 2023

View reviewed changes

CHANGELOG-developer.next.asciidoc Outdated Show resolved Hide resolved

Update CHANGELOG-developer.next.asciidoc

a7b5824

Co-authored-by: Chris Mark <[email protected]>

ChrsMark reviewed Nov 22, 2023

View reviewed changes

libbeat/autodiscover/providers/kubernetes/service.go Outdated Show resolved Hide resolved

ChrsMark reviewed Nov 22, 2023

View reviewed changes

Update options for namespace and node watchers.

0e3091a

Fix service metagen.

fd9a2a4

ChrsMark reviewed Nov 23, 2023

View reviewed changes

Add test on conditions for namespace/node watchers.

f06aebe

constanca-m added backport-7.17 Automated backport to the 7.17 branch with mergify and removed backport-v7.11.0 Automated backport with mergify labels Nov 23, 2023

constanca-m closed this Nov 30, 2023

constanca-m deleted the namespace-watchers branch December 7, 2023 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metricbeat autodiscover][Provider Kubernetes] Add condition to node/namespace watchers #37181

[Metricbeat autodiscover][Provider Kubernetes] Add condition to node/namespace watchers #37181

constanca-m commented Nov 22, 2023 •

edited

Loading

ChrsMark left a comment •

edited

Loading

gizas commented Nov 22, 2023

constanca-m commented Nov 22, 2023

elasticmachine commented Nov 22, 2023

Build stats

Test stats 🧪

`Build&Test / libbeat-unitTest / [empty] – TEST-go-unit.xml`

`Build&Test / libbeat-goIntegTest / [empty] – TEST-go-integration.xml`

`Error signal`

elasticmachine commented Nov 22, 2023

Build stats

Test stats 🧪

`Error signal`

elasticmachine commented Nov 22, 2023

Build stats

Test stats 🧪

ChrsMark Nov 22, 2023 •

edited

Loading

constanca-m Nov 23, 2023

constanca-m Nov 23, 2023

ChrsMark Nov 29, 2023

ChrsMark commented Nov 22, 2023 •

edited

Loading

elasticmachine commented Nov 23, 2023

Build stats

`Error signal`

ChrsMark Nov 23, 2023 •

edited

Loading

constanca-m commented Nov 23, 2023 •

edited

Loading

elasticmachine commented Nov 23, 2023

Build stats

Test stats 🧪

`Error signal`

elasticmachine commented Nov 23, 2023

Build stats

constanca-m commented Nov 30, 2023

[Metricbeat autodiscover][Provider Kubernetes] Add condition to node/namespace watchers #37181

[Metricbeat autodiscover][Provider Kubernetes] Add condition to node/namespace watchers #37181

Conversation

constanca-m commented Nov 22, 2023 • edited Loading

Proposed commit message

Checklist

How to test this PR locally

Related issues

Logs

ChrsMark left a comment • edited Loading

Choose a reason for hiding this comment

gizas commented Nov 22, 2023

constanca-m commented Nov 22, 2023

elasticmachine commented Nov 22, 2023

❕ Build Aborted

Build stats

Test stats 🧪

Test errors

Build&Test / libbeat-unitTest / [empty] – TEST-go-unit.xml

Build&Test / libbeat-goIntegTest / [empty] – TEST-go-integration.xml

Steps errors

Error signal

🤖 GitHub comments

elasticmachine commented Nov 22, 2023

❕ Build Aborted

Build stats

Test stats 🧪

Steps errors

Error signal

🤖 GitHub comments

elasticmachine commented Nov 22, 2023

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

ChrsMark Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

A real use case would be the following:

constanca-m Nov 23, 2023

Choose a reason for hiding this comment

constanca-m Nov 23, 2023

Choose a reason for hiding this comment

ChrsMark Nov 29, 2023

Choose a reason for hiding this comment

ChrsMark commented Nov 22, 2023 • edited Loading

elasticmachine commented Nov 23, 2023

❕ Build Aborted

Build stats

Steps errors

Error signal

🤖 GitHub comments

ChrsMark Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

constanca-m commented Nov 23, 2023 • edited Loading

elasticmachine commented Nov 23, 2023

❕ Build Aborted

Build stats

Test stats 🧪

Steps errors

Error signal

🤖 GitHub comments

elasticmachine commented Nov 23, 2023

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

constanca-m commented Nov 30, 2023

constanca-m commented Nov 22, 2023 •

edited

Loading

ChrsMark left a comment •

edited

Loading

`Build&Test / libbeat-unitTest / [empty] – TEST-go-unit.xml`

`Build&Test / libbeat-goIntegTest / [empty] – TEST-go-integration.xml`

`Error signal`

`Error signal`

ChrsMark Nov 22, 2023 •

edited

Loading

ChrsMark commented Nov 22, 2023 •

edited

Loading

`Error signal`

ChrsMark Nov 23, 2023 •

edited

Loading

constanca-m commented Nov 23, 2023 •

edited

Loading

`Error signal`