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

Cleanup #373

Merged
merged 4 commits into from
Mar 4, 2017
Merged

Cleanup #373

merged 4 commits into from
Mar 4, 2017

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Mar 4, 2017

No description provided.

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

This change is Reviewable

@aledbf aledbf force-pushed the log-format branch 2 times, most recently from ffce209 to 5a40495 Compare March 4, 2017 21:23
// BuildLogFormatUpstream format the log_format upstream using
// proxy_protocol_addr as remote client address if UseProxyProtocol
// is enabled.
func (cfg Configuration) BuildLogFormatUpstream() string {
Copy link
Contributor

@gianrubio gianrubio Mar 4, 2017

Choose a reason for hiding this comment

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

@aledbf Don't forget to compare if the logFormatUpstream is coming from configmap. (--log-format-upstream)

Copy link
Member Author

Choose a reason for hiding this comment

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

@gianrubio no need. At this point the configuration contains the default value or the defined in the configmap.

apiVersion: v1
data:
  log-format-upstream: $remote_user
kind: ConfigMap
metadata:
  name: nginx-template
  namespace: default
I0304 21:53:19.723598       5 nginx.go:224] --- /tmp/a080909879	2017-03-04 21:53:19.000000000 +0000
+++ /tmp/b283781162	2017-03-04 21:53:19.000000000 +0000
@@ -56,7 +56,7 @@

     server_tokens on;

-    log_format upstreaminfo '$remote_addr - [$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"';
+    log_format upstreaminfo '$remote_addr - $remote_user';

     map $request_uri $loggable {
         default 1;
I0304 21:53:20.358319       5 controller.go:426] ingress backend successfully reloaded...

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, if I have this configmap

apiVersion: v1
data:
  use-proxy-protocol: "true"
kind: ConfigMap
metadata:
  name: nginx-template
  namespace: default

and change to this?

apiVersion: v1
kind: ConfigMap
data:
  use-proxy-protocol: "true"
  log-format-upstream: $remote_user
metadata:
  name: nginx-template
  namespace: default

ingress will build a wrong log_format

log_format upstreaminfo '$proxy_protocol_addr - $remote_user';

@coveralls
Copy link

Coverage Status

Coverage increased (+1.0%) to 46.358% when pulling cd924f5 on aledbf:log-format into 75124bc on kubernetes:master.

@aledbf aledbf merged commit 84acaec into kubernetes:master Mar 4, 2017
// is enabled.
func (cfg Configuration) BuildLogFormatUpstream() string {
if cfg.UseProxyProtocol {
return fmt.Sprintf("$proxy_protocol_addr - %s", cfg.LogFormatUpstream)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking overrides of the log format and just seems extremely sloppy as users are now stuck with a hardcoded string format ($ip - $customlogformat) regardless of custom logging formats.

Please revert or fix this as i (and i figure lots of people) want to define my own structured logging format and this IP prefix is preventing this.

@aledbf aledbf deleted the log-format branch March 7, 2017 17:46
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. nginx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants