fix: retry VC status update & properly set failed conditions

Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>

internal/controller/testdata/vc-network.yaml

@@ -13,6 +13,7 @@ spec:
   - chart:
       name: validator-plugin-network
       repository: https://spectrocloud-labs.github.io/validator-plugin-network
+      authSecretName: validator-plugin-network-chart-secret
       version: v0.0.4
     values: |-
       controllerManager:

internal/controller/validationresult_controller.go

@@ -71,7 +71,7 @@ func (r *ValidationResultReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	vr = &v1alpha1.ValidationResult{}
 	vrKey = req.NamespacedName
 
-	if err := r.Get(ctx, req.NamespacedName, vr); err != nil {
+	if err := r.Get(ctx, vrKey, vr); err != nil {
 		if !apierrs.IsNotFound(err) {
 			r.Log.Error(err, "failed to fetch ValidationResult", "key", req)
 		}
@@ -170,7 +170,7 @@ func (r *ValidationResultReconciler) updateStatus(ctx context.Context) error {
 	vr.Status.SinkState = sinkState
 
 	if err := r.Status().Update(context.Background(), vr); err != nil {
-		r.Log.V(1).Info("warning: failed to update ValidationResult status", "error", err.Error())
+		r.Log.V(1).Info("warning: failed to update ValidationResult status", "error", err)
 		return err
 	}
 

internal/controller/validatorconfig_controller.go

@@ -35,6 +35,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	v1alpha1 "github.com/spectrocloud-labs/validator/api/v1alpha1"
+	"github.com/spectrocloud-labs/validator/pkg/constants"
 	"github.com/spectrocloud-labs/validator/pkg/helm"
 )
 
@@ -46,6 +47,12 @@ const (
 	PluginValuesHash = "validator/plugin-values"
 )
 
+var (
+	vc         *v1alpha1.ValidatorConfig
+	vcKey      types.NamespacedName
+	conditions []v1alpha1.ValidatorPluginCondition
+)
+
 // ValidatorConfigReconciler reconciles a ValidatorConfig object
 type ValidatorConfigReconciler struct {
 	client.Client
@@ -62,17 +69,25 @@ type ValidatorConfigReconciler struct {
 func (r *ValidatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	r.Log.V(0).Info("Reconciling ValidatorConfig", "name", req.Name, "namespace", req.Namespace)
 
-	vc := &v1alpha1.ValidatorConfig{}
-	if err := r.Get(ctx, req.NamespacedName, vc); err != nil {
+	vc = &v1alpha1.ValidatorConfig{}
+	vcKey = req.NamespacedName
+
+	if err := r.Get(ctx, vcKey, vc); err != nil {
 		if !apierrs.IsNotFound(err) {
 			r.Log.Error(err, "failed to fetch ValidatorConfig", "key", req)
 		}
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	// Always update the ValidatorConfig's Status
+	// TODO: implement a proper status patcher to avoid this hacky approach with retries & global vars
 	defer func() {
-		r.updateStatus(ctx, vc)
+		// Always update the ValidationConfig's Status with a retry due to race condition with
+		// removeFinalizer and ensureFinalizer.
+		for i := 0; i < constants.StatusUpdateRetries; i++ {
+			if err := r.updateStatus(ctx); err == nil {
+				break
+			}
+		}
 	}()
 
 	// handle ValidatorConfig deletion
@@ -117,17 +132,27 @@ func (r *ValidatorConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 // updateStatus updates the ValidatorConfig's status subresource
-func (r *ValidatorConfigReconciler) updateStatus(ctx context.Context, vc *v1alpha1.ValidatorConfig) {
+func (r *ValidatorConfigReconciler) updateStatus(ctx context.Context) error {
+	if err := r.Get(ctx, vcKey, vc); err != nil {
+		r.Log.V(0).Error(err, "failed to get ValidatorConfig")
+		return err
+	}
+
+	// all status modifications must happen after r.Client.Update
+	vc.Status.Conditions = conditions
+
 	if err := r.Status().Update(context.Background(), vc); err != nil {
-		r.Log.V(0).Error(err, "failed to update ValidatorConfig status")
+		r.Log.V(1).Info("warning: failed to update ValidatorConfig status", "error", err)
 	}
+
 	r.Log.V(0).Info("Updated ValidatorConfig", "conditions", vc.Status.Conditions, "time", time.Now())
+	return nil
 }
 
 // redeployIfNeeded deploys/redeploys each validator plugin in a ValidatorConfig and deletes plugins that have been removed
 func (r *ValidatorConfigReconciler) redeployIfNeeded(ctx context.Context, vc *v1alpha1.ValidatorConfig) error {
 	specPlugins := make(map[string]bool)
-	conditions := make([]v1alpha1.ValidatorPluginCondition, len(vc.Spec.Plugins))
+	conditions = make([]v1alpha1.ValidatorPluginCondition, len(vc.Spec.Plugins))
 
 	for i, p := range vc.Spec.Plugins {
 		specPlugins[p.Chart.Name] = true
@@ -143,47 +168,35 @@ func (r *ValidatorConfigReconciler) redeployIfNeeded(ctx context.Context, vc *v1
 			continue
 		}
 
-		var username, password []byte
+		upgradeOpts := &helm.UpgradeOptions{
+			Chart:                 p.Chart.Name,
+			Repo:                  p.Chart.Repository,
+			Version:               p.Chart.Version,
+			Values:                p.Values,
+			InsecureSkipTlsVerify: p.Chart.InsecureSkipTlsVerify,
+		}
+
 		if p.Chart.AuthSecretName != "" {
-			secret := &corev1.Secret{}
 			nn := types.NamespacedName{Name: p.Chart.AuthSecretName, Namespace: vc.Namespace}
-			if err := r.Get(ctx, nn, secret); err != nil {
-				return fmt.Errorf(
-					"failed to get auth secret %s in namespace %s for chart %s in repo %s: %v",
-					p.Chart.AuthSecretName, vc.Namespace, p.Chart.Name, p.Chart.Repository, err,
-				)
-			}
-			username, ok = secret.Data["username"]
-			if !ok {
-				return fmt.Errorf("auth secret for chart %s in repo %s missing required key: 'username'", p.Chart.Name, p.Chart.Repository)
-			}
-			password, ok = secret.Data["password"]
-			if !ok {
-				return fmt.Errorf("auth secret for chart %s in repo %s missing required key: 'password'", p.Chart.Name, p.Chart.Repository)
+			if err := r.configureHelmBasicAuth(nn, upgradeOpts); err != nil {
+				r.Log.V(0).Error(err, "failed to configure basic auth for Helm upgrade")
+				conditions[i] = buildHelmChartCondition(p.Chart.Name, err)
+				continue
 			}
 		}
 
 		r.Log.V(0).Info("Installing/upgrading plugin Helm chart", "namespace", vc.Namespace, "name", p.Chart.Name)
 
-		err := r.HelmClient.Upgrade(p.Chart.Name, vc.Namespace, helm.UpgradeOptions{
-			Chart:                 p.Chart.Name,
-			Repo:                  p.Chart.Repository,
-			Version:               p.Chart.Version,
-			Values:                p.Values,
-			InsecureSkipTlsVerify: p.Chart.InsecureSkipTlsVerify,
-			Username:              string(username),
-			Password:              string(password),
-		})
+		err := r.HelmClient.Upgrade(p.Chart.Name, vc.Namespace, *upgradeOpts)
 		if err != nil {
 			// if Helm install/upgrade failed, delete the release so installation is reattempted each iteration
 			if strings.Contains(err.Error(), "has no deployed releases") {
 				if err := r.HelmClient.Delete(p.Chart.Name, vc.Namespace); err != nil {
 					r.Log.V(0).Error(err, "failed to delete Helm release")
 				}
 			}
-			return fmt.Errorf("error installing / upgrading ValidatorConfig: %v", err)
 		}
-		conditions[i] = getHelmChartCondition(p.Chart.Name, true)
+		conditions[i] = buildHelmChartCondition(p.Chart.Name, err)
 	}
 
 	// delete any plugins that have been removed
@@ -201,8 +214,29 @@ func (r *ValidatorConfigReconciler) redeployIfNeeded(ctx context.Context, vc *v1
 		return err
 	}
 
-	// all status modifications must happen after r.Client.Update
-	vc.Status.Conditions = conditions
+	return nil
+}
+
+func (r *ValidatorConfigReconciler) configureHelmBasicAuth(nn types.NamespacedName, opts *helm.UpgradeOptions) error {
+	secret := &corev1.Secret{}
+	if err := r.Get(context.TODO(), nn, secret); err != nil {
+		return fmt.Errorf(
+			"failed to get auth secret %s in namespace %s for chart %s in repo %s: %v",
+			nn.Name, nn.Namespace, opts.Chart, opts.Repo, err,
+		)
+	}
+
+	username, ok := secret.Data["username"]
+	if !ok {
+		return fmt.Errorf("auth secret for chart %s in repo %s missing required key: 'username'", opts.Chart, opts.Repo)
+	}
+	opts.Username = string(username)
+
+	password, ok := secret.Data["password"]
+	if !ok {
+		return fmt.Errorf("auth secret for chart %s in repo %s missing required key: 'password'", opts.Chart, opts.Repo)
+	}
+	opts.Password = string(password)
 
 	return nil
 }
@@ -312,18 +346,18 @@ func isConditionTrue(vc *v1alpha1.ValidatorConfig, chartName string, conditionTy
 	return vc.Status.Conditions[idx], vc.Status.Conditions[idx].Status == corev1.ConditionTrue
 }
 
-// getHelmChartCondition builds a ValidatorPluginCondition for a plugin
-func getHelmChartCondition(chartName string, installed bool) v1alpha1.ValidatorPluginCondition {
+// buildHelmChartCondition builds a ValidatorPluginCondition for a plugin
+func buildHelmChartCondition(chartName string, err error) v1alpha1.ValidatorPluginCondition {
 	condition := v1alpha1.ValidatorPluginCondition{
 		Type:               v1alpha1.HelmChartDeployedCondition,
 		PluginName:         chartName,
 		Status:             corev1.ConditionTrue,
 		Message:            fmt.Sprintf("Plugin %s is installed", chartName),
 		LastTransitionTime: metav1.Time{Time: time.Now()},
 	}
-	if !installed {
+	if err != nil {
 		condition.Status = corev1.ConditionFalse
-		condition.Message = fmt.Sprintf("Plugin %s was uninstalled", chartName)
+		condition.Message = err.Error()
 	}
 	return condition
 }

internal/controller/validatorconfig_controller_test.go

@@ -2,20 +2,27 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
+	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	kyaml "sigs.k8s.io/yaml"
 
 	"github.com/spectrocloud-labs/validator/api/v1alpha1"
+	"github.com/spectrocloud-labs/validator/internal/test"
+	"github.com/spectrocloud-labs/validator/pkg/helm"
 	//+kubebuilder:scaffold:imports
 )
 
@@ -41,6 +48,23 @@ var _ = Describe("ValidatorConfig controller", Ordered, func() {
 		}
 	})
 
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "validator-plugin-network-invalid-auth",
+			Namespace: validatorNamespace,
+		},
+		Data: map[string][]byte{
+			"username": []byte("foo"),
+		},
+	}
+
+	validSecret := secret.DeepCopy()
+	validSecret.Name = "validator-plugin-network-chart-secret"
+	validSecret.Data = map[string][]byte{
+		"username": []byte("foo"),
+		"password": []byte("bar"),
+	}
+
 	vc := &v1alpha1.ValidatorConfig{}
 	vcKey := types.NamespacedName{Name: "validator-config-test", Namespace: validatorNamespace}
 	networkPluginDeploymentKey := types.NamespacedName{Name: networkPluginDeploymentName, Namespace: validatorNamespace}
@@ -56,6 +80,8 @@ var _ = Describe("ValidatorConfig controller", Ordered, func() {
 		err = kyaml.Unmarshal(vcBytes, vc)
 		Expect(err).NotTo(HaveOccurred())
 
+		Expect(k8sClient.Create(ctx, secret)).Should(Succeed())
+		Expect(k8sClient.Create(ctx, validSecret)).Should(Succeed())
 		Expect(k8sClient.Create(ctx, vc)).Should(Succeed())
 
 		// Wait for the validator-plugin-network Deployment to be deployed
@@ -110,4 +136,77 @@ var _ = Describe("ValidatorConfig controller", Ordered, func() {
 			return apierrs.IsNotFound(err)
 		}, timeout, interval).Should(BeTrue(), "failed to uninstall validator-plugin-network")
 	})
+
+	It("Should fail to deploy validator-plugin-network-invalid-auth once a ValidatorConfig is created", func() {
+		By("By creating a new ValidatorConfig")
+		ctx := context.Background()
+
+		vc := &v1alpha1.ValidatorConfig{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "validator-plugin-network-invalid-auth",
+				Namespace: validatorNamespace,
+			},
+			Spec: v1alpha1.ValidatorConfigSpec{
+				Plugins: []v1alpha1.HelmRelease{
+					{
+						Chart: v1alpha1.HelmChart{
+							Name:           "foo",
+							Repository:     "bar",
+							AuthSecretName: "chart-secret",
+						},
+					},
+				},
+			},
+		}
+		vcKey := types.NamespacedName{Name: "validator-plugin-network-invalid-auth", Namespace: validatorNamespace}
+
+		Expect(k8sClient.Create(ctx, vc)).Should(Succeed())
+
+		// Wait for the validator-plugin-network-invalid-auth deployment to fail
+		Eventually(func() bool {
+			if err := k8sClient.Get(ctx, vcKey, vc); err != nil {
+				return false
+			}
+			condition, ok := isConditionTrue(vc, "foo", v1alpha1.HelmChartDeployedCondition)
+			return condition.Status == corev1.ConditionFalse && !ok
+		}, timeout, interval).Should(BeTrue(), "failed to deploy validator-plugin-network")
+	})
 })
+
+func TestConfigureHelmBasicAuth(t *testing.T) {
+	cs := []struct {
+		name       string
+		reconciler ValidatorConfigReconciler
+		nn         types.NamespacedName
+		opts       *helm.UpgradeOptions
+		expected   error
+	}{
+		{
+			name: "Fail (get_secret)",
+			nn:   types.NamespacedName{Name: "chart-secret", Namespace: validatorNamespace},
+			reconciler: ValidatorConfigReconciler{
+				Client: test.ClientMock{
+					GetErrors: []error{errors.New("get failed")},
+				},
+			},
+			opts:     &helm.UpgradeOptions{Chart: "foo", Repo: "bar"},
+			expected: errors.New("failed to get auth secret chart-secret in namespace validator for chart foo in repo bar: get failed"),
+		},
+		{
+			name: "Fail (get_username)",
+			nn:   types.NamespacedName{Name: "chart-secret", Namespace: validatorNamespace},
+			reconciler: ValidatorConfigReconciler{
+				Client: test.ClientMock{},
+			},
+			opts:     &helm.UpgradeOptions{Chart: "foo", Repo: "bar"},
+			expected: errors.New("auth secret for chart foo in repo bar missing required key: 'username'"),
+		},
+	}
+	for _, c := range cs {
+		t.Log(c.name)
+		err := c.reconciler.configureHelmBasicAuth(c.nn, c.opts)
+		if err != nil && !reflect.DeepEqual(err.Error(), c.expected.Error()) {
+			t.Errorf("expected (%v), got (%v)", c.expected, err)
+		}
+	}
+}