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

Drop stage with multiple sources results in config unmarshall error #10095

Closed
puppetninja opened this issue Jul 28, 2023 · 5 comments · Fixed by #10848
Closed

Drop stage with multiple sources results in config unmarshall error #10095

puppetninja opened this issue Jul 28, 2023 · 5 comments · Fixed by #10848
Labels
component/promtail type/bug Somehing is not working as expected

Comments

@puppetninja
Copy link

puppetninja commented Jul 28, 2023

Describe the bug
Config drop stage with list of sources

To Reproduce
Steps to reproduce the behavior:

  1. Config Promtail with a drop stage with multiple sources
  2. Started Promtail (2.8.3) to tail '...'

Expected behavior
We followed the drop stage with multiple sources link here
https://grafana.com/docs/loki/v2.8.x/clients/promtail/stages/drop/
And got the config unmarshall error, we can see the test for drop with multiple sources is not covered here
https://github.com/grafana/loki/blob/main/clients/pkg/logentry/stages/drop_test.go#L19

A possible reason could be due to this
https://medium.com/@kosiriki/accepting-both-string-and-array-for-single-yaml-key-in-golang-2e3d7143de8d

Environment:

  • Infrastructure: Kubernetes
  • Deployment tool: helm

Screenshots, Promtail config, or terminal output
If applicable, add any output to help explain your problem.

This is the snippet of the config

      - json:
          expressions:
            http_user_agent: http_user_agent
            status: status
      - drop:
          expression: .*traffic-manager.*;^(2|3|4)[0-9]{2}
          separator: ;
          source: 
             - "http_user_agent"
             - "status"

And this is the error we got:

failed to make file target manager: invalid drop stage config: drop stage source invalid type should be string or list of strings
@JStickler JStickler added type/bug Somehing is not working as expected component/promtail labels Aug 2, 2023
@BlackSmith
Copy link

Any progress? The same error with Promtail (2.8.4) .

@rgroothuijsen
Copy link
Contributor

The problem appears to be a type conversion error between the YAML configuration and the objects in code. source doesn't specify a type since it can be either a string or a list of strings, which unfortunately means that a list of strings gets parsed as an untyped list. This then causes the type check to fail here.

MichelHollands added a commit that referenced this issue Oct 12, 2023
…10848)

**What this PR does / why we need it**:
This PR fixes YAML parsing of the `source` field in the drop stage when
said field is a list of strings. Because this list is not being
recognized in the code as a list of strings, but rather as a list of
generic `interface{}` elements, it fails a type check and throws an
error. In order to fix this, an `interface{}` list is manually converted
to a `string` list. The problem was already reproducible with existing
tests, but the test YAML mistakenly referred to the field as `sources`
rather than `source`.

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

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [x] 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/setup/upgrade/_index.md`
- [ ] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](d10549e)

---------

Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
@galdor
Copy link

galdor commented Nov 16, 2023

I can reproduce this bug on v2.9.2 (released on October 16th) while the patch was merged on Octobre 12th.

@ShadowySpirits
Copy link

I have the same problem in v2.9.3. Is there any update on this issue?

@longtomjr
Copy link
Contributor

It looks like the fix will be released as part of v2.9.5. In the meantime, I am using this workaround that might be useful for others:

      # Workaround for https://github.com/grafana/loki/issues/10095
      # can be removed once we update to 2.9.5
      - template:
          source: merged_drop_string
          template: "{{.uri}};{{.remote_user}};{{.remote_addr}}"
      - drop:
          source: merged_drop_string
          expression: ...

Just list the extracted values in a template block with your preferred separator instead of on the source key, and then use the new template value instead. Can easily be changed once the fix have been released.

rhnasc pushed a commit to inloco/loki that referenced this issue Apr 12, 2024
…rafana#10848)

**What this PR does / why we need it**:
This PR fixes YAML parsing of the `source` field in the drop stage when
said field is a list of strings. Because this list is not being
recognized in the code as a list of strings, but rather as a list of
generic `interface{}` elements, it fails a type check and throws an
error. In order to fix this, an `interface{}` list is manually converted
to a `string` list. The problem was already reproducible with existing
tests, but the test YAML mistakenly referred to the field as `sources`
rather than `source`.

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

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [x] 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/setup/upgrade/_index.md`
- [ ] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](grafana@d10549e)

---------

Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/promtail type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants