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.

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

Cleanup #373

Merged
merged 4 commits into from
Mar 4, 2017
+27 −58
Merged

Cleanup #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2399be8
Cleanup custom log format configuration
aledbf Mar 4, 2017
3c0fb01
Add warning when the ingress controller uses a custom class
aledbf Mar 4, 2017
1473f64
Remove SPDY reference
aledbf Mar 4, 2017
cd924f5
Avoid duplication of ReadConfig function
aledbf Mar 4, 2017
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
26 changes: 5 additions & 21 deletions controllers/nginx/pkg/cmd/controller/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"time"

"github.com/golang/glog"
"github.com/mitchellh/mapstructure"
"github.com/spf13/pflag"

"k8s.io/kubernetes/pkg/api"
Expand Down Expand Up @@ -186,26 +185,7 @@ func (n NGINXController) BackendDefaults() defaults.Backend {
return d.Backend
}

return n.backendDefaults()
}

func (n *NGINXController) backendDefaults() defaults.Backend {
d := config.NewDefault()
config := &mapstructure.DecoderConfig{
Metadata: nil,
WeaklyTypedInput: true,
Result: &d,
TagName: "json",
}
decoder, err := mapstructure.NewDecoder(config)
if err != nil {
glog.Warningf("unexpected error merging defaults: %v", err)
}
err = decoder.Decode(n.configmap.Data)
if err != nil {
glog.Warningf("unexpected error decoding: %v", err)
}
return d.Backend
return ngx_template.ReadConfig(n.configmap.Data).Backend
}

