Fix bug when VCL is not loaded when only the container and not the wh…

…ole pod is restarted (#69) * Fix bug when VCL is not loaded when only the container and not the whole pod is restarted. * Fire an event and log the case when a backend pod is found but couldn't find the port definition we're looking for. * Fire an event if both a templated and not templated version of a VCL file is set in the VCL ConfigMap. Signed-off-by: Tomash Sidei <[email protected]> Co-authored-by: Craig Ingram <[email protected]> * Fix varnish pods watcher predicate. Remove endpoint selector env var config for varnish controller. Now the selector is taken from the CR we get from the API. That allows to avoid rolling upgrades when the backend selector has changed.
IBM · May 16, 2022 · e69f4f7 · e69f4f7
1 parent 1293193
commit e69f4f7
Show file tree

Hide file tree

Showing 7 changed files with 46 additions and 44 deletions.
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/pkg/varnishcluster/controller/varnishcluster_statefulset.go b/pkg/varnishcluster/controller/varnishcluster_statefulset.go
@@ -18,7 +18,6 @@ import (
 	v1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -218,7 +217,6 @@ func (r *ReconcileVarnishCluster) reconcileStatefulSet(ctx context.Context, inst
 								},
 							},
 							Env: []v1.EnvVar{
-								{Name: "ENDPOINT_SELECTOR_STRING", Value: labels.SelectorFromSet(endpointSelector).String()},
 								{Name: "NAMESPACE", Value: instance.Namespace},
 								{Name: "POD_NAME", ValueFrom: &v1.EnvVarSource{FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}}},
 								{Name: "NODE_NAME", ValueFrom: &v1.EnvVarSource{FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "spec.nodeName"}}},

diff --git a/pkg/varnishcluster/controller/varnishcluster_statefulset_test.go b/pkg/varnishcluster/controller/varnishcluster_statefulset_test.go
@@ -9,8 +9,6 @@ import (
 
 	vcapi "github.com/ibm/varnish-operator/api/v1alpha1"
 
-	"k8s.io/apimachinery/pkg/labels"
-
 	"github.com/gogo/protobuf/proto"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -117,11 +115,6 @@ var _ = Describe("statefulset", func() {
 			varnishControllerContainer, err := getContainerByName(podSpec, vcapi.VarnishControllerName)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(varnishControllerContainer.Image).To(Equal("ibmcom/varnish-controller:test"))
-			Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "ENDPOINT_SELECTOR_STRING", Value: labels.SelectorFromSet(map[string]string{
-				vcapi.LabelVarnishOwner:     vcName,
-				vcapi.LabelVarnishComponent: vcapi.VarnishComponentNoCacheService,
-				vcapi.LabelVarnishUID:       string(newVC.UID),
-			}).String()}))
 			Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "NAMESPACE", Value: vcNamespace}))
 			Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "POD_NAME", ValueFrom: &v1.EnvVarSource{FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}}}))
 			Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "VARNISH_CLUSTER_NAME", Value: vcName}))

