From 56c77085bf07d6ce20ede5fa0dd16d042b5118e6 Mon Sep 17 00:00:00 2001 From: Marcus Rodan Date: Mon, 1 Feb 2021 17:30:26 +0100 Subject: [PATCH 1/2] Added checks for API server errors Signed-off-by: Marcus Rodan --- pkg/canary/config_tracker.go | 7 +++---- pkg/canary/config_tracker_test.go | 29 +++++++++++++++++++++++++++ pkg/canary/deployment_fixture_test.go | 7 ++++++- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/pkg/canary/config_tracker.go b/pkg/canary/config_tracker.go index 23e80feb1..4bbc07656 100644 --- a/pkg/canary/config_tracker.go +++ b/pkg/canary/config_tracker.go @@ -208,18 +208,17 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf for configMapName := range configMapNames { config, err := ct.getRefFromConfigMap(configMapName, cd.Namespace) if err != nil { - ct.Logger.Errorf("getRefFromConfigMap failed: %v", err) - continue + return nil, fmt.Errorf("configmap %s.%s get query error: %w", configMapName, cd.Namespace, err) } if config != nil { res[config.GetName()] = *config } } + for secretName := range secretNames { secret, err := ct.getRefFromSecret(secretName, cd.Namespace) if err != nil { - ct.Logger.Errorf("getRefFromSecret failed: %v", err) - continue + return nil, fmt.Errorf("secret %s.%s get query error: %w", secretName, cd.Namespace, err) } if secret != nil { res[secret.GetName()] = *secret diff --git a/pkg/canary/config_tracker_test.go b/pkg/canary/config_tracker_test.go index 3399db95b..f229a5afb 100644 --- a/pkg/canary/config_tracker_test.go +++ b/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) + }) +} diff --git a/pkg/canary/deployment_fixture_test.go b/pkg/canary/deployment_fixture_test.go index 05bc19590..7877bd1ba 100644 --- a/pkg/canary/deployment_fixture_test.go +++ b/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 { From 565b99e210820c85b98f26a1725584db676460b7 Mon Sep 17 00:00:00 2001 From: Marcus Rodan Date: Tue, 2 Feb 2021 09:55:38 +0100 Subject: [PATCH 2/2] Added check for optional Signed-off-by: Marcus Rodan --- pkg/canary/config_tracker.go | 45 +++++++++++++++++---------- pkg/canary/deployment_fixture_test.go | 3 ++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/pkg/canary/config_tracker.go b/pkg/canary/config_tracker.go index 4bbc07656..77737d45d 100644 --- a/pkg/canary/config_tracker.go +++ b/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,38 @@ 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 { - return nil, fmt.Errorf("configmap %s.%s get query error: %w", configMapName, cd.Namespace, 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 { - return nil, fmt.Errorf("secret %s.%s get query error: %w", secretName, cd.Namespace, 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 { res[secret.GetName()] = *secret @@ -228,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) diff --git a/pkg/canary/deployment_fixture_test.go b/pkg/canary/deployment_fixture_test.go index 7877bd1ba..1fc5b99da 100644 --- a/pkg/canary/deployment_fixture_test.go +++ b/pkg/canary/deployment_fixture_test.go @@ -355,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{ @@ -478,6 +479,7 @@ func newDeploymentControllerTest(dc deploymentConfigs) *appsv1.Deployment { LocalObjectReference: corev1.LocalObjectReference{ Name: "podinfo-config-vol", }, + Optional: &optional, }, }, }, @@ -486,6 +488,7 @@ func newDeploymentControllerTest(dc deploymentConfigs) *appsv1.Deployment { VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "podinfo-secret-vol", + Optional: &optional, }, }, },