Enhance add_kubernetes_metadata matcher #28868

tetianakravchenko · 2021-11-08T10:41:17Z

Signed-off-by: Tetiana Kravchenko tetiana.kravchenko@elastic.co

What does this PR do?

add documentation for add_kubernetes_metadata matcher;
support /var/log/pods/ for resource_type: pod

Why is it important?

to improve understanding on how add_kubernetes_metadata works and which logs_path are supported for available resource_type

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.

Author's Checklist

[ ]

How to test this PR locally

Related issues

closes add_kubernetes_metadata needs to respect logs_path setting #28629

Use cases

Screenshots

Logs

…og/pods' for resource_type: pod Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

mergify · 2021-11-08T10:41:31Z

This pull request does not have a backport label. Could you fix it @tetianakravchenko? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2021-11-08T12:12:19Z

💚 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: 2021-11-09T17:12:54.731+0000
Duration: 120 min 56 sec
Commit: de58076

Test stats 🧪

Test	Results
Failed	0
Passed	54222
Skipped	5345
Total	59567

💚 Flaky test report

Tests succeeded.

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

elasticmachine · 2021-11-08T12:27:35Z

Pinging @elastic/integrations (Team:Integrations)

ChrsMark

Great work in breaking this into pieces @tetianakravchenko , I left some comments/suggestions but in general it looks good!

ChrsMark · 2021-11-08T12:47:00Z

filebeat/processor/add_kubernetes_metadata/matchers.go

-		source := value.(string)
-		f.logger.Debugf("Incoming log.file.path value: %s", source)
+	if err != nil {
+		f.logger.Errorf("Error extracting log.file.path from the event")


We may still need to log source here so as to have a more clear view of how the event that is related to this error looks like.

do you mean event instead of source? this error might only occur in case event map does not contain log.file.path(=source) key

Yeap my wrong, event is what I was thinking about. Logging the whole event would be helpful.

Also maybe just a debug message would be enough, since ERRORs would flood a basic case when the processor for some reason does not work.

fixed: e8ab1fe

ChrsMark · 2021-11-08T12:52:56Z

filebeat/processor/add_kubernetes_metadata/matchers.go

 			}