diff --git a/pkg/varnishcontroller/config/config.go b/pkg/varnishcontroller/config/config.go
@@ -5,7 +5,6 @@ import (
 	"strconv"
 	"time"
 
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/caarlos0/env/v6"
@@ -17,21 +16,19 @@ const VCLConfigDir = "/etc/varnish"
 
 // Config that reads in env variables
 type Config struct {
-	EndpointSelectorString string        `env:"ENDPOINT_SELECTOR_STRING,required"`
-	Namespace              string        `env:"NAMESPACE,required"`
-	PodName                string        `env:"POD_NAME,required"`
-	NodeName               string        `env:"NODE_NAME,required"`
-	VarnishClusterName     string        `env:"VARNISH_CLUSTER_NAME,required"`
-	VarnishClusterUID      types.UID     `env:"VARNISH_CLUSTER_UID,required"`
-	VarnishClusterGroup    string        `env:"VARNISH_CLUSTER_GROUP,required"`
-	VarnishClusterVersion  string        `env:"VARNISH_CLUSTER_VERSION,required"`
-	VarnishClusterKind     string        `env:"VARNISH_CLUSTER_KIND,required"`
-	VarnishAdmArgs         []string      `env:"VARNISHADM_ARGS" envDefault:"-S /etc/varnish-secret/secret -T 127.0.0.1:6082" envSeparator:" " `
-	VarnishPingTimeout     time.Duration `env:"VARNISHADM_PING_TIMEOUT" envDefault:"90s"`
-	VarnishPingDelay       time.Duration `env:"VARNISHADM_PING_DELAY" envDefault:"200ms"`
-	LogFormat              string        `env:"LOG_FORMAT,required"`
-	LogLevel               zapcore.Level `env:"LOG_LEVEL,required"`
-	EndpointSelector       labels.Selector
+	Namespace             string        `env:"NAMESPACE,required"`
+	PodName               string        `env:"POD_NAME,required"`
+	NodeName              string        `env:"NODE_NAME,required"`
+	VarnishClusterName    string        `env:"VARNISH_CLUSTER_NAME,required"`
+	VarnishClusterUID     types.UID     `env:"VARNISH_CLUSTER_UID,required"`
+	VarnishClusterGroup   string        `env:"VARNISH_CLUSTER_GROUP,required"`
+	VarnishClusterVersion string        `env:"VARNISH_CLUSTER_VERSION,required"`
+	VarnishClusterKind    string        `env:"VARNISH_CLUSTER_KIND,required"`
+	VarnishAdmArgs        []string      `env:"VARNISHADM_ARGS" envDefault:"-S /etc/varnish-secret/secret -T 127.0.0.1:6082" envSeparator:" " `
+	VarnishPingTimeout    time.Duration `env:"VARNISHADM_PING_TIMEOUT" envDefault:"90s"`
+	VarnishPingDelay      time.Duration `env:"VARNISHADM_PING_DELAY" envDefault:"200ms"`
+	LogFormat             string        `env:"LOG_FORMAT,required"`
+	LogLevel              zapcore.Level `env:"LOG_LEVEL,required"`
 }
 
 // Load uses the caarlos0/env library to read in environment variables into a struct
@@ -63,10 +60,5 @@ func Load() (*Config, error) {
 		return &c, errors.WithStack(err)
 	}
 
-	c.EndpointSelector, err = labels.Parse(c.EndpointSelectorString)
-	if err != nil {
-		return &c, errors.Wrapf(err, "could not parse endpoint selector: %s", c.EndpointSelectorString)
-	}
-
 	return &c, nil
 }
diff --git a/pkg/varnishcontroller/controller/controller.go b/pkg/varnishcontroller/controller/controller.go
@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"fmt"
 	"strings"
 	"time"
 
