Implementation of the 'ValidateV2()' method for Config

All configuration types have validation methods that can be used
to check the values set. It is a final change that assembles all
these validations with validation of the other fields used by
Config type. Now 'praefect configuration validate' subcommand
returns all validation errors of the configuration.

Changelog: addition

Closes: https://gitlab.com/gitlab-org/gitaly/-/issues/4650

4 files changed, 141 insertions, 1 deletions

diff --git a/internal/cli/praefect/subcmd_configuration_validate_test.go b/internal/cli/praefect/subcmd_configuration_validate_test.go
index bcf87ef6f..74152ff63 100644
--- a/internal/cli/praefect/subcmd_configuration_validate_test.go
+++ b/internal/cli/praefect/subcmd_configuration_validate_test.go
@@ -71,6 +71,27 @@ election_strategy = invalid`)
 	One of accept-dataloss, check, configuration, dataloss, dial-nodes, list-storages, list-untracked-repositories, metadata, remove-repository, set-replication-factor, sql-migrate, sql-migrate-down, sql-migrate-status, sql-ping, track-repositories, track-repository, verify
 `,
 		},
+		{
+			name:     "validation failures",
+			exitCode: 2,
+			stdin: func(t *testing.T) io.Reader {
+				return strings.NewReader("")
+			},
+			stdout: `{
+  "errors": [
+    {
+      "message": "none of \"socket_path\", \"listen_addr\" or \"tls_listen_addr\" is set"
+    },
+    {
+      "key": [
+        "virtual_storage"
+      ],
+      "message": "not set"
+    }
+  ]
+}
+`,
+		},
 	} {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go
index c65e6dcc4..57549dfd0 100644
--- a/internal/praefect/config/config.go
+++ b/internal/praefect/config/config.go
@@ -414,7 +414,29 @@ func (c *Config) Validate() error {
 // It exists as a demonstration of the new validation implementation based on the usage
 // of the cfgerror package.
 func (c *Config) ValidateV2() error {
-	return nil
+	errs := cfgerror.New().
+		Append(func() error {
+			if c.SocketPath == "" && c.ListenAddr == "" && c.TLSListenAddr == "" {
+				return fmt.Errorf(`none of "socket_path", "listen_addr" or "tls_listen_addr" is set`)
+			}
+			return nil
+		}()).
+		Append(c.BackgroundVerification.Validate(), "background_verification").
+		Append(c.Reconciliation.Validate(), "reconciliation").
+		Append(c.Replication.Validate(), "replication").
+		Append(c.Prometheus.Validate(), "prometheus").
+		Append(c.TLS.Validate(), "tls").
+		Append(c.Failover.Validate(), "failover").
+		Append(cfgerror.Comparable(c.GracefulStopTimeout.Duration()).GreaterOrEqual(0), "graceful_stop_timeout").
+		Append(c.RepositoriesCleanup.Validate(), "repositories_cleanup").
+		Append(c.Yamux.Validate(), "yamux").
+		Append(cfgerror.NotEmptySlice(c.VirtualStorages), "virtual_storage")
+
+	for i, storage := range c.VirtualStorages {
+		errs = errs.Append(storage.Validate(), "virtual_storage", fmt.Sprintf("[%d]", i))
+	}
+
+	return errs.AsError()
 }
 
 // NeedsSQL returns true if the driver for SQL needs to be initialized
diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go
index 5cc904342..6f53ee32a 100644
--- a/internal/praefect/config/config_test.go
+++ b/internal/praefect/config/config_test.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -18,6 +19,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/sentry"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/duration"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestConfigValidation(t *testing.T) {
@@ -902,3 +905,86 @@ func TestYamux_Validate(t *testing.T) {
 		})
 	}
 }
