Merge pull request #799 from Nerja/main

Check if mandatory secrets/configmaps exist

Author: stefanprodan

pkg/canary/config_tracker.go

@@ -146,31 +146,29 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf
 		return nil, fmt.Errorf("TargetRef.Kind invalid: %s", cd.Spec.TargetRef.Kind)
 	}
 
-	type void struct{}
-	var member void
-	secretNames := map[string]void{}
-	configMapNames := map[string]void{}
+	secretNames := make(map[string]bool)
+	configMapNames := make(map[string]bool)
 
 	// scan volumes
 	for _, volume := range vs {
 		if cmv := volume.ConfigMap; cmv != nil {
 			name := cmv.Name
-			configMapNames[name] = member
+			configMapNames[name] = fieldIsMandatory(cmv.Optional)
 		}
 		if sv := volume.Secret; sv != nil {
 			name := sv.SecretName
-			secretNames[name] = member
+			secretNames[name] = fieldIsMandatory(sv.Optional)
 		}
 
 		if projected := volume.Projected; projected != nil {
 			for _, source := range projected.Sources {
 				if cmv := source.ConfigMap; cmv != nil {
 					name := cmv.Name
-					configMapNames[name] = member
+					configMapNames[name] = fieldIsMandatory(cmv.Optional)
 				}
 				if sv := source.Secret; sv != nil {
 					name := sv.Name
-					secretNames[name] = member
+					secretNames[name] = fieldIsMandatory(sv.Optional)
 				}
 			}
 		}
@@ -183,10 +181,10 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf
 				switch {
 				case env.ValueFrom.ConfigMapKeyRef != nil:
 					name := env.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name
-					configMapNames[name] = member
+					configMapNames[name] = fieldIsMandatory(env.ValueFrom.ConfigMapKeyRef.Optional)
 				case env.ValueFrom.SecretKeyRef != nil:
 					name := env.ValueFrom.SecretKeyRef.LocalObjectReference.Name
-					secretNames[name] = member
+					secretNames[name] = fieldIsMandatory(env.ValueFrom.SecretKeyRef.Optional)
 				}
 			}
 		}
@@ -195,30 +193,37 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf
 			switch {
 			case envFrom.ConfigMapRef != nil:
 				name := envFrom.ConfigMapRef.LocalObjectReference.Name
-				configMapNames[name] = member
+				configMapNames[name] = fieldIsMandatory(envFrom.ConfigMapRef.Optional)
 			case envFrom.SecretRef != nil:
 				name := envFrom.SecretRef.LocalObjectReference.Name
-				secretNames[name] = member
+				secretNames[name] = fieldIsMandatory(envFrom.SecretRef.Optional)
 			}
 		}
 	}
 
 	res := make(map[string]ConfigRef)
 
