From f19c6e5794b324fda1976d0b36dea67af02685e7 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 3 Jan 2025 11:30:09 -0700 Subject: [PATCH 1/5] Use special reporting JWT for conformance tests --- .github/workflows/conformance.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/conformance.yml b/.github/workflows/conformance.yml index fbf04b5cf9..c89301da54 100644 --- a/.github/workflows/conformance.yml +++ b/.github/workflows/conformance.yml @@ -142,7 +142,7 @@ jobs: - name: Setup license file for plus if: ${{ inputs.image == 'plus' }} env: - PLUS_LICENSE: ${{ secrets.JWT_PLUS_REPORTING }} + PLUS_LICENSE: ${{ secrets.JWT_PLUS_EXCEPTION_REPORTING }} run: echo "${PLUS_LICENSE}" > license.jwt - name: Setup conformance tests From 0b432d96a8398640cca2bb250fdf734d3e71d15a Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 3 Jan 2025 11:30:15 -0700 Subject: [PATCH 2/5] Don't fail on nginx errors --- tests/suite/graceful_recovery_test.go | 93 ++++++++++++++++++--------- tests/suite/reconfig_test.go | 8 ++- 2 files changed, 69 insertions(+), 32 deletions(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index a45b23635a..f82f1252a9 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "os/exec" + "slices" "strings" "time" @@ -28,7 +29,7 @@ const ( ngfContainerName = "nginx-gateway" ) -// Since checkContainerLogsForErrors may experience interference from previous tests (as explained in the function +// Since checkNGFContainerLogsForErrors may experience interference from previous tests (as explained in the function // documentation), this test is recommended to be run separate from other tests. var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"), func() { files := []string{ @@ -189,6 +190,9 @@ func runRestartNodeTest(teaURL, coffeeURL string, files []string, ns *core.Names } checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, "", files, ns) + if errorLogs := getUnexpectedNginxErrorLogs(ngfPodName); errorLogs != "" { + Skip(fmt.Sprintf("NGINX has unexpected error logs: \n%s", errorLogs)) + } } func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files []string, ns *core.Namespace) { @@ -219,6 +223,9 @@ func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files } checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, containerName, files, ns) + if errorLogs := getUnexpectedNginxErrorLogs(ngfPodName); errorLogs != "" { + Skip(fmt.Sprintf("NGINX has unexpected error logs: \n%s", errorLogs)) + } } func restartContainer(ngfPodName, containerName string) { @@ -347,13 +354,15 @@ func checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, containerName string, WithPolling(500 * time.Millisecond). Should(Succeed()) - checkContainerLogsForErrors(ngfPodName, containerName == nginxContainerName) + // When the NGINX process is killed, some errors are expected in the NGF logs while we wait for the + // NGINX container to be restarted. Therefore, we don't want to check the NGF logs for errors when the container + // we restarted was NGINX. + if containerName != nginxContainerName { + checkNGFContainerLogsForErrors(ngfPodName) + } } -// checkContainerLogsForErrors checks both nginx and NGF container's logs for any possible errors. -// When the NGINX process is killed, some errors are expected in the NGF logs while we wait for the -// NGINX container to be restarted. -func checkContainerLogsForErrors(ngfPodName string, checkNginxLogsOnly bool) { +func getNginxErrorLogs(ngfPodName string) string { nginxLogs, err := resourceManager.GetPodLogs( ngfNamespace, ngfPodName, @@ -361,38 +370,60 @@ func checkContainerLogsForErrors(ngfPodName string, checkNginxLogsOnly bool) { ) Expect(err).ToNot(HaveOccurred()) + errPrefixes := []string{"[crit]", "[error]", "[warn]", "[alert]", "[emerg]"} + errorLogs := "" + for _, line := range strings.Split(nginxLogs, "\n") { - Expect(line).ToNot(ContainSubstring("[crit]"), line) - Expect(line).ToNot(ContainSubstring("[alert]"), line) - Expect(line).ToNot(ContainSubstring("[emerg]"), line) - if strings.Contains(line, "[error]") { - expectedError1 := "connect() failed (111: Connection refused)" - expectedError2 := "could not be resolved (host not found) during usage report" - expectedError3 := "server returned 429" - // FIXME(salonichf5) remove this error message check - // when https://github.com/nginxinc/nginx-gateway-fabric/issues/2090 is completed. - expectedError4 := "no live upstreams while connecting to upstream" - Expect(line).To(Or( - ContainSubstring(expectedError1), - ContainSubstring(expectedError2), - ContainSubstring(expectedError3), - ContainSubstring(expectedError4), - )) + for _, prefix := range errPrefixes { + if strings.Contains(line, prefix) { + errorLogs += line + "\n" + break + } } } - if !checkNginxLogsOnly { - ngfLogs, err := resourceManager.GetPodLogs( - ngfNamespace, - ngfPodName, - &core.PodLogOptions{Container: ngfContainerName}, - ) - Expect(err).ToNot(HaveOccurred()) + return errorLogs +} + +func getUnexpectedNginxErrorLogs(ngfPodName string) string { + expectedErrStrings := []string{ + "connect() failed (111: Connection refused)", + "could not be resolved (host not found) during usage report", + "server returned 429", + // FIXME(salonichf5) remove this error message check + // when https://github.com/nginxinc/nginx-gateway-fabric/issues/2090 is completed. + "no live upstreams while connecting to upstream", + } + + unexpectedErrors := "" + + errorLogs := getNginxErrorLogs(ngfPodName) - for _, line := range strings.Split(ngfLogs, "\n") { - Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line) + for _, line := range strings.Split(errorLogs, "\n") { + + if !slices.ContainsFunc(expectedErrStrings, func(s string) bool { + return strings.Contains(line, s) + }) { + unexpectedErrors += line } } + + return unexpectedErrors +} + +// checkNGFContainerLogsForErrors checks NGF container's logs for any possible errors. +func checkNGFContainerLogsForErrors(ngfPodName string) { + ngfLogs, err := resourceManager.GetPodLogs( + ngfNamespace, + ngfPodName, + &core.PodLogOptions{Container: ngfContainerName}, + ) + Expect(err).ToNot(HaveOccurred()) + + for _, line := range strings.Split(ngfLogs, "\n") { + Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line) + } + } func checkLeaderLeaseChange(originalLeaseName string) error { diff --git a/tests/suite/reconfig_test.go b/tests/suite/reconfig_test.go index aa39e2c366..18e741c4a8 100644 --- a/tests/suite/reconfig_test.go +++ b/tests/suite/reconfig_test.go @@ -405,7 +405,8 @@ var _ = Describe("Reconfiguration Performance Testing", Ordered, Label("nfr", "r ).WithTimeout(metricExistTimeout).WithPolling(metricExistPolling).Should(Succeed()) } - checkContainerLogsForErrors(ngfPodName, false) + checkNGFContainerLogsForErrors(ngfPodName) + nginxErrorLogs := getNginxErrorLogs(ngfPodName) reloadCount, err := framework.GetReloadCount(promInstance, ngfPodName) Expect(err).ToNot(HaveOccurred()) @@ -447,6 +448,7 @@ var _ = Describe("Reconfiguration Performance Testing", Ordered, Label("nfr", "r TimeToReadyAvgSingle: timeToReadyAvgSingle, NGINXReloads: int(reloadCount), NGINXReloadAvgTime: int(reloadAvgTime), + NGINXErrorLogs: nginxErrorLogs, EventsCount: int(eventsCount), EventsAvgTime: int(eventsAvgTime), } @@ -601,6 +603,7 @@ type reconfigTestResults struct { NumResources int NGINXReloads int NGINXReloadAvgTime int + NGINXErrorLogs string EventsCount int EventsAvgTime int } @@ -627,6 +630,9 @@ const reconfigResultTemplate = ` {{- range .EventsBuckets }} - {{ .Le }}ms: {{ .Val }} {{- end }} + +### NGINX Error Logs +{{ .NGINXErrorLogs }} ` func writeReconfigResults(dest io.Writer, results reconfigTestResults) error { From 15d5e564788cfed2a392b82d89d7cb4d05147f3f Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 3 Jan 2025 12:08:05 -0700 Subject: [PATCH 3/5] Fix lint issues --- tests/suite/graceful_recovery_test.go | 2 -- tests/suite/reconfig_test.go | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index f82f1252a9..5c13961972 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -400,7 +400,6 @@ func getUnexpectedNginxErrorLogs(ngfPodName string) string { errorLogs := getNginxErrorLogs(ngfPodName) for _, line := range strings.Split(errorLogs, "\n") { - if !slices.ContainsFunc(expectedErrStrings, func(s string) bool { return strings.Contains(line, s) }) { @@ -423,7 +422,6 @@ func checkNGFContainerLogsForErrors(ngfPodName string) { for _, line := range strings.Split(ngfLogs, "\n") { Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line) } - } func checkLeaderLeaseChange(originalLeaseName string) error { diff --git a/tests/suite/reconfig_test.go b/tests/suite/reconfig_test.go index 18e741c4a8..3f1d677851 100644 --- a/tests/suite/reconfig_test.go +++ b/tests/suite/reconfig_test.go @@ -598,12 +598,12 @@ type reconfigTestResults struct { TestDescription string TimeToReadyTotal string TimeToReadyAvgSingle string + NGINXErrorLogs string EventsBuckets []framework.Bucket ReloadBuckets []framework.Bucket NumResources int NGINXReloads int NGINXReloadAvgTime int - NGINXErrorLogs string EventsCount int EventsAvgTime int } From 3bd606283f39520305c85e39410171087e9fb9a5 Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 3 Jan 2025 14:13:28 -0700 Subject: [PATCH 4/5] Move nginx log levels to file --- tests/framework/nginx_log_levels.go | 10 ++++++++++ tests/suite/graceful_recovery_test.go | 8 +++++++- tests/suite/scale_test.go | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 tests/framework/nginx_log_levels.go diff --git a/tests/framework/nginx_log_levels.go b/tests/framework/nginx_log_levels.go new file mode 100644 index 0000000000..dcf0485c54 --- /dev/null +++ b/tests/framework/nginx_log_levels.go @@ -0,0 +1,10 @@ +package framework + +// NGINX Log Prefixes for various log levels +const ( + CritNGINXLog = "[crit]" + ErrorNGINXLog = "[error]" + WarnNGINXLog = "[warn]" + AlertNGINXLog = "[alert]" + EmergNGINXLog = "[emerg]" +) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 5c13961972..c952ad4cc7 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -370,7 +370,13 @@ func getNginxErrorLogs(ngfPodName string) string { ) Expect(err).ToNot(HaveOccurred()) - errPrefixes := []string{"[crit]", "[error]", "[warn]", "[alert]", "[emerg]"} + errPrefixes := []string{ + framework.CritNGINXLog, + framework.ErrorNGINXLog, + framework.WarnNGINXLog, + framework.AlertNGINXLog, + framework.EmergNGINXLog, + } errorLogs := "" for _, line := range strings.Split(nginxLogs, "\n") { diff --git a/tests/suite/scale_test.go b/tests/suite/scale_test.go index 42982883fc..27919a7e46 100644 --- a/tests/suite/scale_test.go +++ b/tests/suite/scale_test.go @@ -368,7 +368,7 @@ The logs are attached only if there are errors. ) nginxErrors := checkLogErrors( "nginx", - []string{"[error]", "[emerg]", "[crit]", "[alert]"}, + []string{framework.ErrorNGINXLog, framework.EmergNGINXLog, framework.CritNGINXLog, framework.AlertNGINXLog}, nil, filepath.Join(testResultsDir, framework.CreateResultsFilename("log", "nginx", *plusEnabled)), ) From a8702ce306ef365ebe64ffde82147c094d39161f Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Fri, 3 Jan 2025 15:41:08 -0700 Subject: [PATCH 5/5] Fix lint --- tests/framework/nginx_log_levels.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/framework/nginx_log_levels.go b/tests/framework/nginx_log_levels.go index dcf0485c54..25a3dfb00f 100644 --- a/tests/framework/nginx_log_levels.go +++ b/tests/framework/nginx_log_levels.go @@ -1,6 +1,6 @@ package framework -// NGINX Log Prefixes for various log levels +// NGINX Log Prefixes for various log levels. const ( CritNGINXLog = "[crit]" ErrorNGINXLog = "[error]"