From 5c9c5a301a72fc12b56ee1a975a37f7f125f0b08 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 5 May 2017 12:28:01 -0300 Subject: [PATCH 1/3] Avoid periodic check for secret changes --- core/pkg/ingress/controller/backend_ssl.go | 10 ++++++++-- core/pkg/ingress/controller/backend_ssl_test.go | 5 +++-- core/pkg/ingress/controller/controller.go | 13 ++++++++++--- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/core/pkg/ingress/controller/backend_ssl.go b/core/pkg/ingress/controller/backend_ssl.go index 9587829a1e..c8bd334a08 100644 --- a/core/pkg/ingress/controller/backend_ssl.go +++ b/core/pkg/ingress/controller/backend_ssl.go @@ -34,7 +34,7 @@ import ( // syncSecret keeps in sync Secrets used by Ingress rules with the files on // disk to allow copy of the content of the secret to disk to be used // by external processes. -func (ic *GenericController) syncSecret() { +func (ic *GenericController) syncSecret(key string) { glog.V(3).Infof("starting syncing of secrets") if !ic.controllersInSync() { @@ -46,7 +46,13 @@ func (ic *GenericController) syncSecret() { var cert *ingress.SSLCert var err error - keys := ic.secretTracker.List() + // by default we sync just one secret + keys := []interface{}{key} + // if the key is empty we check all the secrets + if key == "" { + keys = ic.secretTracker.List() + } + for _, k := range keys { key := k.(string) cert, err = ic.getPemCertificate(key) diff --git a/core/pkg/ingress/controller/backend_ssl_test.go b/core/pkg/ingress/controller/backend_ssl_test.go index 47914fccf9..248bb20b24 100644 --- 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" @@ -166,7 +167,7 @@ func TestSyncSecret(t *testing.T) { ic.secrLister.Add(secret) // for add - ic.syncSecret() + ic.syncSecret("") if foo.expectSuccess { // validate _, exist := ic.sslCertTracker.Get(foo.secretName) @@ -174,7 +175,7 @@ func TestSyncSecret(t *testing.T) { t.Errorf("Failed to sync secret: %s", foo.secretName) } else { // for update - ic.syncSecret() + ic.syncSecret("") } } }) diff --git a/core/pkg/ingress/controller/controller.go b/core/pkg/ingress/controller/controller.go index 3ff7a778f1..1985d44f6d 100644 --- a/core/pkg/ingress/controller/controller.go +++ b/core/pkg/ingress/controller/controller.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/wait" clientset "k8s.io/client-go/kubernetes" unversionedcore "k8s.io/client-go/kubernetes/typed/core/v1" def_api "k8s.io/client-go/pkg/api" @@ -203,6 +202,12 @@ func newIngressController(config *Configuration) *GenericController { } secrEventHandler := cache.ResourceEventHandlerFuncs{ + UpdateFunc: func(old, cur interface{}) { + if !reflect.DeepEqual(old, cur) { + sec := cur.(*api.Secret) + ic.syncSecret(fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)) + } + }, DeleteFunc: func(obj interface{}) { sec := obj.(*api.Secret) ic.sslCertTracker.Delete(fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)) @@ -1151,6 +1156,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 { @@ -1191,8 +1200,6 @@ func (ic GenericController) Start() { go ic.syncQueue.Run(10*time.Second, ic.stopCh) - go wait.Forever(ic.syncSecret, 10*time.Second) - if ic.syncStatus != nil { go ic.syncStatus.Run(ic.stopCh) } From 4bd4bf3be65eb13166771d375991ccccc3fff664 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 11 May 2017 15:04:19 -0300 Subject: [PATCH 2/3] Fix remote address in log when protocol is https --- controllers/nginx/pkg/cmd/controller/tcp.go | 23 ++++++++----------- controllers/nginx/pkg/config/config.go | 7 ++---- .../rootfs/etc/nginx/template/nginx.tmpl | 16 ++++++++++++- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/controllers/nginx/pkg/cmd/controller/tcp.go b/controllers/nginx/pkg/cmd/controller/tcp.go index 8a95fd0912..e78192b38e 100644 --- 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 index 8c7b67758c..98017d32e8 100644 --- 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/rootfs/etc/nginx/template/nginx.tmpl b/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl index ba4d8f78e3..505a8b5376 100644 --- 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; From a537d2d0faf68b477883c15a2ae23f6e40cb7bdd Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 11 May 2017 21:50:43 -0300 Subject: [PATCH 3/3] Remove secrets from ingress after a Delete event --- controllers/nginx/pkg/cmd/controller/nginx.go | 12 ++++++------ controllers/nginx/pkg/config/config_test.go | 4 ++-- controllers/nginx/pkg/template/configmap_test.go | 3 +-- core/pkg/ingress/controller/backend_ssl.go | 11 ++--------- core/pkg/ingress/controller/backend_ssl_test.go | 4 ++-- core/pkg/ingress/controller/controller.go | 16 +++++++++++----- 6 files changed, 24 insertions(+), 26 deletions(-) diff --git a/controllers/nginx/pkg/cmd/controller/nginx.go b/controllers/nginx/pkg/cmd/controller/nginx.go index 5d9d9c5471..6f4981700d 100644 --- 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/config/config_test.go b/controllers/nginx/pkg/config/config_test.go index 359cb1306d..f0a511c8e5 100644 --- 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 index 9eb658070a..130a452a60 100644 --- 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/core/pkg/ingress/controller/backend_ssl.go b/core/pkg/ingress/controller/backend_ssl.go index c8bd334a08..aadebd8368 100644 --- a/core/pkg/ingress/controller/backend_ssl.go +++ b/core/pkg/ingress/controller/backend_ssl.go @@ -34,7 +34,7 @@ import ( // syncSecret keeps in sync Secrets used by Ingress rules with the files on // disk to allow copy of the content of the secret to disk to be used // by external processes. -func (ic *GenericController) syncSecret(key string) { +func (ic *GenericController) syncSecret() { glog.V(3).Infof("starting syncing of secrets") if !ic.controllersInSync() { @@ -46,14 +46,7 @@ func (ic *GenericController) syncSecret(key string) { var cert *ingress.SSLCert var err error - // by default we sync just one secret - keys := []interface{}{key} - // if the key is empty we check all the secrets - if key == "" { - 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 index 248bb20b24..e7fb991d50 100644 --- a/core/pkg/ingress/controller/backend_ssl_test.go +++ b/core/pkg/ingress/controller/backend_ssl_test.go @@ -167,7 +167,7 @@ func TestSyncSecret(t *testing.T) { ic.secrLister.Add(secret) // for add - ic.syncSecret("") + ic.syncSecret() if foo.expectSuccess { // validate _, exist := ic.sslCertTracker.Get(foo.secretName) @@ -175,7 +175,7 @@ func TestSyncSecret(t *testing.T) { t.Errorf("Failed to sync secret: %s", foo.secretName) } else { // for update - ic.syncSecret("") + ic.syncSecret() } } }) diff --git a/core/pkg/ingress/controller/controller.go b/core/pkg/ingress/controller/controller.go index 1985d44f6d..5f65ddc553 100644 --- a/core/pkg/ingress/controller/controller.go +++ b/core/pkg/ingress/controller/controller.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/wait" clientset "k8s.io/client-go/kubernetes" unversionedcore "k8s.io/client-go/kubernetes/typed/core/v1" def_api "k8s.io/client-go/pkg/api" @@ -204,13 +205,14 @@ func newIngressController(config *Configuration) *GenericController { secrEventHandler := cache.ResourceEventHandlerFuncs{ UpdateFunc: func(old, cur interface{}) { if !reflect.DeepEqual(old, cur) { - sec := cur.(*api.Secret) - ic.syncSecret(fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)) + 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) }, } @@ -1012,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) } } } @@ -1200,6 +1204,8 @@ func (ic GenericController) Start() { go ic.syncQueue.Run(10*time.Second, ic.stopCh) + go wait.Forever(ic.syncSecret, 10*time.Second) + if ic.syncStatus != nil { go ic.syncStatus.Run(ic.stopCh) }