Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libbeat/processors/add_process_metadata: Add default cgroup.regex for add_process_metadata #36484

Merged
merged 21 commits into from
Sep 8, 2023

Conversation

bhapas
Copy link
Contributor

@bhapas bhapas commented Sep 1, 2023

Proposed commit message

Replace the existing cgroup_prefixes default with a default cgroup_regex. The new default will match the same cgroup paths as the old cgroup_prefixes value plus more. Out of the box it will match cgroup paths from modern Kubernetes and Podman versions. Existing users of cgroup_prefixes and cgroup_regex should see no breaking change.

This removes the undocumented behavior that when cgroup_prefixes was used that it would return a value that matched [\w]{64} for cgroup v2 paths. This was inconsistent because it arbitrary applied only to cgroup v2 paths and it could be prone to false matches.

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.

Related issues

@bhapas bhapas self-assigned this Sep 1, 2023
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 1, 2023
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 1, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2023

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

@bhapas bhapas added 8.11-candidate backport-v8.10.0 Automated backport with mergify backport-v8.9.0 Automated backport with mergify bugfix enhancement and removed >enhancement bugfix labels Sep 1, 2023
@bhapas bhapas force-pushed the cgroup-regex-add-process-metadata branch from 85db624 to a626a63 Compare September 1, 2023 09:21
@bhapas bhapas changed the title libbeat/processors/add_process_metadata: Add default regex for add_process_metadata libbeat/processors/add_process_metadata: Add default cgroup.regex for add_process_metadata Sep 1, 2023
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 1, 2023

❕ 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
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2023-09-07T06:23:47.761+0000

  • Duration: 82 min 5 sec

Test stats 🧪

Test Results
Failed 0
Passed 28247
Skipped 2013
Total 30260

Steps errors 1

Expand to view the steps failures

metricbeat-pythonIntegTest - mage pythonIntegTest
  • Took 31 min 0 sec . View more details here
  • Description: mage pythonIntegTest

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

@bhapas bhapas marked this pull request as ready for review September 1, 2023 12:18
@bhapas bhapas requested a review from a team as a code owner September 1, 2023 12:18
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@bhapas bhapas requested a review from andrewkroh September 1, 2023 12:19
@bhapas bhapas removed the backport-v8.9.0 Automated backport with mergify label Sep 1, 2023
@bhapas bhapas added :Processors Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Sep 1, 2023
@bhapas bhapas requested a review from andrewkroh September 6, 2023 08:44
This enabled validation of the regex at configuration loading time.
It also fixes an inefficiency where the regex was compiled on every
execution.
To avoid a breaking behavior change this checks to see if the configuration
specified either cgroup_prefixes or cgroup_regex, and if not it sets a default
regex value that will match the same cgroups as the previous default plus more.
This behavior was not specified in the documentation. And it was only applied
to cgroup v2 paths. The new default behavior will superceed this.
This test checks that existing configurations with cgroup_prefixes
are not broken by the change. In order to make it clear that the
defaultCgroupRegex is not being used and that the configured
cgroup_prefixes is being honored, use a cgroup path that could
never match the defaultCgroupRegex.
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed some changes to implement a default cgroup_regex without breaking existing configs. PTAL

Proposed commit message:

Replace the existing cgroup_prefixes default with a default cgroup_regex. The new default will match the same cgroup paths as the old cgroup_prefixes value plus more. Out of the box it will match cgroup paths from modern Kubernetes and Podman versions. Existing users of cgroup_prefixes and cgroup_regex should see no breaking change.

This removes the undocumented behavior that when cgroup_prefixes was used that it would return a value that matched [\w]{64} for cgroup v2 paths. This was inconsistent because it arbitrary applied only to cgroup v2 paths and it could be prone to false matches.

Fixes:

    gosigar_cid_provider.go:89:9: type switch on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
@bhapas bhapas requested a review from a team as a code owner September 7, 2023 06:21
@bhapas bhapas force-pushed the cgroup-regex-add-process-metadata branch from 16e1e52 to 6be8318 Compare September 7, 2023 06:23
@bhapas bhapas requested review from andrewkroh and removed request for a team September 7, 2023 06:23
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bhapas bhapas merged commit 80ed33b into elastic:main Sep 8, 2023
@bhapas bhapas deleted the cgroup-regex-add-process-metadata branch September 8, 2023 13:55
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
… add_process_metadata (#36484)

Replace the existing cgroup_prefixes default with a default cgroup_regex. The new default will match the same cgroup paths as the old cgroup_prefixes value plus more. Out of the box it will match cgroup paths from modern Kubernetes and Podman versions. Existing users of cgroup_prefixes and cgroup_regex should see no breaking change.

This removes the undocumented behavior that when cgroup_prefixes was used that it would return a value that matched [\w]{64} for cgroup v2 paths. This was inconsistent because it arbitrary applied only to cgroup v2 paths and it could be prone to false matches.

(cherry picked from commit 80ed33b)
bhapas pushed a commit that referenced this pull request Sep 11, 2023
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
… add_process_metadata (elastic#36484)

Replace the existing cgroup_prefixes default with a default cgroup_regex. The new default will match the same cgroup paths as the old cgroup_prefixes value plus more. Out of the box it will match cgroup paths from modern Kubernetes and Podman versions. Existing users of cgroup_prefixes and cgroup_regex should see no breaking change.

This removes the undocumented behavior that when cgroup_prefixes was used that it would return a value that matched [\w]{64} for cgroup v2 paths. This was inconsistent because it arbitrary applied only to cgroup v2 paths and it could be prone to false matches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.11-candidate backport-v8.10.0 Automated backport with mergify enhancement :Processors Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add_process_metadata: cgroup_regex should support containerd
4 participants