[Metricbeat][Kubernetes] Remove mandatory permissions for namespace and node by constanca-m · Pull Request #38762 · elastic/beats

constanca-m · 2024-04-08T10:29:48Z

Proposed commit message

Issue of the PR is #37179.

Currently, we need to have permissions for namespace and nodes, because we create watchers for these resources.
This makes the option add_resource_metadata.*.enabled useless in this case.

To fix this issue, we need to prevent the creation of watchers in two places:

When using autodiscover
- In this case, we also need to make sure hints.enabled is set to false. Otherwise, the watchers will be created.
When creating the enrichers
- Only create the namespace watcher if add_resource_metadata.namespace.enabled is set to true
- Do the same for node
- Exception: if state_namespace is enabled, then we create the watcher for that resource, regardless of the block add_resource_metadata. Same for node metricsets.

This means having this in the configuration of the kubernetes provider:

add_resource_metadata:
    namespace:
      enabled: false
      node: false

And making sure:

hints.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

Clone this branch.
Deploy the metricbeat built from this branch in a kubernetes cluster.
Make sure to remove namespace permissions from the ClusterRole.

Configure the provider as:

metricbeat.autodiscover:
  providers:
    - type: kubernetes
      hints.enabled: false
      add_resource_metadata:
        namespace:
          enabled: false
        node:
          enabled: false

Results

We no longer have logs with:

W0408 10:14:45.477767       1 reflector.go:324] pkg/mod/k8s.io/client-go@v0.23.4/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
E0408 10:14:45.477909       1 reflector.go:138] pkg/mod/k8s.io/client-go@v0.23.4/tools/cache/reflector.go:167: Failed to watch *v1.Namespace: 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

Related issues

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

Signed-off-by: constanca <constanca.manteigas@elastic.co>

mergify · 2024-04-08T10:30:34Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @constanca-m? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

Signed-off-by: constanca <constanca.manteigas@elastic.co>

mergify · 2024-04-08T10:31:50Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b namespace-permissions upstream/namespace-permissions
git merge upstream/main
git push upstream namespace-permissions

elasticmachine · 2024-04-08T10:38:39Z

💚 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: 172 min 32 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!)

Signed-off-by: constanca <constanca.manteigas@elastic.co>

…space-permissions

libbeat/autodiscover/providers/kubernetes/pod_test.go

tetianakravchenko · 2024-04-08T13:42:18Z

metricbeat/module/kubernetes/util/kubernetes_test.go

 	log := logp.NewLogger("test")
