From c38109136d0098a8e5cea38b4aedcc3d58f98bec Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Tue, 30 Jan 2024 18:25:07 +0000 Subject: [PATCH 1/2] Add version formatting for NGINX pod label (#5009) --- internal/nginx/manager.go | 87 +------------- internal/nginx/manager_test.go | 94 ---------------- internal/nginx/version.go | 101 +++++++++++++++++ internal/nginx/version_test.go | 199 +++++++++++++++++++++++++++++++++ 4 files changed, 301 insertions(+), 180 deletions(-) delete mode 100644 internal/nginx/manager_test.go create mode 100644 internal/nginx/version.go create mode 100644 internal/nginx/version_test.go diff --git a/internal/nginx/manager.go b/internal/nginx/manager.go index 7c5af351fc..f440386ef5 100644 --- a/internal/nginx/manager.go +++ b/internal/nginx/manager.go @@ -48,18 +48,10 @@ const ( ) var ( - re = regexp.MustCompile(`(?P\S+)/(?P\S+)`) + ossre = regexp.MustCompile(`(?P\S+)/(?P\S+)`) plusre = regexp.MustCompile(`(?P\S+)/(?P\S+).\((?P\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 @@ -446,83 +438,6 @@ 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 deleted file mode 100644 index 948326b8ae..0000000000 --- a/internal/nginx/manager_test.go +++ /dev/null @@ -1,94 +0,0 @@ -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) - } - }) - } -} diff --git a/internal/nginx/version.go b/internal/nginx/version.go new file mode 100644 index 0000000000..58a0da77ce --- /dev/null +++ b/internal/nginx/version.go @@ -0,0 +1,101 @@ +package nginx + +import ( + "fmt" + "regexp" + "strconv" +) + +// Version holds the parsed output from `nginx -v`. +type Version struct { + Raw string + OSS string + IsPlus bool + Plus string +} + +// NewVersion takes the output from `nginx -v` and explodes it into the `nginx.Version` struct. +func NewVersion(line string) Version { + matches := ossre.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 ossre.SubexpNames() { + val := matches[i] + if key == "version" { + nv.OSS = val + } + } + } + + return nv +} + +// String returns the raw Nginx version string from `nginx -v`. +func (v Version) String() string { + return v.Raw +} + +// Format returns a string representing Nginx version. +func (v Version) Format() string { + if v.IsPlus { + return fmt.Sprintf("%s-%s", v.OSS, v.Plus) + } + return v.OSS +} + +// 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 +} + +var rePlus = regexp.MustCompile(`-r(\d+)(?:-p(\d+))?`) + +// extractPlusVersionValues +func extractPlusVersionValues(input string) (int, int, error) { + var rValue, pValue int + matches := rePlus.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 +} diff --git a/internal/nginx/version_test.go b/internal/nginx/version_test.go new file mode 100644 index 0000000000..a9eb7d7f52 --- /dev/null +++ b/internal/nginx/version_test.go @@ -0,0 +1,199 @@ +package nginx_test + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/nginxinc/kubernetes-ingress/internal/nginx" +) + +func TestNginxVersionParsing(t *testing.T) { + t.Parallel() + type testCase struct { + input string + expected nginx.Version + } + testCases := []testCase{ + { + input: "nginx version: nginx/1.25.1 (nginx-plus-r30-p1)", + expected: nginx.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: nginx.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: nginx.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 := nginx.NewVersion(tc.input) + if actual != tc.expected { + t.Errorf("expected %v but got %v", tc.expected, actual) + } + }) + } +} + +func TestNginxVersionFormat(t *testing.T) { + t.Parallel() + + tt := []struct { + input string + want string + }{ + { + input: "nginx version: nginx/1.25.1 (nginx-plus-r30-p1)", + want: "1.25.1-nginx-plus-r30-p1", + }, + { + input: "nginx version: nginx/1.25.3 (nginx-plus-r31)", + want: "1.25.3-nginx-plus-r31", + }, + { + input: "nginx version: nginx/1.25.0", + want: "1.25.0", + }, + } + for _, tc := range tt { + t.Run(tc.input, func(t *testing.T) { + v := nginx.NewVersion(tc.input) + got := v.Format() + if got != tc.want { + t.Errorf("want %q but got %q", tc.want, got) + } + }) + } +} + +func TestNginxVersionPlusGreaterThanOrEqualTo(t *testing.T) { + t.Parallel() + type testCase struct { + version nginx.Version + input string + expected bool + } + testCases := []testCase{ + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"), + input: "nginx-plus-r30-p1", + expected: true, + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30)"), + input: "nginx-plus-r30", + expected: true, + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"), + input: "nginx-plus-r30", + expected: true, + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30)"), + input: "nginx-plus-r30-p1", + expected: false, + }, + { + version: nginx.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) + } + }) + } +} + +func TestNginxVersionPlusGreaterThanOrEqualToFailsOnInalidInput(t *testing.T) { + t.Parallel() + + tt := []struct { + version nginx.Version + input string + }{ + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"), + input: "nginx-plus", + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30)"), + input: "nginxr30", + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"), + input: "", + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30)"), + input: "30", + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1"), + input: "-3", + }, + { + version: nginx.NewVersion("nginx version: nginx/1.25.1 (nginx-plus-r30-p1)"), + input: "nginx-plus-rA", + }, + } + for _, tc := range tt { + t.Run(tc.input, func(t *testing.T) { + _, err := tc.version.PlusGreaterThanOrEqualTo(tc.input) + if err == nil { + t.Errorf("want error, got nil") + } + }) + } +} + +func TestNginxNewVersionHandlesRawString(t *testing.T) { + t.Parallel() + tt := []struct { + name string + input string + want nginx.Version + }{ + { + name: "empty string", + input: "", + want: nginx.Version{Raw: ""}, + }, + { + name: "invalid nginx string", + input: "nginx", + want: nginx.Version{Raw: "nginx"}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got := nginx.NewVersion(tc.input) + if !cmp.Equal(tc.want, got) { + t.Error(cmp.Diff(tc.want, got)) + } + }) + } +} From 81a22338cd3ef4df491c934ece377557be0b4dce Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Wed, 31 Jan 2024 12:00:00 +0000 Subject: [PATCH 2/2] Use NGINX Version method to format the pod label string (#5016) --- cmd/nginx-ingress/main.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index fbd82aed21..fe85e480fa 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -79,7 +79,7 @@ func main() { appProtectVersion = getAppProtectVersionInfo() } - go updateSelfWithVersionInfo(kubeClient, version, nginxVersion.String(), appProtectVersion, 10, time.Second*5) + go updateSelfWithVersionInfo(kubeClient, version, appProtectVersion, nginxVersion, 10, time.Second*5) templateExecutor, templateExecutorV2 := createTemplateExecutors() @@ -789,10 +789,7 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf return cfgParams } -func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, nginxVersion, appProtectVersion string, maxRetries int, waitTime time.Duration) { - nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") - replacer := strings.NewReplacer(" ", "-", "(", "", ")", "") - nginxVer = replacer.Replace(nginxVer) +func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, appProtectVersion string, nginxVersion nginx.Version, maxRetries int, waitTime time.Duration) { podUpdated := false for i := 0; (i < maxRetries || maxRetries == 0) && !podUpdated; i++ { @@ -812,7 +809,7 @@ func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, nginxV labels = make(map[string]string) } - labels[nginxVersionLabel] = nginxVer + labels[nginxVersionLabel] = nginxVersion.Format() labels[versionLabel] = strings.TrimPrefix(version, "v") if appProtectVersion != "" { labels[appProtectVersionLabel] = appProtectVersion