-			f.logger.Error("Error extracting container id - source value contains matcher's logs_path, however it is too short to contain a Docker container ID.")
+			f.logger.Error(`Error extracting pod uid - source value does not contains matcher's logs_path,


Wondering if we should prevent this from happening by validating the configuration. wdyt?

could you please elaborate on this? do you mean do smth similar to other processors. like - https://github.com/elastic/beats/blob/master/libbeat/processors/add_kubernetes_metadata/config.go ?

Yes, in

beats/libbeat/processors/add_kubernetes_metadata/config.go

Line 63 in 3d19ec0

func (k *kubeAnnotatorConfig) Validate() error {

we can check if matchers' config is valid too so as to warn the users early on and avoid initialising the processor in case the config is not supported. I see that the matcher's config part is a map so not sure how much mangling this change would require but it should be doable. Let me know what you think.

I will give it a try

@ChrsMark please have a look - ab3c821, let me know wdyt, are there any better ways maybe?

libbeat/processors/add_kubernetes_metadata/docs/indexers_and_matchers.asciidoc

Co-authored-by: Chris Mark <chrismarkou92@gmail.com>

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

…vchenko/beats into 28629-add_kubernetes_metadata

MichaelKatsoulis · 2021-11-08T16:29:27Z

filebeat/processor/add_kubernetes_metadata/matchers.go

+				if len(pathDirs) > podUIDPos {
+					podUID := strings.Split(pathDirs[podUIDPos], "_")
+					if len(podUID) > 2 {
+						f.logger.Debugf("Using pod uid: %s", podUID)


Maybe log the podUID[2] here instead of the array

thank you for pointing it out! fixed: e8ab1fe

mergify · 2021-11-09T04:48:40Z

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 28629-add_kubernetes_metadata upstream/28629-add_kubernetes_metadata
git merge upstream/master
git push upstream 28629-add_kubernetes_metadata

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

ChrsMark

LGTM! Thanks for adding test cases too! Only one minor comment.

ChrsMark · 2021-11-09T12:25:08Z

libbeat/processors/add_kubernetes_metadata/config.go

@@ -69,5 +69,34 @@ func (k *kubeAnnotatorConfig) Validate() error {
 		k.Host = ""
 	}

+	for _, matcher := range k.Matchers {


Looks good! I would suggest adding a comment here explaining why we perform such checks and that it's because we are taking an opinionated approach on what paths we support.

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

luigibk · 2022-02-21T15:58:34Z

Hi @tetianakravchenko any plans to backport this to 8.0 and 7.17?

tetianakravchenko · 2022-02-21T16:04:02Z

@Mergifyio backport 7.17

tetianakravchenko · 2022-02-21T16:04:23Z

@Mergifyio backport 8.0

mergify · 2022-02-21T16:05:13Z

backport 7.17

❌ No backport have been created

Backport to branch 7.17 failed: Git reported the following error:

fatal: couldn't find remote ref master

mergify · 2022-02-21T16:05:16Z

backport 8.0

❌ No backport have been created

Backport to branch 8.0 failed: Git reported the following error:

fatal: couldn't find remote ref master

luigibk · 2022-02-22T10:07:10Z

Hi @tetianakravchenko thanks for the quick reply, mergify returned an error as I think there might be an issue with how it is configured as it's looking for a master branch, whereas it's named main in this repo.

tetianakravchenko · 2022-02-22T10:09:30Z

Hi @luigibk, I will take care of this today

* add documentation for add_kubernetes_metadata matcher; support 'var/log/pods' for resource_type: pod Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * Apply suggestions from code review Co-authored-by: Chris Mark <chrismarkou92@gmail.com> * add record to CHANGELOG.next.asciidoc Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * address comments: log pod id instead of array; log event Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add validation for logs_path matchers config Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add comment for the config validation check Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * set different sourcePath for windows in tests Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> Co-authored-by: Chris Mark <chrismarkou92@gmail.com> (cherry picked from commit ac8275f)

…30526) * Enhance add_kubernetes_metadata matcher (#28868) * add documentation for add_kubernetes_metadata matcher; support 'var/log/pods' for resource_type: pod Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * Apply suggestions from code review Co-authored-by: Chris Mark <chrismarkou92@gmail.com> * add record to CHANGELOG.next.asciidoc Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * address comments: log pod id instead of array; log event Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add validation for logs_path matchers config Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add comment for the config validation check Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * set different sourcePath for windows in tests Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> Co-authored-by: Chris Mark <chrismarkou92@gmail.com> (cherry picked from commit ac8275f) * Update CHANGELOG.next.asciidoc

…30527) * Enhance add_kubernetes_metadata matcher (#28868) * add documentation for add_kubernetes_metadata matcher; support 'var/log/pods' for resource_type: pod Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * Apply suggestions from code review Co-authored-by: Chris Mark <chrismarkou92@gmail.com> * add record to CHANGELOG.next.asciidoc Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * address comments: log pod id instead of array; log event Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add validation for logs_path matchers config Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add comment for the config validation check Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * set different sourcePath for windows in tests Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> Co-authored-by: Chris Mark <chrismarkou92@gmail.com> (cherry picked from commit ac8275f) * Update CHANGELOG.next.asciidoc

add documentation for add_kubernetes_metadata matcher; support 'var/l…

a648d11

…og/pods' for resource_type: pod Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

tetianakravchenko added the enhancement label Nov 8, 2021

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 8, 2021

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Nov 8, 2021

tetianakravchenko requested review from ChrsMark and MichaelKatsoulis November 8, 2021 10:41

mtojek added the Team:Integrations Label for the Integrations team label Nov 8, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 8, 2021

ChrsMark reviewed Nov 8, 2021

View reviewed changes

tetianakravchenko and others added 3 commits November 8, 2021 15:08

Apply suggestions from code review

516cb13

Co-authored-by: Chris Mark <chrismarkou92@gmail.com>

add record to CHANGELOG.next.asciidoc

dc3553b

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

Merge branch '28629-add_kubernetes_metadata' of github.com:tetianakra…

ba54a8f

…vchenko/beats into 28629-add_kubernetes_metadata

MichaelKatsoulis reviewed Nov 8, 2021

View reviewed changes

tetianakravchenko added 3 commits November 9, 2021 13:11

address comments: log pod id instead of array; log event

e8ab1fe

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

add validation for logs_path matchers config

ab3c821

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

Merge branch 'master' into 28629-add_kubernetes_metadata

82a9f2d

ChrsMark approved these changes Nov 9, 2021

View reviewed changes

add comment for the config validation check

2dfe013

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

ChrsMark approved these changes Nov 9, 2021

View reviewed changes

set different sourcePath for windows in tests

de58076

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

tetianakravchenko merged commit ac8275f into elastic:master Nov 10, 2021

tetianakravchenko deleted the 28629-add_kubernetes_metadata branch November 10, 2021 10:11

ChrsMark mentioned this pull request Nov 19, 2021

processor.add_kubernetes_metadata.matchers.resource_type setting won't work #27476

Closed

tetianakravchenko mentioned this pull request Dec 27, 2021

[matcher]is there anyway to change the default log_path for add_kubernetes_metadata matcher? #23606

Closed

tetianakravchenko mentioned this pull request Feb 22, 2022

Cherry-pick #28868 to 7.17: Enhance add_kubernetes_metadata matcher #30526

Merged

6 tasks

tetianakravchenko added the v7.17.1 label Feb 22, 2022

tetianakravchenko mentioned this pull request Feb 22, 2022

Cherry-pick #28868 to 8.0: Enhance add_kubernetes_metadata matcher #30527

Merged

6 tasks

tetianakravchenko added the v8.0.1 label Feb 22, 2022

OvervCW mentioned this pull request Jun 24, 2022

Changelog for 7.17.0 is missing a breaking change in the Filebeat config #32095

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance add_kubernetes_metadata matcher #28868

Enhance add_kubernetes_metadata matcher #28868

tetianakravchenko commented Nov 8, 2021 •

edited

Loading

mergify bot commented Nov 8, 2021

elasticmachine commented Nov 8, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

elasticmachine commented Nov 8, 2021

ChrsMark left a comment

ChrsMark Nov 8, 2021

tetianakravchenko Nov 8, 2021

ChrsMark Nov 8, 2021

ChrsMark Nov 8, 2021

tetianakravchenko Nov 9, 2021

ChrsMark Nov 8, 2021

tetianakravchenko Nov 8, 2021

ChrsMark Nov 8, 2021

tetianakravchenko Nov 9, 2021

tetianakravchenko Nov 9, 2021

MichaelKatsoulis Nov 8, 2021

tetianakravchenko Nov 9, 2021

mergify bot commented Nov 9, 2021

ChrsMark left a comment

ChrsMark Nov 9, 2021

tetianakravchenko Nov 9, 2021

luigibk commented Feb 21, 2022

tetianakravchenko commented Feb 21, 2022

tetianakravchenko commented Feb 21, 2022

mergify bot commented Feb 21, 2022

mergify bot commented Feb 21, 2022

luigibk commented Feb 22, 2022

tetianakravchenko commented Feb 22, 2022

Enhance add_kubernetes_metadata matcher #28868

Enhance add_kubernetes_metadata matcher #28868

Conversation

tetianakravchenko commented Nov 8, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented Nov 8, 2021

elasticmachine commented Nov 8, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Nov 8, 2021

ChrsMark 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

mergify bot commented Nov 9, 2021

ChrsMark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luigibk commented Feb 21, 2022

tetianakravchenko commented Feb 21, 2022

tetianakravchenko commented Feb 21, 2022

mergify bot commented Feb 21, 2022

❌ No backport have been created

mergify bot commented Feb 21, 2022

❌ No backport have been created

luigibk commented Feb 22, 2022

tetianakravchenko commented Feb 22, 2022

tetianakravchenko commented Nov 8, 2021 •

edited

Loading

elasticmachine commented Nov 8, 2021 •

edited by jenkins-beats-ci bot

Loading