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

logging: Perform filtering on arrays of strings (where possible) #5101

Merged
merged 4 commits into from
Oct 5, 2022

Conversation

francislavoie
Copy link
Member

Thanks to @IndeedNotJames for reporting this problem.

When trying to filter HTTP headers in the logs, many filters don't work at all because they didn't handle fields that are array values, which all headers are because they can have multiple values.

These changes are mainly for HTTP headers, but it'll work in general for any LoggableStringArray type being logged.

Now, the regexp, hash and ip_mask filters will correctly iterate over each string in the LoggableStringArray and apply their filtering on those.

Additionally, for ip_mask, we now split on commas and mask each segment that parses as an IP address. This should be useful for X-Forwarded-For type headers which may be a list of comma separated IPs.

And finally, I fixed a bug with rename where it would set the zap field type to String, which would totally clobber any array typed fields with an empty string instead. That was just a total oversight when I implemented that filter, because it was basically copy-paste from the replace filter.

As for the other filters, I didn't touch them for various reasons. replace doesn't iterate because it could be a valid usecase to change the type of a field to string... but also maybe not. I dunno about that one, you can probably just use regexp in its place. And for delete, it's not possible to delete a particular entry in an array; we'd need a new filter to do that, which is totally possible, but I'm not sure the usecase (when is there ever a guarantee that a particular index exists?). Also cookie and query already do their job; they're very specifically targeted at the Cookie header (and iterates itself just fine), and the uri field.

@francislavoie francislavoie requested a review from mholt September 30, 2022 23:50
@francislavoie francislavoie added the bug 🐞 Something isn't working label Sep 30, 2022
@francislavoie francislavoie added this to the v2.6.2 milestone Sep 30, 2022
@francislavoie francislavoie force-pushed the log-filters-on-arrays branch from 95ad462 to 00a028f Compare October 5, 2022 04:17
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Wow, thank you for this fix and for the tests. I haven't tried it myself but I trust it works, given the evidence and who's contributing it :)

LGTM, thanks a bunch!

@mholt mholt merged commit ea58d51 into master Oct 5, 2022
@mholt mholt deleted the log-filters-on-arrays branch October 5, 2022 05:21
@pauljeannot
Copy link
Contributor

pauljeannot commented Aug 22, 2023

Hi!

Thanks for the work you did there that is specifically very useful in my workflow today 😊
However I am facing an issue which is not in line with your last assertion:

Also cookie and query already do their job; they're very specifically targeted at the Cookie header (and iterates itself just fine), and the uri field.

Indeed, the query filter is quite useful also in other fields, like the Referer which is basically a uri. As the Referer is part of the Headers, the current implementation does not iterate on its values and thus the query filter does not work as I could have expected it.

I guess that's not a common usecase. The only workaround I see there is to use regexp, which makes it quite heavy.
Do you see any other workaround that would be easier ? Also, Is there an improvement on that part planned soon ?

Thanks a lot!

@francislavoie
Copy link
Member Author

Ah I didn't think of the Referer header. Good point. If you want to contribute a fix, PRs are welcome.

@pauljeannot
Copy link
Contributor

Alright. I will try to contribute asap!

@mholt
Copy link
Member

mholt commented Sep 7, 2023

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants