[Enhanncement for host.ip and host.mac] Disabling netinfo.enabled option of add-host-metadata processor #36506

gizas · 2023-09-05T13:22:26Z

Enhancement

Proposed commit message

Relates to elastic/integrations#6674

Please explain:

WHAT: We are disabling the netinfo.enabled option of add-host-metadata processor by checking the existence of and environmental variable in the Elastic Agent Pod
WHY: Host.Ips and Host.Mac take a considerable size during indexing and affect both ingestion and visualisation process. As they dont provide meaningful infromation to the users we can disable them accordingly.

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

Checkout this PR fix inside ~/beats folder of your system.
Clone elastic-agent repo in another folder ~/elastic-agent
Build elastic agent image with details provided in https://github.com/elastic/elastic-agent#testing-elastic-agent-on-kubernetes. Command DEV=true SNAPSHOT=true PLATFORMS=linux/arm64 PACKAGES=docker mage package
Build the docker image of your custom agent that you will load later in your setup:

cd ~/elastic-agent
cd build/package/elastic-agent/elastic-agent-linux-arm64.docker/docker-build
docker build --platform linux/amd64 -t custom-agent-image .

Install kind
Load your custom build image into kind : kind load docker-image custom-agent-image:latest --name kind
Install your local ES stack : elastic-package stack up -d -vvv --version=8.11.0-SNAPSHOT
Use the the following manifests as samples to disable host.ip and host.mac

elastic-agent-standalone-kubernetesHostIPs.yml.txt
elastic-agent-standalone-kubernetesHostIPs.yml.txt

This is particular the piece of code that our manifests will need to have from now on:

# The following NETINNFO:false variable will disable the netinfo.enabled option of add-host-metadata processor. This will remove fields host.ip and host.mac.  
            # For more info: https://www.elastic.co/guide/en/beats/metricbeat/current/add-host-metadata.html
            # - name: NETINFO
            #   value: "false"

Related issues

Relates #Consider not adding host.ip metadata to k8s container metrics by default integrations#6674

Screens

Default 8.11.SNAPSHOT with host.ips and host.mac

hots

Standalone Agent with NETINNFO:false

Managed Agent with NETINNFO:false

mergify · 2023-09-05T13:23:04Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @gizas? 🙏.
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

mergify · 2023-09-05T13:45:22Z

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 fixinghostips upstream/fixinghostips
git merge upstream/main
git push upstream fixinghostips

ruflin

This change seems to be only done in metricbeat. Doesn't the same happen in Filebeat and any other Beat?
Do we need additional documentation around this how a user could enable it (again) if they wanted to?

ruflin · 2023-09-05T13:48:13Z

x-pack/metricbeat/cmd/root.go

-		{"add_docker_metadata": nil},
-		{"add_kubernetes_metadata": nil},
+
+	// We check for the existance of environmental variable NETINFO. Related to https://github.com/elastic/integrations/issues/6674


Lets have some more details here in the code docs. Jumping to the very long Github issue is not helpful. We can still leave the reference in, but lets add more details here.

Added some comments in beb7e85

elasticmachine · 2023-09-05T14:17:18Z

💚 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-09-07T08:49:47.976+0000
Duration: 70 min 50 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28237
Skipped	2013
Total	30250

💚 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!)

gizas · 2023-09-05T14:44:53Z

This change seems to be only done in metricbeat. Doesn't the same happen in Filebeat and any other Beat?

Indeed eg. filebeat has the same configuration see

beats/x-pack/filebeat/cmd/root.go

Line 41 in 1c5541e

