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

Allow routing for integrations that are not input packages #6340

Merged
merged 19 commits into from Jun 13, 2023

Conversation

gsantoro
Copy link
Contributor

What does this PR do?

This PR add permissions to reroute logs to logs-*-* for integrations that are not input packages.

The full list of packages to edit is:

kubernetes.container_logs
system.syslog
activemq.log
auditd.log
aws.cloudwatch_logs
aws.ec2_logs
kafka.log
docker.container_logs

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.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@gsantoro gsantoro added the enhancement New feature or request label May 26, 2023
@gsantoro gsantoro requested a review from a team as a code owner May 26, 2023 12:21
@gsantoro gsantoro self-assigned this May 26, 2023
@gsantoro gsantoro requested review from rdner and cmacknz May 26, 2023 12:21
@gsantoro gsantoro requested review from a team as code owners May 26, 2023 12:25
@gsantoro gsantoro added draft Draft and removed draft Draft labels May 26, 2023
@elasticmachine
Copy link

elasticmachine commented May 26, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-06-13T12:11:58.706+0000

  • Duration: 52 min 27 sec

Test stats 🧪

Test Results
Failed 0
Passed 531
Skipped 4
Total 535

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented May 26, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (22/22) 💚
Files 95.833% (23/24) 👎 -4.167
Classes 95.833% (23/24) 👎 -4.167
Methods 84.645% (441/521) 👎 -4.244
Lines 90.563% (12466/13765) 👎 -9.437
Conditionals 100.0% (0/0) 💚

Copy link
Contributor

@tommyers-elastic tommyers-elastic left a comment

Choose a reason for hiding this comment

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

CSP integration changes look good to me - only thing on my mind is the future - someone looking at this file without the context of routing rules etc, they might be able to look up what these settings do, but there is some implicit coupling to other parts of the system here that i think would be good to call out 'at the source'. can we comment what the settings are for?

@gsantoro
Copy link
Contributor Author

about

CSP integration changes look good to me - only thing on my mind is the future - someone looking at this file without the context of routing rules etc, they might be able to look up what these settings do, but there is some implicit coupling to other parts of the system here that i think would be good to call out 'at the source'. can we comment what the settings are for?

I think that we could add a comment like

# Ensures agents have permissions to write data to `logs-nginx.*-*`

like the one you can see at here

packages/activemq/changelog.yml Outdated Show resolved Hide resolved
@felixbarny
Copy link
Member

I've updated the versions so that it increments the minor version, not the bug fix version now.

@felixbarny
Copy link
Member

Can we bump the minor version of the integration rather than the bugfix, as this is adding features we want to follow the semantic versioning.

Done.

Please test any of the packages on an older version of the stack, while we can assume that fleet should ignore those settings, try to take a look at the oldest version of the stack any of these packages support (8.x, 7.x?), and see if that is still the case. If not, please bump the min kibana version of that package.

This will need to wait until Giuseppe is back. @joshdover, since you implemented the flags in fleet, how big do you think is the risk that packages with these flags cause issues when used in older stack versions?

I assume these settings are now documented?

These settings are documented in the package spec: https://github.com/elastic/package-spec/blob/e29dd918bc5a6e81cb0b36ae8a3c4b4738f1d68e/spec/integration/data_stream/manifest.spec.yml#L304-L309

I was a bit unsure why each package has to define the fact that they are allowed to have dynamic settings,

While dynamic dataset/namespace is enabled by default for input packages, regular integrations need to opt-in for each data stream. That's because not all integrations will want to open up the API key permissions. The regular API key permissions are very narrowly scoped to the exact dataset and namespace, for example logs-nginx.access.default. The dynamic_dataset flag results in the dataset part to be a wildcard in the API key permissions. So if a package sets both dynamic_dataset and dynamic_namespace, the API key permissions are expanded to logs-*-*.

will this then show dataset settings in the UI, or is it something else?

No, this will not show in any UI.

@gsantoro
Copy link
Contributor Author

I have tested now the rerouting of logs for the following use cases:

  • docker.container_logs in 8.9.0-SNAPSHOT Elastic stack
  • docker.container_logs in 8.8.0 Elastic stack

@gsantoro
Copy link
Contributor Author

/test

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @gsantoro

@gsantoro gsantoro merged commit 835c3c7 into elastic:main Jun 13, 2023
4 checks passed
@gsantoro gsantoro deleted the feature/allow_routing branch June 13, 2023 13:29
@elasticmachine
Copy link

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

@elasticmachine
Copy link

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

@elasticmachine
Copy link

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

@elasticmachine
Copy link

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

@elasticmachine
Copy link

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

@elasticmachine
Copy link

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

@elasticmachine
Copy link

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

@@ -1,7 +1,7 @@
format_version: 1.0.0
name: system
title: System
version: 1.32.0-beta.2
version: 1.33.0
Copy link
Member

Choose a reason for hiding this comment

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

@gsantoro With this change you accidentally release TSDB which was still in beta!

Copy link
Contributor

Choose a reason for hiding this comment

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

@gsantoro Are we considering System Integration TSDB as GA now or are we planning to revert the change ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

TSDB in system is now approved to be in GA. #6607

sodhikirti07 pushed a commit that referenced this pull request Jun 15, 2023
* fix system test for kubernetes integration for k8s v1.27.0

* minor changes from formatting

* syslong changes

* revert a merge conflict

* add permissions to selected list of datastreams

* update PR id

* fix merge conflict with main

* Increment minor version instead of bugfix version

* added comments for new settings

* update activemq version

---------

Co-authored-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow routing for integrations that are not input packages
10 participants