Skip to content
This repository was archived by the owner on Aug 12, 2024. It is now read-only.

Commit f3d8ff8

Browse files
committed
moved more configmaps source validation to webhooks, fixed bug in duplicate path detection
Signed-off-by: Joe Lanford <[email protected]>
1 parent ab7e5fe commit f3d8ff8

File tree

7 files changed

+199
-164
lines changed

7 files changed

+199
-164
lines changed

api/v1alpha1/bundle_webhook.go

Lines changed: 0 additions & 102 deletions
This file was deleted.

cmd/webhooks/main.go

Lines changed: 10 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,10 @@ import (
2121
"fmt"
2222
"os"
2323

24-
"k8s.io/apimachinery/pkg/labels"
2524
"k8s.io/apimachinery/pkg/runtime"
26-
"k8s.io/apimachinery/pkg/selection"
2725
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
2826
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
2927
ctrl "sigs.k8s.io/controller-runtime"
30-
"sigs.k8s.io/controller-runtime/pkg/cache"
3128
"sigs.k8s.io/controller-runtime/pkg/healthz"
3229
"sigs.k8s.io/controller-runtime/pkg/log/zap"
3330

@@ -50,16 +47,12 @@ func init() {
5047

5148
func main() {
5249
var metricsAddr string
53-
var enableLeaderElection bool
5450
var probeAddr string
5551
var systemNamespace string
5652
var rukpakVersion bool
5753
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
5854
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
5955
flag.StringVar(&systemNamespace, "system-namespace", util.DefaultSystemNamespace, "Configures the namespace that gets used to deploy system resources.")
60-
flag.BoolVar(&enableLeaderElection, "leader-elect", false,
61-
"Enable leader election for controller manager. "+
62-
"Enabling this will ensure there is only one active controller manager.")
6356
flag.BoolVar(&rukpakVersion, "version", false, "Displays rukpak version information")
6457
opts := zap.Options{
6558
Development: true,
@@ -76,39 +69,29 @@ func main() {
7669
setupLog.Info("starting up the rukpak webhooks", "git commit", version.String())
7770

7871
cfg := ctrl.GetConfigOrDie()
79-
dependentRequirement, err := labels.NewRequirement(util.CoreOwnerKindKey, selection.In, []string{rukpakv1alpha1.BundleKind, rukpakv1alpha1.BundleDeploymentKind})
80-
if err != nil {
81-
setupLog.Error(err, "unable to create dependent label selector for cache")
82-
os.Exit(1)
83-
}
84-
dependentSelector := labels.NewSelector().Add(*dependentRequirement)
72+
systemNs := util.PodNamespace(systemNamespace)
8573
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
8674
Scheme: scheme,
75+
Namespace: systemNs,
8776
MetricsBindAddress: metricsAddr,
8877
Port: 9443,
8978
HealthProbeBindAddress: probeAddr,
90-
LeaderElection: enableLeaderElection,
91-
LeaderElectionID: "webhooks.rukpak.io",
92-
NewCache: cache.BuilderWithOptions(cache.Options{
93-
SelectorsByObject: cache.SelectorsByObject{
94-
&rukpakv1alpha1.Bundle{}: {},
95-
&rukpakv1alpha1.BundleDeployment{}: {},
96-
},
97-
DefaultSelector: cache.ObjectSelector{
98-
Label: dependentSelector,
99-
},
100-
}),
10179
})
10280
if err != nil {
103-
setupLog.Error(err, "unable to start manager")
81+
setupLog.Error(err, "unable to create manager")
10482
os.Exit(1)
10583
}
10684

107-
if err = (&rukpakv1alpha1.Bundle{}).SetupWebhookWithManager(mgr); err != nil {
85+
if err = (&webhook.Bundle{
86+
Client: mgr.GetClient(),
87+
SystemNamespace: systemNs,
88+
}).SetupWebhookWithManager(mgr); err != nil {
10889
setupLog.Error(err, "unable to create webhook", "webhook", rukpakv1alpha1.BundleKind)
10990
os.Exit(1)
11091
}
111-
if err = (&webhook.ConfigMap{}).SetupWebhookWithManager(mgr); err != nil {
92+
if err = (&webhook.ConfigMap{
93+
Client: mgr.GetClient(),
94+
}).SetupWebhookWithManager(mgr); err != nil {
11295
setupLog.Error(err, "unable to create webhook", "webhook", "ConfigMap")
11396
os.Exit(1)
11497
}

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ require (
2424
k8s.io/apimachinery v0.26.1
2525
k8s.io/cli-runtime v0.26.1
2626
k8s.io/client-go v0.26.1
27+
k8s.io/utils v0.0.0-20221128185143-99ec85e7a448
2728
sigs.k8s.io/controller-runtime v0.14.4
2829
sigs.k8s.io/yaml v1.3.0
2930
)
@@ -161,7 +162,6 @@ require (
161162
k8s.io/klog/v2 v2.80.1 // indirect
162163
k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect
163164
k8s.io/kubectl v0.26.0 // indirect
164-
k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 // indirect
165165
oras.land/oras-go v1.2.2 // indirect
166166
sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
167167
sigs.k8s.io/kustomize/api v0.12.1 // indirect

internal/source/configmaps.go

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"testing/fstest"
88

99
corev1 "k8s.io/api/core/v1"
10+
utilerrors "k8s.io/apimachinery/pkg/util/errors"
11+
"k8s.io/apimachinery/pkg/util/sets"
1012
"sigs.k8s.io/controller-runtime/pkg/client"
1113

1214
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
@@ -28,54 +30,49 @@ func (o *ConfigMaps) Unpack(ctx context.Context, bundle *rukpakv1alpha1.Bundle)
2830
configMapSources := bundle.Spec.Source.ConfigMaps
2931

3032
bundleFS := fstest.MapFS{}
33+
seenFilepaths := map[string]sets.Set[string]{}
34+
3135
for _, cmSource := range configMapSources {
3236
cmName := cmSource.ConfigMap.Name
3337
dir := filepath.Clean(cmSource.Path)
3438

35-
// Check for paths outside the bundle root is handled in the bundle validation webhook
36-
// if strings.HasPrefix("../", dir) { ... }
39+
// Validating admission webhook handles validation for:
40+
// - paths outside the bundle root
41+
// - configmaps referenced by bundles must be immutable
3742

3843
var cm corev1.ConfigMap
3944
if err := o.Reader.Get(ctx, client.ObjectKey{Name: cmName, Namespace: o.ConfigMapNamespace}, &cm); err != nil {
4045
return nil, fmt.Errorf("get configmap %s/%s: %v", o.ConfigMapNamespace, cmName, err)
4146
}
4247

43-
// TODO: move configmaps immutability check to webhooks
44-
// This would require the webhook to lookup referenced configmaps.
45-
// We would need to implement this in two places:
46-
// 1. During bundle create:
47-
// - if referenced configmap already exists, ensure it is immutable
48-
// - if referenced configmap does not exist, allow the bundle to be created anyway
49-
// 2. During configmap create:
50-
// - if the configmap is referenced by a bundle, ensure it is immutable
51-
// - if not referenced by a bundle, allow the configmap to be created.
52-
if cm.Immutable == nil || *cm.Immutable == false {
53-
return nil, fmt.Errorf("configmap %s/%s is mutable: all bundle configmaps must be immutable", o.ConfigMapNamespace, cmName)
48+
addToBundle := func(configMapName, filename string, data []byte) {
49+
filepath := filepath.Join(dir, filename)
50+
if _, ok := seenFilepaths[filepath]; !ok {
51+
seenFilepaths[filepath] = sets.New[string]()
52+
}
53+
seenFilepaths[filepath].Insert(configMapName)
54+
bundleFS[filepath] = &fstest.MapFile{
55+
Data: data,
56+
}
5457
}
55-
56-
files := map[string][]byte{}
5758
for filename, data := range cm.Data {
58-
files[filename] = []byte(data)
59+
addToBundle(cmName, filename, []byte(data))
5960
}
6061
for filename, data := range cm.BinaryData {
61-
files[filename] = data
62+
addToBundle(cmName, filename, data)
6263
}
64+
}
6365

64-
seenFilepaths := map[string]string{}
65-
for filename, data := range files {
66-
filepath := filepath.Join(dir, filename)
67-
68-
// forbid multiple configmaps in the list from referencing the same destination file.
69-
if existingCmName, ok := seenFilepaths[filepath]; ok {
70-
return nil, fmt.Errorf("configmap %s/%s contains path %q which is already referenced by configmap %s/%s",
71-
o.ConfigMapNamespace, cmName, filepath, o.ConfigMapNamespace, existingCmName)
72-
}
73-
seenFilepaths[filepath] = cmName
74-
bundleFS[filepath] = &fstest.MapFile{
75-
Data: data,
76-
}
66+
errs := []error{}
67+
for filepath, cmNames := range seenFilepaths {
68+
if len(cmNames) > 1 {
69+
errs = append(errs, fmt.Errorf("duplicate path %q found in configmaps %v", filepath, sets.List(cmNames)))
70+
continue
7771
}
7872
}
73+
if len(errs) > 0 {
74+
return nil, utilerrors.NewAggregate(errs)
75+
}
7976

8077
resolvedSource := &rukpakv1alpha1.BundleSource{
8178
Type: rukpakv1alpha1.SourceTypeConfigMaps,

0 commit comments

Comments
 (0)