Skip to content

logging: Experimental except option for delete filter #3711

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.

Sign up for GitHub

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

Closed
francislavoie wants to merge 1 commit into from
Closed

logging: Experimental except option for delete filter #3711

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 46 additions & 4 deletions modules/logging/filters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package logging

import (
"net"
"reflect"
"strconv"

"github.com/caddyserver/caddy/v2"
Expand All @@ -36,7 +37,12 @@ type LogFieldFilter interface {

// DeleteFilter is a Caddy log field filter that
// deletes the field.
type DeleteFilter struct{}
type DeleteFilter struct {
// The list of subfields to keep
ExceptRaw []string `json:"except,omitempty"`

except map[string]bool
}

// CaddyModule returns the Caddy module information.
func (DeleteFilter) CaddyModule() caddy.ModuleInfo {
Expand All @@ -47,13 +53,48 @@ func (DeleteFilter) CaddyModule() caddy.ModuleInfo {
}

// UnmarshalCaddyfile sets up the module from Caddyfile tokens.
func (DeleteFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
func (m *DeleteFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
for d.Next() {
for d.NextBlock(0) {
switch d.Val() {
case "except":
m.ExceptRaw = d.RemainingArgs()
if len(m.ExceptRaw) < 1 {
return d.ArgErr()
}

default:
return d.Errf("unrecognized subdirective %s", d.Val())
}
}
}
return nil
}

// Provision converts the except list to a map for fast lookup.
func (d *DeleteFilter) Provision(ctx caddy.Context) error {
d.except = make(map[string]bool)
for _, val := range d.ExceptRaw {
d.except[val] = true
}
return nil
}

// Filter filters the input field.
func (DeleteFilter) Filter(in zapcore.Field) zapcore.Field {
in.Type = zapcore.SkipType
func (d DeleteFilter) Filter(in zapcore.Field) zapcore.Field {
if len(d.except) == 0 {
in.Type = zapcore.SkipType
return in
}

v := reflect.ValueOf(in.Interface)
if v.Kind() == reflect.Map {
Copy link
Copy Markdown
Member

@mholt mholt Sep 15, 2020

Choose a reason for hiding this comment

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

What if it's a struct/object with fields instead of a map with keys?

Copy link
Copy Markdown
Member Author

@francislavoie francislavoie Sep 15, 2020

Choose a reason for hiding this comment

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

I don't know. I didn't look that deeply to see how that would work. I was primarily concerned with the header usecase because that sounds like the most common usecase. Can you think of an example field where that would apply, that I could test for?

Copy link
Copy Markdown
Member

@mholt mholt Sep 15, 2020

Choose a reason for hiding this comment

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

The request field (parent of header, I think) is a struct/object. Granted, they're not as dynamic since they're strongly typed, but I can see users might want to delete the whole object except for one property.

Honestly this is fine, we can do more based on feature requests if necessary.

Is this still a draft?

Copy link
Copy Markdown
Member Author

@francislavoie francislavoie Sep 15, 2020

Choose a reason for hiding this comment

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

If you're happy with this, then cool.

I'm still a bit concerned about the part where we're straight up modifying the http.Request, seems "destructive". But if this is always the last thing in the middleware chain, then I guess it's fine? But 😬

Anyways - we should merge the other filter PR first, then I'll un-draft this and rebase, to keep those separate.

Copy link
Copy Markdown
Member

@mholt mholt Sep 15, 2020

Choose a reason for hiding this comment

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

Okay... thanks for bringing that up again. Yeah, modifying the underlying map is going to be a problem. You can verify this with:

respond {header.User-Agent}

(Obviously in conjunction with filtered logs.) And I bet you will not see anything in the response, despite clients setting it.

Copy link
Copy Markdown
Member Author

@francislavoie francislavoie Sep 15, 2020

Choose a reason for hiding this comment

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

Yeah, confirmed. respond {header.User-Agent} returns empty when except is used.

I don't know how we could do this otherwise...

Copy link
Copy Markdown
Member

@mholt mholt Sep 15, 2020

Choose a reason for hiding this comment

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

The reason this isn't working is because it's a hack on the design. The proper way to do this is to have some other Caddy app module (or any other external log aggregator service) ingest the logs and filter them after being encoded. I don't know another way to do it.

for _, key := range v.MapKeys() {
if _, ok := d.except[key.String()]; !ok {
v.SetMapIndex(key, reflect.Value{})
}
}
}
return in
}

Expand Down Expand Up @@ -161,5 +202,6 @@ var (
_ caddyfile.Unmarshaler = (*DeleteFilter)(nil)
_ caddyfile.Unmarshaler = (*IPMaskFilter)(nil)

_ caddy.Provisioner = (*DeleteFilter)(nil)
_ caddy.Provisioner = (*IPMaskFilter)(nil)
)