From 95f8da6c6fbe0165dea8559d046a1f41873b8401 Mon Sep 17 00:00:00 2001
From: oseoin <oseoin@users.noreply.github.com>
Date: Thu, 21 Dec 2023 14:09:59 +0000
Subject: [PATCH] Add the ability to have Nginx version checks in templates
 (#4831)

Add the ability to add version dependent template elements
---
 cmd/nginx-ingress/main.go                  | 17 ++--
 internal/configs/config_params.go          |  6 +-
 internal/configs/configmaps.go             |  1 +
 internal/configs/configurator.go           |  1 +
 internal/configs/configurator_test.go      |  2 +
 internal/configs/version1/config.go        |  3 +
 internal/configs/version1/nginx-plus.tmpl  |  6 ++
 internal/configs/version1/template_test.go | 49 +++++++++++
 internal/nginx/fake_manager.go             |  4 +-
 internal/nginx/manager.go                  | 97 +++++++++++++++++++++-
 internal/nginx/manager_test.go             | 94 +++++++++++++++++++++
 11 files changed, 266 insertions(+), 14 deletions(-)
 create mode 100644 internal/nginx/manager_test.go

diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go
index 40339c46e7..8bc440e1c5 100644
--- a/cmd/nginx-ingress/main.go
+++ b/cmd/nginx-ingress/main.go
@@ -79,7 +79,7 @@ func main() {
 		appProtectVersion = getAppProtectVersionInfo()
 	}
 
-	updateSelfWithVersionInfo(kubeClient, version, nginxVersion, appProtectVersion)
+	updateSelfWithVersionInfo(kubeClient, version, nginxVersion.String(), appProtectVersion)
 
 	templateExecutor, templateExecutorV2 := createTemplateExecutors()
 
@@ -118,6 +118,7 @@ func main() {
 		EnableCertManager:              *enableCertManager,
 		DynamicSSLReload:               *enableDynamicSSLReload,
 		StaticSSLPath:                  nginxManager.GetSecretsDir(),
+		NginxVersion:                   nginxVersion,
 	}
 
 	processNginxConfig(staticCfgParams, cfgParams, templateExecutor, nginxManager)
@@ -146,6 +147,7 @@ func main() {
 		IsPrometheusEnabled:       *enablePrometheusMetrics,
 		IsLatencyMetricsEnabled:   *enableLatencyMetrics,
 		IsDynamicSSLReloadEnabled: *enableDynamicSSLReload,
+		NginxVersion:              nginxVersion,
 	})
 
 	controllerNamespace := os.Getenv("POD_NAMESPACE")
@@ -400,17 +402,16 @@ func createNginxManager(managerCollector collectors.ManagerCollector) (nginx.Man
 	return nginxManager, useFakeNginxManager
 }
 
