Merge pull request #690 from aledbf/avoid-empty-secret

Fix IP in logs for https traffic
kubernetes · May 12, 2017 · 12d2c4f · 12d2c4f
2 parents 88ca7a5 + a537d2d
commit 12d2c4f
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 34 deletions.
diff --git a/controllers/nginx/pkg/cmd/controller/nginx.go b/controllers/nginx/pkg/cmd/controller/nginx.go
@@ -85,9 +85,9 @@ func newNGINXController() ingress.Controller {
 		resolver:      h,
 		proxy: &proxy{
 			Default: &server{
-				Hostname: "localhost",
-				IP:       "127.0.0.1",
-				Port:     442,
+				Hostname:      "localhost",
+				IP:            "127.0.0.1",
+				Port:          442,
 				ProxyProtocol: true,
 			},
 		},
@@ -534,9 +534,9 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) ([]byte, er
 
 		//TODO: Allow PassthroughBackends to specify they support proxy-protocol
 		servers = append(servers, &server{
-			Hostname: pb.Hostname,
-			IP:       svc.Spec.ClusterIP,
-			Port:     port,
+			Hostname:      pb.Hostname,
+			IP:            svc.Spec.ClusterIP,
+			Port:          port,
 			ProxyProtocol: false,
 		})
 	}

diff --git a/controllers/nginx/pkg/cmd/controller/tcp.go b/controllers/nginx/pkg/cmd/controller/tcp.go
@@ -10,9 +10,9 @@ import (
 )
 
 type server struct {
-	Hostname string
-	IP       string
-	Port     int
+	Hostname      string
+	IP            string
+	Port          int
 	ProxyProtocol bool
 }
 
@@ -41,19 +41,16 @@ func (p *proxy) Handle(conn net.Conn) {
 		return
 	}
 
-	var proxy *server
+	proxy := p.Default
 	hostname, err := parser.GetHostname(data[:])
 	if err == nil {
-		glog.V(3).Infof("parsed hostname from TLS Client Hello: %s", hostname)
+		glog.V(4).Infof("parsed hostname from TLS Client Hello: %s", hostname)
 		proxy = p.Get(hostname)
-		if proxy == nil {
-			return
-		}
-	} else {
-		proxy = p.Default
-		if proxy == nil {
-			return
-		}
+	}
+
+	if proxy == nil {
+		glog.V(4).Infof("there is no configured proxy for SSL connections")
+		return
 	}
 
 	clientConn, err := net.Dial("tcp", fmt.Sprintf("%s:%d", proxy.IP, proxy.Port))

