Skip to content

Commit 59028c3

Browse files
praddy26praddy26
committed
fix: handle operator crash when using workloadRef
1 parent 1107fee commit 59028c3

File tree

2 files changed

+286
-8
lines changed

2 files changed

+286
-8
lines changed

‎internal/pkg/handler/upgrade.go

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,11 @@ func getContainerUsingResource(upgradeFuncs callbacks.RollingUpgradeFuncs, item
417417
container = getContainerWithVolumeMount(initContainers, volumeMountName)
418418
if container != nil {
419419
// if configmap/secret is being used in init container then return the first Pod container to save reloader env
420-
return &containers[0]
420+
if len(containers) > 0 {
421+
return &containers[0]
422+
}
423+
// No containers available, return nil to avoid crash
424+
return nil
421425
}
422426
} else if container != nil {
423427
return container
@@ -430,13 +434,21 @@ func getContainerUsingResource(upgradeFuncs callbacks.RollingUpgradeFuncs, item
430434
container = getContainerWithEnvReference(initContainers, config.ResourceName, config.Type)
431435
if container != nil {
432436
// if configmap/secret is being used in init container then return the first Pod container to save reloader env
433-
return &containers[0]
437+
if len(containers) > 0 {
438+
return &containers[0]
439+
}
440+
// No containers available, return nil to avoid crash
441+
return nil
434442
}
435443
}
436444

437445
// Get the first container if the annotation is related to specified configmap or secret i.e. configmap.reloader.stakater.com/reload
438446
if container == nil && !autoReload {
439-
return &containers[0]
447+
if len(containers) > 0 {
448+
return &containers[0]
449+
}
450+
// No containers available, return nil to avoid crash
451+
return nil
440452
}
441453

442454
return container

‎internal/pkg/handler/upgrade_test.go

Lines changed: 271 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/stakater/Reloader/internal/pkg/util"
1919
"github.com/stakater/Reloader/pkg/kube"
2020
"github.com/stretchr/testify/assert"
21+
v1 "k8s.io/api/core/v1"
2122
"k8s.io/apimachinery/pkg/api/errors"
2223
"k8s.io/apimachinery/pkg/api/meta"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -672,7 +673,7 @@ func teardownArs() {
672673
logrus.Errorf("Error while deleting statefulSet with secret as env var source %v", statefulSetError)
673674
}
674675

675-
// Deleting Deployment with pasuse annotation
676+
// Deleting Deployment with pause annotation
676677
deploymentError = testutil.DeleteDeployment(clients.KubernetesClient, arsNamespace, arsConfigmapWithPausedDeployment)
677678
if deploymentError != nil {
678679
logrus.Errorf("Error while deleting deployment with configmap %v", deploymentError)
@@ -708,7 +709,7 @@ func teardownArs() {
708709
logrus.Errorf("Error while deleting the configmap %v", err)
709710
}
710711

711-
// Deleting Configmap used projected volume in init containers
712+
// Deleting secret used in projected volume in init containers
712713
err = testutil.DeleteSecret(clients.KubernetesClient, arsNamespace, arsProjectedSecretWithInitContainer)
713714
if err != nil {
714715
logrus.Errorf("Error while deleting the secret %v", err)
@@ -1392,7 +1393,7 @@ func teardownErs() {
13921393
logrus.Errorf("Error while deleting the configmap %v", err)
13931394
}
13941395

1395-
// Deleting Configmap used projected volume in init containers
1396+
// Deleting secret used in projected volume in init containers
13961397
err = testutil.DeleteSecret(clients.KubernetesClient, ersNamespace, ersProjectedSecretWithInitContainer)
13971398
if err != nil {
13981399
logrus.Errorf("Error while deleting the secret %v", err)
@@ -1475,7 +1476,7 @@ func teardownErs() {
14751476
logrus.Errorf("Error while deleting the configmap used with configmap exclude annotation: %v", err)
14761477
}
14771478

1478-
// Deleting ConfigMap for testins pausing deployments
1479+
// Deleting ConfigMap for testing pausing deployments
14791480
err = testutil.DeleteConfigMap(clients.KubernetesClient, ersNamespace, ersConfigmapWithPausedDeployment)
14801481
if err != nil {
14811482
logrus.Errorf("Error while deleting the configmap: %v", err)
@@ -2247,7 +2248,7 @@ func TestRollingUpgradeForDeploymentWithSecretExcludeAnnotationUsingArs(t *testi
22472248
logrus.Infof("Verifying deployment did not update")
22482249
updated := testutil.VerifyResourceAnnotationUpdate(clients, config, deploymentFuncs)
22492250
if updated {
2250-
t.Errorf("Deployment which had to be exluded was updated")
2251+
t.Errorf("Deployment which had to be excluded was updated")
22512252
}
22522253
}
22532254

@@ -4214,3 +4215,268 @@ func waitForDeploymentPausedAtAnnotation(clients kube.Clients, deploymentFuncs c
42144215

42154216
return fmt.Errorf("timeout waiting for deployment %s to have pause-period annotation", deploymentName)
42164217
}
4218+
4219+
// MockDeploymentWithEmptyContainers creates a mock deployment with no containers
4220+
// This simulates the scenario where Argo Rollouts with workloadRef return empty containers
4221+
func MockDeploymentWithEmptyContainers(namespace, name string) *runtime.Object {
4222+
deployment := &v1.Pod{
4223+
ObjectMeta: metav1.ObjectMeta{
4224+
Name: name,
4225+
Namespace: namespace,
4226+
},
4227+
Spec: v1.PodSpec{
4228+
Containers: []v1.Container{}, // Empty containers slice
4229+
InitContainers: []v1.Container{}, // Empty init containers slice
4230+
},
4231+
}
4232+
var obj runtime.Object = deployment
4233+
return &obj
4234+
}
4235+
4236+
// MockUpgradeFuncsWithEmptyContainers creates upgrade functions that return empty containers
4237+
// This simulates the behavior of GetRolloutContainers for workloadRef rollouts
4238+
func MockUpgradeFuncsWithEmptyContainers() callbacks.RollingUpgradeFuncs {
4239+
return callbacks.RollingUpgradeFuncs{
4240+
ContainersFunc: func(item runtime.Object) []v1.Container {
4241+
return []v1.Container{} // Always return empty slice
4242+
},
4243+
InitContainersFunc: func(item runtime.Object) []v1.Container {
4244+
return []v1.Container{} // Always return empty slice
4245+
},
4246+
VolumesFunc: func(item runtime.Object) []v1.Volume {
4247+
return []v1.Volume{} // No volumes
4248+
},
4249+
ResourceType: "MockResource",
4250+
}
4251+
}
4252+
4253+
// TestGetContainerUsingResourceWithEmptyContainers tests that the function
4254+
// handles empty containers gracefully without panicking
4255+
func TestGetContainerUsingResourceWithEmptyContainers(t *testing.T) {
4256+
// Setup test data
4257+
namespace := "test-namespace"
4258+
resourceName := "test-resource"
4259+
4260+
// Create a mock deployment with empty containers
4261+
mockDeployment := MockDeploymentWithEmptyContainers(namespace, "test-deployment")
4262+
4263+
// Create upgrade functions that return empty containers (simulating workloadRef rollouts)
4264+
upgradeFuncs := MockUpgradeFuncsWithEmptyContainers()
4265+
4266+
// Create config for configmap test
4267+
config := util.Config{
4268+
Namespace: namespace,
4269+
ResourceName: resourceName,
4270+
Type: constants.ConfigmapEnvVarPostfix,
4271+
SHAValue: "test-sha",
4272+
}
4273+
4274+
// Test cases
4275+
testCases := []struct {
4276+
name string
4277+
autoReload bool
4278+
expected *v1.Container
4279+
}{
4280+
{
4281+
name: "Non-auto reload with empty containers should return nil",
4282+
autoReload: false,
4283+
expected: nil,
4284+
},
4285+
{
4286+
name: "Auto reload with empty containers should return nil",
4287+
autoReload: true,
4288+
expected: nil,
4289+
},
4290+
}
4291+
4292+
for _, tc := range testCases {
4293+
t.Run(tc.name, func(t *testing.T) {
4294+
// This should not panic - our fix prevents the index out of range error
4295+
result := getContainerUsingResource(upgradeFuncs, *mockDeployment, config, tc.autoReload)
4296+
4297+
if result != tc.expected {
4298+
t.Errorf("Expected %v, got %v", tc.expected, result)
4299+
}
4300+
})
4301+
}
4302+
}
4303+
4304+
// TestGetContainerUsingResourceWithEmptyInitContainers tests scenarios where
4305+
// init containers exist but are empty, and main containers are also empty
4306+
func TestGetContainerUsingResourceWithEmptyInitContainers(t *testing.T) {
4307+
namespace := "test-namespace"
4308+
resourceName := "test-resource"
4309+
4310+
// Create upgrade functions with empty init containers as well
4311+
upgradeFuncs := callbacks.RollingUpgradeFuncs{
4312+
ContainersFunc: func(item runtime.Object) []v1.Container {
4313+
return []v1.Container{} // Empty main containers
4314+
},
4315+
InitContainersFunc: func(item runtime.Object) []v1.Container {
4316+
// Return init containers that use the resource, but main containers are empty
4317+
return []v1.Container{
4318+
{
4319+
Name: "init-container",
4320+
Env: []v1.EnvVar{
4321+
{
4322+
Name: "CONFIGMAP_" + util.ConvertToEnvVarName(resourceName),
4323+
ValueFrom: &v1.EnvVarSource{
4324+
ConfigMapKeyRef: &v1.ConfigMapKeySelector{
4325+
LocalObjectReference: v1.LocalObjectReference{
4326+
Name: resourceName,
4327+
},
4328+
Key: "test.key",
4329+
},
4330+
},
4331+
},
4332+
},
4333+
},
4334+
}
4335+
},
4336+
VolumesFunc: func(item runtime.Object) []v1.Volume {
4337+
return []v1.Volume{}
4338+
},
4339+
ResourceType: "MockResource",
4340+
}
4341+
4342+
mockDeployment := MockDeploymentWithEmptyContainers(namespace, "test-deployment")
4343+
4344+
config := util.Config{
4345+
Namespace: namespace,
4346+
ResourceName: resourceName,
4347+
Type: constants.ConfigmapEnvVarPostfix,
4348+
SHAValue: "test-sha",
4349+
}
4350+
4351+
// This should return nil instead of panicking when trying to access containers[0]
4352+
result := getContainerUsingResource(upgradeFuncs, *mockDeployment, config, false)
4353+
4354+
if result != nil {
4355+
t.Errorf("Expected nil when main containers are empty, got %v", result)
4356+
}
4357+
}
4358+
4359+
// TestGetContainerUsingResourceWithVolumeMount tests the volume mount path with empty containers
4360+
func TestGetContainerUsingResourceWithVolumeMount(t *testing.T) {
4361+
namespace := "test-namespace"
4362+
resourceName := "test-resource"
4363+
volumeName := "test-volume"
4364+
4365+
// Create upgrade functions that return a volume but no containers
4366+
upgradeFuncs := callbacks.RollingUpgradeFuncs{
4367+
ContainersFunc: func(item runtime.Object) []v1.Container {
4368+
return []v1.Container{} // Empty containers
4369+
},
4370+
InitContainersFunc: func(item runtime.Object) []v1.Container {
4371+
// Init container with volume mount
4372+
return []v1.Container{
4373+
{
4374+
Name: "init-container",
4375+
VolumeMounts: []v1.VolumeMount{
4376+
{
4377+
Name: volumeName,
4378+
MountPath: "/test",
4379+
},
4380+
},
4381+
},
4382+
}
4383+
},
4384+
VolumesFunc: func(item runtime.Object) []v1.Volume {
4385+
return []v1.Volume{
4386+
{
4387+
Name: volumeName,
4388+
VolumeSource: v1.VolumeSource{
4389+
ConfigMap: &v1.ConfigMapVolumeSource{
4390+
LocalObjectReference: v1.LocalObjectReference{
4391+
Name: resourceName,
4392+
},
4393+
},
4394+
},
4395+
},
4396+
}
4397+
},
4398+
ResourceType: "MockResource",
4399+
}
4400+
4401+
mockDeployment := MockDeploymentWithEmptyContainers(namespace, "test-deployment")
4402+
4403+
config := util.Config{
4404+
Namespace: namespace,
4405+
ResourceName: resourceName,
4406+
Type: constants.ConfigmapEnvVarPostfix,
4407+
SHAValue: "test-sha",
4408+
}
4409+
4410+
// This should return nil instead of panicking when containers are empty
4411+
result := getContainerUsingResource(upgradeFuncs, *mockDeployment, config, false)
4412+
4413+
if result != nil {
4414+
t.Errorf("Expected nil when main containers are empty even with volume mount, got %v", result)
4415+
}
4416+
}
4417+
4418+
// TestGetContainerUsingResourceWithEmptyContainersSecret tests the fix with Secret resources
4419+
func TestGetContainerUsingResourceWithEmptyContainersSecret(t *testing.T) {
4420+
namespace := "test-namespace"
4421+
resourceName := "test-secret"
4422+
4423+
mockDeployment := MockDeploymentWithEmptyContainers(namespace, "test-deployment")
4424+
upgradeFuncs := MockUpgradeFuncsWithEmptyContainers()
4425+
4426+
// Test with Secret instead of ConfigMap
4427+
config := util.Config{
4428+
Namespace: namespace,
4429+
ResourceName: resourceName,
4430+
Type: constants.SecretEnvVarPostfix,
4431+
SHAValue: "test-sha",
4432+
}
4433+
4434+
// Both autoReload scenarios should return nil without panicking
4435+
result1 := getContainerUsingResource(upgradeFuncs, *mockDeployment, config, false)
4436+
result2 := getContainerUsingResource(upgradeFuncs, *mockDeployment, config, true)
4437+
4438+
if result1 != nil || result2 != nil {
4439+
t.Errorf("Expected nil for both autoReload scenarios with empty containers and Secret resource")
4440+
}
4441+
}
4442+
4443+
// TestGetContainerUsingResourceWithArgoRolloutEmptyContainers tests with real Argo Rollout functions
4444+
func TestGetContainerUsingResourceWithArgoRolloutEmptyContainers(t *testing.T) {
4445+
namespace := "test-namespace"
4446+
resourceName := "test-configmap"
4447+
4448+
// Use real Argo Rollout functions but mock the containers function
4449+
rolloutFuncs := GetArgoRolloutRollingUpgradeFuncs()
4450+
originalContainersFunc := rolloutFuncs.ContainersFunc
4451+
originalInitContainersFunc := rolloutFuncs.InitContainersFunc
4452+
4453+
// Override to return empty containers (simulating workloadRef scenario)
4454+
rolloutFuncs.ContainersFunc = func(item runtime.Object) []v1.Container {
4455+
return []v1.Container{} // Empty like workloadRef rollouts
4456+
}
4457+
rolloutFuncs.InitContainersFunc = func(item runtime.Object) []v1.Container {
4458+
return []v1.Container{} // Empty like workloadRef rollouts
4459+
}
4460+
4461+
// Restore original functions after test
4462+
defer func() {
4463+
rolloutFuncs.ContainersFunc = originalContainersFunc
4464+
rolloutFuncs.InitContainersFunc = originalInitContainersFunc
4465+
}()
4466+
4467+
mockRollout := MockDeploymentWithEmptyContainers(namespace, "test-rollout")
4468+
4469+
config := util.Config{
4470+
Namespace: namespace,
4471+
ResourceName: resourceName,
4472+
Type: constants.ConfigmapEnvVarPostfix,
4473+
SHAValue: "test-sha",
4474+
}
4475+
4476+
// This tests the actual fix in the context of Argo Rollouts
4477+
result := getContainerUsingResource(rolloutFuncs, *mockRollout, config, false)
4478+
4479+
if result != nil {
4480+
t.Errorf("Expected nil when using real Argo Rollout functions with empty containers (workloadRef scenario), got %v", result)
4481+
}
4482+
}

0 commit comments

Comments
 (0)