@@ -44,13 +45,10 @@ type PodInfo struct {
 // SetupVarnishReconciler creates a new VarnishCluster Controller and adds it to the Manager with default RBAC. The Manager will set fields on the Controller
 // and Start it when the Manager is Started.
 func SetupVarnishReconciler(mgr manager.Manager, cfg *config.Config, varnish varnishadm.VarnishAdministrator, metrics *metrics.VarnishControllerMetrics, logr *logger.Logger) error {
-	backendsLabels, err := labels.ConvertSelectorToLabelsMap(cfg.EndpointSelectorString)
-	if err != nil {
-		return err
-	}
-
+	// stub, backends selector will be set and updated on reconcile
+	backendsSelector := labels.SelectorFromSet(labels.Set{})
 	backendNamespacePredicate := predicates.NewNamespacesMatcherPredicate([]string{cfg.Namespace}, logr)
-	backendLabelsPredicate := predicates.NewLabelMatcherPredicate(backendsLabels.AsSelector(), logr)
+	backendLabelsPredicate := predicates.NewLabelMatcherPredicate(backendsSelector, logr)
 
 	r := &ReconcileVarnish{
 		config:                     cfg,
@@ -90,7 +88,7 @@ func SetupVarnishReconciler(mgr manager.Manager, cfg *config.Config, varnish var
 
 	varnishPodsSelector := labels.SelectorFromSet(labels.Set{
 		v1alpha1.LabelVarnishOwner:     cfg.VarnishClusterName,
-		v1alpha1.LabelVarnishComponent: v1alpha1.VarnishComponentCacheService,
+		v1alpha1.LabelVarnishComponent: v1alpha1.VarnishComponentVarnish,
 		v1alpha1.LabelVarnishUID:       string(cfg.VarnishClusterUID),
 	})
 	builder.Watches(
@@ -202,8 +200,10 @@ func (r *ReconcileVarnish) reconcileWithContext(ctx context.Context, request rec
 
 	for fileName, contents := range templatizedFiles {
 		if _, found := newFiles[fileName]; found {
-			// TODO: probably want to create event for this
-			return reconcile.Result{}, errors.Errorf("ConfigMap has %s and %s.tmpl entries. Cannot include file and template with same name", fileName, fileName)
+			errMsg := fmt.Sprintf("VCL ConfigMap %s has %s and %s.tmpl entries. Cannot include file and template with same name",
+				*vc.Spec.VCL.ConfigMapName, fileName, fileName)
+			r.eventHandler.Warning(vc, events.EventReasonInvalidVCLConfigMap, errMsg)
+			return reconcile.Result{}, errors.Errorf(errMsg)
 		}
 		newFiles[fileName] = contents
 	}
@@ -218,7 +218,13 @@ func (r *ReconcileVarnish) reconcileWithContext(ctx context.Context, request rec
 		return reconcile.Result{}, errors.WithStack(err)
 	}
 
-	if filesTouched {
+	configName, err := r.varnish.GetActiveConfigurationName()
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	// reload if files changed, or we didn't load the VCL yet (happens when only the container restarted and not the whole pod)
+	if filesTouched || configName == "boot" {
 		if err = r.reconcileVarnish(ctx, vc, pod, cm); err != nil {
 			return reconcile.Result{}, errors.WithStack(err)
 		}

diff --git a/pkg/varnishcontroller/controller/endpoints.go b/pkg/varnishcontroller/controller/endpoints.go
@@ -2,8 +2,11 @@ package controller
 
 import (
 	"context"
+	"fmt"
 	"sort"
 
+	"github.com/ibm/varnish-operator/pkg/varnishcontroller/events"
+
 	"github.com/ibm/varnish-operator/api/v1alpha1"
 	vclabels "github.com/ibm/varnish-operator/pkg/labels"
 
@@ -36,7 +39,7 @@ func (r *ReconcileVarnish) getBackendEndpoints(ctx context.Context, vc *v1alpha1
 		namespaces = vc.Spec.Backend.Namespaces
 	}
 
-	backendList, portNumber, err := r.getPodsInfo(ctx, namespaces, labels.SelectorFromSet(vc.Spec.Backend.Selector), *vc.Spec.Backend.Port)
+	backendList, portNumber, err := r.getPodsInfo(ctx, vc, namespaces, labels.SelectorFromSet(vc.Spec.Backend.Selector), *vc.Spec.Backend.Port)
 	if err != nil {
 		return nil, 0, 0, 0, errors.WithStack(err)
 	}
@@ -102,15 +105,15 @@ func (r *ReconcileVarnish) getVarnishEndpoints(ctx context.Context, vc *v1alpha1
 	varnishLables := labels.SelectorFromSet(vclabels.CombinedComponentLabels(vc, v1alpha1.VarnishComponentVarnish))
 	varnishPort := intstr.FromString(v1alpha1.VarnishPortName)
 
-	varnishEndpoints, _, err := r.getPodsInfo(ctx, []string{r.config.Namespace}, varnishLables, varnishPort)
+	varnishEndpoints, _, err := r.getPodsInfo(ctx, vc, []string{r.config.Namespace}, varnishLables, varnishPort)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
 
 	return varnishEndpoints, nil
 }
 
-func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, namespaces []string, labels labels.Selector, validPort intstr.IntOrString) ([]PodInfo, int32, error) {
+func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, vc *v1alpha1.VarnishCluster, namespaces []string, labels labels.Selector, validPort intstr.IntOrString) ([]PodInfo, int32, error) {
 	var pods []v1.Pod
 	for _, namespace := range namespaces {
 		listOptions := []client.ListOption{
@@ -139,9 +142,11 @@ func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, namespaces []string,
 			continue
 		}
 
+		portFound := false
 		for _, container := range pod.Spec.Containers {
 			for _, containerPort := range container.Ports {
 				if containerPort.ContainerPort == validPort.IntVal || containerPort.Name == validPort.StrVal {
+					portFound = true
 					portNumber = containerPort.ContainerPort
 					var backendWeight = 1.0
 					nodeLabels, err := r.getNodeLabels(ctx, pod.Spec.NodeName)
@@ -154,6 +159,12 @@ func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, namespaces []string,
 				}
 			}
 		}
+
+		if !portFound {
+			errMsg := fmt.Sprintf("Backend pod %s/%s ignored since none of its containers have port %q defined", pod.Namespace, pod.Name, validPort.String())
+			r.eventHandler.Warning(vc, events.EventReasonBackendIgnored, errMsg)
+			r.logger.Warnf(errMsg)
+		}
 	}
 
 	// sort slices so they also look the same for the code using it

diff --git a/pkg/varnishcontroller/events/events.go b/pkg/varnishcontroller/events/events.go
@@ -11,6 +11,8 @@ const (
 
 	EventReasonReloadError         EventReason = "ReloadError"
 	EventReasonVCLCompilationError EventReason = "VCLCompilationError"
+	EventReasonInvalidVCLConfigMap EventReason = "InvalidVCLConfigMap"
+	EventReasonBackendIgnored      EventReason = "BackendIgnored"
 
 	annotationSourcePod string = "sourcePod"
 )