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: Implement Caddyfile support for filter encoder #3578

Merged
merged 5 commits into from
Sep 15, 2020

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Jul 14, 2020

Closes #3694

This is essentially done but lacks tests and proper support for the ip_mask filter.

Caddyfile:

:80

log {
	output stdout
	format filter {
		wrap console
		fields {
			request>headers>Authorization delete
			request>headers>Server delete
			request>remote_addr ip_mask {
				ipv4 24
				ipv6 32
			}
		}
	}
}

Adapted JSON:

{
	"logging": {
		"logs": {
			"default": {
				"exclude": [
					"http.log.access.log0"
				]
			},
			"log0": {
				"writer": {
					"output": "stdout"
				},
				"encoder": {
					"fields": {
						"request\u003eheaders\u003eAuthorization": {
							"filter": "delete"
						},
						"request\u003eheaders\u003eServer": {
							"filter": "delete"
						},
						"request\u003eremote_addr": {
							"filter": "ip_mask",
							"ipv4_cidr": 24,
							"ipv6_cidr": 32
						}
					},
					"format": "filter",
					"wrap": {
						"format": "console"
					}
				},
				"include": [
					"http.log.access.log0"
				]
			}
		}
	},
	"apps": {
		"http": {
			"servers": {
				"srv0": {
					"listen": [
						":80"
					],
					"logs": {
						"default_logger_name": "log0"
					}
				}
			}
		}
	}
}

@francislavoie francislavoie added this to the v2.2.0 milestone Jul 14, 2020
@mohammed90
Copy link
Member

Is the \u003e part of the actual adapter output or an artifact of copy-paste/console output?

request​\u003e​headers​\u003e​Authorization

@francislavoie
Copy link
Member Author

Yeah I found it strange, I think it's just Golang's JSON encoder being funky. I don't do anything special here. I asked Matt and he said he's not concerned about it.

@mholt
Copy link
Member

mholt commented Jul 14, 2020

Apparently we could switch to an Encoder instead of json.Marshal if we don't want the HTML escaping:

String values encode as JSON strings coerced to valid UTF-8, replacing invalid bytes with the Unicode replacement rune. So that the JSON will be safe to embed inside HTML <script> tags, the string is encoded using HTMLEscape, which replaces "<", ">", "&", U+2028, and U+2029 are escaped to "\u003c","\u003e", "\u0026", "\u2028", and "\u2029". This replacement can be disabled when using an Encoder, by calling SetEscapeHTML(false).

https://golang.org/pkg/encoding/json/#Marshal

To clarify, however, I'm indifferent about this. It's technically correct as-is. I don't think this will be injected into HTML script tags, so I'm also not worried about that. It's probably fine either way.

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.

LGTM so far!

@mholt
Copy link
Member

mholt commented Aug 4, 2020

When you have a chance, I'm hoping to do a final pass for 2.2 reviews next week. :)

@francislavoie
Copy link
Member Author

francislavoie commented Sep 2, 2020

Okay, this now supports IP filters. I also added support for CIDR in the format that ParseCIDR supports (which tbh required some JSON gymnastics lol, but whatever, I think it's kinda useful, even if the IP portion is thrown away).

:80

log {
    output stdout
    format filter {
        wrap console
        fields {
            request>remote_addr ip_mask {
                ipv4 24
                ipv6 2001:db8::/32
            }
        }
    }
}
{
  "logging": {
    "logs": {
      "default": {
        "exclude": [
          "http.log.access.log0"
        ]
      },
      "log0": {
        "writer": {
          "output": "stdout"
        },
        "encoder": {
          "fields": {
            "request\u003eremote_addr": {
              "filter": "ip_mask",
              "ipv4_cidr": 24,
              "ipv6_cidr": "2001:db8::/32"
            }
          },
          "format": "filter",
          "wrap": {
            "format": "console"
          }
        },
        "include": [
          "http.log.access.log0"
        ]
      }
    }
  },
  "apps": {
    "http": {
      "servers": {
        "srv0": {
          "listen": [
            ":80"
          ],
          "logs": {
            "default_logger_name": "log0"
          }
        }
      }
    }
  }
}

I still need to add a few adapt tests then this should be done.

A task for tomorrow.

@francislavoie francislavoie marked this pull request as ready for review September 3, 2020 06:34
@francislavoie
Copy link
Member Author

Okay, this is all done.

I got rid of that abstraction I made that allows stuff like 2001:db8::/32 because frankly that's pretty dumb. I was working under incorrect assumptions with what the int for CIDR was at first (I thought it was itself the mask, as an int), which is why I went down the rabbithole of implementing that string thing. But it's not useful.

I kept the optimization I made of parsing the int into an IP mask at provision time, it's a super tiny optimization but it removes that overhead from every request being logged, so I think it's good to have. Small wins.

@francislavoie francislavoie modified the milestones: 2.x, v2.2.0 Sep 3, 2020
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.

Kewl, LGTM. Shall we merge this?

@francislavoie
Copy link
Member Author

Yes 😄

@mholt mholt merged commit 309c1fe into caddyserver:master Sep 15, 2020
@utilitynerd
Copy link

A big thank you to @francislavoie for implementing this and @mholt for getting it merged!

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.

Add filter encoder support to caddyfile log directive
4 participants