fix: improve config management (#6808)

* fix: improve config logs

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

* notification

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

---------

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

cmd/cleanup-controller/main.go

@@ -80,6 +80,8 @@ func main() {
 	// setup metrics
 	ctx, logger, metricsConfig, sdown := internal.Setup("kyverno-cleanup-controller")
 	defer sdown()
+	// configuration
+	configuration := config.NewDefaultConfiguration(false)
 	// create instrumented clients
 	kubeClient := internal.CreateKubernetesClient(logger, kubeclient.WithMetrics(metricsConfig, metrics.KubeClient), kubeclient.WithTracing())
 	leaderElectionClient := internal.CreateKubernetesClient(logger, kubeclient.WithMetrics(metricsConfig, metrics.KubeClient), kubeclient.WithTracing())
@@ -124,7 +126,6 @@ func main() {
 					kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations(),
 					kubeInformer.Admissionregistration().V1().ValidatingWebhookConfigurations(),
 					kubeKyvernoInformer.Core().V1().Secrets(),
-					kubeKyvernoInformer.Core().V1().ConfigMaps(),
 					config.CleanupValidatingWebhookConfigurationName,
 					config.CleanupValidatingWebhookServicePath,
 					serverIP,
@@ -145,6 +146,7 @@ func main() {
 					}},
 					genericwebhookcontroller.Fail,
 					genericwebhookcontroller.None,
+					configuration,
 				),
 				webhookWorkers,
 			)
@@ -225,7 +227,7 @@ func main() {
 			DumpPayload: dumpPayload,
 		},
 		probes{},
-		config.NewDefaultConfiguration(false),
+		configuration,
 	)
 	// start server
 	server.Run(ctx.Done())

cmd/kyverno/main.go

