moved more configmaps source validation to webhooks, fixed bug in dup…

…licate path detection

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

cmd/webhooks/main.go

@@ -21,13 +21,10 @@ import (
 	"fmt"
 	"os"
 
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/selection"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
@@ -50,16 +47,12 @@ func init() {
 
 func main() {
 	var metricsAddr string
-	var enableLeaderElection bool
 	var probeAddr string
 	var systemNamespace string
 	var rukpakVersion bool
 	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
 	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
 	flag.StringVar(&systemNamespace, "system-namespace", util.DefaultSystemNamespace, "Configures the namespace that gets used to deploy system resources.")
-	flag.BoolVar(&enableLeaderElection, "leader-elect", false,
-		"Enable leader election for controller manager. "+
-			"Enabling this will ensure there is only one active controller manager.")
 	flag.BoolVar(&rukpakVersion, "version", false, "Displays rukpak version information")
 	opts := zap.Options{
 		Development: true,
@@ -76,39 +69,29 @@ func main() {
 	setupLog.Info("starting up the rukpak webhooks", "git commit", version.String())
 
 	cfg := ctrl.GetConfigOrDie()
-	dependentRequirement, err := labels.NewRequirement(util.CoreOwnerKindKey, selection.In, []string{rukpakv1alpha1.BundleKind, rukpakv1alpha1.BundleDeploymentKind})
-	if err != nil {
-		setupLog.Error(err, "unable to create dependent label selector for cache")
-		os.Exit(1)
-	}
-	dependentSelector := labels.NewSelector().Add(*dependentRequirement)
+	systemNs := util.PodNamespace(systemNamespace)
 	mgr, err := ctrl.NewManager(cfg, ctrl.Options{
 		Scheme:                 scheme,
+		Namespace:              systemNs,
 		MetricsBindAddress:     metricsAddr,
 		Port:                   9443,
 		HealthProbeBindAddress: probeAddr,
-		LeaderElection:         enableLeaderElection,
-		LeaderElectionID:       "webhooks.rukpak.io",
-		NewCache: cache.BuilderWithOptions(cache.Options{
-			SelectorsByObject: cache.SelectorsByObject{
-				&rukpakv1alpha1.Bundle{}:           {},
-				&rukpakv1alpha1.BundleDeployment{}: {},
-			},
-			DefaultSelector: cache.ObjectSelector{
-				Label: dependentSelector,
-			},
-		}),
 	})
 	if err != nil {
-		setupLog.Error(err, "unable to start manager")
+		setupLog.Error(err, "unable to create manager")
 		os.Exit(1)
 	}
 
-	if err = (&rukpakv1alpha1.Bundle{}).SetupWebhookWithManager(mgr); err != nil {
+	if err = (&webhook.Bundle{
+		Client:          mgr.GetClient(),
+		SystemNamespace: systemNs,
+	}).SetupWebhookWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", rukpakv1alpha1.BundleKind)
 		os.Exit(1)
 	}
-	if err = (&webhook.ConfigMap{}).SetupWebhookWithManager(mgr); err != nil {
+	if err = (&webhook.ConfigMap{
+		Client: mgr.GetClient(),
+	}).SetupWebhookWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", "ConfigMap")
 		os.Exit(1)
 	}

go.mod

@@ -24,6 +24,7 @@ require (
 	k8s.io/apimachinery v0.26.1
 	k8s.io/cli-runtime v0.26.1
 	k8s.io/client-go v0.26.1
+	k8s.io/utils v0.0.0-20221128185143-99ec85e7a448
 	sigs.k8s.io/controller-runtime v0.14.4
 	sigs.k8s.io/yaml v1.3.0
 )
@@ -161,7 +162,6 @@ require (
 	k8s.io/klog/v2 v2.80.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect
 	k8s.io/kubectl v0.26.0 // indirect
-	k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 // indirect
 	oras.land/oras-go v1.2.2 // indirect
 	sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
 	sigs.k8s.io/kustomize/api v0.12.1 // indirect

internal/source/configmaps.go

@@ -2,11 +2,13 @@ package source
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"testing/fstest"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
@@ -28,54 +30,49 @@ func (o *ConfigMaps) Unpack(ctx context.Context, bundle *rukpakv1alpha1.Bundle)
 	configMapSources := bundle.Spec.Source.ConfigMaps
 
 	bundleFS := fstest.MapFS{}
+	seenFilepaths := map[string]sets.Set[string]{}
+
 	for _, cmSource := range configMapSources {
 		cmName := cmSource.ConfigMap.Name
 		dir := filepath.Clean(cmSource.Path)
 
-		// Check for paths outside the bundle root is handled in the bundle validation webhook
-		// if strings.HasPrefix("../", dir) { ... }
+		// Validating admission webhook handles validation for:
+		//  - paths outside the bundle root
+		//  - configmaps referenced by bundles must be immutable
 
 		var cm corev1.ConfigMap
 		if err := o.Reader.Get(ctx, client.ObjectKey{Name: cmName, Namespace: o.ConfigMapNamespace}, &cm); err != nil {
 			return nil, fmt.Errorf("get configmap %s/%s: %v", o.ConfigMapNamespace, cmName, err)
 		}
 
-		// TODO: move configmaps immutability check to webhooks
-		//   This would require the webhook to lookup referenced configmaps.
-		//   We would need to implement this in two places:
-		//     1. During bundle create:
-		//         - if referenced configmap already exists, ensure it is immutable
-		//         - if referenced configmap does not exist, allow the bundle to be created anyway
-		//     2. During configmap create:
-		//         - if the configmap is referenced by a bundle, ensure it is immutable
-		//         - if not referenced by a bundle, allow the configmap to be created.
-		if cm.Immutable == nil || *cm.Immutable == false {
-			return nil, fmt.Errorf("configmap %s/%s is mutable: all bundle configmaps must be immutable", o.ConfigMapNamespace, cmName)
+		addToBundle := func(configMapName, filename string, data []byte) {
+			filepath := filepath.Join(dir, filename)
+			if _, ok := seenFilepaths[filepath]; !ok {
+				seenFilepaths[filepath] = sets.New[string]()
+			}
+			seenFilepaths[filepath].Insert(configMapName)
+			bundleFS[filepath] = &fstest.MapFile{
+				Data: data,
+			}
 		}
-
-		files := map[string][]byte{}
 		for filename, data := range cm.Data {
-			files[filename] = []byte(data)
+			addToBundle(cmName, filename, []byte(data))
 		}
 		for filename, data := range cm.BinaryData {
-			files[filename] = data
+			addToBundle(cmName, filename, data)
 		}
+	}
 
-		seenFilepaths := map[string]string{}
-		for filename, data := range files {
-			filepath := filepath.Join(dir, filename)
-
-			// forbid multiple configmaps in the list from referencing the same destination file.
-			if existingCmName, ok := seenFilepaths[filepath]; ok {
-				return nil, fmt.Errorf("configmap %s/%s contains path %q which is already referenced by configmap %s/%s",
-					o.ConfigMapNamespace, cmName, filepath, o.ConfigMapNamespace, existingCmName)
-			}
-			seenFilepaths[filepath] = cmName
-			bundleFS[filepath] = &fstest.MapFile{
-				Data: data,
-			}
+	errs := []error{}
+	for filepath, cmNames := range seenFilepaths {
+		if len(cmNames) > 1 {
+			errs = append(errs, fmt.Errorf("duplicate path %q found in configmaps %v", filepath, sets.List(cmNames)))
+			continue
 		}
 	}
+	if len(errs) > 0 {
+		return nil, errors.Join(errs...)
+	}
 
 	resolvedSource := &rukpakv1alpha1.BundleSource{
 		Type:       rukpakv1alpha1.SourceTypeConfigMaps,