From 2c13a8409cf2a4875b2c1fa66d9aa8437ef85da0 Mon Sep 17 00:00:00 2001 From: Kirill Trofimenkov Date: Mon, 28 Jun 2021 16:33:25 +0300 Subject: [PATCH] Rewrite clean-nginx-conf.sh in Go to speed up admission webhook (#7076) * 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 | 1 + .../ingress/controller/template/template.go | 104 +++++++++- .../controller/template/template_test.go | 41 ++++ rootfs/Dockerfile | 1 - rootfs/ingress-controller/clean-nginx-conf.sh | 27 --- rootfs/ingress-controller/indent.sh | 22 --- test/data/cleanConf.expected.conf | 139 +++++++++++++ test/data/cleanConf.src.conf | 187 ++++++++++++++++++ 8 files changed, 462 insertions(+), 60 deletions(-) delete mode 100755 rootfs/ingress-controller/clean-nginx-conf.sh delete mode 100755 rootfs/ingress-controller/indent.sh create mode 100644 test/data/cleanConf.expected.conf create mode 100644 test/data/cleanConf.src.conf diff --git a/go.mod b/go.mod index 181e72f7de..b3e3ac7036 100644 --- a/go.mod +++ b/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 diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 98e737a634..3aa521c877 100644 --- a/internal/ingress/controller/template/template.go +++ b/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 diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 1af9881970..411b243492 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/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) + } +} diff --git a/rootfs/Dockerfile b/rootfs/Dockerfile index 49abbdac2b..7fc8cfee3e 100644 --- a/rootfs/Dockerfile +++ b/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 / diff --git a/rootfs/ingress-controller/clean-nginx-conf.sh b/rootfs/ingress-controller/clean-nginx-conf.sh deleted file mode 100755 index 07900981e1..0000000000 --- a/rootfs/ingress-controller/clean-nginx-conf.sh +++ /dev/null @@ -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 diff --git a/rootfs/ingress-controller/indent.sh b/rootfs/ingress-controller/indent.sh deleted file mode 100755 index 83c0f00057..0000000000 --- a/rootfs/ingress-controller/indent.sh +++ /dev/null @@ -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