diff --git a/api/v1alpha1/bundle_webhook.go b/api/v1alpha1/bundle_webhook.go deleted file mode 100644 index 8697bf34..00000000 --- a/api/v1alpha1/bundle_webhook.go +++ /dev/null @@ -1,102 +0,0 @@ -/* -Copyright 2022. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package v1alpha1 - -import ( - "errors" - "fmt" - "path/filepath" - "strings" - - "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/apimachinery/pkg/runtime" - ctrl "sigs.k8s.io/controller-runtime" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook" -) - -// log is for logging in this package. -var bundlelog = logf.Log.WithName("bundle-resource") - -func (b *Bundle) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(b). - Complete() -} - -//+kubebuilder:webhook:path=/validate-core-rukpak-io-v1alpha1-bundle,mutating=false,failurePolicy=fail,sideEffects=None,groups=core.rukpak.io,resources=bundles,verbs=create;update,versions=v1alpha1,name=vbundles.core.rukpak.io,admissionReviewVersions=v1 - -var _ webhook.Validator = &Bundle{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type -func (b *Bundle) ValidateCreate() error { - bundlelog.V(1).Info("validate create", "name", b.Name) - - return checkBundleSource(b) -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (b *Bundle) ValidateUpdate(old runtime.Object) error { - bundlelog.V(1).Info("validate update", "name", b.Name) - - oldBundle := old.(*Bundle) - if err := checkImmutableSpec(oldBundle, b); err != nil { - return err - } - - return checkBundleSource(b) -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type -func (b *Bundle) ValidateDelete() error { - bundlelog.V(1).Info("validate delete", "name", b.Name) - - return nil -} - -func checkBundleSource(r *Bundle) error { - switch typ := r.Spec.Source.Type; typ { - case SourceTypeImage: - if r.Spec.Source.Image == nil { - return fmt.Errorf("bundle.spec.source.image must be set for source type \"image\"") - } - case SourceTypeGit: - if r.Spec.Source.Git == nil { - return fmt.Errorf("bundle.spec.source.git must be set for source type \"git\"") - } - if strings.HasPrefix(filepath.Clean(r.Spec.Source.Git.Directory), "../") { - return fmt.Errorf(`bundle.spec.source.git.directory begins with "../": directory must define path within the repository`) - } - case SourceTypeConfigMaps: - if len(r.Spec.Source.ConfigMaps) == 0 { - return fmt.Errorf(`bundle.spec.source.configmaps must be set for source type "configmaps"`) - } - for i, cm := range r.Spec.Source.ConfigMaps { - if strings.HasPrefix(filepath.Clean(cm.Path), ".."+string(filepath.Separator)) { - return fmt.Errorf("bundle.spec.source.configmaps[%d].path is invalid: %s is outside bundle root", i, cm.Path) - } - } - } - return nil -} - -func checkImmutableSpec(oldBundle, newBundle *Bundle) error { - if !equality.Semantic.DeepEqual(oldBundle.Spec, newBundle.Spec) { - return errors.New("bundle.spec is immutable") - } - return nil -} diff --git a/cmd/webhooks/main.go b/cmd/webhooks/main.go index 6261be7e..a3b8136d 100644 --- a/cmd/webhooks/main.go +++ b/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) } diff --git a/go.mod b/go.mod index 7034ff12..0e1eaa60 100644 --- a/go.mod +++ b/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 diff --git a/internal/source/configmaps.go b/internal/source/configmaps.go index 9b1f4d86..d6716d0c 100644 --- a/internal/source/configmaps.go +++ b/internal/source/configmaps.go @@ -7,6 +7,8 @@ import ( "testing/fstest" corev1 "k8s.io/api/core/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "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, utilerrors.NewAggregate(errs) + } resolvedSource := &rukpakv1alpha1.BundleSource{ Type: rukpakv1alpha1.SourceTypeConfigMaps, diff --git a/internal/webhook/bundle.go b/internal/webhook/bundle.go new file mode 100644 index 00000000..5c393925 --- /dev/null +++ b/internal/webhook/bundle.go @@ -0,0 +1,125 @@ +/* +Copyright 2022. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhook + +import ( + "context" + "errors" + "fmt" + "path/filepath" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" +) + +type Bundle struct { + Client client.Client + SystemNamespace string +} + +//+kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch +//+kubebuilder:webhook:path=/validate-core-rukpak-io-v1alpha1-bundle,mutating=false,failurePolicy=fail,sideEffects=None,groups=core.rukpak.io,resources=bundles,verbs=create;update,versions=v1alpha1,name=vbundles.core.rukpak.io,admissionReviewVersions=v1 + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type +func (b *Bundle) ValidateCreate(ctx context.Context, obj runtime.Object) error { + bundle := obj.(*rukpakv1alpha1.Bundle) + return b.checkBundleSource(ctx, bundle) +} + +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type +func (b *Bundle) ValidateUpdate(ctx context.Context, oldObj runtime.Object, newObj runtime.Object) error { + oldBundle := oldObj.(*rukpakv1alpha1.Bundle) + newBundle := newObj.(*rukpakv1alpha1.Bundle) + if err := checkImmutableSpec(oldBundle, newBundle); err != nil { + return err + } + + return b.checkBundleSource(ctx, newBundle) +} + +// ValidateDelete implements webhook.Validator so a webhook will be registered for the type +func (b *Bundle) ValidateDelete(_ context.Context, _ runtime.Object) error { + return nil +} + +func (b *Bundle) checkBundleSource(ctx context.Context, bundle *rukpakv1alpha1.Bundle) error { + switch typ := bundle.Spec.Source.Type; typ { + case rukpakv1alpha1.SourceTypeImage: + if bundle.Spec.Source.Image == nil { + return fmt.Errorf("bundle.spec.source.image must be set for source type \"image\"") + } + case rukpakv1alpha1.SourceTypeGit: + if bundle.Spec.Source.Git == nil { + return fmt.Errorf("bundle.spec.source.git must be set for source type \"git\"") + } + if strings.HasPrefix(filepath.Clean(bundle.Spec.Source.Git.Directory), "../") { + return fmt.Errorf(`bundle.spec.source.git.directory begins with "../": directory must define path within the repository`) + } + case rukpakv1alpha1.SourceTypeConfigMaps: + if len(bundle.Spec.Source.ConfigMaps) == 0 { + return fmt.Errorf(`bundle.spec.source.configmaps must be set for source type "configmaps"`) + } + errs := []error{} + for i, cmSource := range bundle.Spec.Source.ConfigMaps { + if strings.HasPrefix(filepath.Clean(cmSource.Path), ".."+string(filepath.Separator)) { + errs = append(errs, fmt.Errorf("bundle.spec.source.configmaps[%d].path is invalid: %q is outside bundle root", i, cmSource.Path)) + } + if err := b.verifyConfigMapImmutable(ctx, cmSource.ConfigMap.Name); err != nil { + errs = append(errs, fmt.Errorf("bundle.spec.source.configmaps[%d].configmap.name is invalid: %v", i, err)) + } + } + if len(errs) > 0 { + return utilerrors.NewAggregate(errs) + } + } + return nil +} + +func checkImmutableSpec(oldBundle, newBundle *rukpakv1alpha1.Bundle) error { + if !equality.Semantic.DeepEqual(oldBundle.Spec, newBundle.Spec) { + return errors.New("bundle.spec is immutable") + } + return nil +} + +func (b *Bundle) verifyConfigMapImmutable(ctx context.Context, configMapName string) error { + var cm corev1.ConfigMap + err := b.Client.Get(ctx, client.ObjectKey{Namespace: b.SystemNamespace, Name: configMapName}, &cm) + if err != nil { + return client.IgnoreNotFound(err) + } + if cm.Immutable == nil || !*cm.Immutable { + return fmt.Errorf("configmap %q is not immutable", configMapName) + } + return nil +} + +func (b *Bundle) SetupWebhookWithManager(mgr ctrl.Manager) error { + mgr.GetWebhookServer().Register("/validate-core-rukpak-io-v1alpha1-bundle", admission.WithCustomValidator(&rukpakv1alpha1.Bundle{}, b).WithRecoverPanic(true)) + return nil +} + +var _ webhook.CustomValidator = &Bundle{} diff --git a/internal/webhook/configmaps.go b/internal/webhook/configmaps.go index e2e411c0..24689a22 100644 --- a/internal/webhook/configmaps.go +++ b/internal/webhook/configmaps.go @@ -18,10 +18,36 @@ import ( //+kubebuilder:webhook:path=/validate-core-v1-configmap,mutating=false,failurePolicy=fail,sideEffects=None,groups="",resources=configmaps,verbs=create;delete,versions=v1,name=vconfigmaps.core.rukpak.io,admissionReviewVersions=v1 type ConfigMap struct { - cl client.Client + Client client.Client } -func (w *ConfigMap) ValidateCreate(_ context.Context, _ runtime.Object) error { +func (w *ConfigMap) ValidateCreate(ctx context.Context, obj runtime.Object) error { + // Only allow configmap to be created if either of the following is true: + // 1. The configmap is immutable. + // 2. The configmap is not referenced by a bundle. + + cm := obj.(*corev1.ConfigMap) + if cm.Immutable != nil && *cm.Immutable { + return nil + } + + bundleList := &rukpakv1alpha1.BundleList{} + if err := w.Client.List(ctx, bundleList); err != nil { + return err + } + bundleReferrers := []string{} + for _, bundle := range bundleList.Items { + if bundle.Spec.Source.Type == rukpakv1alpha1.SourceTypeConfigMaps { + for _, bundleConfigMapRef := range bundle.Spec.Source.ConfigMaps { + if bundleConfigMapRef.ConfigMap.Name == cm.Name { + bundleReferrers = append(bundleReferrers, bundle.Name) + } + } + } + } + if len(bundleReferrers) > 0 { + return fmt.Errorf("configmap %q is referenced in .spec.source.configMaps[].configMap.name by bundles %v; referenced configmaps must have .immutable == true", cm.Name, bundleReferrers) + } return nil } @@ -33,7 +59,7 @@ func (w *ConfigMap) ValidateDelete(ctx context.Context, obj runtime.Object) erro cm := obj.(*corev1.ConfigMap) bundleList := &rukpakv1alpha1.BundleList{} - if err := w.cl.List(ctx, bundleList); err != nil { + if err := w.Client.List(ctx, bundleList); err != nil { return err } for _, b := range bundleList.Items { @@ -47,7 +73,6 @@ func (w *ConfigMap) ValidateDelete(ctx context.Context, obj runtime.Object) erro } func (w *ConfigMap) SetupWebhookWithManager(mgr ctrl.Manager) error { - w.cl = mgr.GetClient() mgr.GetWebhookServer().Register("/validate-core-v1-configmap", admission.WithCustomValidator(&corev1.ConfigMap{}, w).WithRecoverPanic(true)) return nil } diff --git a/manifests/apis/webhooks/resources/cluster_role.yaml b/manifests/apis/webhooks/resources/cluster_role.yaml index 1e620ef9..d011433b 100644 --- a/manifests/apis/webhooks/resources/cluster_role.yaml +++ b/manifests/apis/webhooks/resources/cluster_role.yaml @@ -5,6 +5,13 @@ metadata: creationTimestamp: null name: webhooks-admin rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - list + - watch - apiGroups: - core.rukpak.io resources: