diff --git a/internal/collector/postgres.go b/internal/collector/postgres.go index 892748c0a..6dc2eddcd 100644 --- a/internal/collector/postgres.go +++ b/internal/collector/postgres.go @@ -19,9 +19,60 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +func PostgreSQLParameters(ctx context.Context, + inCluster *v1beta1.PostgresCluster, + outParameters *postgres.Parameters, +) { + version := inCluster.Spec.PostgresVersion + + if OpenTelemetryLogsEnabled(ctx, inCluster) { + var spec *v1beta1.InstrumentationLogsSpec + if inCluster != nil && inCluster.Spec.Instrumentation != nil { + spec = inCluster.Spec.Instrumentation.Logs + } + + // Retain logs for a short time unless specified. + retention := metav1.Duration{Duration: 24 * time.Hour} + if spec != nil && spec.RetentionPeriod != nil { + retention = spec.RetentionPeriod.AsDuration() + } + + // Rotate log files according to retention and name them for the OpenTelemetry Collector. + // + // The ".log" suffix is replaced by ".csv" for CSV log files, and + // the ".log" suffix is replaced by ".json" for JSON log files. + // + // https://www.postgresql.org/docs/current/runtime-config-logging.html + for k, v := range postgres.LogRotation(retention, "postgresql-", ".log") { + outParameters.Mandatory.Add(k, v) + } + + // Enable logging to file. Postgres uses a "logging collector" to safely write concurrent messages. + // NOTE: That collector is designed to not lose messages. When it is overloaded, other Postgres processes block. + // + // https://www.postgresql.org/docs/current/runtime-config-logging.html + outParameters.Mandatory.Add("logging_collector", "on") + + // PostgreSQL v8.3 adds support for CSV logging, and + // PostgreSQL v15 adds support for JSON logging. + // The latter is preferred because newlines are escaped as "\n", U+005C + U+006E. + if version >= 15 { + outParameters.Mandatory.Add("log_destination", "jsonlog") + } else { + outParameters.Mandatory.Add("log_destination", "csvlog") + } + + // Log in a timezone the OpenTelemetry Collector understands. + outParameters.Mandatory.Add("log_timezone", "UTC") + + // TODO(logs): Remove this call and do it in [postgres.NewParameters] regardless of the gate. + outParameters.Mandatory.Add("log_directory", fmt.Sprintf("%s/logs/postgres", postgres.DataStorage(inCluster))) + } +} + func NewConfigForPostgresPod(ctx context.Context, inCluster *v1beta1.PostgresCluster, - outParameters *postgres.ParameterSet, + inParameters *postgres.ParameterSet, ) *Config { config := NewConfig(inCluster.Spec.Instrumentation) @@ -30,7 +81,7 @@ func NewConfigForPostgresPod(ctx context.Context, EnablePatroniMetrics(ctx, inCluster, config) // Logging - EnablePostgresLogging(ctx, inCluster, config, outParameters) + EnablePostgresLogging(ctx, inCluster, inParameters, config) EnablePatroniLogging(ctx, inCluster, config) return config @@ -76,8 +127,8 @@ func postgresCSVNames(version int) string { func EnablePostgresLogging( ctx context.Context, inCluster *v1beta1.PostgresCluster, + inParameters *postgres.ParameterSet, outConfig *Config, - outParameters *postgres.ParameterSet, ) { var spec *v1beta1.InstrumentationLogsSpec if inCluster != nil && inCluster.Spec.Instrumentation != nil { @@ -85,42 +136,9 @@ func EnablePostgresLogging( } if OpenTelemetryLogsEnabled(ctx, inCluster) { - directory := postgres.LogDirectory() + directory := inParameters.Value("log_directory") version := inCluster.Spec.PostgresVersion - // https://www.postgresql.org/docs/current/runtime-config-logging.html - outParameters.Add("logging_collector", "on") - outParameters.Add("log_directory", directory) - - // PostgreSQL v8.3 adds support for CSV logging, and - // PostgreSQL v15 adds support for JSON logging. The latter is preferred - // because newlines are escaped as "\n", U+005C + U+006E. - if version < 15 { - outParameters.Add("log_destination", "csvlog") - } else { - outParameters.Add("log_destination", "jsonlog") - } - - // If retentionPeriod is set in the spec, use that value; otherwise, we want - // to use a reasonably short duration. Defaulting to 1 day. - retentionPeriod := metav1.Duration{Duration: 24 * time.Hour} - if spec != nil && spec.RetentionPeriod != nil { - retentionPeriod = spec.RetentionPeriod.AsDuration() - } - - // Rotate log files according to retention. - // - // The ".log" suffix is replaced by ".csv" for CSV log files, and - // the ".log" suffix is replaced by ".json" for JSON log files. - // - // https://www.postgresql.org/docs/current/runtime-config-logging.html - for k, v := range postgres.LogRotation(retentionPeriod, "postgresql-", ".log") { - outParameters.Add(k, v) - } - - // Log in a timezone that the OpenTelemetry Collector will understand. - outParameters.Add("log_timezone", "UTC") - // Keep track of what log records and files have been processed. // Use a subdirectory of the logs directory to stay within the same failure domain. // TODO(log-rotation): Create this directory during Collector startup. @@ -145,8 +163,8 @@ func EnablePostgresLogging( // The 2nd through 5th fields are optional, so match through to the 7th field. // This should do a decent job of not matching the middle of some SQL statement. // - // The number of fields has changed over the years, but the first few - // are always formatted the same way. + // The number of fields has changed over the years, but the first few are always formatted the same way. + // [PostgreSQLParameters] ensures the timezone is UTC. // // NOTE: This regexp is invoked in multi-line mode. https://go.dev/s/re2syntax "multiline": map[string]string{ diff --git a/internal/collector/postgres_test.go b/internal/collector/postgres_test.go index 89f5f5225..a1a221bd6 100644 --- a/internal/collector/postgres_test.go +++ b/internal/collector/postgres_test.go @@ -31,9 +31,11 @@ func TestEnablePostgresLogging(t *testing.T) { }`) config := NewConfig(nil) - params := postgres.NewParameterSet() + params := postgres.NewParameters() - EnablePostgresLogging(ctx, cluster, config, params) + // NOTE: This call is necessary only because the default "log_directory" is not absolute. + PostgreSQLParameters(ctx, cluster, ¶ms) + EnablePostgresLogging(ctx, cluster, params.Mandatory, config) result, err := config.ToYAML() assert.NilError(t, err) @@ -293,9 +295,11 @@ service: cluster.Spec.Instrumentation = testInstrumentationSpec() config := NewConfig(cluster.Spec.Instrumentation) - params := postgres.NewParameterSet() + params := postgres.NewParameters() - EnablePostgresLogging(ctx, cluster, config, params) + // NOTE: This call is necessary only because the default "log_directory" is not absolute. + PostgreSQLParameters(ctx, cluster, ¶ms) + EnablePostgresLogging(ctx, cluster, params.Mandatory, config) result, err := config.ToYAML() assert.NilError(t, err) diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index bbe141c0b..c3264d0a3 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -339,7 +339,7 @@ func (r *Reconciler) Reconcile( ctx, cluster, clusterConfigMap, clusterReplicationSecret, rootCA, clusterPodService, instanceServiceAccount, instances, patroniLeaderService, primaryCertificate, clusterVolumes, exporterQueriesConfig, exporterWebConfig, - backupsSpecFound, otelConfig, + backupsSpecFound, otelConfig, pgParameters, ) } diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 0c91ca715..46a18be71 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -593,6 +593,7 @@ func (r *Reconciler) reconcileInstanceSets( exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, backupsSpecFound bool, otelConfig *collector.Config, + pgParameters *postgres.ParameterSet, ) error { // Go through the observed instances and check if a primary has been determined. @@ -630,7 +631,7 @@ func (r *Reconciler) reconcileInstanceSets( patroniLeaderService, primaryCertificate, findAvailableInstanceNames(*set, instances, clusterVolumes), numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig, - backupsSpecFound, otelConfig, + backupsSpecFound, otelConfig, pgParameters, ) if err == nil { @@ -1066,6 +1067,7 @@ func (r *Reconciler) scaleUpInstances( exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, backupsSpecFound bool, otelConfig *collector.Config, + pgParameters *postgres.ParameterSet, ) ([]*appsv1.StatefulSet, error) { log := logging.FromContext(ctx) @@ -1112,7 +1114,7 @@ func (r *Reconciler) scaleUpInstances( rootCA, clusterPodService, instanceServiceAccount, patroniLeaderService, primaryCertificate, instances[i], numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig, - backupsSpecFound, otelConfig, + backupsSpecFound, otelConfig, pgParameters, ) } if err == nil { @@ -1144,6 +1146,7 @@ func (r *Reconciler) reconcileInstance( exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, backupsSpecFound bool, otelConfig *collector.Config, + pgParameters *postgres.ParameterSet, ) error { log := logging.FromContext(ctx).WithValues("instance", instance.Name) ctx = logging.NewContext(ctx, log) @@ -1187,7 +1190,7 @@ func (r *Reconciler) reconcileInstance( postgres.InstancePod( ctx, cluster, spec, primaryCertificate, replicationCertSecretProjection(clusterReplicationSecret), - postgresDataVolume, postgresWALVolume, tablespaceVolumes, + postgresDataVolume, postgresWALVolume, tablespaceVolumes, pgParameters, &instance.Spec.Template) if backupsSpecFound { diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index 8922e5f73..489b9ad06 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/crunchydata/postgres-operator/internal/collector" "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/logging" @@ -130,6 +131,7 @@ func (*Reconciler) generatePostgresParameters( ctx context.Context, cluster *v1beta1.PostgresCluster, backupsSpecFound bool, ) *postgres.ParameterSet { builtin := postgres.NewParameters() + collector.PostgreSQLParameters(ctx, cluster, &builtin) pgaudit.PostgreSQLParameters(&builtin) pgbackrest.PostgreSQLParameters(cluster, &builtin, backupsSpecFound) pgmonitor.PostgreSQLParameters(ctx, cluster, &builtin) diff --git a/internal/postgres/config.go b/internal/postgres/config.go index 174aee34b..583314d3e 100644 --- a/internal/postgres/config.go +++ b/internal/postgres/config.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "math" + "path" "strings" corev1 "k8s.io/api/core/v1" @@ -89,13 +90,13 @@ func ConfigDirectory(cluster *v1beta1.PostgresCluster) string { // DataDirectory returns the absolute path to the "data_directory" of cluster. // - https://www.postgresql.org/docs/current/runtime-config-file-locations.html func DataDirectory(cluster *v1beta1.PostgresCluster) string { - return fmt.Sprintf("%s/pg%d", dataMountPath, cluster.Spec.PostgresVersion) + return fmt.Sprintf("%s/pg%d", DataStorage(cluster), cluster.Spec.PostgresVersion) } -// LogDirectory returns the absolute path to the "log_directory" of cluster. -// - https://www.postgresql.org/docs/current/runtime-config-logging.html -func LogDirectory() string { - return fmt.Sprintf("%s/logs/postgres", dataMountPath) +// DataStorage returns the absolute path to the disk where cluster stores its data. +// Use [DataDirectory] for the exact directory that Postgres uses. +func DataStorage(cluster *v1beta1.PostgresCluster) string { + return dataMountPath } // LogRotation returns parameters that rotate log files while keeping a minimum amount. @@ -310,11 +311,33 @@ done // PostgreSQL. func startupCommand( ctx context.Context, - cluster *v1beta1.PostgresCluster, instance *v1beta1.PostgresInstanceSetSpec, + cluster *v1beta1.PostgresCluster, + instance *v1beta1.PostgresInstanceSetSpec, + parameters *ParameterSet, ) []string { version := fmt.Sprint(cluster.Spec.PostgresVersion) + dataDir := DataDirectory(cluster) walDir := WALDirectory(cluster, instance) + // TODO: describe this? + mkdirs := make([]string, 0, 6) + mkdir := func(b, p string) { + if path.IsAbs(p) { + p = path.Clean(p) + } else { + p = path.Join(dataDir, p) + } + + // Create directories unless they are in the empty Postgres data directory. + mkdirs = append(mkdirs, + `[[ `+shell.QuoteWord(p)+` != "${postgres_data_directory}"* || -f "${postgres_data_directory}/PG_VERSION" ]] &&`, + `{ (`+shell.MakeDirectories(b, p)+`) || halt "$(permissions `+shell.QuoteWord(p)+` ||:)"; }`, + ) + } + mkdir(dataMountPath, naming.PGBackRestPGDataLogPath) + mkdir(dataMountPath, naming.PatroniPGDataLogPath) + mkdir(dataDir, parameters.Value("log_directory")) + // If the user requests tablespaces, we want to make sure the directories exist with the // correct owner and permissions. tablespaceCmd := "" @@ -442,13 +465,7 @@ chmod +x /tmp/pg_rewind_tde.sh `else (halt Permissions!); fi ||`, `halt "$(permissions "${postgres_data_directory}" ||:)"`, - // Create log directories. - `(` + shell.MakeDirectories(dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`, - `halt "$(permissions ` + naming.PGBackRestPGDataLogPath + ` ||:)"`, - `(` + shell.MakeDirectories(dataMountPath, naming.PatroniPGDataLogPath) + `) ||`, - `halt "$(permissions ` + naming.PatroniPGDataLogPath + ` ||:)"`, - `(` + shell.MakeDirectories(dataMountPath, LogDirectory()) + `) ||`, - `halt "$(permissions ` + LogDirectory() + ` ||:)"`, + strings.Join(mkdirs, "\n"), // Copy replication client certificate files // from the /pgconf/tls/replication directory to the /tmp/replication directory in order diff --git a/internal/postgres/config_test.go b/internal/postgres/config_test.go index 762bd8a0b..32490df26 100644 --- a/internal/postgres/config_test.go +++ b/internal/postgres/config_test.go @@ -40,6 +40,13 @@ func TestDataDirectory(t *testing.T) { assert.Equal(t, DataDirectory(cluster), "/pgdata/pg12") } +func TestDataStorage(t *testing.T) { + cluster := new(v1beta1.PostgresCluster) + cluster.Spec.PostgresVersion = rand.IntN(20) + + assert.Equal(t, DataStorage(cluster), "/pgdata") +} + func TestLogRotation(t *testing.T) { t.Parallel() @@ -543,8 +550,10 @@ func TestStartupCommand(t *testing.T) { cluster.Spec.PostgresVersion = 13 instance := new(v1beta1.PostgresInstanceSetSpec) + parameters := NewParameters().Default + ctx := context.Background() - command := startupCommand(ctx, cluster, instance) + command := startupCommand(ctx, cluster, instance, parameters) // Expect a bash command with an inline script. assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"}) @@ -579,7 +588,7 @@ func TestStartupCommand(t *testing.T) { }, }, } - command := startupCommand(ctx, cluster, instance) + command := startupCommand(ctx, cluster, instance, parameters) assert.Assert(t, len(command) > 3) assert.Assert(t, strings.Contains(command[3], `cat << "EOF" > /tmp/pg_rewind_tde.sh #!/bin/sh diff --git a/internal/postgres/parameters.go b/internal/postgres/parameters.go index 6fb7b0d2f..3590816b4 100644 --- a/internal/postgres/parameters.go +++ b/internal/postgres/parameters.go @@ -48,6 +48,17 @@ func NewParameters() Parameters { // - https://www.postgresql.org/docs/current/auth-password.html parameters.Default.Add("password_encryption", "scram-sha-256") + // Explicitly use Postgres' default log directory. This value is relative to the "data_directory" parameter. + // + // Controllers look at this parameter to grant group-write S_IWGRP on the directory. + // Postgres does not grant this permission on the directories it creates. + // + // TODO(logs): A better default would be *outside* the data directory. + // This parameter needs to be configurable and documented before the default can change. + // + // PostgreSQL must be reloaded when changing this parameter. + parameters.Default.Add("log_directory", "log") + // Pod "securityContext.fsGroup" ensures processes and filesystems agree on a GID; // use the same permissions for group and owner. // This allows every process in the pod to read Postgres log files. diff --git a/internal/postgres/parameters_test.go b/internal/postgres/parameters_test.go index ad8c6e90c..54095750f 100644 --- a/internal/postgres/parameters_test.go +++ b/internal/postgres/parameters_test.go @@ -28,6 +28,8 @@ func TestNewParameters(t *testing.T) { assert.DeepEqual(t, parameters.Default.AsMap(), map[string]string{ "jit": "off", + "log_directory": "log", + "password_encryption": "scram-sha-256", }) } diff --git a/internal/postgres/reconcile.go b/internal/postgres/reconcile.go index 5041140b0..157b56292 100644 --- a/internal/postgres/reconcile.go +++ b/internal/postgres/reconcile.go @@ -68,6 +68,7 @@ func InstancePod(ctx context.Context, inClusterCertificates, inClientCertificates *corev1.SecretProjection, inDataVolume, inWALVolume *corev1.PersistentVolumeClaim, inTablespaceVolumes []*corev1.PersistentVolumeClaim, + inParameters *ParameterSet, outInstancePod *corev1.PodTemplateSpec, ) { certVolumeMount := corev1.VolumeMount{ @@ -201,7 +202,7 @@ func InstancePod(ctx context.Context, startup := corev1.Container{ Name: naming.ContainerPostgresStartup, - Command: startupCommand(ctx, inCluster, inInstanceSpec), + Command: startupCommand(ctx, inCluster, inInstanceSpec, inParameters), Env: Environment(inCluster), Image: container.Image, diff --git a/internal/postgres/reconcile_test.go b/internal/postgres/reconcile_test.go index 61a85d5cd..ad24e1989 100644 --- a/internal/postgres/reconcile_test.go +++ b/internal/postgres/reconcile_test.go @@ -69,6 +69,8 @@ func TestInstancePod(t *testing.T) { cluster.Spec.ImagePullPolicy = corev1.PullAlways cluster.Spec.PostgresVersion = 11 + parameters := NewParameters().Default + dataVolume := new(corev1.PersistentVolumeClaim) dataVolume.Name = "datavol" @@ -117,7 +119,7 @@ func TestInstancePod(t *testing.T) { // without WAL volume nor WAL volume spec pod := new(corev1.PodTemplateSpec) InstancePod(ctx, cluster, instance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, parameters, pod) assert.Assert(t, cmp.MarshalMatches(pod.Spec, ` containers: @@ -268,12 +270,12 @@ initContainers: recreate "${postgres_data_directory}" '0750' else (halt Permissions!); fi || halt "$(permissions "${postgres_data_directory}" ||:)" - (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) || - halt "$(permissions /pgdata/pgbackrest/log ||:)" - (mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) || - halt "$(permissions /pgdata/patroni/log ||:)" - (mkdir -p '/pgdata/logs/postgres' && { chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs' || :; }) || - halt "$(permissions /pgdata/logs/postgres ||:)" + [[ '/pgdata/pgbackrest/log' != "${postgres_data_directory}"* || -f "${postgres_data_directory}/PG_VERSION" ]] && + { (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) || halt "$(permissions '/pgdata/pgbackrest/log' ||:)"; } + [[ '/pgdata/patroni/log' != "${postgres_data_directory}"* || -f "${postgres_data_directory}/PG_VERSION" ]] && + { (mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) || halt "$(permissions '/pgdata/patroni/log' ||:)"; } + [[ '/pgdata/pg11/log' != "${postgres_data_directory}"* || -f "${postgres_data_directory}/PG_VERSION" ]] && + { (mkdir -p '/pgdata/pg11/log' && { chmod 0775 '/pgdata/pg11/log' || :; }) || halt "$(permissions '/pgdata/pg11/log' ||:)"; } install -D --mode=0600 -t "/tmp/replication" "/pgconf/tls/replication"/{tls.crt,tls.key,ca.crt} @@ -385,7 +387,7 @@ volumes: pod := new(corev1.PodTemplateSpec) InstancePod(ctx, cluster, instance, - serverSecretProjection, clientSecretProjection, dataVolume, walVolume, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, walVolume, nil, parameters, pod) assert.Assert(t, len(pod.Spec.Containers) > 0) assert.Assert(t, len(pod.Spec.InitContainers) > 0) @@ -486,7 +488,7 @@ volumes: pod := new(corev1.PodTemplateSpec) InstancePod(ctx, clusterWithConfig, instance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, parameters, pod) assert.Assert(t, len(pod.Spec.Containers) > 0) assert.Assert(t, len(pod.Spec.InitContainers) > 0) @@ -523,7 +525,7 @@ volumes: t.Run("SidecarNotEnabled", func(t *testing.T) { InstancePod(ctx, cluster, sidecarInstance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, parameters, pod) assert.Equal(t, len(pod.Spec.Containers), 2, "expected 2 containers in Pod") }) @@ -536,7 +538,7 @@ volumes: ctx := feature.NewContext(ctx, gate) InstancePod(ctx, cluster, sidecarInstance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, parameters, pod) assert.Equal(t, len(pod.Spec.Containers), 3, "expected 3 containers in Pod") @@ -573,7 +575,7 @@ volumes: tablespaceVolumes := []*corev1.PersistentVolumeClaim{tablespaceVolume1, tablespaceVolume2} InstancePod(ctx, cluster, instance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, tablespaceVolumes, pod) + serverSecretProjection, clientSecretProjection, dataVolume, nil, tablespaceVolumes, parameters, pod) assert.Assert(t, cmp.MarshalMatches(pod.Spec.Containers[0].VolumeMounts, ` - mountPath: /pgconf/tls @@ -611,7 +613,7 @@ volumes: pod := new(corev1.PodTemplateSpec) InstancePod(ctx, cluster, instance, - serverSecretProjection, clientSecretProjection, dataVolume, walVolume, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, walVolume, nil, parameters, pod) assert.Assert(t, len(pod.Spec.Containers) > 0) assert.Assert(t, len(pod.Spec.InitContainers) > 0) @@ -713,7 +715,7 @@ volumes: pod := new(corev1.PodTemplateSpec) InstancePod(ctx, cluster, instance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, pod) + serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, parameters, pod) assert.Assert(t, len(pod.Spec.Containers) > 0) assert.Assert(t, cmp.MarshalContains(pod.Spec.Containers[0].VolumeMounts, ` @@ -743,7 +745,7 @@ volumes: annotated.Labels = map[string]string{"gg": "asdf"} InstancePod(ctx, cluster, instance, - serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, annotated) + serverSecretProjection, clientSecretProjection, dataVolume, nil, nil, parameters, annotated) assert.Assert(t, cmp.MarshalContains(annotated.Spec.Volumes, ` - ephemeral: diff --git a/internal/shell/paths.go b/internal/shell/paths.go index 701144694..5b4cafe30 100644 --- a/internal/shell/paths.go +++ b/internal/shell/paths.go @@ -36,6 +36,8 @@ func CleanFileName(path string) string { // exists. It creates every directory leading to path from (but not including) // base and sets their permissions for Kubernetes, regardless of umask. // +// Relative paths are expanded relative to base. +// // See: // - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/chmod.html // - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/mkdir.html @@ -47,8 +49,17 @@ func MakeDirectories(base string, paths ...string) string { return `test -d ` + QuoteWord(base) } - allPaths := slices.Clone(paths) - for _, p := range paths { + // Expand each path relative to the base path. + expandedPaths := slices.Clone(paths) + for i, p := range expandedPaths { + if !filepath.IsAbs(p) { + expandedPaths[i] = filepath.Join(base, p) + } + } + + // Gather parent directories of each path. + allPaths := slices.Clone(expandedPaths) + for _, p := range expandedPaths { if r, err := filepath.Rel(base, p); err == nil && filepath.IsLocal(r) { // The result of [filepath.Rel] is a shorter representation of the full path; skip it. r = filepath.Dir(r) @@ -72,7 +83,7 @@ func MakeDirectories(base string, paths ...string) string { return `` + // Create all the paths and any missing parents. - `mkdir -p ` + strings.Join(QuoteWords(paths...), " ") + + `mkdir -p ` + strings.Join(QuoteWords(expandedPaths...), " ") + // Try to set the permissions of every path and each parent. // This swallows the exit status of `chmod` because not all filesystems diff --git a/internal/shell/paths_test.go b/internal/shell/paths_test.go index b5adb69b1..f3b9aea82 100644 --- a/internal/shell/paths_test.go +++ b/internal/shell/paths_test.go @@ -94,6 +94,38 @@ func TestMakeDirectories(t *testing.T) { }) }) + t.Run("Relative", func(t *testing.T) { + assert.Equal(t, + MakeDirectories("/x", "one", "two/three"), + `mkdir -p '/x/one' '/x/two/three' && { chmod 0775 '/x/one' '/x/two/three' '/x/two' || :; }`, + "expected paths relative to base") + + assert.Equal(t, + MakeDirectories("/x/y/z", "../one", "./two", "../../../../three"), + `mkdir -p '/x/y/one' '/x/y/z/two' '/three' && { chmod 0775 '/x/y/one' '/x/y/z/two' '/three' || :; }`, + "expected paths relative to base") + + script := MakeDirectories("x/y", "../one", "./two", "../../../../three") + assert.Equal(t, script, + `mkdir -p 'x/one' 'x/y/two' '../../three' && { chmod 0775 'x/one' 'x/y/two' '../../three' || :; }`, + "expected paths relative to base") + + // Calling `mkdir -p '../..'` works, but run it by ShellCheck as a precaution. + t.Run("ShellCheckPOSIX", func(t *testing.T) { + shellcheck := require.ShellCheck(t) + + dir := t.TempDir() + file := filepath.Join(dir, "script.sh") + assert.NilError(t, os.WriteFile(file, []byte(script), 0o600)) + + // Expect ShellCheck for "sh" to be happy. + // - https://www.shellcheck.net/wiki/SC2148 + cmd := exec.CommandContext(t.Context(), shellcheck, "--enable=all", "--shell=sh", file) + output, err := cmd.CombinedOutput() + assert.NilError(t, err, "%q\n%s", cmd.Args, output) + }) + }) + t.Run("Unrelated", func(t *testing.T) { assert.Equal(t, MakeDirectories("/one", "/two/three/four"),