filters/accesslog: Add `maskAccessLogQuery` filter #3471

ponimas · 2025-04-07T13:19:20Z

Introduce a new maskAccessLogQuery filter that masks/obfuscates values of specific sensitive query parameters in access logs.

Additionally, refactor accesslog/control_test.go to use testify/assert for improved test readability.

See the previous PR for discussion details - #2674
Closing #2156, #2674

filters/accesslog/control.go

MustafaSaber · 2025-04-09T10:04:45Z

filters/accesslog/control_test.go

-				} else {
-					return
-				}
+				require.Error(t, err, "Expected error creating filter")


minor: you can also use require.Condition to remove the outer if.

Seems that it will look more complex with Condition

I guess @MustafaSaber meant to remove the if ti.isError and replace it with a oneliner err check.
I don't like require/assert lib but feel free to let or change it.

logging/access.go

MustafaSaber · 2025-04-09T10:13:42Z

logging/access.go


+// maskQueryParams masks (i.e., hashing) specific query parameters in the provided request's URI.
+// Returns the obfuscated URI.
+func maskQueryParams(req *http.Request, maskedQueryParams map[string]bool) string {


I think we can also inline map len check here and return early

But this function returns string, means we need to allocate new one even for early return.

MustafaSaber · 2025-04-09T10:13:59Z

logging/access.go

 		if stripQuery {
 			uri = stripQueryString(uri)
+		} else if keys, ok := additional[KeyMaskedQueryParams].(map[string]bool); ok {
+			if len(keys) > 0 {


see https://github.com/zalando/skipper/pull/3471/files#r2035043394

the link is broken for me

was this one #3471 (comment)

logging/access_test.go

AlexanderYastrebov · 2025-04-10T08:06:05Z

filters/accesslog/control.go

-	bag[AccessLogEnabledKey] = al
+	if f, ok := bag[AccessLogEnabledKey]; ok {
+		a := f.(*AccessLogFilter)
+		maps.Copy(a.MaskedQueryParams, al.MaskedQueryParams)


This modifies filter instance from parallel requests and will panic.

szuecs · 2025-11-04T21:28:55Z

@ponimas can you please create user visible docs: docs/reference/filters.md in logs section ?

Rest seems good to go. I classify this as major change because it modifies code paths that are already in use, which means it does not classify as completely "new" filter.

zalando-robot · 2025-11-24T16:26:47Z

Deployment Checklist

This change falls under the deployment policy.

💁 Since Nov 10th, we are in the RED deployment zone. This means all changes released to production must adhere to the following requirements:

Detailed release notes are provided in this PR’s description.
Thorough load-testing has been performed, and is documented in the description/comment.
You can enable/disable the change via feature toggles, and have confirmed these toggles work as expected.
Technical review: A Principal Engineer, Engineering Manager or Head of Engineering have green-lit your changes, and the reviewer is named in the description/comments.
Application Owner (Director+) approval is given about the PR, and the approver is named in the description/comments.

👉 Regardless of which boxes you click in this comment, merge/deployment will not be blocked.
Reports about deployment policy adherence will be circulated daily.

ponimas · 2025-12-04T09:25:47Z

👍

Test: reduce logging in test Test: fix flaky test that depend on tcp.Listen to find a free port to use in tests. The implementation was done twice and had race conditions caused by a TOCTOU Test: fix flakytest TestRedisAddrUpdater --------- Signed-off-by: Sandor Szücs <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

feature: allow to set redis username fix: reduce logging and remove unnecessary roundtrips to redis redis/go-redis#3536 ref: redis/go-redis#3536 (comment) Signed-off-by: Sandor Szücs <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

follow up on zalando#3737 Signed-off-by: Mustafa Abdelrahman <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

follow up on zalando#3740 Signed-off-by: Mustafa Abdelrahman <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Signed-off-by: Remy Chantenay <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

…parameters filtering This change allows to use several maskAccessLogQuery filters in one route. This allows us to have default masked values in prepend filters. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

…ions Replace google/go-cmp with stretchr/testify in accessLogFormat control tests. The change also improves readability by replacing nested conditional statements with straightforward assertions. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

The commit corrects a bug in the AccessLogFilter.Request method where an incorrect type assertion was being performed. The code was attempting to assert a value from the state bag as an AccessLogFilter value rather than a pointer (*AccessLogFilter), which would cause runtime errors when two or more instances of the filter are used. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

This commit adds tests to verify that the access log control filters correctly merge masked query parameters from multiple filters. The tests validate that parameters from different filters are correctly combined and that repeated parameters are properly handled. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

This commit removes the redundant array type declaration in the test files by using the short syntax for array literals. The Go compiler can infer the type from the context. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Using map[string]struct{} for MaskedQueryParams to represent a set of query parameters more efficiently. This is a common Go pattern for implementing sets, where we care only about the existence of a key and not any associated value. The empty struct requires no memory allocation for values. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

This change replaces a classic for loop indexing with a more idiomatic range iteration over values in the maskAccessLogQuery.CreateFilter method. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

This test was testing the xxhash.Sum64String library function Signed-off-by: Aleksandr Ponimaskin <[email protected]>

This commit simplifies the logic in maskQueryParams by removing an unnecessary intermediate variable for the hashed value and directly using the hash function result in the params.Set() call. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Consolidated nested if statements in access.go for better readability while preserving the same functionality. The code now checks both conditions (keys exist and length > 0) in a single if statement rather than using nested conditionals. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Replace `struct{}{}` with `{}` in map declarations for the `struct{}` type in the test files. This uses Go's shorthand struct literal syntax, making the code more concise and readable while maintaining the same functionality. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

This commit moves the responsibility for managing masked query parameters from the proxy to the AccessLogFilter. It properly stores masquerading information in the state bag under the AccessLogAdditionalDataKey, ensuring better isolation between components. A new constant KeyMaskedQueryParams has been moved from logging to accesslog package for better organization, and the code now uses maps.Copy for compatibility. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

The test was incorrectly comparing the entire AccessLogFilter struct when it should only be comparing the masked query parameters map. This change updates the test to correctly access the masked query parameters from the state bag and compare only that specific field. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Signed-off-by: Aleksandr Ponimaskin <[email protected]>

ponimas force-pushed the add-mask-access-log-query-filter branch 3 times, most recently from 50aca5c to fcff629 Compare April 7, 2025 13:39

ponimas added the minor no risk changes, for example new filters label Apr 7, 2025

ponimas force-pushed the add-mask-access-log-query-filter branch from b844976 to 7af633f Compare April 7, 2025 14:42

ponimas changed the title ~~filters/accesslog: Add mask access log query filter~~ filters/accesslog: Add maskAccessLogAuery filter Apr 7, 2025

ponimas force-pushed the add-mask-access-log-query-filter branch 2 times, most recently from 3fb7525 to c69d2c7 Compare April 7, 2025 15:52

ponimas marked this pull request as ready for review April 7, 2025 15:57

ponimas changed the title ~~filters/accesslog: Add maskAccessLogAuery filter~~ filters/accesslog: Add maskAccessLogQuery filter Apr 7, 2025

MustafaSaber reviewed Apr 9, 2025

View reviewed changes

filters/accesslog/control.go Outdated Show resolved Hide resolved

MustafaSaber reviewed Apr 9, 2025

View reviewed changes

filters/accesslog/control.go Outdated Show resolved Hide resolved

MustafaSaber reviewed Apr 9, 2025

View reviewed changes

logging/access.go Outdated Show resolved Hide resolved

MustafaSaber reviewed Apr 9, 2025

View reviewed changes

logging/access_test.go Outdated Show resolved Hide resolved

ponimas force-pushed the add-mask-access-log-query-filter branch from ff42068 to 46828d4 Compare April 9, 2025 11:40

AlexanderYastrebov reviewed Apr 10, 2025

View reviewed changes

ponimas marked this pull request as draft April 10, 2025 12:44

ponimas force-pushed the add-mask-access-log-query-filter branch from f6d8b28 to 5e2ba42 Compare April 10, 2025 17:36

ponimas marked this pull request as ready for review April 11, 2025 20:22

szuecs added major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs and removed minor no risk changes, for example new filters labels Nov 4, 2025

ponimas force-pushed the add-mask-access-log-query-filter branch from 5e2ba42 to 2b2d1a8 Compare November 5, 2025 12:48

ponimas force-pushed the add-mask-access-log-query-filter branch from 2b2d1a8 to 92f8424 Compare November 24, 2025 16:26

github-actions bot added the documentation label Nov 24, 2025

ponimas force-pushed the add-mask-access-log-query-filter branch from 92f8424 to f2101d8 Compare November 24, 2025 16:27

ponimas force-pushed the add-mask-access-log-query-filter branch from 4e29ed5 to 37c6cfb Compare November 26, 2025 10:02

github-actions bot added the codefreeze This label is marked for convenience if the base branch is cw2025 label Nov 26, 2025

szuecs force-pushed the cw2025 branch 2 times, most recently from e298dd4 to 264ca18 Compare December 2, 2025 16:40

ponimas changed the base branch from cw2025 to master December 3, 2025 16:21

ponimas force-pushed the add-mask-access-log-query-filter branch from 37c6cfb to e2dce2c Compare December 3, 2025 16:23

ponimas requested a review from szuecs December 3, 2025 16:24

szuecs and others added 21 commits December 4, 2025 10:28

actions: add workflow to enable labeler (zalando#3740)

f08d551

follow up on zalando#3737 Signed-off-by: Mustafa Abdelrahman <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

gh-action: fix security scan and labeler syntax (zalando#3746)

e961c01

follow up on zalando#3740 Signed-off-by: Mustafa Abdelrahman <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

New filter: maskAccessLogQuery

80c2d38

Signed-off-by: Remy Chantenay <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Replace murmur3 with xxhash for hashing

da869b1

Signed-off-by: Remy Chantenay <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

Update approach to passing masked keys

258d600

Signed-off-by: Remy Chantenay <[email protected]> Signed-off-by: Aleksandr Ponimaskin <[email protected]>

filters/accesslog: Simplify accesslog control loop with range value

976e9b0

This change replaces a classic for loop indexing with a more idiomatic range iteration over values in the maskAccessLogQuery.CreateFilter method. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

logging: removed TestHashQueryParamValue test

7728d32

This test was testing the xxhash.Sum64String library function Signed-off-by: Aleksandr Ponimaskin <[email protected]>

logging: Remove unnecessary variable

990e6eb

This commit simplifies the logic in maskQueryParams by removing an unnecessary intermediate variable for the hashed value and directly using the hash function result in the params.Set() call. Signed-off-by: Aleksandr Ponimaskin <[email protected]>

docs: add maskAccessLogQuery filter to documentation

ae3afc1

Signed-off-by: Aleksandr Ponimaskin <[email protected]>

ponimas force-pushed the add-mask-access-log-query-filter branch from e2dce2c to ae3afc1 Compare December 4, 2025 09:29

filters/accesslog: Add maskAccessLogQuery filter #3471

Are you sure you want to change the base?

filters/accesslog: Add maskAccessLogQuery filter #3471

Uh oh!

Conversation

ponimas commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szuecs commented Nov 4, 2025

Uh oh!

zalando-robot commented Nov 24, 2025

Deployment Checklist

Uh oh!

ponimas commented Dec 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

filters/accesslog: Add `maskAccessLogQuery` filter #3471

filters/accesslog: Add `maskAccessLogQuery` filter #3471

ponimas commented Apr 7, 2025 •

edited

Loading