-func getNginxVersionInfo(nginxManager nginx.Manager) string {
-	nginxVersion := nginxManager.Version()
-	isPlus := strings.Contains(nginxVersion, "plus")
-	glog.Infof("Using %s", nginxVersion)
+func getNginxVersionInfo(nginxManager nginx.Manager) nginx.Version {
+	nginxInfo := nginxManager.Version()
+	glog.Infof("Using %s", nginxInfo.String())
 
-	if *nginxPlus && !isPlus {
+	if *nginxPlus && !nginxInfo.IsPlus {
 		glog.Fatal("NGINX Plus flag enabled (-nginx-plus) without NGINX Plus binary")
-	} else if !*nginxPlus && isPlus {
+	} else if !*nginxPlus && nginxInfo.IsPlus {
 		glog.Fatal("NGINX Plus binary found without NGINX Plus flag (-nginx-plus)")
 	}
-	return nginxVersion
+	return nginxInfo
 }
 
 func getAppProtectVersionInfo() string {
diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go
index f76a944663..908463024e 100644
--- a/internal/configs/config_params.go
+++ b/internal/configs/config_params.go
@@ -1,6 +1,9 @@
 package configs
 
-import conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
+import (
+	"github.com/nginxinc/kubernetes-ingress/internal/nginx"
+	conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
+)
 
 // ConfigParams holds NGINX configuration parameters that affect the main NGINX config
 // as well as configs for Ingress resources.
@@ -136,6 +139,7 @@ type StaticConfigParams struct {
 	EnableCertManager              bool
 	DynamicSSLReload               bool
 	StaticSSLPath                  string
+	NginxVersion                   nginx.Version
 }
 
 // GlobalConfigParams holds global configuration parameters. For now, it only holds listeners.
diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go
index 0b653408f9..fbcd63b68d 100644
--- a/internal/configs/configmaps.go
+++ b/internal/configs/configmaps.go
@@ -581,6 +581,7 @@ func GenerateNginxMainConfig(staticCfgParams *StaticConfigParams, config *Config
 		OIDC:                               staticCfgParams.EnableOIDC,
 		DynamicSSLReloadEnabled:            staticCfgParams.DynamicSSLReload,
 		StaticSSLPath:                      staticCfgParams.StaticSSLPath,
+		NginxVersion:                       staticCfgParams.NginxVersion,
 	}
 	return nginxCfg
 }
diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go
index 4d03c03be9..8574589ae4 100644
--- a/internal/configs/configurator.go
+++ b/internal/configs/configurator.go
@@ -145,6 +145,7 @@ type ConfiguratorParams struct {
 	IsWildcardEnabled         bool
 	IsLatencyMetricsEnabled   bool
 	IsDynamicSSLReloadEnabled bool
+	NginxVersion              nginx.Version
 }
 
 // NewConfigurator creates a new Configurator.
diff --git a/internal/configs/configurator_test.go b/internal/configs/configurator_test.go
index a20240f438..6a9720494c 100644
--- a/internal/configs/configurator_test.go
+++ b/internal/configs/configurator_test.go
@@ -25,6 +25,7 @@ func createTestStaticConfigParams() *StaticConfigParams {
 		NginxStatusAllowCIDRs:          []string{"127.0.0.1"},
 		NginxStatusPort:                8080,
 		StubStatusOverUnixSocketForOSS: false,
+		NginxVersion:                   nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 }
 
@@ -53,6 +54,7 @@ func createTestConfigurator(t *testing.T) *Configurator {
 		IsWildcardEnabled:       false,
 		IsPrometheusEnabled:     false,
 		IsLatencyMetricsEnabled: false,
+		NginxVersion:            nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	})
 	cnf.isReloadsEnabled = true
 	return cnf
diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go
index e1569a7bdb..888c19b336 100644
--- a/internal/configs/version1/config.go
+++ b/internal/configs/version1/config.go
@@ -1,5 +1,7 @@
 package version1
 
+import "github.com/nginxinc/kubernetes-ingress/internal/nginx"
+
 // UpstreamLabels describes the Prometheus labels for an NGINX upstream.
 type UpstreamLabels struct {
 	Service           string
@@ -234,6 +236,7 @@ type MainConfig struct {
 	OIDC                               bool
 	DynamicSSLReloadEnabled            bool
 	StaticSSLPath                      string
+	NginxVersion                       nginx.Version
 }
 
 // NewUpstreamWithDefaultServer creates an upstream with the default server.
diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl
index cb7cbd8706..0dbef3471f 100644
--- a/internal/configs/version1/nginx-plus.tmpl
+++ b/internal/configs/version1/nginx-plus.tmpl
@@ -346,3 +346,9 @@ stream {
 
     include /etc/nginx/stream-conf.d/*.conf;
 }
+
+{{- if (.NginxVersion.PlusGreaterThanOrEqualTo "nginx-plus-r31") }}
+mgmt {
+    usage_report interval=0s;
+}
+{{- end}}
diff --git a/internal/configs/version1/template_test.go b/internal/configs/version1/template_test.go
index 395dcd77a8..9172863df2 100644
--- a/internal/configs/version1/template_test.go
+++ b/internal/configs/version1/template_test.go
@@ -5,6 +5,8 @@ import (
 	"strings"
 	"testing"
 	"text/template"
+
+	"github.com/nginxinc/kubernetes-ingress/internal/nginx"
 )
 
 func TestExecuteMainTemplateForNGINXPlus(t *testing.T) {
@@ -20,6 +22,19 @@ func TestExecuteMainTemplateForNGINXPlus(t *testing.T) {
 	t.Log(buf.String())
 }
 
+func TestExecuteMainTemplateForNGINXPlusR31(t *testing.T) {
+	t.Parallel()
+
+	tmpl := newNGINXPlusMainTmpl(t)
+	buf := &bytes.Buffer{}
+
+	err := tmpl.Execute(buf, mainCfgR31)
+	if err != nil {
+		t.Error(err)
+	}
+	t.Log(buf.String())
+}
+
 func TestExecuteMainTemplateForNGINX(t *testing.T) {
 	t.Parallel()
 
@@ -1291,6 +1306,33 @@ var (
 		KeepaliveRequests:        100,
 		VariablesHashBucketSize:  256,
 		VariablesHashMaxSize:     1024,
+		NginxVersion:             nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
+	}
+
+	mainCfgR31 = MainConfig{
+		DefaultHTTPListenerPort:  80,
+		DefaultHTTPSListenerPort: 443,
+		ServerNamesHashMaxSize:   "512",
+		ServerTokens:             "off",
+		WorkerProcesses:          "auto",
+		WorkerCPUAffinity:        "auto",
+		WorkerShutdownTimeout:    "1m",
+		WorkerConnections:        "1024",
+		WorkerRlimitNofile:       "65536",
+		LogFormat:                []string{"$remote_addr", "$remote_user"},
+		LogFormatEscaping:        "default",
+		StreamSnippets:           []string{"# comment"},
+		StreamLogFormat:          []string{"$remote_addr", "$remote_user"},
+		StreamLogFormatEscaping:  "none",
+		ResolverAddresses:        []string{"example.com", "127.0.0.1"},
+		ResolverIPV6:             false,
+		ResolverValid:            "10s",
+		ResolverTimeout:          "15s",
+		KeepaliveTimeout:         "65s",
+		KeepaliveRequests:        100,
+		VariablesHashBucketSize:  256,
+		VariablesHashMaxSize:     1024,
+		NginxVersion:             nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgHTTP2On = MainConfig{
@@ -1317,6 +1359,7 @@ var (
 		KeepaliveRequests:        100,
 		VariablesHashBucketSize:  256,
 		VariablesHashMaxSize:     1024,
+		NginxVersion:             nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgCustomTLSPassthroughPort = MainConfig{
@@ -1342,6 +1385,7 @@ var (
 		VariablesHashMaxSize:    1024,
 		TLSPassthrough:          true,
 		TLSPassthroughPort:      8443,
+		NginxVersion:            nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgWithoutTLSPassthrough = MainConfig{
@@ -1367,6 +1411,7 @@ var (
 		VariablesHashMaxSize:    1024,
 		TLSPassthrough:          false,
 		TLSPassthroughPort:      8443,
+		NginxVersion:            nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgDefaultTLSPassthroughPort = MainConfig{
@@ -1392,6 +1437,7 @@ var (
 		VariablesHashMaxSize:    1024,
 		TLSPassthrough:          true,
 		TLSPassthroughPort:      443,
+		NginxVersion:            nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgCustomDefaultHTTPAndHTTPSListenerPorts = MainConfig{
@@ -1417,6 +1463,7 @@ var (
 		KeepaliveRequests:        100,
 		VariablesHashBucketSize:  256,
 		VariablesHashMaxSize:     1024,
+		NginxVersion:             nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgCustomDefaultHTTPListenerPort = MainConfig{
@@ -1442,6 +1489,7 @@ var (
 		KeepaliveRequests:        100,
 		VariablesHashBucketSize:  256,
 		VariablesHashMaxSize:     1024,
+		NginxVersion:             nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	mainCfgCustomDefaultHTTPSListenerPort = MainConfig{
@@ -1467,6 +1515,7 @@ var (
 		KeepaliveRequests:        100,
 		VariablesHashBucketSize:  256,
 		VariablesHashMaxSize:     1024,
+		NginxVersion:             nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"),
 	}
 
 	// Vars for Mergable Ingress Master - Minion tests
diff --git a/internal/nginx/fake_manager.go b/internal/nginx/fake_manager.go
index 5533fb9180..1e42c51ed8 100644
--- a/internal/nginx/fake_manager.go
+++ b/internal/nginx/fake_manager.go
@@ -101,9 +101,9 @@ func (fm *FakeManager) CreateDHParam(_ string) (string, error) {
 }
 
 // Version provides a fake implementation of Version.
-func (*FakeManager) Version() string {
+func (*FakeManager) Version() Version {
 	glog.V(3).Info("Printing nginx version")
-	return "fake version plus"
+	return Version{}
 }
 
 // Start provides a fake implementation of Start.
diff --git a/internal/nginx/manager.go b/internal/nginx/manager.go
index b4ac67d110..7c5af351fc 100644
--- a/internal/nginx/manager.go
+++ b/internal/nginx/manager.go
@@ -8,6 +8,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 	"time"
@@ -46,6 +47,19 @@ const (
 	appProtectDosAgentStartDebugCmd = "/usr/bin/admd -d --standalone --log debug"
 )
 
+var (
+	re     = regexp.MustCompile(`(?P<name>\S+)/(?P<version>\S+)`)
+	plusre = regexp.MustCompile(`(?P<name>\S+)/(?P<version>\S+).\((?P<plus>\S+plus\S+)\)`)
+)
+
+// Version holds the parsed output from `nginx -v`
+type Version struct {
+	raw    string
+	OSS    string
+	IsPlus bool
+	Plus   string
+}
+
 // ServerConfig holds the config data for an upstream server in NGINX Plus.
 type ServerConfig struct {
 	MaxFails    int
@@ -72,7 +86,7 @@ type Manager interface {
 	CreateDHParam(content string) (string, error)
 	CreateOpenTracingTracerConfig(content string) error
 	Start(done chan error)
-	Version() string
+	Version() Version
 	Reload(isEndpointsUpdate bool) error
 	Quit()
 	UpdateConfigVersionFile(openTracing bool)
@@ -334,13 +348,13 @@ func (lm *LocalManager) Quit() {
 }
 
 // Version returns NGINX version
-func (lm *LocalManager) Version() string {
+func (lm *LocalManager) Version() Version {
 	binaryFilename := getBinaryFileName(lm.debug)
 	out, err := exec.Command(binaryFilename, "-v").CombinedOutput()
 	if err != nil {
 		glog.Fatalf("Failed to get nginx version: %v", err)
 	}
-	return string(out)
+	return NewVersion(string(out))
 }
 
 // UpdateConfigVersionFile writes the config version file.
@@ -432,6 +446,83 @@ func (lm *LocalManager) CreateOpenTracingTracerConfig(content string) error {
 	return nil
 }
 
+// Return the raw Nginx version string from `nginx -v`
+func (v *Version) String() string {
+	return v.raw
+}
+
+// PlusGreaterThanOrEqualTo compares the supplied nginx-plus version string with the Version{} struct
+func (v Version) PlusGreaterThanOrEqualTo(target string) (bool, error) {
+	r, p, err := extractPlusVersionValues(v.String())
+	if err != nil {
+		return false, err
+	}
+	tr, tp, err := extractPlusVersionValues(target)
+	if err != nil {
+		return false, err
+	}
+
+	return (r > tr || (r == tr && p >= tp)), nil
+}
+
+// NewVersion will take the output from `nginx -v` and explodes it into the `nginx.Version` struct
+func NewVersion(line string) Version {
+	matches := re.FindStringSubmatch(line)
+	plusmatches := plusre.FindStringSubmatch(line)
+	nv := Version{
+		raw: line,
+	}
+
+	if len(plusmatches) > 0 {
+		subNames := plusre.SubexpNames()
+		nv.IsPlus = true
+		for i, v := range plusmatches {
+			switch subNames[i] {
+			case "plus":
+				nv.Plus = v
+			case "version":
+				nv.OSS = v
+			}
+		}
+	}
+
+	if len(matches) > 0 {
+		for i, key := range re.SubexpNames() {
+			val := matches[i]
+			if key == "version" {
+				nv.OSS = val
+			}
+		}
+	}
+
+	return nv
+}
+
+// extractPlusVersionValues
+func extractPlusVersionValues(input string) (int, int, error) {
+	var rValue, pValue int
+	re := regexp.MustCompile(`-r(\d+)(?:-p(\d+))?`)
+	matches := re.FindStringSubmatch(input)
+
+	if len(matches) < 2 {
+		return 0, 0, fmt.Errorf("no matches found in the input string")
+	}
+
+	rValue, err := strconv.Atoi(matches[1])
+	if err != nil {
+		return 0, 0, fmt.Errorf("failed to convert rValue to integer: %w", err)
+	}
+
+	if len(matches) > 2 && len(matches[2]) > 0 {
+		pValue, err = strconv.Atoi(matches[2])
+		if err != nil {
+			return 0, 0, fmt.Errorf("failed to convert pValue to integer: %w", err)
+		}
+	}
+
+	return rValue, pValue, nil
+}
+
 // verifyConfigVersion is used to check if the worker process that the API client is connected
 // to is using the latest version of nginx config. This way we avoid making changes on
 // a worker processes that is being shut down.
diff --git a/internal/nginx/manager_test.go b/internal/nginx/manager_test.go
new file mode 100644
index 0000000000..948326b8ae
--- /dev/null
+++ b/internal/nginx/manager_test.go
@@ -0,0 +1,94 @@
+package nginx
+
+import (
+	"testing"
+)
+
+func TestNginxVersionParsing(t *testing.T) {
+	t.Parallel()
+	type testCase struct {
+		input    string
+		expected Version
+	}
+	testCases := []testCase{
+		{
+			input: "nginx version: nginx/1.25.1 (nginx-plus-r30-p1)",
+			expected: Version{
+				raw:    "nginx version: nginx/1.25.1 (nginx-plus-r30-p1)",
+				OSS:    "1.25.1",
+				IsPlus: true,
+				Plus:   "nginx-plus-r30-p1",
+			},
+		},
+		{
+			input: "nginx version: nginx/1.25.3 (nginx-plus-r31)",
+			expected: Version{
+				raw:    "nginx version: nginx/1.25.3 (nginx-plus-r31)",
+				OSS:    "1.25.3",
+				IsPlus: true,
+				Plus:   "nginx-plus-r31",
+			},
+		},
+		{
+			input: "nginx version: nginx/1.25.0",
+			expected: Version{
+				raw:    "nginx version: nginx/1.25.0",
+				OSS:    "1.25.0",
+				IsPlus: false,
+				Plus:   "",
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.input, func(t *testing.T) {
+			actual := NewVersion(tc.input)
+			if actual != tc.expected {
+				t.Errorf("expected %v but got %v", tc.expected, actual)
+			}
+		})
+	}
+}
+
+func TestNginxVersionPlusGreaterThanOrEqualTo(t *testing.T) {
+	t.Parallel()
+	type testCase struct {
+		version  Version
+		input    string
+		expected bool
+	}
+	testCases := []testCase{
+		{
+			version:  NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"),
+			input:    "nginx-plus-r30-p1",
+			expected: true,
+		},
+		{
+			version:  NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30)"),
+			input:    "nginx-plus-r30",
+			expected: true,
+		},
+		{
+			version:  NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"),
+			input:    "nginx-plus-r30",
+			expected: true,
+		},
+		{
+			version:  NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30)"),
+			input:    "nginx-plus-r30-p1",
+			expected: false,
+		},
+		{
+			version:  NewVersion("nginx version: nginx/1.25.1"),
+			input:    "nginx-plus-r30-p1",
+			expected: false,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.input, func(t *testing.T) {
+			actual, _ := tc.version.PlusGreaterThanOrEqualTo(tc.input)
+			if actual != tc.expected {
+				t.Errorf("expected %v but got %v", tc.expected, actual)
+			}
+		})
+	}
+}