[Kubernetes] Update namespace filter on data streams #10213

constanca-m · 2024-06-21T14:11:59Z

Proposed commit message

Some datastreams have present the namespace filter. I will update it to:

      - name: namespace
        type: text
        title: Namespace
        description: Enrich events with metadata from the resources in this namespace. Empty value means all namespaces.
        multi: false
        required: false
        show_user: false

The namespace filter should be moved to the group level.

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

Build Kubernetes integration:

elastic-package build

Update registry:

elastic-package stack up --services package-registry

Install elastic agent with image 8.15.0-SNAPSHOT. It should use a policy with Kubernetes integration.

Related issues

Relates elastic/beats#38978

Results

See #10213 (comment).

constanca-m · 2024-06-25T13:54:53Z

/test

gizas · 2024-06-25T14:35:57Z

packages/kubernetes/data_stream/state_persistentvolume/manifest.yml

@@ -41,12 +41,6 @@ streams:
        multi: false
        required: false
        show_user: false
-      - name: namespace


We needed here and not in the claim right?
See
https://github.com/elastic/beats/pull/39934/files#diff-1d7b17f1ef239e6177db5ee660f2234bfd69c82e93494d4e46ee97f8a8617cfdR285

gizas · 2024-06-25T14:36:12Z

packages/kubernetes/data_stream/state_persistentvolumeclaim/manifest.yml

@@ -43,7 +43,8 @@ streams:
        show_user: false
      - name: namespace
        type: text
-        title: Namespace to watch resources from for metadata
+        title: Namespace


So here not needed

PVC needs it, PV does not

packages/kubernetes/data_stream/state_statefulset/manifest.yml

packages/kubernetes/data_stream/state_resourcequota/manifest.yml

MichaelKatsoulis · 2024-06-26T07:57:26Z

packages/kubernetes/manifest.yml

@@ -10,7 +10,7 @@ categories:
  - kubernetes
 conditions:
  kibana:
-    version: "^8.14.0"
+    version: "^8.15.0"


Why? This will work for 8.14 as well, with only exception of namespace watcher. But it is not a feature of 8.15.

You are right, but in that case 8.14 already has the namespace filters for most of data streams. I wanted to push for 8.15 to be on track with the other related PRs. Should I fix in 8.14 instead - and maybe remove namespace filter for state_namespace?

This would require then another PR. I would suggest to keep 8.15 but only merge it after the feature freeze of 8.15

but only merge it after the feature freeze of 8.15

Yes, I agree. I have added a reminder for the day of the release of 8.15, so we won't have to backport in integrations

tetianakravchenko · 2024-06-26T07:58:46Z

packages/kubernetes/data_stream/container/manifest.yml

@@ -60,6 +60,13 @@ streams:
        multi: true
        required: false
        show_user: false
+      - name: namespace


should we also add namespace to the top level? similar to the condition

integrations/packages/kubernetes/manifest.yml

Line 40 in 95efd07

- name: condition

?

I am not sure. Some data streams have the option node that I considered at the same level as namespace, and node is also not at the top level. Should I change this?

Do we need to put the namespace filter in each data stream? Can't we put in the group level and check if it applies for all the data streams of the group?
It feels a bit cumbersome for a user to set the namespace value so many times. And a mistake in one of the data streams would break the others as well, because the watchers are shared.

I agree with @MichaelKatsoulis on this regard, that we shouldn't provide the option to set it for each datastream, but on the higher level instead to avoid possible issues, since watchers are shared.

I would split node and namespace work and don't touch node in this PR

I replied in #10213 (comment).

packages/kubernetes/changelog.yml

MichaelKatsoulis · 2024-06-26T08:15:07Z

Do we need to put the namespace filter in each data stream? Can't we put in the group level and check if it applies for all the data streams of the group?
It feels a bit cumbersome for a user to set the namespace value so many times. And a mistake in one of the data streams would break the others as well, because the watchers are shared.
Of course, we should check the update process and how a value in namespace set in a previous version would or not be carried to the newer version. This could be a breaking change....

About the text Enrich events with metadata from the resources in this namespace. Empty value means all namespaces.
I would change it to Enrich events from resources of the selected namespace with kubernetes metadata. Empty value means all namespaces.

Also for this PR to be ready for merge, it should be tested that setting the namespace field does what it supposes to.
We should have screenshots of metadata added only for resources in a selected namespace.