@@ -153,6 +153,7 @@ func createrLeaderControllers(
 	certRenewer tls.CertRenewer,
 	runtime runtimeutils.Runtime,
 	servicePort int32,
+	configuration config.Configuration,
 ) ([]internal.Controller, func(context.Context) error, error) {
 	certManager := certmanager.NewController(
 		kubeKyvernoInformer.Core().V1().Secrets(),
@@ -169,7 +170,6 @@ func createrLeaderControllers(
 		kyvernoInformer.Kyverno().V1().ClusterPolicies(),
 		kyvernoInformer.Kyverno().V1().Policies(),
 		kubeKyvernoInformer.Core().V1().Secrets(),
-		kubeKyvernoInformer.Core().V1().ConfigMaps(),
 		kubeKyvernoInformer.Coordination().V1().Leases(),
 		kubeInformer.Rbac().V1().ClusterRoles(),
 		serverIP,
@@ -178,13 +178,13 @@ func createrLeaderControllers(
 		autoUpdateWebhooks,
 		admissionReports,
 		runtime,
+		configuration,
 	)
 	exceptionWebhookController := genericwebhookcontroller.NewController(
 		exceptionWebhookControllerName,
 		kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations(),
 		kubeInformer.Admissionregistration().V1().ValidatingWebhookConfigurations(),
 		kubeKyvernoInformer.Core().V1().Secrets(),
-		kubeKyvernoInformer.Core().V1().ConfigMaps(),
 		config.ExceptionValidatingWebhookConfigurationName,
 		config.ExceptionValidatingWebhookServicePath,
 		serverIP,
@@ -202,6 +202,7 @@ func createrLeaderControllers(
 		}},
 		genericwebhookcontroller.Fail,
 		genericwebhookcontroller.None,
+		configuration,
 	)
 	return []internal.Controller{
 			internal.NewController(certmanager.ControllerName, certManager, certmanager.Workers),
@@ -450,6 +451,7 @@ func main() {
 				certRenewer,
 				runtime,
 				int32(servicePort),
+				configuration,
 			)
 			if err != nil {
 				logger.Error(err, "failed to create leader controllers")

pkg/config/config.go

@@ -2,6 +2,7 @@ package config
 
 import (
 	"context"
+	"errors"
 	"strconv"
 	"sync"
 
@@ -9,7 +10,7 @@ import (
 	osutils "github.com/kyverno/kyverno/pkg/utils/os"
 	"github.com/kyverno/kyverno/pkg/utils/wildcard"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
@@ -151,7 +152,9 @@ type Configuration interface {
 	// GetWebhookAnnotations returns annotations to set on webhook configs
 	GetWebhookAnnotations() map[string]string
 	// Load loads configuration from a configmap
-	Load(cm *corev1.ConfigMap)
+	Load(*corev1.ConfigMap)
+	// OnChanged adds a callback to be invoked when the configuration is reloaded
+	OnChanged(func())
 }
 
 // configuration stores the configuration
@@ -168,6 +171,7 @@ type configuration struct {
 	webhooks                      []WebhookConfig
 	webhookAnnotations            map[string]string
 	mux                           sync.RWMutex
+	callbacks                     []func()
 }
 
 // NewDefaultConfiguration ...
@@ -183,7 +187,7 @@ func NewDefaultConfiguration(skipResourceFilters bool) *configuration {
 func NewConfiguration(client kubernetes.Interface, skipResourceFilters bool) (Configuration, error) {
 	cd := NewDefaultConfiguration(skipResourceFilters)
 	if cm, err := client.CoreV1().ConfigMaps(kyvernoNamespace).Get(context.TODO(), kyvernoConfigMapName, metav1.GetOptions{}); err != nil {
-		if !errors.IsNotFound(err) {
+		if !apierrors.IsNotFound(err) {
 			return nil, err
 		}
 	} else {
@@ -192,6 +196,12 @@ func NewConfiguration(client kubernetes.Interface, skipResourceFilters bool) (Co
 	return cd, nil
 }
 
+func (cd *configuration) OnChanged(callback func()) {
+	cd.mux.Lock()
+	defer cd.mux.Unlock()
+	cd.callbacks = append(cd.callbacks, callback)
+}
+
 func (cd *configuration) ToFilter(gvk schema.GroupVersionKind, subresource, namespace, name string) bool {
 	cd.mux.RLock()
 	defer cd.mux.RUnlock()
@@ -282,94 +292,120 @@ func (cd *configuration) load(cm *corev1.ConfigMap) {
 	}
 	cd.mux.Lock()
 	defer cd.mux.Unlock()
+	defer cd.notify()
 	// reset
-	cd.filters = []filter{}
+	cd.defaultRegistry = "docker.io"
+	cd.enableDefaultRegistryMutation = true
 	cd.excludedUsernames = []string{}
 	cd.excludedGroups = []string{}
 	cd.excludedRoles = []string{}
 	cd.excludedClusterRoles = []string{}
+	cd.filters = []filter{}
 	cd.generateSuccessEvents = false
 	cd.webhooks = nil
+	cd.webhookAnnotations = nil
 	// load filters
 	cd.filters = parseKinds(cm.Data["resourceFilters"])
-	newDefaultRegistry, ok := cm.Data["defaultRegistry"]
+	logger.Info("filters configured", "filters", cd.filters)
+	// load defaultRegistry
+	defaultRegistry, ok := cm.Data["defaultRegistry"]
 	if !ok {
-		logger.V(6).Info("configuration: No defaultRegistry defined in ConfigMap")
+		logger.Info("defaultRegistry not set")
 	} else {
-		if valid.IsDNSName(newDefaultRegistry) {
+		logger := logger.WithValues("defaultRegistry", defaultRegistry)
-			logger.V(4).Info("Updated defaultRegistry config parameter.", "oldDefaultRegistry", cd.defaultRegistry, "newDefaultRegistry", newDefaultRegistry)
+		if valid.IsDNSName(defaultRegistry) {
-			cd.defaultRegistry = newDefaultRegistry
+			cd.defaultRegistry = defaultRegistry
+			logger.Info("defaultRegistry configured")
 		} else {
-			logger.V(4).Info("defaultRegistry didn't change because the provided config value isn't a valid DNS hostname")
+			logger.Error(errors.New("defaultRegistry is not a valid DNS hostname"), "failed to configure defaultRegistry")
 		}
 	}
+	// load enableDefaultRegistryMutation
 	enableDefaultRegistryMutation, ok := cm.Data["enableDefaultRegistryMutation"]
 	if !ok {
-		logger.V(6).Info("configuration: No enableDefaultRegistryMutation defined in ConfigMap")
+		logger.Info("enableDefaultRegistryMutation not set")
 	} else {
-		newEnableDefaultRegistryMutation, err := strconv.ParseBool(enableDefaultRegistryMutation)
+		logger := logger.WithValues("enableDefaultRegistryMutation", enableDefaultRegistryMutation)
+		enableDefaultRegistryMutation, err := strconv.ParseBool(enableDefaultRegistryMutation)
 		if err != nil {
-			logger.V(4).Info("configuration: Invalid value for enableDefaultRegistryMutation defined in ConfigMap. enableDefaultRegistryMutation didn't change")
+			logger.Error(err, "enableDefaultRegistryMutation is not a boolean")
+		} else {
+			cd.enableDefaultRegistryMutation = enableDefaultRegistryMutation
+			logger.Info("enableDefaultRegistryMutation configured")
 		}
-		logger.V(4).Info("Updated enableDefaultRegistryMutation config parameter", "oldEnableDefaultRegistryMutation", cd.enableDefaultRegistryMutation, "newEnableDefaultRegistryMutation", newEnableDefaultRegistryMutation)
-		cd.enableDefaultRegistryMutation = newEnableDefaultRegistryMutation
 	}
 	// load excludeGroupRole
 	excludedGroups, ok := cm.Data["excludeGroups"]
 	if !ok {
-		logger.V(6).Info("configuration: No excludeGroups defined in ConfigMap")
+		logger.Info("excludeGroups not set")
 	} else {
 		cd.excludedGroups = parseStrings(excludedGroups)
+		logger.Info("excludedGroups configured", "excludeGroups", cd.excludedGroups)
 	}
 	// load excludeUsername
 	excludedUsernames, ok := cm.Data["excludeUsernames"]
 	if !ok {
-		logger.V(6).Info("configuration: No excludeUsernames defined in ConfigMap")
+		logger.Info("excludeUsernames not set")
 	} else {
 		cd.excludedUsernames = parseStrings(excludedUsernames)
+		logger.Info("excludedUsernames configured", "excludeUsernames", cd.excludedUsernames)
 	}
 	// load excludeRoles
 	excludedRoles, ok := cm.Data["excludeRoles"]
 	if !ok {
-		logger.V(6).Info("configuration: No excludeRoles defined in ConfigMap")
+		logger.Info("excludeRoles not set")
 	} else {
 		cd.excludedRoles = parseStrings(excludedRoles)
+		logger.Info("excludedRoles configured", "excludeRoles", cd.excludedRoles)
 	}
 	// load excludeClusterRoles
 	excludedClusterRoles, ok := cm.Data["excludeClusterRoles"]
 	if !ok {
-		logger.V(6).Info("configuration: No excludeClusterRoles defined in ConfigMap")
+		logger.Info("excludeClusterRoles not set")
 	} else {
 		cd.excludedClusterRoles = parseStrings(excludedClusterRoles)
+		logger.Info("excludedClusterRoles configured", "excludeClusterRoles", cd.excludedClusterRoles)
 	}
 	// load generateSuccessEvents
 	generateSuccessEvents, ok := cm.Data["generateSuccessEvents"]
-	if ok {
+	if !ok {
+		logger.Info("generateSuccessEvents not set")
+	} else {
+		logger := logger.WithValues("generateSuccessEvents", generateSuccessEvents)
 		generateSuccessEvents, err := strconv.ParseBool(generateSuccessEvents)
 		if err != nil {
-			logger.Error(err, "failed to parse generateSuccessEvents")
+			logger.Error(err, "generateSuccessEvents is not a boolean")
 		} else {
 			cd.generateSuccessEvents = generateSuccessEvents
+			logger.Info("generateSuccessEvents configured")
 		}
 	}
 	// load webhooks
 	webhooks, ok := cm.Data["webhooks"]
-	if ok {
+	if !ok {
+		logger.Info("webhooks not set")
+	} else {
+		logger := logger.WithValues("webhooks", webhooks)
 		webhooks, err := parseWebhooks(webhooks)
 		if err != nil {
 			logger.Error(err, "failed to parse webhooks")
 		} else {
 			cd.webhooks = webhooks
+			logger.Info("webhooks configured")
 		}
 	}
 	// load webhook annotations
 	webhookAnnotations, ok := cm.Data["webhookAnnotations"]
-	if ok {
+	if !ok {
+		logger.Info("webhookAnnotations not set")
+	} else {
+		logger := logger.WithValues("webhookAnnotations", webhookAnnotations)
 		webhookAnnotations, err := parseWebhookAnnotations(webhookAnnotations)
 		if err != nil {
 			logger.Error(err, "failed to parse webhook annotations")
 		} else {
 			cd.webhookAnnotations = webhookAnnotations
+			logger.Info("webhookAnnotations configured")
 		}
 	}
 }
@@ -377,14 +413,21 @@ func (cd *configuration) load(cm *corev1.ConfigMap) {
 func (cd *configuration) unload() {
 	cd.mux.Lock()
 	defer cd.mux.Unlock()
-	cd.filters = []filter{}
+	defer cd.notify()
 	cd.defaultRegistry = "docker.io"
 	cd.enableDefaultRegistryMutation = true
 	cd.excludedUsernames = []string{}
 	cd.excludedGroups = []string{}
 	cd.excludedRoles = []string{}
 	cd.excludedClusterRoles = []string{}
+	cd.filters = []filter{}
 	cd.generateSuccessEvents = false
 	cd.webhooks = nil
 	cd.webhookAnnotations = nil
 }
+
+func (cd *configuration) notify() {
+	for _, callback := range cd.callbacks {
+		callback()
+	}
+}

pkg/controllers/generic/webhook/controller.go

@@ -40,9 +40,8 @@ type controller struct {
 	vwcClient controllerutils.ObjectClient[*admissionregistrationv1.ValidatingWebhookConfiguration]
 
 	// listers
-	vwcLister       admissionregistrationv1listers.ValidatingWebhookConfigurationLister
+	vwcLister    admissionregistrationv1listers.ValidatingWebhookConfigurationLister
-	secretLister    corev1listers.SecretNamespaceLister
+	secretLister corev1listers.SecretNamespaceLister
-	configMapLister corev1listers.ConfigMapLister
 
 	// queue
 	queue workqueue.RateLimitingInterface
@@ -57,6 +56,7 @@ type controller struct {
 	rules          []admissionregistrationv1.RuleWithOperations
 	failurePolicy  *admissionregistrationv1.FailurePolicyType
 	sideEffects    *admissionregistrationv1.SideEffectClass
+	configuration  config.Configuration
 }
 
 func NewController(
@@ -64,7 +64,6 @@ func NewController(
 	vwcClient controllerutils.ObjectClient[*admissionregistrationv1.ValidatingWebhookConfiguration],
 	vwcInformer admissionregistrationv1informers.ValidatingWebhookConfigurationInformer,
 	secretInformer corev1informers.SecretInformer,
-	configMapInformer corev1informers.ConfigMapInformer,
 	webhookName string,
 	path string,
 	server string,
@@ -72,23 +71,24 @@ func NewController(
 	rules []admissionregistrationv1.RuleWithOperations,
 	failurePolicy *admissionregistrationv1.FailurePolicyType,
 	sideEffects *admissionregistrationv1.SideEffectClass,
+	configuration config.Configuration,
 ) controllers.Controller {
 	queue := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), controllerName)
 	c := controller{
-		vwcClient:       vwcClient,
+		vwcClient:      vwcClient,
-		vwcLister:       vwcInformer.Lister(),
+		vwcLister:      vwcInformer.Lister(),
-		secretLister:    secretInformer.Lister().Secrets(config.KyvernoNamespace()),
+		secretLister:   secretInformer.Lister().Secrets(config.KyvernoNamespace()),
-		configMapLister: configMapInformer.Lister(),
+		queue:          queue,
-		queue:           queue,
+		controllerName: controllerName,
-		controllerName:  controllerName,
+		logger:         logging.ControllerLogger(controllerName),
-		logger:          logging.ControllerLogger(controllerName),
+		webhookName:    webhookName,
-		webhookName:     webhookName,
+		path:           path,
-		path:            path,
+		server:         server,
-		server:          server,
+		servicePort:    servicePort,
-		servicePort:     servicePort,
+		rules:          rules,
-		rules:           rules,
+		failurePolicy:  failurePolicy,
-		failurePolicy:   failurePolicy,
+		sideEffects:    sideEffects,
-		sideEffects:     sideEffects,
+		configuration:  configuration,
 	}
 	controllerutils.AddDefaultEventHandlers(c.logger, vwcInformer.Informer(), queue)
 	controllerutils.AddEventHandlersT(
@@ -109,24 +109,7 @@ func NewController(
 			}
 		},
 	)
-	controllerutils.AddEventHandlersT(
+	configuration.OnChanged(c.enqueue)
-		configMapInformer.Informer(),
-		func(obj *corev1.ConfigMap) {
-			if obj.GetNamespace() == config.KyvernoNamespace() && obj.GetName() == config.KyvernoConfigMapName() {
-				c.enqueue()
-			}
-		},
-		func(_, obj *corev1.ConfigMap) {
-			if obj.GetNamespace() == config.KyvernoNamespace() && obj.GetName() == config.KyvernoConfigMapName() {
-				c.enqueue()
-			}
-		},
-		func(obj *corev1.ConfigMap) {
-			if obj.GetNamespace() == config.KyvernoNamespace() && obj.GetName() == config.KyvernoConfigMapName() {
-				c.enqueue()
-			}
-		},
-	)
 	return &c
 }
 
@@ -139,15 +122,6 @@ func (c *controller) enqueue() {
 	c.queue.Add(c.webhookName)
 }
 
-func (c *controller) loadConfig() config.Configuration {
-	cfg := config.NewDefaultConfiguration(false)
-	cm, err := c.configMapLister.ConfigMaps(config.KyvernoNamespace()).Get(config.KyvernoConfigMapName())
-	if err == nil {
-		cfg.Load(cm)
-	}
-	return cfg
-}
-
 func (c *controller) reconcile(ctx context.Context, logger logr.Logger, key, _, _ string) error {
 	if key != c.webhookName {
 		return nil
@@ -156,7 +130,7 @@ func (c *controller) reconcile(ctx context.Context, logger logr.Logger, key, _,
 	if err != nil {
 		return err
 	}
-	desired, err := c.build(c.loadConfig(), caData)
+	desired, err := c.build(c.configuration, caData)
 	if err != nil {
 		return err
 	}

pkg/controllers/webhook/controller.go

@@ -85,7 +85,6 @@ type controller struct {
 	cpolLister        kyvernov1listers.ClusterPolicyLister
 	polLister         kyvernov1listers.PolicyLister
 	secretLister      corev1listers.SecretLister
-	configMapLister   corev1listers.ConfigMapLister
 	leaseLister       coordinationv1listers.LeaseLister
 	clusterroleLister rbacv1listers.ClusterRoleLister
 
@@ -99,6 +98,7 @@ type controller struct {
 	autoUpdateWebhooks bool
 	admissionReports   bool
 	runtime            runtimeutils.Runtime
+	configuration      config.Configuration
 
 	// state
 	lock        sync.Mutex
@@ -116,7 +116,6 @@ func NewController(
 	cpolInformer kyvernov1informers.ClusterPolicyInformer,
 	polInformer kyvernov1informers.PolicyInformer,
 	secretInformer corev1informers.SecretInformer,
-	configMapInformer corev1informers.ConfigMapInformer,
 	leaseInformer coordinationv1informers.LeaseInformer,
 	clusterroleInformer rbacv1informers.ClusterRoleInformer,
 	server string,
@@ -125,6 +124,7 @@ func NewController(
 	autoUpdateWebhooks bool,
 	admissionReports bool,
 	runtime runtimeutils.Runtime,
+	configuration config.Configuration,
 ) controllers.Controller {
 	queue := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), ControllerName)
 	c := controller{
@@ -138,7 +138,6 @@ func NewController(
 		cpolLister:         cpolInformer.Lister(),
 		polLister:          polInformer.Lister(),
 		secretLister:       secretInformer.Lister(),
-		configMapLister:    configMapInformer.Lister(),
 		leaseLister:        leaseInformer.Lister(),
 		clusterroleLister:  clusterroleInformer.Lister(),
 		queue:              queue,
@@ -148,6 +147,7 @@ func NewController(
 		autoUpdateWebhooks: autoUpdateWebhooks,
 		admissionReports:   admissionReports,
 		runtime:            runtime,
+		configuration:      configuration,
 		policyState: map[string]sets.Set[string]{
 			config.MutatingWebhookConfigurationName:   sets.New[string](),
 			config.ValidatingWebhookConfigurationName: sets.New[string](),
@@ -173,24 +173,6 @@ func NewController(
 			}
 		},
 	)
-	controllerutils.AddEventHandlersT(
-		configMapInformer.Informer(),
-		func(obj *corev1.ConfigMap) {
-			if obj.GetNamespace() == config.KyvernoNamespace() && obj.GetName() == config.KyvernoConfigMapName() {
-				c.enqueueAll()
-			}
-		},
-		func(_, obj *corev1.ConfigMap) {
-			if obj.GetNamespace() == config.KyvernoNamespace() && obj.GetName() == config.KyvernoConfigMapName() {
-				c.enqueueAll()
-			}
-		},
-		func(obj *corev1.ConfigMap) {
-			if obj.GetNamespace() == config.KyvernoNamespace() && obj.GetName() == config.KyvernoConfigMapName() {
-				c.enqueueAll()
-			}
-		},
-	)
 	controllerutils.AddEventHandlers(
 		cpolInformer.Informer(),
 		func(interface{}) { c.enqueueResourceWebhooks(0) },
@@ -203,6 +185,7 @@ func NewController(
 		func(interface{}, interface{}) { c.enqueueResourceWebhooks(0) },
 		func(interface{}) { c.enqueueResourceWebhooks(0) },
 	)
+	configuration.OnChanged(c.enqueueAll)
 	return &c
 }
 
@@ -293,15 +276,6 @@ func (c *controller) enqueueVerifyWebhook() {
 	c.queue.Add(config.VerifyMutatingWebhookConfigurationName)
 }
 
-func (c *controller) loadConfig() config.Configuration {
-	cfg := config.NewDefaultConfiguration(false)
-	cm, err := c.configMapLister.ConfigMaps(config.KyvernoNamespace()).Get(config.KyvernoConfigMapName())
-	if err == nil {
-		cfg.Load(cm)
-	}
-	return cfg
-}
-
 func (c *controller) recordPolicyState(webhookConfigurationName string, policies ...kyvernov1.PolicyInterface) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
@@ -370,7 +344,7 @@ func (c *controller) reconcileValidatingWebhookConfiguration(ctx context.Context
 	if err != nil {
 		return err
 	}
-	desired, err := build(c.loadConfig(), caData)
+	desired, err := build(c.configuration, caData)
 	if err != nil {
 		return err
 	}
@@ -400,7 +374,7 @@ func (c *controller) reconcileMutatingWebhookConfiguration(ctx context.Context,
 	if err != nil {
 		return err
 	}
-	desired, err := build(c.loadConfig(), caData)
+	desired, err := build(c.configuration, caData)
 	if err != nil {
 		return err
 	}