// isReloadRequired check if the new configuration file is different
Expand Down Expand Up @@ -260,6 +240,10 @@ func (n NGINXController) Info() *ingress.BackendInfo {

// OverrideFlags customize NGINX controller flags
func (n NGINXController) OverrideFlags(flags *pflag.FlagSet) {
ig, err := flags.GetString("ingress-class")
if err == nil && ig != "" && ig != defIngressClass {
glog.Warningf("only Ingress with class %v will be processed by this ingress controller", ig)
}
flags.Set("ingress-class", defIngressClass)
}

Expand Down
34 changes: 11 additions & 23 deletions controllers/nginx/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ limitations under the License.
package config

import (
"fmt"
"runtime"

"github.com/golang/glog"

"fmt"
"k8s.io/ingress/core/pkg/ingress"
"k8s.io/ingress/core/pkg/ingress/defaults"
)
Expand All @@ -47,9 +47,9 @@ const (

gzipTypes = "application/atom+xml application/javascript application/x-javascript application/json application/rss+xml application/vnd.ms-fontobject application/x-font-ttf application/x-web-app-manifest+json application/xhtml+xml application/xml font/opentype image/svg+xml image/x-icon text/css text/plain text/x-component"

logFormatUpstream = "'%v - [$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'"
logFormatUpstream = `[$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"`

logFormatStream = "'$remote_addr [$time_local] $protocol [$ssl_preread_server_name] [$stream_upstream] $status $bytes_sent $bytes_received $session_time'"
logFormatStream = `[$time_local] $protocol [$ssl_preread_server_name] [$stream_upstream] $status $bytes_sent $bytes_received $session_time`

// http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_buffer_size
// Sets the size of the buffer used for sending data.
Expand Down Expand Up @@ -97,11 +97,6 @@ type Configuration struct {
//http://nginx.org/en/docs/http/ngx_http_log_module.html
DisableAccessLog bool `json:"disable-access-log,omitempty"`

// EnableSPDY enables spdy and use ALPN and NPN to advertise the availability of the two protocols
// https://blog.cloudflare.com/open-sourcing-our-nginx-http-2-spdy-code
// By default this is enabled
EnableSPDY bool `json:"enable-spdy"`

// EnableStickySessions enabled sticky sessions using cookies
// https://bitbucket.org/nginx-goodies/nginx-sticky-module-ng
// By default this is disabled
Expand Down Expand Up @@ -255,7 +250,6 @@ func NewDefault() Configuration {
ClientHeaderBufferSize: "1k",
DisableAccessLog: false,
EnableDynamicTLSRecords: true,
EnableSPDY: false,
ErrorLogLevel: errorLevel,
HSTS: true,
HSTSIncludeSubdomains: true,
Expand All @@ -264,7 +258,7 @@ func NewDefault() Configuration {
KeepAlive: 75,
LargeClientHeaderBuffers: "4 8k",
LogFormatStream: logFormatStream,
LogFormatUpstream: BuildLogFormatUpstream(false, ""),
LogFormatUpstream: logFormatUpstream,
MaxWorkerConnections: 16384,
MapHashBucketSize: 64,
ProxyRealIPCIDR: defIPCIDR,
Expand Down Expand Up @@ -307,20 +301,14 @@ func NewDefault() Configuration {
return cfg
}

// BuildLogFormatUpstream format the log_format upstream based on proxy_protocol
func BuildLogFormatUpstream(useProxyProtocol bool, curLogFormatUpstream string) string {

// test if log_format comes from configmap
if curLogFormatUpstream != "" &&
curLogFormatUpstream != fmt.Sprintf(logFormatUpstream, "$proxy_protocol_addr") &&
curLogFormatUpstream != fmt.Sprintf(logFormatUpstream, "$remote_addr") {
return curLogFormatUpstream
}

if useProxyProtocol {
return fmt.Sprintf(logFormatUpstream, "$proxy_protocol_addr")
// 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 •
edited
Loading

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';

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.

}
return fmt.Sprintf(logFormatUpstream, "$remote_addr")
return fmt.Sprintf("$remote_addr - %s", cfg.LogFormatUpstream)
}

// TemplateConfig contains the nginx configuration to render the file nginx.conf
Expand Down
16 changes: 8 additions & 8 deletions controllers/nginx/pkg/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,19 @@ func TestBuildLogFormatUpstream(t *testing.T) {
curLogFormat string
expected string
}{
{true, "", fmt.Sprintf(logFormatUpstream, "$proxy_protocol_addr")},
{false, "", fmt.Sprintf(logFormatUpstream, "$remote_addr")},
{true, "my-log-format", "my-log-format"},
{false, "john-log-format", "john-log-format"},
{true, logFormatUpstream, fmt.Sprintf("$proxy_protocol_addr - %s", logFormatUpstream)},
{false, logFormatUpstream, fmt.Sprintf("$remote_addr - %s", logFormatUpstream)},
{true, "my-log-format", "$proxy_protocol_addr - my-log-format"},
{false, "john-log-format", "$remote_addr - john-log-format"},
}

for _, testCase := range testCases {

result := BuildLogFormatUpstream(testCase.useProxyProtocol, testCase.curLogFormat)

cfg := NewDefault()
cfg.UseProxyProtocol = testCase.useProxyProtocol
cfg.LogFormatUpstream = testCase.curLogFormat
result := cfg.BuildLogFormatUpstream()
if result != testCase.expected {
t.Errorf(" expected %v but return %v", testCase.expected, result)
}

}
}
7 changes: 2 additions & 5 deletions controllers/nginx/pkg/template/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"github.com/golang/glog"

"k8s.io/ingress/controllers/nginx/pkg/config"
nginxconfig "k8s.io/ingress/controllers/nginx/pkg/config"
"k8s.io/ingress/core/pkg/ingress"
ing_net "k8s.io/ingress/core/pkg/net"
"k8s.io/ingress/core/pkg/watch"
Expand Down Expand Up @@ -229,14 +228,12 @@ func buildAuthLocation(input interface{}) string {
}

func buildLogFormatUpstream(input interface{}) string {
config, ok := input.(config.Configuration)

cfg, ok := input.(config.Configuration)
if !ok {
glog.Errorf("error an ingress.buildLogFormatUpstream type but %T was returned", input)
}

return nginxconfig.BuildLogFormatUpstream(config.UseProxyProtocol, config.LogFormatUpstream)

return cfg.BuildLogFormatUpstream()
}

// buildProxyPass produces the proxy pass string, if the ingress has redirects
Expand Down
2 changes: 1 addition & 1 deletion controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ http {

server_tokens {{ if $cfg.ShowServerTokens }}on{{ else }}off{{ end }};

log_format upstreaminfo {{ buildLogFormatUpstream $cfg }};
log_format upstreaminfo '{{ buildLogFormatUpstream $cfg }}';

{{/* map urls that should not appear in access.log */}}
{{/* http://nginx.org/en/docs/http/ngx_http_log_module.html#access_log */}}
Expand Down