Co-authored-by: Tetiana Kravchenko <[email protected]>

constanca-m · 2024-06-28T12:18:05Z

Test for using namespace inside data stream cronjob:

1st test

Setting namespace: default:

We see that the namespace_uid is only added to cronjobs in default namespace.

Removing namespace: default:

The results are still the same as 1. So I don't think the watchers are updated to all namespaces.

2nd test

I am restarting the agent. I don't have the namespace set:

The results are as expected.

I will now filter by namespace: default, without restarting the agent again:

Conclusion: The watchers are only updated when the agent is restarted.

constanca-m · 2024-06-28T12:25:16Z

Do we need to put the namespace filter in each data stream? Can't we put in the group level and check if it applies for all the data streams of the group?

I have just tried this:

With the same tests as #10213 (comment).

And the results:

So it does not work @MichaelKatsoulis @tetianakravchenko. What do you think?

constanca-m · 2024-06-28T12:27:36Z

It seems that having the namespace option in integrations will bring quite a few problems @gizas @tetianakravchenko @MichaelKatsoulis . The watchers are shared, so I could start the Pod watcher in data stream pod from namespace-A, and then for state_container for namespace-B. What then? It does not work, maybe this option should not be available here.

MichaelKatsoulis · 2024-07-01T08:20:59Z

The watchers are shared, so I could start the Pod watcher in data stream pod from namespace-A, and then for state_container for namespace-B. What then?

@constanca-m Yes this is tricky and error prone. That is why I believe the way to do this, is to have the namespace option in group level as you tried in #10213 (comment).

If you add the namespace in the group level in the

integrations/packages/kubernetes/manifest.yml

Line 24 in c2ceb26

policy_templates:

, then I believe you should go to the data streams that the namespace option is needed and configure the handlebar files like:

