Skip to content

Commit 9eb2aaf

Browse files
committed
fix(webhook): remove shadowed err variable and add routes volume override test
**Shadowed variable fix:** - Remove `var err error` inside agentRuntime block (line 1363) - Use the function's named return parameter directly - Prevents fragile error handling where marshal failure might not propagate **Test coverage:** - Add TestOverrideRoutesConfigMapInVolumes following the pattern of TestOverrideEnvoyConfigMapInVolumes - Verifies volume swap without mutation - Checks that Optional is set to false for per-agent routes Addresses review comments from #517 Assisted-By: Claude Code Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
1 parent 2992cb1 commit 9eb2aaf

2 files changed

Lines changed: 74 additions & 1 deletion

File tree

operator/internal/webhook/injector/pod_mutator.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1360,7 +1360,6 @@ func (m *PodMutator) ensurePerAgentConfigMap(
13601360
routes = append(routes, route)
13611361
}
13621362

1363-
var err error
13641363
routesData, err = yaml.Marshal(routes)
13651364
if err != nil {
13661365
return "", "", fmt.Errorf("failed to marshal routes for %s/%s: %w", namespace, crName, err)

operator/internal/webhook/injector/volume_builder_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,3 +246,77 @@ func TestOverrideEnvoyConfigMapInVolumes(t *testing.T) {
246246
})
247247
}
248248
}
249+
250+
// TestOverrideRoutesConfigMapInVolumes verifies the per-agent routes override:
251+
// when AgentRuntime has spec.auth.outbound routes, the authproxy-routes volume
252+
// is redirected to authbridge-routes-<crName> without mutating the input.
253+
func TestOverrideRoutesConfigMapInVolumes(t *testing.T) {
254+
tests := []struct {
255+
name string
256+
volumes func() []corev1.Volume
257+
newCM string
258+
found bool
259+
}{
260+
{
261+
name: "volume found, name swapped",
262+
volumes: func() []corev1.Volume {
263+
return BuildRequiredVolumes()
264+
},
265+
newCM: "authbridge-routes-my-agent",
266+
found: true,
267+
},
268+
{
269+
name: "no authproxy-routes volume, list unchanged",
270+
volumes: func() []corev1.Volume {
271+
return []corev1.Volume{{
272+
Name: "shared-data",
273+
VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}},
274+
}}
275+
},
276+
newCM: "authbridge-routes-my-agent",
277+
found: false,
278+
},
279+
}
280+
281+
for _, tt := range tests {
282+
t.Run(tt.name, func(t *testing.T) {
283+
original := tt.volumes()
284+
overridden := overrideRoutesConfigMapInVolumes(original, tt.newCM)
285+
286+
// Original must not be mutated
287+
for _, v := range original {
288+
if v.Name == "authproxy-routes" && v.ConfigMap != nil {
289+
if v.ConfigMap.Name != AuthproxyRoutesConfigMapName {
290+
t.Errorf("original was mutated: got %q", v.ConfigMap.Name)
291+
}
292+
}
293+
}
294+
295+
// Output length matches input length
296+
if len(overridden) != len(original) {
297+
t.Fatalf("overridden length = %d, want %d", len(overridden), len(original))
298+
}
299+
300+
// Find-and-swap behavior
301+
swappedFound := false
302+
for _, v := range overridden {
303+
if v.Name == "authproxy-routes" && v.ConfigMap != nil {
304+
swappedFound = true
305+
if v.ConfigMap.Name != tt.newCM {
306+
t.Errorf("authproxy-routes CM name = %q, want %q", v.ConfigMap.Name, tt.newCM)
307+
}
308+
// Verify Optional is set to false for per-agent routes
309+
if v.ConfigMap.Optional == nil || *v.ConfigMap.Optional != false {
310+
t.Errorf("authproxy-routes Optional should be false when overridden")
311+
}
312+
}
313+
}
314+
if tt.found && !swappedFound {
315+
t.Fatal("expected authproxy-routes volume in overridden but didn't find it")
316+
}
317+
if !tt.found && swappedFound {
318+
t.Fatal("authproxy-routes volume should not have been added")
319+
}
320+
})
321+
}
322+
}

0 commit comments

Comments
 (0)