diff --git a/controllers/nginx/pkg/config/config.go b/controllers/nginx/pkg/config/config.go
@@ -48,7 +48,7 @@ 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 = `%v - [$the_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 = `[$time_local] $protocol $status $bytes_sent $bytes_received $session_time`
 
@@ -332,10 +332,7 @@ func NewDefault() Configuration {
 // is enabled.
 func (cfg Configuration) BuildLogFormatUpstream() string {
 	if cfg.LogFormatUpstream == logFormatUpstream {
-		if cfg.UseProxyProtocol {
-			return fmt.Sprintf(cfg.LogFormatUpstream, "$proxy_protocol_addr")
-		}
-		return fmt.Sprintf(cfg.LogFormatUpstream, "$remote_addr")
+		return fmt.Sprintf(cfg.LogFormatUpstream, "$the_x_forwarded_for")
 	}
 
 	return cfg.LogFormatUpstream

diff --git a/controllers/nginx/pkg/config/config_test.go b/controllers/nginx/pkg/config/config_test.go
@@ -28,8 +28,8 @@ func TestBuildLogFormatUpstream(t *testing.T) {
 		curLogFormat     string
 		expected         string
 	}{
-		{true, logFormatUpstream, fmt.Sprintf(logFormatUpstream, "$proxy_protocol_addr")},
-		{false, logFormatUpstream, fmt.Sprintf(logFormatUpstream, "$remote_addr")},
+		{true, logFormatUpstream, fmt.Sprintf(logFormatUpstream, "$the_x_forwarded_for")},
+		{false, logFormatUpstream, fmt.Sprintf(logFormatUpstream, "$the_x_forwarded_for")},
 		{true, "my-log-format", "my-log-format"},
 		{false, "john-log-format", "john-log-format"},
 	}

diff --git a/controllers/nginx/pkg/template/configmap_test.go b/controllers/nginx/pkg/template/configmap_test.go
@@ -76,8 +76,7 @@ func TestMergeConfigMapToStruct(t *testing.T) {
 }
 
 func TestDefaultLoadBalance(t *testing.T) {
-	conf := map[string]string{
-	}
+	conf := map[string]string{}
 	to := ReadConfig(conf)
 	if to.LoadBalanceAlgorithm != "least_conn" {
 		t.Errorf("default load balance algorithm wrong")

diff --git a/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl
@@ -85,6 +85,9 @@ http {
 
     server_tokens {{ if $cfg.ShowServerTokens }}on{{ else }}off{{ end }};
 
+    # disable warnings
+    uninitialized_variable_warn off;
+
     log_format upstreaminfo '{{ buildLogFormatUpstream $cfg }}';
 
     {{/* map urls that should not appear in access.log */}}
@@ -127,6 +130,16 @@ http {
        ''                $server_port;
     }
 
+    map $pass_access_scheme $the_x_forwarded_for {
+       default           $remote_addr;
+       https             $proxy_protocol_addr;
+    }
+
+    map $pass_access_scheme $the_real_ip {
+       default           $remote_addr;
+       https             $proxy_protocol_addr;
+    }
+
     # map port 442 to 443 for header X-Forwarded-Port
     map $pass_server_port $pass_port {
         442              443;
@@ -352,7 +365,8 @@ http {
             proxy_set_header                        Upgrade           $http_upgrade;
             proxy_set_header                        Connection        $connection_upgrade;
 
-            proxy_set_header X-Forwarded-For        $proxy_add_x_forwarded_for;
+            proxy_set_header X-Real-IP              $the_real_ip;
+            proxy_set_header X-Forwarded-For        $the_x_forwarded_for;
             proxy_set_header X-Forwarded-Host       $best_http_host;
             proxy_set_header X-Forwarded-Port       $pass_port;
             proxy_set_header X-Forwarded-Proto      $pass_access_scheme;

diff --git a/core/pkg/ingress/controller/backend_ssl.go b/core/pkg/ingress/controller/backend_ssl.go
@@ -46,8 +46,7 @@ func (ic *GenericController) syncSecret() {
 	var cert *ingress.SSLCert
 	var err error
 
-	keys := ic.secretTracker.List()
-	for _, k := range keys {
+	for _, k := range ic.secretTracker.List() {
 		key := k.(string)
 		cert, err = ic.getPemCertificate(key)
 		if err != nil {

diff --git a/core/pkg/ingress/controller/backend_ssl_test.go b/core/pkg/ingress/controller/backend_ssl_test.go
@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"fmt"
+
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	testclient "k8s.io/client-go/kubernetes/fake"
 	api_v1 "k8s.io/client-go/pkg/api/v1"

diff --git a/core/pkg/ingress/controller/controller.go b/core/pkg/ingress/controller/controller.go
@@ -203,9 +203,16 @@ func newIngressController(config *Configuration) *GenericController {
 	}
 
 	secrEventHandler := cache.ResourceEventHandlerFuncs{
+		UpdateFunc: func(old, cur interface{}) {
+			if !reflect.DeepEqual(old, cur) {
+				ic.syncSecret()
+			}
+		},
 		DeleteFunc: func(obj interface{}) {
 			sec := obj.(*api.Secret)
-			ic.sslCertTracker.Delete(fmt.Sprintf("%v/%v", sec.Namespace, sec.Name))
+			key := fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)
+			ic.sslCertTracker.Delete(key)
+			ic.secretTracker.Delete(key)
 		},
 	}
 
@@ -1007,9 +1014,11 @@ func (ic *GenericController) createServers(data []interface{},
 					} else {
 						glog.Warningf("ssl certificate %v does not contain a common name for host %v", key, host)
 					}
-				} else {
-					glog.Warningf("ssl certificate \"%v\" does not exist in local store", key)
+
+					continue
 				}
+
+				glog.Infof("ssl certificate \"%v\" does not exist in local store", key)
 			}
 		}
 	}
@@ -1151,6 +1160,10 @@ func (ic GenericController) extractSecretNames(ing *extensions.Ingress) {
 	}
 
 	for _, tls := range ing.Spec.TLS {
+		if tls.SecretName == "" {
+			continue
+		}
+
 		key := fmt.Sprintf("%v/%v", ing.Namespace, tls.SecretName)
 		_, exists := ic.secretTracker.Get(key)
 		if !exists {