{{#if namespace}}
namespace: {{namespace}}
{{/if}}

Looking a the files changed in this PR, I don't see any update in the handlebar files? How will the namespace option be passed to the metricsets? For example in state_pod there is already there

constanca-m · 2024-07-02T06:50:06Z

@MichaelKatsoulis I tried setting the namespace per group. I reported the results in #10213 (comment). It does not work, so unfortunately that is not an option. The only way it works is to set the namespace per metricset, which is very prone to errors.

constanca-m · 2024-07-02T07:12:57Z

Looking a the files changed in this PR, I don't see any update in the handlebar files?

Thank you, I wasn't aware of those files. I have updated in the latest commit.

Example of adding namespace option to pod data stream:

And we can see the option is no longer there for state node too:

Same for other data streams.

MichaelKatsoulis · 2024-07-02T07:41:45Z

@MichaelKatsoulis I tried setting the namespace per group. I reported the results in #10213 (comment). It does not work, so unfortunately that is not an option. The only way it works is to set the namespace per metricset, which is very prone to errors.

It wasn't working because you hadn't set the namespace to all hbs files probably. If the namespace var is per group, then the hbs file can read the variable as they already do with condition var.
Also the reason that an agent restart is needed when you make a change in the namespace option, is because in https://github.com/elastic/beats/blob/53bf67ce40fe6564efd15f25cd6da0249f360a12/metricbeat/module/kubernetes/util/kubernetes.go#L343 we only check for a change in the nodescope to update a watcher.
Changing a data streams only re renders that part of the agent config. The watchers of the module have already started with the previous namespace option, so they don't get restarted.
I think that is fine.

constanca-m · 2024-07-02T07:54:47Z

It wasn't working because you hadn't set the namespace to all hbs files probably.

I don't think this is it. I tried with state_cronjob with the hbs file and it does not work.

constanca-m · 2024-07-02T12:24:08Z

It wasn't working because you hadn't set the namespace to all hbs files probably.

Thank you @MichaelKatsoulis , this was the problem.

I deployed 2 cronjobs in default and kube-system namespace. I set the integration for state data streams like this:

I checked the data from the state_cronjob data stream, and only the default cronjob had metadata as expected:

MichaelKatsoulis · 2024-07-09T08:30:38Z

packages/kubernetes/data_stream/event/manifest.yml

@@ -31,6 +31,13 @@ streams:
        required: true
        show_user: true
        default: true
+      - name: namespace


In order to be more aligned with the rest of the data stream groups, we should also add the namespace option in the central manifest in the events group.

- name: events title: Kubernetes Event Metrics

This group only contains events data stream but as we do it for the rest, we should be uniform.

Done in the latest commit.

MichaelKatsoulis · 2024-07-09T08:45:59Z

@lucabelluccini I wanted to inform you that with this PR we introduce a small breaking change to Kubernetes Integration.

This is actual a fix as the way a parameter was set in the data streams until 1.63.0 version would not work as expected due to changes (for cpu and memory consumption improvements) we have in beats 8.14.0 version.

In more details some of the data streams of Kubernetes Integration included a namespace option parameter which was supposed to configure the following:

title: Namespace
description: Enrich events with metadata from the resources in this namespace. Empty value means all namespaces.

The problem was that this option even if set, would be overwritten by the other data streams(which did not even had this option), due to the underlying architecture of the metadata enrichment process with kubernetes api watchers mechanism.

To tackle this issue, we decided to move the namespace option to the group level of data streams(wherever it makes sense).
Now the user can set the namespace option in a more central way, and thus not get overriden from other data streams.

The breaking change would be that, if a user has a previous version of kubernetes integration where in some data streams the namespace option is set (although not working properly. We really expect very few users to use this), and then decides to upgrade to this new version. This would mean that the namespace option will not be inherited. They will need to set it properly in the group level.
We really thing that this change is important because:

If a user does not have permissions to access another namespace other than default (or whatever) , then by that option they can stop unwanted calls to the rest of the namespaces which would cause error logs.

Should we include this information somewhere in order for support to have in mind in an unlike future SDH?

MichaelKatsoulis · 2024-07-09T12:28:28Z

@constanca-m we should create a follow up issue to check the right place for node option as well.
Currently it is in state data streams like it was with namespace option before

…-filter

elasticmachine · 2024-07-15T14:30:12Z

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

tetianakravchenko · 2024-07-15T14:49:49Z

packages/kubernetes/changelog.yml

@@ -1,4 +1,9 @@
 # newer versions go on top
+- version: 1.64.0
+  changes:
+    - description: Update namespace filter on data streams


it should be reflected in the description that namespace filter was moved to the group level

packages/kubernetes/changelog.yml

Co-authored-by: Tetiana Kravchenko <[email protected]>

elasticmachine · 2024-07-16T06:58:08Z

💚 Build Succeeded

Buildkite Build
Commit: 5d56ae1

History

💚 Build #13676 succeeded dde85a9
💚 Build #13672 succeeded 63e6bee
💔 Build #13403 failed 8f52dc9
💔 Build #13367 failed ea8a48e
💔 Build #13130 failed cebd37a
💔 Build #13095 failed d1999d5

cc @constanca-m

elastic-sonarqube · 2024-07-16T06:58:12Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
82.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

elasticmachine · 2024-07-16T07:12:49Z

Package kubernetes - 1.64.0 containing this change is available at https://epr.elastic.co/search?package=kubernetes

* Update namespace filter Co-authored-by: Tetiana Kravchenko <[email protected]> --------- Co-authored-by: Tetiana Kravchenko <[email protected]>

Update namespace filter

27d1a53

constanca-m added Integration:kubernetes Kubernetes Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team [elastic/obs-cloudnative-monitoring] labels Jun 21, 2024

constanca-m self-assigned this Jun 21, 2024

constanca-m requested a review from a team as a code owner June 21, 2024 14:12

constanca-m requested review from gizas and tetianakravchenko June 21, 2024 14:12

Update changelog.

95efd07

constanca-m mentioned this pull request Jun 21, 2024

[META][Kubernetes provider/Kubernetes module] Set watcher options namespace based on configuration elastic/beats#38978

Closed

6 tasks

gizas reviewed Jun 25, 2024

View reviewed changes

packages/kubernetes/data_stream/state_statefulset/manifest.yml Show resolved Hide resolved

MichaelKatsoulis reviewed Jun 26, 2024

View reviewed changes

packages/kubernetes/data_stream/state_resourcequota/manifest.yml Show resolved Hide resolved

MichaelKatsoulis reviewed Jun 26, 2024

View reviewed changes

tetianakravchenko reviewed Jun 26, 2024

View reviewed changes

Update packages/kubernetes/changelog.yml

5e5cac0

Co-authored-by: Tetiana Kravchenko <[email protected]>

Add namespace to yml.hbs

7f955e8

Merge branch 'main' into namespace-filter

d1999d5

elastic deleted a comment from elasticmachine Jul 2, 2024

Add namespace to group

62c577e

constanca-m and others added 2 commits July 2, 2024 14:25

Update kibana version

cebd37a

Merge branch 'main' into namespace-filter

ea8a48e

MichaelKatsoulis reviewed Jul 9, 2024

View reviewed changes

move namespace to events group

8f52dc9

constanca-m and others added 3 commits July 15, 2024 15:46

Merge branch 'main' into namespace-filter

9614a50

revert change

91c635a

Merge remote-tracking branch 'origin/namespace-filter' into namespace…

63e6bee

…-filter

constanca-m mentioned this pull request Jul 15, 2024

[META][Kubernetes provider/Kubernetes module] Set watcher options node based on configuration elastic/beats#40246

Open

5 tasks

gizas approved these changes Jul 15, 2024

View reviewed changes

tetianakravchenko reviewed Jul 15, 2024

View reviewed changes

update changelog.yml

dde85a9

tetianakravchenko approved these changes Jul 15, 2024

View reviewed changes

packages/kubernetes/changelog.yml Outdated Show resolved Hide resolved

Update packages/kubernetes/changelog.yml

5d56ae1

Co-authored-by: Tetiana Kravchenko <[email protected]>

constanca-m enabled auto-merge (squash) July 16, 2024 06:14

MichaelKatsoulis approved these changes Jul 16, 2024

View reviewed changes

constanca-m merged commit 3f34e40 into elastic:main Jul 16, 2024
3 checks passed

constanca-m deleted the namespace-filter branch July 16, 2024 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kubernetes] Update namespace filter on data streams #10213

[Kubernetes] Update namespace filter on data streams #10213

constanca-m commented Jun 21, 2024 •

edited

Loading

constanca-m commented Jun 25, 2024

gizas Jun 25, 2024

gizas Jun 25, 2024

constanca-m Jun 26, 2024

MichaelKatsoulis Jun 26, 2024

constanca-m Jun 26, 2024

MichaelKatsoulis Jun 26, 2024

constanca-m Jun 26, 2024

tetianakravchenko Jun 26, 2024 •

edited

Loading

constanca-m Jun 26, 2024

tetianakravchenko Jun 27, 2024

constanca-m Jun 28, 2024

MichaelKatsoulis commented Jun 26, 2024

constanca-m commented Jun 28, 2024

constanca-m commented Jun 28, 2024

constanca-m commented Jun 28, 2024

MichaelKatsoulis commented Jul 1, 2024

constanca-m commented Jul 2, 2024

constanca-m commented Jul 2, 2024

MichaelKatsoulis commented Jul 2, 2024

constanca-m commented Jul 2, 2024

constanca-m commented Jul 2, 2024

MichaelKatsoulis Jul 9, 2024

constanca-m Jul 9, 2024

MichaelKatsoulis commented Jul 9, 2024

MichaelKatsoulis commented Jul 9, 2024

elasticmachine commented Jul 15, 2024

tetianakravchenko Jul 15, 2024

constanca-m Jul 15, 2024

elasticmachine commented Jul 16, 2024

elastic-sonarqube bot commented Jul 16, 2024

elasticmachine commented Jul 16, 2024

[Kubernetes] Update namespace filter on data streams #10213

[Kubernetes] Update namespace filter on data streams #10213

Conversation

constanca-m commented Jun 21, 2024 • edited Loading

Proposed commit message

Checklist

How to test this PR locally

Related issues

Results

constanca-m commented Jun 25, 2024

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

tetianakravchenko Jun 26, 2024 • 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

MichaelKatsoulis commented Jun 26, 2024

constanca-m commented Jun 28, 2024

constanca-m commented Jun 28, 2024

constanca-m commented Jun 28, 2024

MichaelKatsoulis commented Jul 1, 2024

constanca-m commented Jul 2, 2024

constanca-m commented Jul 2, 2024

MichaelKatsoulis commented Jul 2, 2024

constanca-m commented Jul 2, 2024

constanca-m commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelKatsoulis commented Jul 9, 2024

MichaelKatsoulis commented Jul 9, 2024

elasticmachine commented Jul 15, 2024

🚀 Benchmarks report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 16, 2024

💚 Build Succeeded

History

elastic-sonarqube bot commented Jul 16, 2024

Quality Gate passed

elasticmachine commented Jul 16, 2024

constanca-m commented Jun 21, 2024 •

edited

Loading

tetianakravchenko Jun 26, 2024 •

edited

Loading