-	for configMapName := range configMapNames {
+	for configMapName, required := range configMapNames {
 		config, err := ct.getRefFromConfigMap(configMapName, cd.Namespace)
 		if err != nil {
-			ct.Logger.Errorf("getRefFromConfigMap failed: %v", err)
+			if required {
+				return nil, fmt.Errorf("configmap %s.%s get query error: %w", configMapName, cd.Namespace, err)
+			}
+			ct.Logger.Errorf("configmap %s.%s get query failed: %w", configMapName, cd.Namespace, err)
 			continue
 		}
 		if config != nil {
 			res[config.GetName()] = *config
 		}
 	}
-	for secretName := range secretNames {
+
+	for secretName, required := range secretNames {
 		secret, err := ct.getRefFromSecret(secretName, cd.Namespace)
 		if err != nil {
-			ct.Logger.Errorf("getRefFromSecret failed: %v", err)
+			if required {
+				return nil, fmt.Errorf("secret %s.%s get query error: %v", secretName, cd.Namespace, err)
+			}
+			ct.Logger.Errorf("secret %s.%s get query failed: %v", secretName, cd.Namespace, err)
 			continue
 		}
 		if secret != nil {
@@ -229,6 +234,13 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf
 	return res, nil
 }
 
+func fieldIsMandatory(p *bool) bool {
+	if p == nil {
+		return false
+	}
+	return !*p
+}
+
 // GetConfigRefs returns a map of configs and their checksum
 func (ct *ConfigTracker) GetConfigRefs(cd *flaggerv1.Canary) (*map[string]string, error) {
 	res := make(map[string]string)

pkg/canary/config_tracker_test.go

@@ -18,12 +18,15 @@ package canary
 
 import (
 	"context"
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	k8sTesting "k8s.io/client-go/testing"
 )
 
 func TestConfigIsDisabled(t *testing.T) {
@@ -303,3 +306,29 @@ func TestConfigTracker_Secrets(t *testing.T) {
 		assert.True(t, originalVolPresent, "Volume for original secret with disabled tracking should be present")
 	})
 }
+
+func TestConfigTracker_HasConfigChanged_ShouldReturnErrorWhenAPIServerIsDown(t *testing.T) {
+	t.Run("secret", func(t *testing.T) {
+		dc := deploymentConfigs{name: "podinfo", label: "name", labelValue: "podinfo"}
+		mocks, kubeClient := newCustomizableFixture(dc)
+
+		kubeClient.PrependReactor("get", "secrets", func(action k8sTesting.Action) (bool, runtime.Object, error) {
+			return true, nil, errors.New("server error")
+		})
+
+		_, err := mocks.controller.configTracker.HasConfigChanged(mocks.canary)
+		assert.Error(t, err)
+	})
+
+	t.Run("configmap", func(t *testing.T) {
+		dc := deploymentConfigs{name: "podinfo", label: "name", labelValue: "podinfo"}
+		mocks, kubeClient := newCustomizableFixture(dc)
+
+		kubeClient.PrependReactor("get", "configmaps", func(action k8sTesting.Action) (bool, runtime.Object, error) {
+			return true, nil, errors.New("server error")
+		})
+
+		_, err := mocks.controller.configTracker.HasConfigChanged(mocks.canary)
+		assert.Error(t, err)
+	})
+}

pkg/canary/deployment_fixture_test.go

@@ -78,6 +78,11 @@ func (d deploymentControllerFixture) initializeCanary(t *testing.T) {
 }
 
 func newDeploymentFixture(dc deploymentConfigs) deploymentControllerFixture {
+	fixture, _ := newCustomizableFixture(dc)
+	return fixture
+}
+
+func newCustomizableFixture(dc deploymentConfigs) (deploymentControllerFixture, *fake.Clientset) {
 	// init canary
 	cc := canaryConfigs{targetName: dc.name}
 	canary := newDeploymentControllerTestCanary(cc)
@@ -121,7 +126,7 @@ func newDeploymentFixture(dc deploymentConfigs) deploymentControllerFixture {
 		logger:        logger,
 		flaggerClient: flaggerClient,
 		kubeClient:    kubeClient,
-	}
+	}, kubeClient
 }
 
 func newDeploymentControllerTestConfigMap() *corev1.ConfigMap {
@@ -350,6 +355,7 @@ func newDeploymentControllerTestCanary(cc canaryConfigs) *flaggerv1.Canary {
 }
 
 func newDeploymentControllerTest(dc deploymentConfigs) *appsv1.Deployment {
+	var optional bool = false
 	d := &appsv1.Deployment{
 		TypeMeta: metav1.TypeMeta{APIVersion: appsv1.SchemeGroupVersion.String()},
 		ObjectMeta: metav1.ObjectMeta{
@@ -473,6 +479,7 @@ func newDeploymentControllerTest(dc deploymentConfigs) *appsv1.Deployment {
 									LocalObjectReference: corev1.LocalObjectReference{
 										Name: "podinfo-config-vol",
 									},
+									Optional: &optional,
 								},
 							},
 						},
@@ -481,6 +488,7 @@ func newDeploymentControllerTest(dc deploymentConfigs) *appsv1.Deployment {
 							VolumeSource: corev1.VolumeSource{
 								Secret: &corev1.SecretVolumeSource{
 									SecretName: "podinfo-secret-vol",
+									Optional:   &optional,
 								},
 							},
 						},