Sitelet https://github.com/helm/helm/commit/7c8010322b8639cdf7844ac1ae5f8d444db43935
Skip to content

Commit 7c80103

Browse files
SebTardifscottrigby
SebTardif
authored and
scottrigby
committed
fix: fetch logs from all containers in test pods
When a test pod contains multiple containers (e.g. Istio/Consul/Vault sidecars), 'helm test --logs' failed with 'a container name must be specified'. This happened because GetPodLogs called the Kubernetes log API without specifying a container name. The fix fetches the pod spec first, then iterates over all containers (init containers + regular containers) and requests logs for each one explicitly. Errors from individual containers are collected and returned together via errors.Join rather than aborting on the first failure. Also fixes a typo: hooksByWight -> hooksByWeight. Closes #6902 Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca> (cherry picked from commit 854f7f6)
1 parent ecc9cd2 commit 7c80103

2 files changed

Lines changed: 141 additions & 15 deletions

File tree

‎pkg/action/release_testing.go‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,16 @@ package action
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
2223
"io"
2324
"slices"
2425
"sort"
2526
"time"
2627

2728
v1 "k8s.io/api/core/v1"
29+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
30+
"k8s.io/client-go/kubernetes"
2831

2932
chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
3033
"helm.sh/helm/v4/pkg/kube"
@@ -124,9 +127,9 @@ func (r *ReleaseTesting) GetPodLogs(out io.Writer, rel *release.Release) error {
124127
return fmt.Errorf("unable to get kubernetes client to fetch pod logs: %w", err)
125128
}
126129

