From 1e6a6ec2d9678c614b20caef4fb8c6b7f56a45a4 Mon Sep 17 00:00:00 2001 From: Pradeep Lakshmi Narasimha Date: Wed, 6 Aug 2025 21:10:11 +0530 Subject: [PATCH] fix: handle operator crash when using workloadRef --- internal/pkg/handler/upgrade.go | 18 +++++-- internal/pkg/handler/upgrade_test.go | 81 ++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/internal/pkg/handler/upgrade.go b/internal/pkg/handler/upgrade.go index 1e45c392..914056d4 100644 --- a/internal/pkg/handler/upgrade.go +++ b/internal/pkg/handler/upgrade.go @@ -417,7 +417,11 @@ func getContainerUsingResource(upgradeFuncs callbacks.RollingUpgradeFuncs, item container = getContainerWithVolumeMount(initContainers, volumeMountName) if container != nil { // if configmap/secret is being used in init container then return the first Pod container to save reloader env - return &containers[0] + if len(containers) > 0 { + return &containers[0] + } + // No containers available, return nil to avoid crash + return nil } } else if container != nil { return container @@ -430,13 +434,21 @@ func getContainerUsingResource(upgradeFuncs callbacks.RollingUpgradeFuncs, item container = getContainerWithEnvReference(initContainers, config.ResourceName, config.Type) if container != nil { // if configmap/secret is being used in init container then return the first Pod container to save reloader env - return &containers[0] + if len(containers) > 0 { + return &containers[0] + } + // No containers available, return nil to avoid crash + return nil } } // Get the first container if the annotation is related to specified configmap or secret i.e. configmap.reloader.stakater.com/reload if container == nil && !autoReload { - return &containers[0] + if len(containers) > 0 { + return &containers[0] + } + // No containers available, return nil to avoid crash + return nil } return container diff --git a/internal/pkg/handler/upgrade_test.go b/internal/pkg/handler/upgrade_test.go index 891dd1f4..3d2dcbf9 100644 --- a/internal/pkg/handler/upgrade_test.go +++ b/internal/pkg/handler/upgrade_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + argorolloutv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/prometheus/client_golang/prometheus" promtestutil "github.com/prometheus/client_golang/prometheus/testutil" "github.com/sirupsen/logrus" @@ -18,6 +19,7 @@ import ( "github.com/stakater/Reloader/internal/pkg/util" "github.com/stakater/Reloader/pkg/kube" "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -672,7 +674,7 @@ func teardownArs() { logrus.Errorf("Error while deleting statefulSet with secret as env var source %v", statefulSetError) } - // Deleting Deployment with pasuse annotation + // Deleting Deployment with pause annotation deploymentError = testutil.DeleteDeployment(clients.KubernetesClient, arsNamespace, arsConfigmapWithPausedDeployment) if deploymentError != nil { logrus.Errorf("Error while deleting deployment with configmap %v", deploymentError) @@ -708,7 +710,7 @@ func teardownArs() { logrus.Errorf("Error while deleting the configmap %v", err) } - // Deleting Configmap used projected volume in init containers + // Deleting secret used in projected volume in init containers err = testutil.DeleteSecret(clients.KubernetesClient, arsNamespace, arsProjectedSecretWithInitContainer) if err != nil { logrus.Errorf("Error while deleting the secret %v", err) @@ -1392,7 +1394,7 @@ func teardownErs() { logrus.Errorf("Error while deleting the configmap %v", err) } - // Deleting Configmap used projected volume in init containers + // Deleting secret used in projected volume in init containers err = testutil.DeleteSecret(clients.KubernetesClient, ersNamespace, ersProjectedSecretWithInitContainer) if err != nil { logrus.Errorf("Error while deleting the secret %v", err) @@ -1475,7 +1477,7 @@ func teardownErs() { logrus.Errorf("Error while deleting the configmap used with configmap exclude annotation: %v", err) } - // Deleting ConfigMap for testins pausing deployments + // Deleting ConfigMap for testing pausing deployments err = testutil.DeleteConfigMap(clients.KubernetesClient, ersNamespace, ersConfigmapWithPausedDeployment) if err != nil { logrus.Errorf("Error while deleting the configmap: %v", err) @@ -2247,7 +2249,7 @@ func TestRollingUpgradeForDeploymentWithSecretExcludeAnnotationUsingArs(t *testi logrus.Infof("Verifying deployment did not update") updated := testutil.VerifyResourceAnnotationUpdate(clients, config, deploymentFuncs) if updated { - t.Errorf("Deployment which had to be exluded was updated") + t.Errorf("Deployment which had to be excluded was updated") } } @@ -4214,3 +4216,72 @@ func waitForDeploymentPausedAtAnnotation(clients kube.Clients, deploymentFuncs c return fmt.Errorf("timeout waiting for deployment %s to have pause-period annotation", deploymentName) } + +// MockArgoRolloutWithEmptyContainers creates a mock Argo Rollout with no containers +// This simulates the scenario where Argo Rollouts with workloadRef return empty containers +func MockArgoRolloutWithEmptyContainers(namespace, name string) *runtime.Object { + rollout := &argorolloutv1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: argorolloutv1alpha1.RolloutSpec{ + Template: v1.PodTemplateSpec{ + Spec: v1.PodSpec{ + Containers: []v1.Container{}, // Empty containers slice + InitContainers: []v1.Container{}, // Empty init containers slice + Volumes: []v1.Volume{}, // Empty volumes slice + }, + }, + }, + } + var obj runtime.Object = rollout + return &obj +} + +// TestGetContainerUsingResourceWithArgoRolloutEmptyContainers tests with real Argo Rollout functions +func TestGetContainerUsingResourceWithArgoRolloutEmptyContainers(t *testing.T) { + namespace := "test-namespace" + resourceName := "test-configmap" + + // Use real Argo Rollout functions but mock the containers function + rolloutFuncs := GetArgoRolloutRollingUpgradeFuncs() + originalContainersFunc := rolloutFuncs.ContainersFunc + originalInitContainersFunc := rolloutFuncs.InitContainersFunc + + // Override to return empty containers (simulating workloadRef scenario) + rolloutFuncs.ContainersFunc = func(item runtime.Object) []v1.Container { + return []v1.Container{} // Empty like workloadRef rollouts + } + rolloutFuncs.InitContainersFunc = func(item runtime.Object) []v1.Container { + return []v1.Container{} // Empty like workloadRef rollouts + } + + // Restore original functions after test + defer func() { + rolloutFuncs.ContainersFunc = originalContainersFunc + rolloutFuncs.InitContainersFunc = originalInitContainersFunc + }() + + // Use proper Argo Rollout object instead of Pod + mockRollout := MockArgoRolloutWithEmptyContainers(namespace, "test-rollout") + + config := util.Config{ + Namespace: namespace, + ResourceName: resourceName, + Type: constants.ConfigmapEnvVarPostfix, + SHAValue: "test-sha", + } + + // Test both autoReload scenarios using subtests as suggested by Felix + for _, autoReload := range []bool{true, false} { + t.Run(fmt.Sprintf("autoReload_%t", autoReload), func(t *testing.T) { + // This tests the actual fix in the context of Argo Rollouts + result := getContainerUsingResource(rolloutFuncs, *mockRollout, config, autoReload) + + if result != nil { + t.Errorf("Expected nil when using real Argo Rollout functions with empty containers (workloadRef scenario), got %v", result) + } + }) + } +}