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

Allow for LogRequestMiddleware to filter the headers it outputs #433

Merged
merged 9 commits into from
May 6, 2024

Conversation

adam-fowler
Copy link
Member

@adam-fowler adam-fowler commented May 1, 2024

Instead of displaying all the headers, only display ones you care about

router.middlewares.add(LogRequestMiddleware(.debug, includesHeaders: .some([.contentType, .contentLength]))

Options are .none, .all or .some([HTTPField.Name])

@adam-fowler adam-fowler requested a review from Joannis May 1, 2024 14:44
Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

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

Good idea but could be better implemented IMO.
I think we should aim for something like what traefik does.
It accepts a defaultMode, and you can then provide overrides for specific headers. The setting options are keep, redact, drop.

@adam-fowler
Copy link
Member Author

Good idea but could be better implemented IMO. I think we should aim for something like what traefik does. It accepts a defaultMode, and you can then provide overrides for specific headers. The setting options are keep, redact, drop.

Given the default mode is none. Not sure I see the point of drop. redact could be of use though, but we could also add a redaction lists for headers we don't want to report the contents of.

@MahdiBM
Copy link
Contributor

MahdiBM commented May 4, 2024

@adam-fowler it's useful if you want to log all headers, but not include a few. I've had usages for this in the past, personally.

Generally, how traefik does it is the most flexible way, so users can just do whatever they want, without us making unnecessary assumptions about their needs.

@adam-fowler
Copy link
Member Author

@adam-fowler it's useful if you want to log all headers, but not include a few. I've had usages for this in the past, personally.

Generally, how traefik does it is the most flexible way, so users can just do whatever they want, without us making unnecessary assumptions about their needs.

So you have two situations I want these specific headers or I want all headers except these ones. Adding an ignore list to .all would provide that list for you.

@MahdiBM
Copy link
Contributor

MahdiBM commented May 4, 2024

Yes, an ignore list would work. But then I'd want to only redact some headers, not ignore them, so I'll need a redact list too

@adam-fowler
Copy link
Member Author

Yes, an ignore list would work. But then I'd want to only redact some headers, not ignore them, so I'll need a redact list too

I'm just looking at the redact list now.

@MahdiBM
Copy link
Contributor

MahdiBM commented May 4, 2024

Example: show me all headers, redact Authorization or other sensitive headers, and ignore all access control headers.

@adam-fowler
Copy link
Member Author

adam-fowler commented May 4, 2024

Example: show me all headers, redact Authorization or other sensitive headers, and ignore all access control headers.

let accessControlHeaders: [HTTPField.Name] = [
  .accessControlAllowCredentials,
  .accessControlAllowHeaders,
  .accessControlAllowMethods,
  ...
]
LogRequestsMiddleware(
  .info, 
  includeHeaders: .all(except: accessControlHeaders),
  redactHeaders: [.authorization]
}

Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

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

One concern, otherwise looks good (and thank you) 🙂

metadata: [
"hb_uri": .stringConvertible(request.uri),
"hb_method": .string(request.method.rawValue),
"hb_headers": .string(self.filterHeaders(headers: request.headers, filter: filter)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of formatting the headers instead of passing an e.g. Array or Dictionary as .stringConvertible.
We use https://github.com/Brainfinance/StackdriverLogging as our swift-log backend which formats stuff properly as a JSON if it can, which makes them look decent on Google Cloud. By passing a String, a logging backend can't try to format the values anymore.

To be clear i haven't seen too many logging backends try to manually format metadata (my own DiscordLogger doesn't either), but it does look useful when done appropriately, like with https://github.com/Brainfinance/StackdriverLogging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok makes sense. I can output a dictionary with values from duplicate keys concatenated with a "," inbetween

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

Looks good.

Sources/Hummingbird/Middleware/LogRequestMiddleware.swift Outdated Show resolved Hide resolved
Sources/Hummingbird/Middleware/LogRequestMiddleware.swift Outdated Show resolved Hide resolved
adam-fowler and others added 3 commits May 6, 2024 08:19
Co-authored-by: Joannis Orlandos <joannis@orlandos.nl>
Co-authored-by: Joannis Orlandos <joannis@orlandos.nl>
@adam-fowler adam-fowler requested a review from Joannis May 6, 2024 07:50
@adam-fowler adam-fowler merged commit 3531f5c into main May 6, 2024
4 of 5 checks passed
@adam-fowler adam-fowler deleted the filter-log-request-headers branch May 6, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants