proxy: Extended logging of requests #964
Conversation
When logging L7 requests to the access log using --access-log. The logfile will now contain extended information about the request written as individual JSON objects of type proxy.LogRecord Additional metadata can be passed into using the --agent-labels option to annotate log records Signed-off-by: Thomas Graf <thomas@cilium.io>
tgraf
added
the
kind/enhancement
pkg/proxy/proxy.go
Outdated
| code int | ||
| req *http.Request | ||
| // ProxyConfiguration is used to pass configuration into CreateOrUpdateRedirect | ||
| type ProxyConfiguration struct { |
There was a problem hiding this comment.
type name will be used as proxy.ProxyConfiguration by other packages, and that stutters; consider calling this Configuration
pkg/proxy/record.go
Outdated
| // Direction is the direction of the flow | ||
| Direction FlowDirection | ||
|
|
||
| // StartTime is the timestamp when the flow has started. See Duration |
There was a problem hiding this comment.
Please specify the format as RFC3339Nano. The timestamp must be in UTC and include fraction of a second.
For e.g. time.Now().UTC().Format(time.RFC3339Nano) returns 2017-06-08T21:21:04.698805823Z.
pkg/proxy/record.go
Outdated
|
|
||
| // StartTime is the timestamp when the flow has started. See Duration | ||
| // for more details | ||
| StartTime time.Time |
There was a problem hiding this comment.
Please add an EndTime for response.
There was a problem hiding this comment.
I changed it to a single Timestamp field of type string with nanosecond precision
What's the benefit? |
pkg/proxy/record.go
Outdated
| // DirectionIngress indicates event was generated at ingress | ||
| Ingress ObservationPoint = "Ingress" | ||
|
|
||
| // DirectionEgress indicates event was generated at egress |
There was a problem hiding this comment.
comment on exported const Egress should be of the form "Egress ..."
pkg/proxy/record.go
Outdated
| type ObservationPoint string | ||
|
|
||
| const ( | ||
| // DirectionIngress indicates event was generated at ingress |
There was a problem hiding this comment.
comment on exported const Ingress should be of the form "Ingress ..."
pkg/proxy/record.go
Outdated
| VerdictError = "Error" | ||
| ) | ||
|
|
||
| type ObservationPoint string |
There was a problem hiding this comment.
exported type ObservationPoint should have comment or be unexported
pkg/proxy/record.go
Outdated
| NodeInfo NodeInfo | ||
|
|
||
| // Ingress is true if log record was created at ingress | ||
| Ingress bool |
There was a problem hiding this comment.
Please make this a type backed by a string. It will be easier to incorporate an "unknown" state if needed.
pkg/proxy/record.go
Outdated
|
|
||
| // Source is the IP and port of the endpoint that generated the | ||
| // request | ||
| Source string |
There was a problem hiding this comment.
Can we separate this as SourceIp and SourcePort ? Ditto for Destination
If we change the name of the struct fields, the exported json fieldnames won't be modified. |
Exactly, so we can add a JSON annotation if we ever rename one of the fields. |
| // following should ever be set. Unused fields will be omitted | ||
|
|
||
| // HTTP contains information for HTTP request/responses | ||
| HTTP LogRecordHTTP `json:"HTTP,omitempty"` |
There was a problem hiding this comment.
HTTP *LogRecordHTTP. That will help the code that parses/uses an object of this type to check for nil. Otherwise, there is no way to figure out whether the HTTP object is valid.
| Ingress ObservationPoint = "Ingress" | ||
|
|
||
| // Egress indicates event was generated at egress | ||
| Egress = "Egress" |
There was a problem hiding this comment.
Egresss ObservationPoint = "Egress"
| ) | ||
|
|
||
| // FlowDirection is the type to indicate the flow direction | ||
| type FlowDirection string |
There was a problem hiding this comment.
Please consider renaming this to FlowType. FlowDirection can also mean ingress/egress.
| // Log logs a record to the logfile and flushes the buffer | ||
| func Log(l *LogRecord, direction FlowDirection, verdict FlowVerdict, code int) { | ||
| if logBuf == nil { | ||
| return |
There was a problem hiding this comment.
Log where? This means access logging is disabled.
| ObservationPoint ObservationPoint | ||
|
|
||
| // SourceEndpoint is information about the soure endpoint if available | ||
| SourceEndpoint EndpointInfo `json:"SourceEndpoint,omitempty"` |
| SourceEndpoint EndpointInfo `json:"SourceEndpoint,omitempty"` | ||
|
|
||
| // DestinationEndpoint is information about the soure endpoint if available | ||
| DestinationEndpoint EndpointInfo `json:"DestinationEndpoint,omitempty"` |
When logging L7 requests to the access log using --access-log. The
logfile will now contain extended information about the request written
as individual JSON objects of type proxy.LogRecord
Additional metadata can be passed into using the --agent-labels option
to annotate log records
Signed-off-by: Thomas Graf thomas@cilium.io