+
+	namespaceConfig, err := conf.NewConfigFrom(map[string]interface{}{
+		"enabled": true,


is it needed to set explicitly? by default it is already true, no?

By default is true, yes, but in this case we need to at it explicitly because the config is not unpacking the default one, like we now are doing for pod_test and service_test.

Signed-off-by: constanca <constanca.manteigas@elastic.co>

gizas · 2024-04-16T07:15:34Z

metricbeat/module/kubernetes/util/kubernetes.go

+		if addResourceMetadata.Node.Enabled() {
+			extra = append(extra, NodeResource)
+		}
+		if addResourceMetadata.Namespace.Enabled() {


This made think of the scenario:
a) kubernetes.provider has add_resource.metadata.namespace.enabled: false and the module add_resource.metadata.namespace.enabled: true --> This means that the watcher will start. Always the module config is more specific correct?

Yes, I believe the flow is provider > module > metricset.

If you are talking about kubernetes module, it does not use the kubernetes provider. The kubernetes autodiscover provider can be used to start different modules (like nginx based on hints or templates) and also is used in log collection.

gizas · 2024-04-16T07:22:58Z

libbeat/autodiscover/providers/kubernetes/pod.go

-	if err != nil {
-		logger.Errorf("couldn't create watcher for %T due to error %+v", &kubernetes.Namespace{}, err)
+
+	if metaConf.Namespace.Enabled() || config.Hints.Enabled() {


Do we actually need config.Hints.Enabled()?
Especially that now metaConf.Namespace has default value true?

Maybe not, but we had this before and I think changing it could be a breaking change, correct?

constanca-m · 2024-04-16T11:48:50Z

After today's meeting, we decided the following:

The resource of each metricset takes priority over the metadata. This means that:
- If namespace metadata is disabled and state_namespace is enabled, then the namespace watcher is created
- If node metadata is disabled and state_node or node is enabled, then the node watcher is created

MichaelKatsoulis · 2024-04-18T10:42:45Z

@constanca-m The add_kubernetes_metadata processor also includes the add_resource_metdata in its configuration. The processor also creates namespaceWatcher in all cases . We should tackle this as well.

beats/libbeat/processors/add_kubernetes_metadata/kubernetes.go

Line 210 in 0d3a8b6

    
           namespaceWatcher, err := kubernetes.NewNamedWatcher("add_kubernetes_metadata_namespace", client, &kubernetes.Namespace{}, kubernetes.WatchOptions{

Signed-off-by: constanca <constanca.manteigas@elastic.co>

constanca-m · 2024-04-18T10:48:20Z

Thank you @MichaelKatsoulis. I added the condition for both namespace and node watchers in the latest commit.

CHANGELOG-developer.next.asciidoc

Co-authored-by: Michael Katsoulis <michaelkatsoulis88@gmail.com>

MichaelKatsoulis

Good to go. Just a small change in the changelog description.

blakerouse

Overall change is mechanical. Looks good to me!

remove mandatory permissions

9f18a2e

Signed-off-by: constanca <constanca.manteigas@elastic.co>

constanca-m added enhancement Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team labels Apr 8, 2024

constanca-m self-assigned this Apr 8, 2024

constanca-m requested review from a team as code owners April 8, 2024 10:29

constanca-m requested review from MichaelKatsoulis, faec, gsantoro and leehinman April 8, 2024 10:29

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 Apr 8, 2024

constanca-m removed the enhancement label Apr 8, 2024

update CHANGELOG-developer.next.asciidoc

ed53fc6

Signed-off-by: constanca <constanca.manteigas@elastic.co>

constanca-m mentioned this pull request Apr 8, 2024

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

Closed

3 tasks

Merge branch 'main' into namespace-permissions

b9bab0d

constanca-m added 2 commits April 8, 2024 14:14

Fix kubernetes tests

5665d43

Signed-off-by: constanca <constanca.manteigas@elastic.co>

Merge remote-tracking branch 'origin/namespace-permissions' into name…

da534ae

…space-permissions

tetianakravchenko reviewed Apr 8, 2024

View reviewed changes

Add new test for defaults

227b92f

Signed-off-by: constanca <constanca.manteigas@elastic.co>

tetianakravchenko approved these changes Apr 15, 2024

View reviewed changes

Merge branch 'main' into namespace-permissions

741a222

gizas reviewed Apr 16, 2024

View reviewed changes

gizas approved these changes Apr 16, 2024

View reviewed changes

Check for watcher creation on processor

6ebb6a2

Signed-off-by: constanca <constanca.manteigas@elastic.co>

MichaelKatsoulis reviewed Apr 23, 2024

View reviewed changes

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

Update CHANGELOG-developer.next.asciidoc

710ed63

Co-authored-by: Michael Katsoulis <michaelkatsoulis88@gmail.com>

MichaelKatsoulis approved these changes Apr 23, 2024

View reviewed changes

Merge branch 'main' into namespace-permissions

8eaaca1

blakerouse approved these changes Apr 23, 2024

View reviewed changes

pierrehilbert approved these changes Apr 23, 2024

View reviewed changes

constanca-m merged commit e53eb0c into elastic:main Apr 23, 2024

constanca-m deleted the namespace-permissions branch April 23, 2024 15:42

constanca-m mentioned this pull request Apr 24, 2024

[Kubernetes] Check AddResourceMetadata and hints before starting namespace and node watchers elastic/elastic-agent#4618

Merged

7 tasks

Conversation

constanca-m commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed commit message

Checklist

How to test this PR locally

Results

Related issues

Uh oh!

mergify bot commented Apr 8, 2024

Uh oh!

mergify bot commented Apr 8, 2024

Uh oh!

elasticmachine commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

Uh oh!

Uh oh!

tetianakravchenko Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

constanca-m Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

gizas Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

MichaelKatsoulis Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

gizas Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

constanca-m Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

constanca-m commented Apr 16, 2024

Uh oh!

MichaelKatsoulis commented Apr 18, 2024

Uh oh!

constanca-m commented Apr 18, 2024

Uh oh!

Uh oh!

MichaelKatsoulis left a comment

Choose a reason for hiding this comment

Uh oh!

blakerouse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

constanca-m commented Apr 8, 2024 •

edited

Loading

elasticmachine commented Apr 8, 2024 •

edited

Loading

gizas Apr 16, 2024 •

edited

Loading