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

Add ability to customize upstream and stream log format #352

Merged
merged 1 commit into from
Mar 1, 2017

Conversation

gianrubio
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 27, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 45.73% when pulling 60080d3e101e42f86a3cf22ba3ccdb177323ef0f on gianrubio:customize-logformat into 376519e on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Feb 27, 2017

@gianrubio please set the default value like the ssl ciphers (to avoid the if else logic in the template)

@gianrubio
Copy link
Contributor Author

@aledbf I can do this for LogFormatStream but not for LogFormatUpstream. Because this var has an if condition

logFormatUpstream = "'{{ if $cfg.UseProxyProtocol }}$proxy_protocol_addr{{ else }}$remote_addr{{ end }} - [$proxy_add_x_forwarded_for] - $remote_user [$time_local] \"$request\" $status $body_bytes_sent \"$http_referer\" \"$http_user_agent\" $request_length $request_time [$proxy_upstream_name] $upstream_addr $upstream_response_length $upstream_response_time $upstream_status'"

@aledbf
Copy link
Member

aledbf commented Feb 28, 2017

@gianrubio add all the content in the string and add a comment saying that is a golang template because of the conditional variables. If this introduce issues we can iterate and change in a future PR

@gianrubio gianrubio force-pushed the customize-logformat branch 6 times, most recently from b0e9b11 to d61fe32 Compare February 28, 2017 17:33
@gianrubio
Copy link
Contributor Author

@aledbf I already did your first suggestion, could you review again?

@aledbf aledbf self-assigned this Feb 28, 2017
@aledbf
Copy link
Member

aledbf commented Feb 28, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017
@gianrubio gianrubio force-pushed the customize-logformat branch 2 times, most recently from b13f3f4 to 19a791d Compare March 1, 2017 07:43
@gianrubio
Copy link
Contributor Author

@aledbf I tried to push the code again because the test was receiving a 504 from coveralls. But looks like travis has some problem after the s3 outage.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 45.381% when pulling 0ca3aef on gianrubio:customize-logformat into fb8e2d7 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 45.381% when pulling 0ca3aef on gianrubio:customize-logformat into fb8e2d7 on kubernetes:master.

@aledbf aledbf merged commit b1ba356 into kubernetes:master Mar 1, 2017
@aledbf
Copy link
Member

aledbf commented Mar 1, 2017

@gianrubio thanks!

@rikatz
Copy link
Contributor

rikatz commented Jun 9, 2017

Hey @gianrubio, I'm looking exactly for something like this, as I need to customize my log format to contain also the namespaces :)

Do you have the docs for this anywhere? (changing the logFormat in configMap).

Thanks!!

cc @mrrandrade @jcmoraisjr

@gianrubio
Copy link
Contributor Author

@rikatz this is my current log format. I'm using json as output so fluentd doesn't need to parse the log.

apiVersion: v1
data: 
  log-format-upstream: '{ "time": "$time_iso8601", "remote_addr": "$proxy_protocol_addr",
    "x-forward-for": "$proxy_add_x_forwarded_for", "request_id": "$request_id", "remote_user":
    "$remote_user", "bytes_sent": $bytes_sent, "request_time": $request_time, "status":
    $status, "vhost": "$host", "request_proto": "$server_protocol", "path": "$uri",
    "request_query": "$args", "request_length": $request_length, "duration": $request_time,
    "method": "$request_method", "http_referrer": "$http_referer", "http_user_agent":
    "$http_user_agent" }'  
kind: ConfigMap
metadata:  
  name: nginx-load-balancer-conf
  namespace: kube-system

Are you interested to log the current ingress namespace? So just hardcode the value. Ex:

   log-format-upstream: '{ "namespace": "kube-system"....

Otherwise you can log the upstream (that contains the namespace in the name) using the $proxy_host variable.

Ex :

 upstream logging-kibana-80 {
        least_conn;
        server 10.0.0.1:5601 max_fails=0 fail_timeout=0;
    }
log-format-upstream: '{ "upstream": "$proxy_host",....

@rikatz
Copy link
Contributor

rikatz commented Jun 9, 2017

@gianrubio yeah, this is nice. I was missing this in the [docs] (https://github.com/kubernetes/ingress/blob/5d17c7cc173b93be216ad70da22307d048494b63/controllers/nginx/configuration.md#allowed-parameters-in-configuration-configmap) so I've asked here as I didn't know the right parameter :D

I'll think in something here, like defining a 'set' directive with the namespace inside each vhost, and then this could be used in the logging.

But this needs some code change, and I don't know whether this is good or not to ingress. But will try to figure out here.

Thanks!!!

@gianrubio
Copy link
Contributor Author

@rikatz thanks for notice, I did a PR #854 with the docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants