filters/accesslog: Add maskAccessLogQuery filter
#3471
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
2 times, most recently
from
72d732b to
50aca5c
Compare
Signed-off-by: Remy Chantenay <[email protected]>
Signed-off-by: Remy Chantenay <[email protected]>
Signed-off-by: Remy Chantenay <[email protected]>
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
from
50aca5c to
fcff629
Compare
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
from
b844976 to
7af633f
Compare
ponimas
changed the title
maskAccessLogAuery filter
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
from
018897d to
3fb7525
Compare
…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]>
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
from
3fb7525 to
c69d2c7
Compare
ponimas
changed the title
maskAccessLogAuery filtermaskAccessLogQuery filter
MustafaSaber
reviewed
Apr 9, 2025
MustafaSaber
reviewed
Apr 9, 2025
MustafaSaber
reviewed
Apr 9, 2025
| } else { | ||
| return | ||
| } | ||
| require.Error(t, err, "Expected error creating filter") |
There was a problem hiding this comment.
minor: you can also use require.Condition to remove the outer if.
There was a problem hiding this comment.
Seems that it will look more complex with Condition
MustafaSaber
reviewed
Apr 9, 2025
MustafaSaber
reviewed
Apr 9, 2025
logging/access.go
Outdated
| @@ -109,6 +115,29 @@ func stripQueryString(u string) string { | |||
| } | |||
| } | |||
|
|
|||
| // 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 { | |||
There was a problem hiding this comment.
I think we can also inline map len check here and return early
There was a problem hiding this comment.
But this function returns string, means we need to allocate new one even for early return.
MustafaSaber
reviewed
Apr 9, 2025
logging/access.go
Outdated
| @@ -144,6 +173,10 @@ func LogAccess(entry *AccessEntry, additional map[string]interface{}) { | |||
| uri = entry.Request.RequestURI | |||
| if stripQuery { | |||
| uri = stripQueryString(uri) | |||
| } else if keys, ok := additional[KeyMaskedQueryParams].(map[string]bool); ok { | |||
| if len(keys) > 0 { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
the link is broken for me
There was a problem hiding this comment.
was this one #3471 (comment)
MustafaSaber
reviewed
Apr 9, 2025
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]>
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
from
ff42068 to
46828d4
Compare
filters/accesslog/control.go
Outdated
| bag[AccessLogEnabledKey] = al | ||
| if f, ok := bag[AccessLogEnabledKey]; ok { | ||
| a := f.(*AccessLogFilter) | ||
| maps.Copy(a.MaskedQueryParams, al.MaskedQueryParams) |
There was a problem hiding this comment.
This modifies filter instance from parallel requests and will panic.
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]>
ponimas
force-pushed
the
add-mask-access-log-query-filter
branch
from
f6d8b28 to
5e2ba42
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a new
maskAccessLogQueryfilter that masks/obfuscates values of specific sensitive query parameters in access logs.Additionally, refactor
accesslog/control_test.goto use testify/assert for improved test readability.See the previous PR for discussion details - #2674
Closing #2156, #2674