Rewrite clean-nginx-conf.sh in Go to speed up admission webhook (#7076) (#7322)

* Rewrite clean-nginx-conf.sh to speed up admission webhook

* Less diff with original clean-nginx-conf.sh

* Add error handling, add documentation, add unit test

* indent code

* Don't ignore Getwd() error

go.mod

@@ -20,6 +20,7 @@ require (
 	github.com/onsi/ginkgo v1.16.4
 	github.com/opencontainers/runc v1.0.0-rc92
 	github.com/pkg/errors v0.9.1
+	github.com/pmezard/go-difflib v1.0.0
 	github.com/prometheus/client_golang v1.7.1
 	github.com/prometheus/client_model v0.2.0
 	github.com/prometheus/common v0.14.0

internal/ingress/controller/template/template.go

@@ -23,12 +23,12 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"math/rand" // #nosec
 	"net"
 	"net/url"
 	"os"
-	"os/exec"
 	"reflect"
 	"regexp"
 	"sort"
@@ -50,9 +50,15 @@ import (
 )
 
 const (
-	slash         = "/"
-	nonIdempotent = "non_idempotent"
-	defBufferSize = 65535
+	slash                   = "/"
+	nonIdempotent           = "non_idempotent"
+	defBufferSize           = 65535
+	writeIndentOnEmptyLines = true // backward-compatibility
+)
+
+const (
+	stateCode = iota
+	stateComment
 )
 
 // TemplateWriter is the interface to render a template
@@ -86,6 +92,87 @@ func NewTemplate(file string) (*Template, error) {
 	}, nil
 }
 
+// 1. Removes carriage return symbol (\r)
+// 2. Collapses multiple empty lines to single one
+// 3. Re-indent
+// (ATW: always returns nil)
+func cleanConf(in *bytes.Buffer, out *bytes.Buffer) error {
+	depth := 0
+	lineStarted := false
+	emptyLineWritten := false
+	state := stateCode
+	for {
+		c, err := in.ReadByte()
+		if err != nil {
+			if err == io.EOF {
+				return nil
+			}
+			return err // unreachable
+		}
+
+		needOutput := false
+		nextDepth := depth
+		nextLineStarted := lineStarted
+
+		switch state {
+		case stateCode:
+			switch c {
+			case '{':
+				needOutput = true
+				nextDepth = depth + 1
+				nextLineStarted = true
+			case '}':
+				needOutput = true
+				depth--
+				nextDepth = depth
+				nextLineStarted = true
+			case ' ', '\t':
+				needOutput = lineStarted
+			case '\r':
+			case '\n':
+				needOutput = !(!lineStarted && emptyLineWritten)
+				nextLineStarted = false
+			case '#':
+				needOutput = true
+				nextLineStarted = true
+				state = stateComment
+			default:
+				needOutput = true
+				nextLineStarted = true
+			}
+		case stateComment:
+			switch c {
+			case '\r':
+			case '\n':
+				needOutput = true
+				nextLineStarted = false
+				state = stateCode
+			default:
+				needOutput = true
+			}
+		}
+
+		if needOutput {
+			if !lineStarted && (writeIndentOnEmptyLines || c != '\n') {
+				for i := 0; i < depth; i++ {
+					err = out.WriteByte('\t') // always nil
+					if err != nil {
+						return err
+					}
+				}
+			}
+			emptyLineWritten = !lineStarted
+			err = out.WriteByte(c) // always nil
+			if err != nil {
+				return err
+			}
+		}
+
+		depth = nextDepth
+		lineStarted = nextLineStarted
+	}
+}
+
 // Write populates a buffer using a template with NGINX configuration
 // and the servers and upstreams created by Ingress rules
 func (t *Template) Write(conf config.TemplateConfig) ([]byte, error) {
@@ -110,12 +197,9 @@ func (t *Template) Write(conf config.TemplateConfig) ([]byte, error) {
 
 	// squeezes multiple adjacent empty lines to be single
 	// spaced this is to avoid the use of regular expressions
-	cmd := exec.Command("/ingress-controller/clean-nginx-conf.sh")
-	cmd.Stdin = tmplBuf
-	cmd.Stdout = outCmdBuf
-	if err := cmd.Run(); err != nil {
-		klog.Warningf("unexpected error cleaning template: %v", err)
-		return tmplBuf.Bytes(), nil
+	err = cleanConf(tmplBuf, outCmdBuf)
+	if err != nil {
+		return nil, err
 	}
 
 	return outCmdBuf.Bytes(), nil

internal/ingress/controller/template/template_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package template
 
 import (
+	"bytes"
 	"encoding/base64"
 	"fmt"
 	"io/ioutil"
@@ -29,6 +30,7 @@ import (
 	"testing"
 
 	jsoniter "github.com/json-iterator/go"
+	"github.com/pmezard/go-difflib/difflib"
 	apiv1 "k8s.io/api/core/v1"
 	networking "k8s.io/api/networking/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -178,6 +180,14 @@ proxy_pass http://upstream_balancer;`,
 	}
 )
 
+func getTestDataDir() (string, error) {
+	pwd, err := os.Getwd()
+	if err != nil {
+		return "", err
+	}
+	return path.Join(pwd, "../../../../test/data"), nil
+}
+
 func TestBuildLuaSharedDictionaries(t *testing.T) {
 	invalidType := &ingress.Ingress{}
 	expected := ""
@@ -1576,3 +1586,34 @@ func TestConvertGoSliceIntoLuaTablet(t *testing.T) {
 		}
 	}
 }
+
+func TestCleanConf(t *testing.T) {
+	testDataDir, err := getTestDataDir()
+	if err != nil {
+		t.Error("unexpected error reading conf file: ", err)
+	}
+	actual := &bytes.Buffer{}
+	{
+		data, err := ioutil.ReadFile(testDataDir + "/cleanConf.src.conf")
+		if err != nil {
+			t.Error("unexpected error reading conf file: ", err)
+		}
+		in := bytes.NewBuffer(data)
+		err = cleanConf(in, actual)
+		if err != nil {
+			t.Error("cleanConf failed: ", err)
+		}
+	}
+
+	expected, err := ioutil.ReadFile(testDataDir + "/cleanConf.expected.conf")
+	if err != nil {
+		t.Error("unexpected error reading conf file: ", err)
+	}
+	if !bytes.Equal(expected, actual.Bytes()) {
+		diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{A: strings.SplitAfter(string(expected), "\n"), B: strings.SplitAfter(actual.String(), "\n"), Context: 3})
+		if err != nil {
+			t.Error("failed to get diff for cleanConf", err)
+		}
+		t.Errorf("cleanConf result don't match with expected: %s", diff)
+	}
+}

rootfs/Dockerfile

@@ -40,7 +40,6 @@ RUN apk update \
   && rm -rf /var/cache/apk/*
 
 COPY --chown=www-data:www-data etc /etc
-COPY --chown=www-data:www-data ingress-controller /ingress-controller
 
 COPY --chown=www-data:www-data bin/${TARGETARCH}/dbg /
 COPY --chown=www-data:www-data bin/${TARGETARCH}/nginx-ingress-controller /

rootfs/ingress-controller/clean-nginx-conf.sh

@@ -1,27 +0,0 @@
-#!/bin/bash
-
-# Copyright 2017 The Kubernetes Authors.
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-# This script removes consecutive empty lines in nginx.conf
-# Using sed is more simple than using a go regex
-
-# Sed commands:
-# 1. remove the return carrier character/s
-# 2. remove empty lines
-# 3. replace multiple empty lines
-
-SCRIPT_ROOT=$(dirname ${BASH_SOURCE})
-
-sed -e 's/\r//g' | sed -e 's/^  *$/\'$'\n/g' | sed -e '/^$/{N;/^\n$/D;}' | ${SCRIPT_ROOT}/indent.sh

rootfs/ingress-controller/indent.sh

@@ -1,22 +0,0 @@
-#!/usr/bin/awk -f
-
-# Copyright 2017 The Kubernetes Authors.
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-# Credits to https://evasive.ru/f29bd7ebacf24a50c582f973a55eee28.html
-
-{sub(/^[ \t]+/,"");idx=0}
-/\{/{ctx++;idx=1}
-/\}/{ctx--}
-{id="";for(i=idx;i<ctx;i++)id=sprintf("%s%s", id, "\t");printf "%s%s\n", id, $0}

test/data/cleanConf.expected.conf

@@ -0,0 +1,139 @@
+# Configuration checksum: 
+
+# setup custom paths that do not require root access
+pid /tmp/nginx.pid;
+
+daemon off;
+
+worker_processes 8;
+
+worker_rlimit_nofile 130048;
+
+worker_shutdown_timeout 240s ;
+
+events {
+	multi_accept        on;
+	worker_connections  16384;
+	use                 epoll;
+}
+
+http {
+	lua_package_path "/etc/nginx/lua/?.lua;;";
+	
+	lua_shared_dict balancer_ewma 10M;
+	lua_shared_dict balancer_ewma_last_touched_at 10M;
+	lua_shared_dict balancer_ewma_locks 1M;
+	lua_shared_dict certificate_data 20M;
+	lua_shared_dict certificate_servers 5M;
+	lua_shared_dict configuration_data 20M;
+	lua_shared_dict ocsp_response_cache 5M;
+	
+	init_by_lua_block {
+		collectgarbage("collect")
+		
+		-- init modules
+		local ok, res
+		
+		ok, res = pcall(require, "lua_ingress")
+		if not ok then
+		error("require failed: " .. tostring(res))
+		else
+		lua_ingress = res
+		lua_ingress.set_config({
+			use_forwarded_headers = true,
+			use_proxy_protocol = false,
+			is_ssl_passthrough_enabled = false,
+			http_redirect_code = 308,
+			listen_ports = { ssl_proxy = "442", https = "443" },
+			
+			hsts = true,
+			hsts_max_age = 15724800,
+			hsts_include_subdomains = true,
+			hsts_preload = false,
+		})
+		end
+		
+		ok, res = pcall(require, "monitor")
+		if not ok then
+		error("require failed: " .. tostring(res))
+		else
+		monitor = res
+		end
+		
+	}
+	
+	init_worker_by_lua_block {
+		lua_ingress.init_worker()
+		balancer.init_worker()
+		
+		monitor.init_worker(10000)
+		
+		plugins.run()
+	}
+	
+	map $request_uri $loggable {
+		
+		default 1;
+	}
+	
+	access_log /var/log/nginx/access.log upstreaminfo  if=$loggable;
+	
+	error_log  /var/log/nginx/error.log notice;
+	
+	resolver 169.254.25.10 valid=30s ipv6=off;
+	
+	# See https://www.nginx.com/blog/websocket-nginx
+	map $http_upgrade $connection_upgrade {
+		default          upgrade;
+		
+		# See http://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive
+		''               '';
+		
+	}
+	
+	## start server _
+	server {
+		server_name _ ;
+		
+		listen 80 default_server reuseport backlog=4096 ;
+		listen 443 default_server reuseport backlog=4096 ssl http2 ;
+		
+		set $proxy_upstream_name "-";
+		
+		ssl_certificate_by_lua_block {
+			certificate.call()
+		}
+		
+		location / {
+			
+			set $namespace      "";
+			set $ingress_name   "";
+			set $service_name   "";
+			set $service_port   "";
+			set $location_path  "";
+			
+			rewrite_by_lua_block {
+				lua_ingress.rewrite({
+					force_ssl_redirect = false,
+					ssl_redirect = false,
+					force_no_ssl_redirect = false,
+					use_port_in_redirects = false,
+				})
+				balancer.rewrite()
+				plugins.run()
+			}
+			
+			# be careful with `access_by_lua_block` and `satisfy any` directives as satisfy any
+			# other authentication method such as basic auth or external auth useless - all requests will be allowed.
+			#access_by_lua_block {
+			#}
+			
+			header_filter_by_lua_block {
+				lua_ingress.header()
+				plugins.run()
+			}
+			
+		}
+		## end server _
+		
+	}

test/data/cleanConf.src.conf

@@ -0,0 +1,187 @@
+# Configuration checksum: 
+
+# setup custom paths that do not require root access
+pid /tmp/nginx.pid;
+
+
+
+
+
+
+
+
+
+
+
+
+
+daemon off;
+
+worker_processes 8;
+
+
+worker_rlimit_nofile 130048;
+
+
+
+worker_shutdown_timeout 240s ;
+
+
+
+events {
+    multi_accept        on;
+    worker_connections  16384;
+    use                 epoll;
+}
+
+http {
+    lua_package_path "/etc/nginx/lua/?.lua;;";
+
+    lua_shared_dict balancer_ewma 10M;
+lua_shared_dict balancer_ewma_last_touched_at 10M;
+lua_shared_dict balancer_ewma_locks 1M;
+lua_shared_dict certificate_data 20M;
+lua_shared_dict certificate_servers 5M;
+lua_shared_dict configuration_data 20M;
+lua_shared_dict ocsp_response_cache 5M;
+
+
+    init_by_lua_block {
+        collectgarbage("collect")
+
+        -- init modules
+        local ok, res
+
+        ok, res = pcall(require, "lua_ingress")
+        if not ok then
+          error("require failed: " .. tostring(res))
+        else
+          lua_ingress = res
+          lua_ingress.set_config({
+		use_forwarded_headers = true,
+		use_proxy_protocol = false,
+		is_ssl_passthrough_enabled = false,
+		http_redirect_code = 308,
+		listen_ports = { ssl_proxy = "442", https = "443" },
+
+		hsts = true,
+		hsts_max_age = 15724800,
+		hsts_include_subdomains = true,
+		hsts_preload = false,
+	})
+        end
+
+
+        
+        ok, res = pcall(require, "monitor")
+        if not ok then
+          error("require failed: " .. tostring(res))
+        else
+          monitor = res
+        end
+        
+
+    }
+
+    init_worker_by_lua_block {
+        lua_ingress.init_worker()
+        balancer.init_worker()
+        
+        monitor.init_worker(10000)
+        
+
+        plugins.run()
+    }
+
+    
+    
+    map $request_uri $loggable {
+        
+        default 1;
+    }
+
+    
+    
+    access_log /var/log/nginx/access.log upstreaminfo  if=$loggable;
+    
+    
+
+    
+    error_log  /var/log/nginx/error.log notice;
+    
+
+    resolver 169.254.25.10 valid=30s ipv6=off;
+
+    # See https://www.nginx.com/blog/websocket-nginx
+    map $http_upgrade $connection_upgrade {
+        default          upgrade;
+        
+        # See http://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive
+        ''               '';
+        
+    }
+
+
+    ## start server _
+    server {
+        server_name _ ;
+
+        
+        
+
+        
+        
+        
+
+        listen 80 default_server reuseport backlog=4096 ;
+        listen 443 default_server reuseport backlog=4096 ssl http2 ;
+
+        set $proxy_upstream_name "-";
+
+        ssl_certificate_by_lua_block {
+            certificate.call()
+        }
+
+
+
+        location / {
+            
+            set $namespace      "";
+            set $ingress_name   "";
+            set $service_name   "";
+            set $service_port   "";
+            set $location_path  "";
+
+            
+
+            
+
+            rewrite_by_lua_block {
+                lua_ingress.rewrite({
+		force_ssl_redirect = false,
+		ssl_redirect = false,
+		force_no_ssl_redirect = false,
+		use_port_in_redirects = false,
+	})
+                balancer.rewrite()
+                plugins.run()
+            }
+
+            # be careful with `access_by_lua_block` and `satisfy any` directives as satisfy any
+            # other authentication method such as basic auth or external auth useless - all requests will be allowed.
+            #access_by_lua_block {
+            #}
+
+            header_filter_by_lua_block {
+                lua_ingress.header()
+                plugins.run()
+            }
+
+
+    }
+    ## end server _
+
+    
+
+    
+}