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

Promtail: Fix excludepath not evaluated on newly added files #9831

Merged

Conversation

SijmenHuizenga
Copy link
Contributor

@SijmenHuizenga SijmenHuizenga commented Jun 29, 2023

What this PR does / why we need it:

In promtail, when a file matching the exclude path is created in a directory that is being watched, this file should not be tailed by promtail. However, previously this was the case. As described in #7115. This change checks the filename to the excludePath and if it matches, ignores the file.

Which issue(s) this PR fixes:
Fixes #7115

Special notes for your reviewer:
We tested manually this by running Promtail locally and creating a bunch of files which should and should not be tailed by Promtail.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@SijmenHuizenga SijmenHuizenga requested a review from a team as a code owner June 29, 2023 19:13
@CLAassistant
Copy link

CLAassistant commented Jun 29, 2023

CLA assistant check
All committers have signed the CLA.

@SijmenHuizenga SijmenHuizenga changed the title Promtail: Bugfix, excludepath not evaluated on newly added files Promtail: Fix excludepath not evaluated on newly added files Jun 29, 2023
@MasslessParticle
Copy link
Contributor

MasslessParticle commented Jun 30, 2023

@SijmenHuizenga Thanks for the contribution, this looks good. Can you write a test that goes along with this change?

@yangfan-witcher
Copy link

This function is very important, waiting

@cstyan
Copy link
Contributor

cstyan commented Nov 7, 2023

@SijmenHuizenga Hello, thanks for your PR.

We're currently reevaluating promtails position as a project within Grafana Labs. Internally we're actually using the Agent for both metrics and logs collection at this point. Additionally, the agent team is more likely to have time to dedicate to your PR. We can likely consider this a bugfix and move forward anyways, but as Travis has mentioned a test should be added.

While we haven't made a formal decision yet, we expect in the near future that all new feature work will be done in the Agent's log collection pipelines rather than in Promtail.

@xpader
Copy link

xpader commented Nov 8, 2023

God, it's been 4 months ago now.

Copy link
Contributor

github-actions bot commented Nov 17, 2023

Trivy scan found the following vulnerabilities:

  • HIGH, Target: docker.io/grafana/loki:main-3373cc5 (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
  • HIGH, Target: docker.io/grafana/loki:main-3373cc5 (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0
    \nTo see more details on these vulnerabilities, and how/where to fix them, please run docker build -t grafana/loki:main-3373cc5 -f cmd/loki/Dockerfile .
    trivy i grafana/loki:main-3373cc5 on your branch. If these were not introduced by your PR, please considering fixing them in via a subsequent PR. Thanks!

@SijmenHuizenga SijmenHuizenga force-pushed the promtail-fix-filewatcher-excludepath branch from dc57086 to b67f354 Compare November 17, 2023 16:26
@cstyan cstyan self-assigned this Nov 17, 2023
@SijmenHuizenga
Copy link
Contributor Author

Thanks @cstyan for the context about Agent and Promtail. I've added the unit test and rebased onto main. I'm happy to hear if there's anything should be improved.

@cstyan
Copy link
Contributor

cstyan commented Jan 5, 2024

@SijmenHuizenga sorry for the delayed response. It looks like there's a test failure on windows related to the go.mod toolchain directive. I think that failure will just be fixed if you merge the current main branch into yours.

After that we can merge, and I'll notify the agent team of this fix as well so they can include it there.

@SijmenHuizenga
Copy link
Contributor Author

@cstyan thank you for explaining how we can move this forward. main has been merged into here.

@cstyan cstyan merged commit 5e50ea0 into grafana:main Jan 8, 2024
7 checks passed
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
…#9831)

**What this PR does / why we need it**:

In promtail, when a file matching the exclude path is created in a
directory that is being watched, this file should not be tailed by
promtail. However, previously this was the case. As described in grafana#7115.
This change checks the filename to the excludePath and if it matches,
ignores the file.

**Which issue(s) this PR fixes**:
Fixes grafana#7115

**Special notes for your reviewer**:
We tested manually this by running Promtail locally and creating a bunch
of files which should and should not be tailed by Promtail.

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promtail Path exclude is not working as expected
6 participants