+
+func TestConfig_ValidateV2(t *testing.T) {
+	t.Parallel()
+
+	t.Run("valid", func(t *testing.T) {
+		cfg := Config{
+			Replication: Replication{
+				BatchSize:                        1,
+				ParallelStorageProcessingWorkers: 1,
+			},
+			ListenAddr: "localhost",
+			VirtualStorages: []*VirtualStorage{{
+				Name: "name",
+				Nodes: []*Node{{
+					Storage: "storage",
+					Address: "localhost",
+				}},
+			}},
+			Yamux: Yamux{
+				MaximumStreamWindowSizeBytes: 300000,
+				AcceptBacklog:                1,
+			},
+		}
+		require.NoError(t, cfg.ValidateV2())
+	})
+
+	t.Run("invalid", func(t *testing.T) {
+		tmpDir := testhelper.TempDir(t)
+		tmpFile := filepath.Join(tmpDir, "file")
+		require.NoError(t, os.WriteFile(tmpFile, nil, perm.PublicFile))
+		cfg := Config{
+			BackgroundVerification: BackgroundVerification{
+				VerificationInterval: duration.Duration(-1),
+			},
+			Reconciliation: Reconciliation{
+				SchedulingInterval: duration.Duration(-1),
+			},
+			Replication: Replication{
+				BatchSize:                        0,
+				ParallelStorageProcessingWorkers: 1,
+			},
+			Prometheus: prometheus.Config{
+				ScrapeTimeout:      duration.Duration(-1),
+				GRPCLatencyBuckets: []float64{1},
+			},
+			PrometheusExcludeDatabaseFromDefaultMetrics: false,
+			TLS: config.TLS{
+				CertPath: "/doesnt/exist",
+				KeyPath:  tmpFile,
+			},
+			Failover: Failover{
+				Enabled:          true,
+				ElectionStrategy: ElectionStrategy("invalid"),
+			},
+			GracefulStopTimeout: duration.Duration(-1),
+			RepositoriesCleanup: RepositoriesCleanup{
+				CheckInterval:       duration.Duration(time.Hour),
+				RunInterval:         duration.Duration(1),
+				RepositoriesInBatch: 1,
+			},
+			Yamux: Yamux{
+				MaximumStreamWindowSizeBytes: 0,
+				AcceptBacklog:                1,
+			},
+		}
+		err := cfg.ValidateV2()
+
+		negativeDurationErr := fmt.Errorf("%w: -1ns is not greater than or equal to 0s", cfgerror.ErrNotInRange)
+		require.Equal(t, cfgerror.ValidationErrors{
+			cfgerror.NewValidationError(errors.New(`none of "socket_path", "listen_addr" or "tls_listen_addr" is set`)),
+			cfgerror.NewValidationError(negativeDurationErr, "background_verification", "verification_interval"),
+			cfgerror.NewValidationError(negativeDurationErr, "reconciliation", "scheduling_interval"),
+			cfgerror.NewValidationError(fmt.Errorf("%w: 0 is not greater than or equal to 1", cfgerror.ErrNotInRange), "replication", "batch_size"),
+			cfgerror.NewValidationError(negativeDurationErr, "prometheus", "scrape_timeout"),
+			cfgerror.NewValidationError(fmt.Errorf(`%w: "/doesnt/exist"`, cfgerror.ErrDoesntExist), "tls", "certificate_path"),
+			cfgerror.NewValidationError(fmt.Errorf(`%w: "invalid"`, cfgerror.ErrUnsupportedValue), "failover", "election_strategy"),
+			cfgerror.NewValidationError(negativeDurationErr, "graceful_stop_timeout"),
+			cfgerror.NewValidationError(fmt.Errorf("%w: 1ns is not greater than or equal to 1m0s", cfgerror.ErrNotInRange), "repositories_cleanup", "run_interval"),
+			cfgerror.NewValidationError(fmt.Errorf("%w: 0 is not greater than or equal to 262144", cfgerror.ErrNotInRange), "yamux", "maximum_stream_window_size_bytes"),
+			cfgerror.NewValidationError(cfgerror.ErrNotSet, "virtual_storage"),
+		}, err)
+	})
+}
diff --git a/internal/praefect/config/testhelper_test.go b/internal/praefect/config/testhelper_test.go
new file mode 100644
index 000000000..94a8084b7
--- /dev/null
+++ b/internal/praefect/config/testhelper_test.go
@@ -0,0 +1,11 @@
+package config
+
+import (
+	"testing"
+
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+)
+
+func TestMain(m *testing.M) {
+	testhelper.Run(m)
+}