diff --git a/controllers/mustgather/template.go b/controllers/mustgather/template.go index e906892a8..a636fd163 100644 --- a/controllers/mustgather/template.go +++ b/controllers/mustgather/template.go @@ -3,7 +3,9 @@ package mustgather import ( "fmt" "math" + "path" "strconv" + "strings" "time" @@ -47,8 +49,41 @@ const ( // SSH directory and known hosts file sshDir = "/tmp/must-gather-operator/.ssh" knownHostsFile = "/tmp/must-gather-operator/.ssh/known_hosts" + + // Environment variable specifying the must-gather image + defaultMustGatherImageEnv = "DEFAULT_MUST_GATHER_IMAGE" + + // Downward API environment variables used for subPathExpr expansion. + podNameEnvVar = "POD_NAME" ) +func outputSubPathExpr(storage *v1alpha1.Storage) (string, bool) { + if storage == nil || storage.Type != v1alpha1.StorageTypePersistentVolume { + return "", false + } + + base := strings.TrimSpace(storage.PersistentVolume.SubPath) + base = strings.Trim(base, "/") + if base == "" { + return "", false + } + + // Use user-provided base path, but isolate each run using the pod name + // to avoid overwriting prior collections on the PVC. + return path.Join(base, fmt.Sprintf("$(%s)", podNameEnvVar)), true +} + +func podNameEnvVars() []corev1.EnvVar { + return []corev1.EnvVar{ + { + Name: podNameEnvVar, + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}, + }, + }, + } +} + func getJobTemplate(image string, operatorImage string, mustGather v1alpha1.MustGather, trustedCAConfigMapName string) *batchv1.Job { job := initializeJobTemplate(mustGather.Name, mustGather.Namespace, mustGather.Spec.ServiceAccountName, mustGather.Spec.Storage, trustedCAConfigMapName) @@ -100,6 +135,7 @@ func getJobTemplate(image string, operatorImage string, mustGather v1alpha1.Must s.CaseID, s.Host, s.InternalUser, + mustGather.Spec.Storage, httpProxy, httpsProxy, noProxy, @@ -205,8 +241,9 @@ func getGatherContainer(image string, audit bool, timeout time.Duration, storage Name: outputVolumeName, } - if storage != nil && storage.Type == v1alpha1.StorageTypePersistentVolume && storage.PersistentVolume.SubPath != "" { - volumeMount.SubPath = storage.PersistentVolume.SubPath + subPathExpr, hasSubPathExpr := outputSubPathExpr(storage) + if hasSubPathExpr { + volumeMount.SubPathExpr = subPathExpr } volumeMounts := []corev1.VolumeMount{volumeMount} @@ -240,6 +277,11 @@ func getGatherContainer(image string, audit bool, timeout time.Duration, storage container.Args = args } + // Provide pod name env var only when SubPathExpr is used (PVC subPath is set). + if hasSubPathExpr { + container.Env = append(container.Env, podNameEnvVars()...) + } + return container } @@ -248,6 +290,7 @@ func getUploadContainer( caseId string, host string, internalUser bool, + storage *v1alpha1.Storage, httpProxy string, httpsProxy string, noProxy string, @@ -258,11 +301,17 @@ func getUploadContainer( uploadCommandWithSSH := fmt.Sprintf("mkdir -p %s; touch %s; chmod 700 %s; chmod 600 %s; %s", sshDir, knownHostsFile, sshDir, knownHostsFile, uploadCommand) + outputMount := corev1.VolumeMount{ + MountPath: volumeMountPath, + Name: outputVolumeName, + } + subPathExpr, hasSubPathExpr := outputSubPathExpr(storage) + if hasSubPathExpr { + outputMount.SubPathExpr = subPathExpr + } + volumeMounts := []corev1.VolumeMount{ - { - MountPath: volumeMountPath, - Name: outputVolumeName, - }, + outputMount, { MountPath: volumeUploadMountPath, Name: uploadVolumeName, @@ -328,6 +377,11 @@ func getUploadContainer( }, } + // Provide pod name env var only when SubPathExpr is used (PVC subPath is set). + if hasSubPathExpr { + container.Env = append(container.Env, podNameEnvVars()...) + } + if httpProxy != "" { container.Env = append(container.Env, corev1.EnvVar{Name: uploadEnvHttpProxy, Value: httpProxy}) } diff --git a/controllers/mustgather/template_test.go b/controllers/mustgather/template_test.go index 511dc675a..dac5fc69a 100644 --- a/controllers/mustgather/template_test.go +++ b/controllers/mustgather/template_test.go @@ -138,6 +138,39 @@ func Test_getGatherContainer(t *testing.T) { }, }, }, + { + name: "with PVC empty subPath does not set subPathExpr", + timeout: 5 * time.Second, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"}, + SubPath: "", + }, + }, + }, + { + name: "with PVC whitespace subPath does not set subPathExpr", + timeout: 5 * time.Second, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"}, + SubPath: " ", + }, + }, + }, + { + name: "with PVC slash-only subPath does not set subPathExpr", + timeout: 5 * time.Second, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"}, + SubPath: "/", + }, + }, + }, { name: "robust timeout", timeout: 6*time.Hour + 5*time.Minute + 3*time.Second, // 6h5m3s @@ -188,8 +221,43 @@ func Test_getGatherContainer(t *testing.T) { if volumeMount.Name != outputVolumeName { t.Fatalf("volume mount name was not correctly set. got %v, wanted %v", volumeMount.Name, outputVolumeName) } - if volumeMount.SubPath != tt.storage.PersistentVolume.SubPath { - t.Fatalf("volume mount subpath was not correctly set. got %v, wanted %v", volumeMount.SubPath, tt.storage.PersistentVolume.SubPath) + base := strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/") + if base == "" { + if volumeMount.SubPathExpr != "" { + t.Fatalf("did not expect volume mount subPathExpr to be set when base subPath is empty, got %q", volumeMount.SubPathExpr) + } + } else { + wantExpr := fmt.Sprintf("%s/$(POD_NAME)", base) + if volumeMount.SubPathExpr != wantExpr { + t.Fatalf("volume mount subPathExpr was not correctly set. got %q, wanted %q", volumeMount.SubPathExpr, wantExpr) + } + } + if volumeMount.SubPath != "" { + t.Fatalf("did not expect volume mount subPath to be set when using subPathExpr, got %q", volumeMount.SubPath) + } + } + + // POD_NAME env var should be present only when SubPathExpr is used. + hasPodNameEnv := false + for _, env := range container.Env { + if env.Name == podNameEnvVar { + hasPodNameEnv = true + if env.ValueFrom == nil || env.ValueFrom.FieldRef == nil || env.ValueFrom.FieldRef.FieldPath != "metadata.name" { + t.Fatalf("expected %s env var to be sourced from metadata.name via fieldRef, got %#v", podNameEnvVar, env) + } + } + } + subPathBase := "" + if tt.storage != nil && tt.storage.Type == mustgatherv1alpha1.StorageTypePersistentVolume { + subPathBase = strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/") + } + if subPathBase == "" { + if hasPodNameEnv { + t.Fatalf("did not expect %s env var when PVC subPath is empty", podNameEnvVar) + } + } else { + if !hasPodNameEnv { + t.Fatalf("expected %s env var when PVC subPath is set (base=%q)", podNameEnvVar, subPathBase) } } }) @@ -203,6 +271,7 @@ func Test_getUploadContainer(t *testing.T) { caseId string host string internalUser bool + storage *mustgatherv1alpha1.Storage httpProxy string httpsProxy string noProxy string @@ -262,11 +331,73 @@ func Test_getUploadContainer(t *testing.T) { secretKeyRefName: v1.LocalObjectReference{Name: "testSecretKeyRefName"}, mountCAConfigMap: true, }, + { + name: "With PVC subPath", + operatorImage: "testImage", + caseId: "1234", + secretKeyRefName: v1.LocalObjectReference{ + Name: "testSecretKeyRefName", + }, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{ + Name: "test-pvc", + }, + SubPath: "test-path", + }, + }, + }, + { + name: "With PVC empty subPath does not set subPathExpr", + operatorImage: "testImage", + caseId: "1234", + secretKeyRefName: v1.LocalObjectReference{ + Name: "testSecretKeyRefName", + }, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"}, + SubPath: "", + }, + }, + }, + { + name: "With PVC whitespace subPath does not set subPathExpr", + operatorImage: "testImage", + caseId: "1234", + secretKeyRefName: v1.LocalObjectReference{ + Name: "testSecretKeyRefName", + }, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"}, + SubPath: " ", + }, + }, + }, + { + name: "With PVC slash-only subPath does not set subPathExpr", + operatorImage: "testImage", + caseId: "1234", + secretKeyRefName: v1.LocalObjectReference{ + Name: "testSecretKeyRefName", + }, + storage: &mustgatherv1alpha1.Storage{ + Type: mustgatherv1alpha1.StorageTypePersistentVolume, + PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{ + Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"}, + SubPath: "/", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { testFailed := false - container := getUploadContainer(tt.operatorImage, tt.caseId, tt.host, tt.internalUser, tt.httpProxy, tt.httpsProxy, tt.noProxy, tt.secretKeyRefName, tt.mountCAConfigMap) + container := getUploadContainer(tt.operatorImage, tt.caseId, tt.host, tt.internalUser, tt.storage, tt.httpProxy, tt.httpsProxy, tt.noProxy, tt.secretKeyRefName, tt.mountCAConfigMap) if container.Image != tt.operatorImage { t.Fatalf("expected container image %v but got %v", tt.operatorImage, container.Image) @@ -285,6 +416,57 @@ func Test_getUploadContainer(t *testing.T) { } } + if tt.storage != nil && tt.storage.Type == mustgatherv1alpha1.StorageTypePersistentVolume { + var outputMount *v1.VolumeMount + for i := range container.VolumeMounts { + if container.VolumeMounts[i].Name == outputVolumeName { + outputMount = &container.VolumeMounts[i] + break + } + } + if outputMount == nil { + t.Fatalf("expected output volume mount %q to be present", outputVolumeName) + } + base := strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/") + if base == "" { + if outputMount.SubPathExpr != "" { + t.Fatalf("did not expect output volume mount subPathExpr to be set when base subPath is empty, got %q", outputMount.SubPathExpr) + } + } else { + wantExpr := fmt.Sprintf("%s/$(POD_NAME)", base) + if outputMount.SubPathExpr != wantExpr { + t.Fatalf("expected output volume mount subPathExpr %q but got %q", wantExpr, outputMount.SubPathExpr) + } + } + if outputMount.SubPath != "" { + t.Fatalf("did not expect output volume mount subPath to be set when using subPathExpr, got %q", outputMount.SubPath) + } + } + + // POD_NAME env var should be present only when SubPathExpr is used. + hasPodNameEnv := false + for _, env := range container.Env { + if env.Name == podNameEnvVar { + hasPodNameEnv = true + if env.ValueFrom == nil || env.ValueFrom.FieldRef == nil || env.ValueFrom.FieldRef.FieldPath != "metadata.name" { + t.Fatalf("expected %s env var to be sourced from metadata.name via fieldRef, got %#v", podNameEnvVar, env) + } + } + } + subPathBase := "" + if tt.storage != nil && tt.storage.Type == mustgatherv1alpha1.StorageTypePersistentVolume { + subPathBase = strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/") + } + if subPathBase == "" { + if hasPodNameEnv { + t.Fatalf("did not expect %s env var when PVC subPath is empty", podNameEnvVar) + } + } else { + if !hasPodNameEnv { + t.Fatalf("expected %s env var when PVC subPath is set (base=%q)", podNameEnvVar, subPathBase) + } + } + for _, env := range container.Env { switch env.Name { case uploadEnvCaseId: diff --git a/deploy/must-gather-pvc.yaml b/deploy/must-gather-pvc.yaml new file mode 100644 index 000000000..cac7bd481 --- /dev/null +++ b/deploy/must-gather-pvc.yaml @@ -0,0 +1,11 @@ +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: must-gather-pvc + namespace: must-gather-operator +spec: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 5Gi \ No newline at end of file diff --git a/examples/mustgather_with_pvc_subpath.yaml b/examples/mustgather_with_pvc_subpath.yaml new file mode 100644 index 000000000..d98e9fdce --- /dev/null +++ b/examples/mustgather_with_pvc_subpath.yaml @@ -0,0 +1,19 @@ +apiVersion: operator.openshift.io/v1alpha1 +kind: MustGather +metadata: + name: example-mustgather-pvc-subpath +spec: + serviceAccountName: must-gather-admin + storage: + type: PersistentVolume + persistentVolume: + claim: + name: must-gather-pvc + subPath: must-gather-data + uploadTarget: + type: SFTP + sftp: + caseID: '04230315' + caseManagementAccountSecretRef: + name: case-management-creds + internalUser: true diff --git a/test/e2e/must_gather_operator_test.go b/test/e2e/must_gather_operator_test.go index 765f9e3a6..29f718387 100644 --- a/test/e2e/must_gather_operator_test.go +++ b/test/e2e/must_gather_operator_test.go @@ -1473,7 +1473,8 @@ var _ = ginkgo.Describe("MustGather resource", ginkgo.Ordered, func() { } } Expect(outputMount).NotTo(BeNil(), "Gather container should have output volume mount") - Expect(outputMount.SubPath).To(Equal(subPath), "Volume mount should have subPath configured") + Expect(outputMount.SubPathExpr).To(Equal(subPath+"/$(POD_NAME)"), "Volume mount should have subPathExpr configured") + Expect(outputMount.SubPath).To(BeEmpty(), "Volume mount subPath should be empty when using subPathExpr") }) ginkgo.It("should create MustGather with PVC storage, configure Job correctly, and persist data", func() {