func defaultProcessors() []mapstr.M {

My first thought here is that we need it only for metricbeat and filestream (if we decide to) for now as these are related to kubernetes.

By a first look, auditbeat, osquerybeat and packetbeat initialise add_host_metadata processor as well, but dont need we need it for k8s.

@martijnvg do you think we need it also for filebeat? Our tests were run only for metrics that is why we focused only in metricbeat I guess

Do we need additional documentation around this how a user could enable it (again) if they wanted to?

For now I added it only in the manifests https://github.com/elastic/elastic-agent/pull/3354/files#diff-b85244108b202e9acc2f4eca1f918bb448bbd10a816a83123ec17f7cd3ca92b6

The other place I see that might feet is here: https://www.elastic.co/guide/en/fleet/current/agent-environment-variables.html

martijnvg · 2023-09-06T06:24:23Z

do you think we need it also for filebeat? Our tests were run only for metrics that is why we focused only in metricbeat I guess

I think this depends on whether local-link addresses add any value in the non metric use cases? I don't think so? But I think I'm not the person to answer this question.

ruflin · 2023-09-06T07:04:47Z

By a first look, auditbeat, osquerybeat and packetbeat initialise add_host_metadata processor as well, but dont need we need it for k8s.

My understanding is that when any of these Beats / Elastic Agent are run under k8s, the problem would exist. Is there anything special here around metrics? Is the assumption that the other beats are not run in k8s?

ChrsMark · 2023-09-06T08:42:12Z

x-pack/metricbeat/cmd/root.go

+	if valueNETINFO == "false" && status == true {
+		return []mapstr.M{
+			{"add_host_metadata": mapstr.M{
+				"netinfo.enabled": "false",


Why to enable/disable this at this level and not inside the processor's implementation?
Wouldn't that be more generic without the need to configure every single Beat (also covering Agent)? And that would cover @ruflin 's concern at #36506 (comment), right?

👍 to do that inside the processor implementation. I think in the implementation we could even automatically check if this is running inside k8s or inside a container and either disable netinfo completely or filter out link-local addresses. See also elastic/integrations#6674 (comment).

Thank you all team.
Reverted changes per beat module and made those inside the processor. Building a new image and retesting and let you know

Adding the fix only to add-host-metadata processor (https://github.com/elastic/beats/pull/36506/files#diff-03b979565735e6707425275586e3215f44192f46e1ac3b456516d93c653eba13R38) seems that works only for metricbeat. Somehow seems we override it for filestream? Will keep looking

gizas · 2023-09-06T12:58:32Z

Lessons learned, you need to remove both builds in order to force agent image to build from local code:
rm -rf x-pack/metricbeat/build/distributions/metricbeat-8.11.0-SNAPSHOT-linux-*
rm -rf x-pack/filebeat/build/distributions/filebeat-8.11.0-SNAPSHOT-linux-*

So all work by disabling netinfo in processor :
Filestream:

Metricbeat:

For documentation please review: elastic/ingest-docs#463

ChrsMark

The change LGTM. I left one comment about the env variable's name.

I think that skipping by default if k8s or containarized environment is detected would possibly be a breaking change at this point. Maybe some users want these data even in k8s envs.
So I think it's safer to go with the current approach.

ChrsMark · 2023-09-06T13:06:57Z

libbeat/processors/add_host_metadata/config.go

+	// Setting environmental variable NETINFO:false in Elastic Agent pod will disable the netinfo.enabled option of add_host_metadata processor
+	// This will result to events not being enhanced with host.ip and host.mac
+	// Related to https://github.com/elastic/integrations/issues/6674
+	valueNETINFO, _ := os.LookupEnv("NETINFO")


My only question would be the naming we want to choose here. Maybe sth like ELASTIC_NETINFO would be more explicit?

I agree lets not introduce a breaking change (I was thinking an old comment you had done).

For the naming you are killing me :) but I was expecting the debate , so seeing the page again https://www.elastic.co/guide/en/fleet/current/agent-environment-variables.html, I would go for ELASTIC_AGENT_NETINFO as strickly speaking this is a change on the agent side.

I would wait until tomorrow to see if others have any opinion and I would need to change all prs

ruflin · 2023-09-07T08:26:10Z

I think that skipping by default if k8s or containarized environment is detected would possibly be a breaking change at this point. Maybe some users want these data even in k8s envs.

I agree that this can potentially be considered a breaking change, at the same I'm not sure the use case where all these ip addresses would be used. If we don't change the default, most users will not get the benefits and have to make active changes to get the performance and storage benefits.

Do we agree false is the better default for most users? If we don't change it now, when would we change it? Maybe we can also separate the discussion and first get the change in to make it possible to disable and then figure out how we change the default in a good way. @felixbarny Thoughts?

ruflin · 2023-09-07T08:27:23Z

libbeat/processors/add_host_metadata/config.go

+	// Setting environmental variable ELASTIC_NETINFO:false in Elastic Agent pod will disable the netinfo.enabled option of add_host_metadata processor
+	// This will result to events not being enhanced with host.ip and host.mac
+	// Related to https://github.com/elastic/integrations/issues/6674
+	valueNETINFO, _ := os.LookupEnv("ELASTIC_NETINFO")


Lets make sure we have this documented. Can you expand the changelog to contain a mention of this env variable?

Let me know if this is ok, I added some info also there.

Also see elastic/ingest-docs#463

ChrsMark · 2023-09-07T08:59:44Z

Do we agree false is the better default for most users?

I believe we could just set ELASTIC_NETINFO to false by default within our k8s manifests as a first step. This has 2 main advantages:

The codebase itself does not introduce any possible breaking change.
We only affect the k8s use-case but from the configuration management point of view. This means that we have a reasonable default (to false) only for k8s deployments which can be easily changed from users by removing the variable definition from the k8s manifest (if needed).

If we agree on this here we can even change the manifests in this PR, or change those in a follow-up if we want to discuss it more. Whatever that fits better.

felixbarny · 2023-09-07T09:03:33Z

SGTM. The main thing that's important to me is that host.ip/host.mac is either truncated or removed from k8s data sets by default.

gizas · 2023-09-07T10:04:27Z

SGTM. The main thing that's important to me is that host.ip/host.mac is either truncated or removed from k8s data sets by default.

Thanks again all for the quick feedback. Default value in manifests is ELASTIC_NETINFO:false. So the host.ip and host.mac will be removed by default

felixbarny · 2023-09-07T10:36:32Z

So the host.ip and host.mac will be removed by default

I that also true for logs or just for metrics?

gizas · 2023-09-07T11:38:41Z

So the host.ip and host.mac will be removed by default

I that also true for logs or just for metrics?

Both . See tests #36506 (comment)

ChrsMark

The patch LGTM! Thanks!

ruflin

LGTM. Lets make sure we get this deployed directly in our test environments with the new configs and also get it into our cloud environments.

felixbarny · 2023-09-07T12:45:34Z

libbeat/processors/add_host_metadata/config.go

+	if valueNETINFO == "false" {
+		return Config{
+			NetInfoEnabled:      false,
+			CacheTTL:            5 * time.Minute,
+			ExpireUpdateTimeout: time.Second * 10,
+			ReplaceFields:       true,
+		}
+	} else {
+		return Config{
+			NetInfoEnabled:      true,
+			CacheTTL:            5 * time.Minute,
+			ExpireUpdateTimeout: time.Second * 10,
+			ReplaceFields:       true,
+		}


Optional suggestion to increase readability

Suggested change

if valueNETINFO == "false" {

return Config{

NetInfoEnabled: false,

CacheTTL: 5 * time.Minute,

ExpireUpdateTimeout: time.Second * 10,

ReplaceFields: true,

}

} else {

return Config{

NetInfoEnabled: true,

CacheTTL: 5 * time.Minute,

ExpireUpdateTimeout: time.Second * 10,

ReplaceFields: true,

}

return Config{

NetInfoEnabled: valueNETINFO == "true",

CacheTTL: 5 * time.Minute,

ExpireUpdateTimeout: time.Second * 10,

ReplaceFields: true,

}

…ion of add-host-metadata processor (elastic#36506) * adding check for host processor in case of K8s * Changing variable to ELASTIC_NETINFO * Updating changelog for variable ELASTIC_NETINFO

gizas added 4 commits June 29, 2023 13:20

adding check for host processor in case of K8s

d7a20b7

adding check for host processor in case of K8s

3c9f832

Updating root file to only have env variable check

9dbd74f

Adding comment

37b40d9

gizas requested a review from a team as a code owner September 5, 2023 13:22

gizas requested review from belimawr and rdner September 5, 2023 13:22

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 5, 2023

gizas self-assigned this Sep 5, 2023

gizas added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Sep 5, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 5, 2023

gizas requested review from ChrsMark, tommyers-elastic, ruflin and cmacknz September 5, 2023 13:24

gizas mentioned this pull request Sep 5, 2023

Updating Elastic Manifests with NETINFO variable elastic/elastic-agent#3354

Merged

7 tasks

Adding CHANGELOG.next.asciidoc

96f32f9

gizas mentioned this pull request Sep 5, 2023

Consider not adding host.ip metadata to k8s container metrics by default elastic/integrations#6674

Closed

ruflin reviewed Sep 5, 2023

View reviewed changes

gizas added 4 commits September 5, 2023 17:07

Adding comments for the change introduced

41c0534

Adding comments for the change introduced

beb7e85

Merge branch 'main' of github.com:elastic/beats into fixinghostips

1daafec

Adding comments for the change introduced

1c5541e

Adding filebeat fix

c1313cf

ChrsMark reviewed Sep 6, 2023

View reviewed changes

gizas added 2 commits September 6, 2023 12:44

Adding fix only t add-host-metadata processor and removed per beats

bc4f64e

Adding fix only t add-host-metadata processor and removed per beats

27d147a

gizas mentioned this pull request Sep 6, 2023

Adding information for NETINFO environmental variable elastic/ingest-docs#463

Merged

Adding comments

6a03eb4

ChrsMark reviewed Sep 6, 2023

View reviewed changes

Changing variable to ELASTIC_NETINFO

d26c658

ruflin reviewed Sep 7, 2023

View reviewed changes

Updating changelof for variable ELASTIC_NETINFO

a7ed8ee

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Sep 7, 2023

ChrsMark approved these changes Sep 7, 2023

View reviewed changes

ruflin approved these changes Sep 7, 2023

View reviewed changes

felixbarny approved these changes Sep 7, 2023

View reviewed changes

cmacknz approved these changes Sep 7, 2023

View reviewed changes

gizas merged commit 0f47b30 into main Sep 8, 2023
8 checks passed

gizas deleted the fixinghostips branch September 8, 2023 08:00

gizas mentioned this pull request Dec 12, 2023

Allow disabling and configuring the add_x_metadata default global processors elastic/elastic-agent#4670

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhanncement for host.ip and host.mac] Disabling netinfo.enabled option of add-host-metadata processor #36506

[Enhanncement for host.ip and host.mac] Disabling netinfo.enabled option of add-host-metadata processor #36506

gizas commented Sep 5, 2023

mergify bot commented Sep 5, 2023

mergify bot commented Sep 5, 2023

ruflin left a comment

ruflin Sep 5, 2023

gizas Sep 5, 2023

elasticmachine commented Sep 5, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

gizas commented Sep 5, 2023 •

edited

Loading

martijnvg commented Sep 6, 2023

ruflin commented Sep 6, 2023

ChrsMark Sep 6, 2023 •

edited

Loading

felixbarny Sep 6, 2023

gizas Sep 6, 2023

gizas Sep 6, 2023

gizas commented Sep 6, 2023

ChrsMark left a comment

ChrsMark Sep 6, 2023

gizas Sep 6, 2023 •

edited

Loading

ruflin commented Sep 7, 2023

ruflin Sep 7, 2023

gizas Sep 7, 2023

ChrsMark commented Sep 7, 2023

felixbarny commented Sep 7, 2023

gizas commented Sep 7, 2023

felixbarny commented Sep 7, 2023

gizas commented Sep 7, 2023

ChrsMark left a comment •

edited

Loading

ruflin left a comment

felixbarny Sep 7, 2023

[Enhanncement for host.ip and host.mac] Disabling netinfo.enabled option of add-host-metadata processor #36506

[Enhanncement for host.ip and host.mac] Disabling netinfo.enabled option of add-host-metadata processor #36506

Conversation

gizas commented Sep 5, 2023

Proposed commit message

Checklist

How to test this PR locally

Related issues

Screens

mergify bot commented Sep 5, 2023

mergify bot commented Sep 5, 2023

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

gizas commented Sep 5, 2023 • edited Loading

martijnvg commented Sep 6, 2023

ruflin commented Sep 6, 2023

ChrsMark Sep 6, 2023 • 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

gizas commented Sep 6, 2023

ChrsMark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gizas Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

ruflin commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrsMark commented Sep 7, 2023

felixbarny commented Sep 7, 2023

gizas commented Sep 7, 2023

felixbarny commented Sep 7, 2023

gizas commented Sep 7, 2023

ChrsMark left a comment • edited Loading

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2023 •

edited by jenkins-beats-ci bot

Loading

gizas commented Sep 5, 2023 •

edited

Loading

ChrsMark Sep 6, 2023 •

edited

Loading

gizas Sep 6, 2023 •

edited

Loading

ChrsMark left a comment •

edited

Loading