127-
hooksByWight := append([]*release.Hook{}, rel.Hooks...)
128-
sort.Stable(hookByWeight(hooksByWight))
129-
for _, h := range hooksByWight {
130+
hooksByWeight := append([]*release.Hook{}, rel.Hooks...)
131+
sort.Stable(hookByWeight(hooksByWeight))
132+
for _, h := range hooksByWeight {
130133
for _, e := range h.Events {
131134
if e == release.HookTest {
132135
if slices.Contains(r.Filters[ExcludeNameFilter], h.Name) {
@@ -135,20 +138,42 @@ func (r *ReleaseTesting) GetPodLogs(out io.Writer, rel *release.Release) error {
135138
if len(r.Filters[IncludeNameFilter]) > 0 && !slices.Contains(r.Filters[IncludeNameFilter], h.Name) {
136139
continue
137140
}
138-
req := client.CoreV1().Pods(r.Namespace).GetLogs(h.Name, &v1.PodLogOptions{})
139-
logReader, err := req.Stream(context.Background())
140-
if err != nil {
141-
return fmt.Errorf("unable to get pod logs for %s: %w", h.Name, err)
142-
}
143-
144-
fmt.Fprintf(out, "POD LOGS: %s\n", h.Name)
145-
_, err = io.Copy(out, logReader)
146-
fmt.Fprintln(out)
147-
if err != nil {
148-
return fmt.Errorf("unable to write pod logs for %s: %w", h.Name, err)
141+
if err := r.getContainerLogs(out, client, h.Name); err != nil {
142+
return err
149143
}
150144
}
151145
}
152146
}
153147
return nil
154148
}
149+
150+
// getContainerLogs fetches logs from all containers (init and regular) in the
151+
// named pod and writes them to out. It continues on per-container errors and
152+
// returns all of them joined at the end.
153+
func (r *ReleaseTesting) getContainerLogs(out io.Writer, client kubernetes.Interface, podName string) error {
154+
pod, err := client.CoreV1().Pods(r.Namespace).Get(context.Background(), podName, metav1.GetOptions{})
155+
if err != nil {
156+
return fmt.Errorf("unable to get pod %s: %w", podName, err)
157+
}
158+
159+
allContainers := append(pod.Spec.InitContainers, pod.Spec.Containers...)
160+
161+
var errs []error
162+
for _, c := range allContainers {
163+
opts := &v1.PodLogOptions{Container: c.Name}
164+
req := client.CoreV1().Pods(r.Namespace).GetLogs(podName, opts)
165+
logReader, err := req.Stream(context.Background())
166+
if err != nil {
167+
errs = append(errs, fmt.Errorf("unable to get logs for pod %s, container %s: %w", podName, c.Name, err))
168+
continue
169+
}
170+
171+
fmt.Fprintf(out, "POD LOGS: %s (%s)\n", podName, c.Name)
172+
_, err = io.Copy(out, logReader)
173+
fmt.Fprintln(out)
174+
if err != nil {
175+
errs = append(errs, fmt.Errorf("unable to write logs for pod %s, container %s: %w", podName, c.Name, err))
176+
}
177+
}
178+
return errors.Join(errs...)
179+
}

‎pkg/action/release_testing_test.go‎

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,14 @@ import (
2222
"errors"
2323
"io"
2424
"os"
25+
"strings"
2526
"testing"
2627

2728
"github.com/stretchr/testify/assert"
2829
"github.com/stretchr/testify/require"
30+
v1 "k8s.io/api/core/v1"
31+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
32+
fakeclientset "k8s.io/client-go/kubernetes/fake"
2933

3034
"helm.sh/helm/v4/pkg/cli"
3135
"helm.sh/helm/v4/pkg/kube"
@@ -89,7 +93,7 @@ func TestReleaseTestingGetPodLogs_PodRetrievalError(t *testing.T) {
8993
},
9094
}
9195

92-
require.ErrorContains(t, client.GetPodLogs(&bytes.Buffer{}, &release.Release{Hooks: hooks}), "unable to get pod logs")
96+
require.ErrorContains(t, client.GetPodLogs(&bytes.Buffer{}, &release.Release{Hooks: hooks}), "unable to get pod")
9397
}
9498

9599
func TestReleaseTesting_WaitOptionsPassedDownstream(t *testing.T) {
@@ -117,3 +121,100 @@ func TestReleaseTesting_WaitOptionsPassedDownstream(t *testing.T) {
117121
// Verify that WaitOptions were passed to GetWaiter
118122
is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be passed to GetWaiter")
119123
}
124+
125+
func TestGetContainerLogs_MultipleContainers(t *testing.T) {
126+
pod := &v1.Pod{
127+
ObjectMeta: metav1.ObjectMeta{
128+
Name: "test-pod",
129+
Namespace: "default",
130+
},
131+
Spec: v1.PodSpec{
132+
Containers: []v1.Container{
133+
{Name: "main"},
134+
{Name: "sidecar"},
135+
},
136+
},
137+
}
138+
139+
client := fakeclientset.NewClientset(pod)
140+
rt := &ReleaseTesting{Namespace: "default"}
141+
142+
var buf bytes.Buffer
143+
err := rt.getContainerLogs(&buf, client, "test-pod")
144+
// The fake client doesn't serve real log streams, so we expect
145+
// per-container errors rather than success, but critically it should
146+
// NOT fail with "a container name must be specified".
147+
if err != nil {
148+
assert.NotContains(t, err.Error(), "a container name must be specified")
149+
assert.Contains(t, err.Error(), "container main")
150+
assert.Contains(t, err.Error(), "container sidecar")
151+
}
152+
}
153+
154+
func TestGetContainerLogs_WithInitContainers(t *testing.T) {
155+
pod := &v1.Pod{
156+
ObjectMeta: metav1.ObjectMeta{
157+
Name: "test-pod",
158+
Namespace: "default",
159+
},
160+
Spec: v1.PodSpec{
161+
InitContainers: []v1.Container{
162+
{Name: "init-setup"},
163+
},
164+
Containers: []v1.Container{
165+
{Name: "main"},
166+
},
167+
},
168+
}
169+
170+
client := fakeclientset.NewClientset(pod)
171+
rt := &ReleaseTesting{Namespace: "default"}
172+
173+
var buf bytes.Buffer
174+
err := rt.getContainerLogs(&buf, client, "test-pod")
175+
if err != nil {
176+
// Both init and regular containers should be attempted
177+
assert.Contains(t, err.Error(), "container init-setup")
178+
assert.Contains(t, err.Error(), "container main")
179+
}
180+
}
181+
182+
func TestGetContainerLogs_PodNotFound(t *testing.T) {
183+
client := fakeclientset.NewClientset()
184+
rt := &ReleaseTesting{Namespace: "default"}
185+
186+
var buf bytes.Buffer
187+
err := rt.getContainerLogs(&buf, client, "nonexistent-pod")
188+
require.Error(t, err)
189+
assert.Contains(t, err.Error(), "unable to get pod nonexistent-pod")
190+
}
191+
192+
func TestGetPodLogs_MultiContainerOutput(t *testing.T) {
193+
pod := &v1.Pod{
194+
ObjectMeta: metav1.ObjectMeta{
195+
Name: "multi-test",
196+
Namespace: "default",
197+
},
198+
Spec: v1.PodSpec{
199+
Containers: []v1.Container{
200+
{Name: "container-a"},
201+
{Name: "container-b"},
202+
},
203+
},
204+
}
205+
206+
client := fakeclientset.NewClientset(pod)
207+
rt := &ReleaseTesting{
208+
Namespace: "default",
209+
Filters: map[string][]string{},
210+
}
211+
212+
// Call getContainerLogs directly to test output formatting
213+
var buf bytes.Buffer
214+
_ = rt.getContainerLogs(&buf, client, "multi-test")
215+
output := buf.String()
216+
// Even if logs fail, check that header formatting uses container names
217+
if len(output) > 0 {
218+
assert.True(t, strings.Contains(output, "(container-a)") || strings.Contains(output, "(container-b)"))
219+
}
220+
}

0 commit comments

Comments
 (0)