From 87804b4656399ea1dbce37470a84102ac2544c86 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 3 May 2022 18:41:26 +0100 Subject: [PATCH 01/13] Adds NamespaceSelector to Bundle Target API Signed-off-by: joshvanl --- pkg/apis/trust/v1alpha1/types_bundle.go | 10 +++++++ .../trust/v1alpha1/zz_generated.deepcopy.go | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/pkg/apis/trust/v1alpha1/types_bundle.go b/pkg/apis/trust/v1alpha1/types_bundle.go index a87166d3..fa601e45 100644 --- a/pkg/apis/trust/v1alpha1/types_bundle.go +++ b/pkg/apis/trust/v1alpha1/types_bundle.go @@ -84,6 +84,16 @@ type BundleTarget struct { // ConfigMap is the target ConfigMap in all Namespaces that all Bundle source // data will be synced to. ConfigMap *KeySelector `json:"configMap,omitempty"` + + // NamespaceSelector will, if set, only sync the target resource in + // Namespaces which match the selector. + NamespaceSelector *NamespaceSelector `json:"namespaceSelector,omitempty"` +} + +// NamespaceSelector defines selectors to match on Namespaces. +type NamespaceSelector struct { + // LabelSelector defines a selector which matches on the Namespace labels. + *metav1.LabelSelector `json:",inline,omitempty"` } // SourceObjectKeySelector is a reference to a source object and its `data` key diff --git a/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go index 747b00ba..67013e5e 100644 --- a/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go @@ -1,3 +1,4 @@ +//go:build !ignore_autogenerated // +build !ignore_autogenerated /* @@ -21,6 +22,7 @@ limitations under the License. package v1alpha1 import ( + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -196,6 +198,11 @@ func (in *BundleTarget) DeepCopyInto(out *BundleTarget) { *out = new(KeySelector) **out = **in } + if in.NamespaceSelector != nil { + in, out := &in.NamespaceSelector, &out.NamespaceSelector + *out = new(NamespaceSelector) + (*in).DeepCopyInto(*out) + } return } @@ -225,6 +232,27 @@ func (in *KeySelector) DeepCopy() *KeySelector { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *NamespaceSelector) DeepCopyInto(out *NamespaceSelector) { + *out = *in + if in.LabelSelector != nil { + in, out := &in.LabelSelector, &out.LabelSelector + *out = new(v1.LabelSelector) + (*in).DeepCopyInto(*out) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NamespaceSelector. +func (in *NamespaceSelector) DeepCopy() *NamespaceSelector { + if in == nil { + return nil + } + out := new(NamespaceSelector) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SourceObjectKeySelector) DeepCopyInto(out *SourceObjectKeySelector) { *out = *in From 2da265b49d157bf4badebfec319af306cf415fe8 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 3 May 2022 18:42:12 +0100 Subject: [PATCH 02/13] Adds namespaceSelector functionality to to bundle controller Signed-off-by: joshvanl --- pkg/bundle/bundle.go | 28 ++++- pkg/bundle/bundle_test.go | 133 ++++++++++++++++++--- pkg/bundle/sync.go | 36 +++++- pkg/bundle/sync_test.go | 240 +++++++++++++++++++++++++++++++------- pkg/bundle/test/suite.go | 55 +++++++-- 5 files changed, 411 insertions(+), 81 deletions(-) diff --git a/pkg/bundle/bundle.go b/pkg/bundle/bundle.go index 73107522..e9d4e7c8 100644 --- a/pkg/bundle/bundle.go +++ b/pkg/bundle/bundle.go @@ -26,6 +26,7 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/tools/record" "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" @@ -85,6 +86,18 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, return ctrl.Result{}, fmt.Errorf("failed to get %q: %s", req.NamespacedName, err) } + var namespaceSelector labels.Selector + if nsSelector := bundle.Spec.Target.NamespaceSelector; nsSelector != nil && nsSelector.LabelSelector != nil { + var err error + namespaceSelector, err = metav1.LabelSelectorAsSelector(nsSelector.LabelSelector) + if err != nil { + b.recorder.Eventf(&bundle, corev1.EventTypeWarning, "NamespaceSelectorError", "Failed to build namespace selector: %s", err) + return ctrl.Result{}, fmt.Errorf("failed to build NamespaceSelector: %w", err) + } + } else { + namespaceSelector = labels.Everything() + } + var namespaceList corev1.NamespaceList if err := b.lister.List(ctx, &namespaceList); err != nil { log.Error(err, "failed to list namespaces") @@ -166,7 +179,7 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, continue } - synced, err := b.syncTarget(ctx, log, &bundle, namespace.Name, data) + synced, err := b.syncTarget(ctx, log, &bundle, namespaceSelector, &namespace, data) if err != nil { log.Error(err, "failed sync bundle to target namespace") b.recorder.Eventf(&bundle, corev1.EventTypeWarning, "SyncTargetFailed", "Failed to sync target in Namespace %q: %s", namespace.Name, err) @@ -192,11 +205,20 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, needsUpdate = true } + var message string + + if nsSelector := bundle.Spec.Target.NamespaceSelector; nsSelector != nil && nsSelector.LabelSelector != nil { + message = fmt.Sprintf("Successfully synced Bundle to namespaces with selector [matchLabels:%v matchExpressions: %v]", + nsSelector.MatchLabels, nsSelector.MatchExpressions) + } else { + message = "Successfully synced Bundle to all namespaces" + } + syncedCondition := trustapi.BundleCondition{ Type: trustapi.BundleConditionSynced, Status: corev1.ConditionTrue, Reason: "Synced", - Message: "Successfully synced Bundle to all namespaces", + Message: message, } if !needsUpdate && bundleHasCondition(&bundle, syncedCondition) { @@ -206,6 +228,6 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, log.V(2).Info("successfully synced bundle") b.setBundleCondition(&bundle, syncedCondition) - b.recorder.Eventf(&bundle, corev1.EventTypeNormal, "Synced", "Successfully synced Bundle to all namespaces") + b.recorder.Eventf(&bundle, corev1.EventTypeNormal, "Synced", message) return ctrl.Result{}, b.client.Status().Update(ctx, &bundle) } diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index 9d260be3..27d3d946 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -302,18 +303,93 @@ func Test_Reconcile(t *testing.T) { gen.SetBundleResourceVersion("1001"), gen.SetBundleStatus(trustapi.BundleStatus{ Target: &trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: targetKey}}, - Conditions: []trustapi.BundleCondition{ - { - Type: trustapi.BundleConditionSynced, - Status: corev1.ConditionTrue, - LastTransitionTime: fixedmetatime, - Reason: "Synced", - Message: "Successfully synced Bundle to all namespaces", - ObservedGeneration: bundleGeneration, + Conditions: []trustapi.BundleCondition{{ + Type: trustapi.BundleConditionSynced, + Status: corev1.ConditionTrue, + LastTransitionTime: fixedmetatime, + Reason: "Synced", + Message: "Successfully synced Bundle to all namespaces", + ObservedGeneration: bundleGeneration, + }}, + }), + ), + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: trustNamespace, Name: baseBundle.Name, OwnerReferences: baseBundleOwnerRef, ResourceVersion: "1"}, + Data: map[string]string{targetKey: "A\nB\nC\n"}, + }, + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns-1", Name: baseBundle.Name, OwnerReferences: baseBundleOwnerRef, ResourceVersion: "1"}, + Data: map[string]string{targetKey: "A\nB\nC\n"}, + }, + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns-2", Name: baseBundle.Name, OwnerReferences: baseBundleOwnerRef, ResourceVersion: "1"}, + Data: map[string]string{targetKey: "A\nB\nC\n"}, + }, + ), + expEvent: "Normal Synced Successfully synced Bundle to all namespaces", + }, + "if Bundle not synced everywhere, sync except Namespaces that don't match labels and update Synced": { + existingObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, + gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + ), + &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "random-namespace", + Labels: map[string]string{"foo": "bar"}, + }, + }, + &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "another-random-namespace", + Labels: map[string]string{"foo": "bar"}, + }, + }, + ), + expResult: ctrl.Result{}, + expError: false, + expObjects: append(namespaces, sourceConfigMap, sourceSecret, + gen.BundleFrom(baseBundle, + gen.SetBundleResourceVersion("1001"), + gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + gen.SetBundleStatus(trustapi.BundleStatus{ + Target: &trustapi.BundleTarget{ + ConfigMap: &trustapi.KeySelector{Key: targetKey}, + NamespaceSelector: &trustapi.NamespaceSelector{ + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, }, }, + Conditions: []trustapi.BundleCondition{{ + Type: trustapi.BundleConditionSynced, + Status: corev1.ConditionTrue, + LastTransitionTime: &metav1.Time{Time: fixedclock.Now().Local()}, + Reason: "Synced", + Message: "Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + ObservedGeneration: bundleGeneration, + }}, }), ), + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "random-namespace", Name: baseBundle.Name, OwnerReferences: baseBundleOwnerRef, ResourceVersion: "1"}, + Data: map[string]string{targetKey: "A\nB\nC\n"}, + }, + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "another-random-namespace", Name: baseBundle.Name, OwnerReferences: baseBundleOwnerRef, ResourceVersion: "1"}, + Data: map[string]string{targetKey: "A\nB\nC\n"}, + }, + ), + expEvent: "Normal Synced Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + }, + "if Bundle not synced everywhere, sync except Namespaces that don't match labels and update Synced. Should delete ConfigMaps in wrong namespaces.": { + existingObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, + gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + ), &corev1.ConfigMap{ TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, ObjectMeta: metav1.ObjectMeta{Namespace: trustNamespace, Name: baseBundle.Name, OwnerReferences: baseBundleOwnerRef, ResourceVersion: "1"}, @@ -330,13 +406,39 @@ func Test_Reconcile(t *testing.T) { Data: map[string]string{targetKey: "A\nB\nC\n"}, }, ), - expEvent: "Normal Synced Successfully synced Bundle to all namespaces", + expResult: ctrl.Result{}, + expError: false, + expObjects: append(namespaces, sourceConfigMap, sourceSecret, + gen.BundleFrom(baseBundle, + gen.SetBundleResourceVersion("1001"), + gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + gen.SetBundleStatus(trustapi.BundleStatus{ + Target: &trustapi.BundleTarget{ + ConfigMap: &trustapi.KeySelector{Key: targetKey}, + NamespaceSelector: &trustapi.NamespaceSelector{ + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + }, + }, + Conditions: []trustapi.BundleCondition{{ + Type: trustapi.BundleConditionSynced, + Status: corev1.ConditionTrue, + LastTransitionTime: &metav1.Time{Time: fixedclock.Now().Local()}, + Reason: "Synced", + Message: "Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + ObservedGeneration: bundleGeneration, + }}, + }), + ), + ), + expEvent: "Normal Synced Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", }, "if Bundle synced but doesn't have owner reference, should sync and update": { existingObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, gen.SetBundleStatus(trustapi.BundleStatus{ - Target: &trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: targetKey}}, + Target: &trustapi.BundleTarget{ + ConfigMap: &trustapi.KeySelector{Key: targetKey}, + }, Conditions: []trustapi.BundleCondition{ { Type: trustapi.BundleConditionSynced, @@ -557,9 +659,7 @@ func Test_Reconcile(t *testing.T) { case event = <-fakerecorder.Events: default: } - if event != test.expEvent { - t.Errorf("unexpected event, exp=%q got=%q", test.expEvent, event) - } + assert.Equal(t, test.expEvent, event) for _, expectedObject := range test.expObjects { expObj := expectedObject.(client.Object) @@ -578,10 +678,9 @@ func Test_Reconcile(t *testing.T) { } err := fakeclient.Get(context.TODO(), client.ObjectKeyFromObject(expObj), actual) - if err != nil { - t.Errorf("unexpected error getting expected object: %s", err) - } else if !apiequality.Semantic.DeepEqual(expObj, actual) { - t.Errorf("unexpected expected object, exp=%#+v got=%#+v", expObj, actual) + assert.NoError(t, err) + if !apiequality.Semantic.DeepEqual(expObj, actual) { + t.Errorf("unexpected expected object\nexp=%#+v\ngot=%#+v", expObj, actual) } } }) diff --git a/pkg/bundle/sync.go b/pkg/bundle/sync.go index 5eedd83e..8667abe9 100644 --- a/pkg/bundle/sync.go +++ b/pkg/bundle/sync.go @@ -26,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" trustapi "github.com/cert-manager/trust/pkg/apis/trust/v1alpha1" @@ -118,22 +119,36 @@ func (b *bundle) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKey // Ensures the ConfigMap is owned by the given Bundle, and the data is up to // date. // Returns true if the ConfigMap has been created or was updated. -func (b *bundle) syncTarget(ctx context.Context, log logr.Logger, bundle *trustapi.Bundle, namespace, data string) (bool, error) { +func (b *bundle) syncTarget(ctx context.Context, log logr.Logger, + bundle *trustapi.Bundle, + namespaceSelector labels.Selector, + namespace *corev1.Namespace, + data string, +) (bool, error) { target := bundle.Spec.Target if target.ConfigMap == nil { return false, errors.New("target not defined") } + matchNamespace := namespaceSelector.Matches(labels.Set(namespace.Labels)) + var configMap corev1.ConfigMap - err := b.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: bundle.Name}, &configMap) + err := b.client.Get(ctx, client.ObjectKey{Namespace: namespace.Name, Name: bundle.Name}, &configMap) - // If the ConfigMap doesn't exist yet, create it + // If the ConfigMap doesn't exist yet, create it. if apierrors.IsNotFound(err) { + // If the namespace doesn't match selector we do nothing since we don't + // want to create it, and it also doesn't exist. + if !matchNamespace { + log.V(4).Info("ignoring namespace as it doesn't match selector", "labels", namespace.Labels) + return false, nil + } + configMap = corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: bundle.Name, - Namespace: namespace, + Namespace: namespace.Name, OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(bundle, trustapi.SchemeGroupVersion.WithKind("Bundle"))}, }, Data: map[string]string{ @@ -148,8 +163,19 @@ func (b *bundle) syncTarget(ctx context.Context, log logr.Logger, bundle *trusta return false, fmt.Errorf("failed to get configmap %s/%s: %w", namespace, bundle.Name, err) } - var needsUpdate bool + // Here, the config map exists, but the selector doesn't match the namespace. + if !matchNamespace { + // The ConfigMap is owned by this controller- delete it. + if metav1.IsControlledBy(&configMap, bundle) { + log.V(2).Info("deleting bundle from Namespace since namespaceSelector does not match") + return true, b.client.Delete(ctx, &configMap) + } + // The ConfigMap isn't owned by us, so we shouldn't delete it. Return that + // we did nothing. + return false, nil + } + var needsUpdate bool // If ConfigMap is missing OwnerReference, add it back. if !metav1.IsControlledBy(&configMap, bundle) { configMap.OwnerReferences = append(configMap.OwnerReferences, *metav1.NewControllerRef(bundle, trustapi.SchemeGroupVersion.WithKind("Bundle"))) diff --git a/pkg/bundle/sync_test.go b/pkg/bundle/sync_test.go index 4cc394d3..3c403c96 100644 --- a/pkg/bundle/sync_test.go +++ b/pkg/bundle/sync_test.go @@ -21,10 +21,13 @@ import ( "errors" "testing" + "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" - apiequality "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/selection" "k8s.io/klog/v2/klogr" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,35 +39,56 @@ import ( func Test_syncTarget(t *testing.T) { const ( bundleName = "test-bundle" - namespace = "test-namespace" key = "key" data = "data" ) + labelEverything := func(*testing.T) labels.Selector { + return labels.Everything() + } + tests := map[string]struct { - object runtime.Object - expNeedsUpdate bool + object runtime.Object + namespace corev1.Namespace + selector func(t *testing.T) labels.Selector + // Expect the configmap to exist at the end of the sync. + expExists bool + // Expect the owner reference of the configmap to point to the bundle. + expOwnerReference bool + expNeedsUpdate bool }{ "if object doesn't exist, expect update": { - object: nil, - expNeedsUpdate: true, + object: nil, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, }, "if object exists but without data or owner, expect update": { - object: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: bundleName, Namespace: namespace}}, - expNeedsUpdate: true, + object: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: bundleName, Namespace: "test-namespace"}}, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, }, "if object exists with data but no owner, expect update": { object: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: bundleName, Namespace: namespace}, + ObjectMeta: metav1.ObjectMeta{Name: bundleName, Namespace: "test-namespace"}, Data: map[string]string{key: data}, }, - expNeedsUpdate: true, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, }, "if object exists with owner but no data, expect update": { object: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: bundleName, - Namespace: namespace, + Namespace: "test-namespace", OwnerReferences: []metav1.OwnerReference{ { Kind: "Bundle", @@ -76,13 +100,17 @@ func Test_syncTarget(t *testing.T) { }, }, }, - expNeedsUpdate: true, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, }, "if object exists with owner but wrong data, expect update": { object: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: bundleName, - Namespace: namespace, + Namespace: "test-namespace", OwnerReferences: []metav1.OwnerReference{ { Kind: "Bundle", @@ -95,13 +123,17 @@ func Test_syncTarget(t *testing.T) { }, Data: map[string]string{key: "wrong data"}, }, - expNeedsUpdate: true, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, }, "if object exists with owner but wrong key, expect update": { object: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: bundleName, - Namespace: namespace, + Namespace: "test-namespace", OwnerReferences: []metav1.OwnerReference{ { Kind: "Bundle", @@ -114,13 +146,17 @@ func Test_syncTarget(t *testing.T) { }, Data: map[string]string{"wrong key": data}, }, - expNeedsUpdate: true, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, }, "if object exists with correct data, expect no update": { object: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: bundleName, - Namespace: namespace, + Namespace: "test-namespace", OwnerReferences: []metav1.OwnerReference{ { Kind: "Bundle", @@ -133,13 +169,17 @@ func Test_syncTarget(t *testing.T) { }, Data: map[string]string{key: data}, }, - expNeedsUpdate: false, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: false, }, "if object exists with correct data and some extra data and owner, expect no update": { object: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: bundleName, - Namespace: namespace, + Namespace: "test-namespace", OwnerReferences: []metav1.OwnerReference{ { Kind: "Bundle", @@ -159,7 +199,122 @@ func Test_syncTarget(t *testing.T) { }, Data: map[string]string{key: data, "another-key": "another-data"}, }, - expNeedsUpdate: false, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, + selector: labelEverything, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: false, + }, + "if object doesn't exist and labels match, expect update": { + object: nil, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + Labels: map[string]string{"foo": "bar"}, + }}, + selector: func(t *testing.T) labels.Selector { + req, err := labels.NewRequirement("foo", selection.Equals, []string{"bar"}) + assert.NoError(t, err) + return labels.NewSelector().Add(*req) + }, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: true, + }, + "if object doesn't exist and labels don't match, don't expect update": { + object: nil, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + Labels: map[string]string{"bar": "foo"}, + }}, + selector: func(t *testing.T) labels.Selector { + req, err := labels.NewRequirement("foo", selection.Equals, []string{"bar"}) + assert.NoError(t, err) + return labels.NewSelector().Add(*req) + }, + expExists: false, + expOwnerReference: true, + expNeedsUpdate: false, + }, + "if object exists with correct data and labels match, expect no update": { + object: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: bundleName, + Namespace: "test-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Bundle", + APIVersion: "trust.cert-manager.io/v1alpha1", + Name: bundleName, + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), + }, + }, + }, + Data: map[string]string{key: data}, + }, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + Labels: map[string]string{"foo": "bar"}, + }}, + selector: func(t *testing.T) labels.Selector { + req, err := labels.NewRequirement("foo", selection.Equals, []string{"bar"}) + assert.NoError(t, err) + return labels.NewSelector().Add(*req) + }, + expExists: true, + expOwnerReference: true, + expNeedsUpdate: false, + }, + "if object exists with correct data but labels don't match, expect deletion": { + object: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: bundleName, + Namespace: "test-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Bundle", + APIVersion: "trust.cert-manager.io/v1alpha1", + Name: bundleName, + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), + }, + }, + }, + Data: map[string]string{key: data}, + }, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + Labels: map[string]string{"bar": "foo"}, + }}, + selector: func(t *testing.T) labels.Selector { + req, err := labels.NewRequirement("foo", selection.Equals, []string{"bar"}) + assert.NoError(t, err) + return labels.NewSelector().Add(*req) + }, + expExists: false, + expOwnerReference: false, + expNeedsUpdate: true, + }, + "if object exists and labels don't match, but controller doesn't have ownership, expect no update": { + object: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: bundleName, + Namespace: "test-namespace", + }, + Data: map[string]string{key: data}, + }, + namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + Labels: map[string]string{"bar": "foo"}, + }}, + selector: func(t *testing.T) labels.Selector { + req, err := labels.NewRequirement("foo", selection.Equals, []string{"bar"}) + assert.NoError(t, err) + return labels.NewSelector().Add(*req) + }, + expExists: true, + expOwnerReference: false, + expNeedsUpdate: false, }, } @@ -177,35 +332,30 @@ func Test_syncTarget(t *testing.T) { needsUpdate, err := b.syncTarget(context.TODO(), klogr.New(), &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: bundleName}, Spec: trustapi.BundleSpec{Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: key}}}, - }, namespace, data) - if err != nil { - t.Errorf("unexpected error: %s", err) - } - if needsUpdate != test.expNeedsUpdate { - t.Errorf("unexpected needsUpdate, exp=%t got=%t", test.expNeedsUpdate, needsUpdate) - } + }, test.selector(t), &test.namespace, data) + assert.NoError(t, err) + + assert.Equalf(t, test.expNeedsUpdate, needsUpdate, "unexpected needsUpdate, exp=%t got=%t", test.expNeedsUpdate, needsUpdate) var configMap corev1.ConfigMap - if err := fakeclient.Get(context.TODO(), client.ObjectKey{Namespace: namespace, Name: bundleName}, &configMap); err != nil { - t.Errorf("unexpected error: %s", err) - } + err = fakeclient.Get(context.TODO(), client.ObjectKey{Namespace: test.namespace.Name, Name: bundleName}, &configMap) + assert.Equalf(t, test.expExists, !apierrors.IsNotFound(err), "unexpected is not found: %v", err) - if configMap.Data[key] != data { - t.Errorf("unexpected data on ConfigMap: exp=%s:%s got=%v", key, data, configMap.Data) - } - if configMap.Data[key] != data { - t.Errorf("unexpected data on ConfigMap: exp=%s:%s got=%v", key, data, configMap.Data) - } + if test.expExists { + assert.Equalf(t, data, configMap.Data[key], "unexpected data on ConfigMap: exp=%s:%s got=%v", key, data, configMap.Data) - exptedOwnerReference := metav1.OwnerReference{ - Kind: "Bundle", - APIVersion: "trust.cert-manager.io/v1alpha1", - Name: bundleName, - Controller: pointer.Bool(true), - BlockOwnerDeletion: pointer.Bool(true), - } - if !apiequality.Semantic.DeepEqual(configMap.OwnerReferences[0], exptedOwnerReference) { - t.Errorf("unexpected owner reference: exp=%v got=%v", exptedOwnerReference, configMap.OwnerReferences) + expectedOwnerReference := metav1.OwnerReference{ + Kind: "Bundle", + APIVersion: "trust.cert-manager.io/v1alpha1", + Name: bundleName, + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), + } + if test.expOwnerReference { + assert.Equalf(t, expectedOwnerReference, configMap.OwnerReferences[0], "unexpected data on ConfigMap: exp=%s:%s got=%v", key, data, configMap.Data) + } else { + assert.NotContains(t, configMap.OwnerReferences, expectedOwnerReference) + } } }) } diff --git a/pkg/bundle/test/suite.go b/pkg/bundle/test/suite.go index 1163c525..a03caf78 100644 --- a/pkg/bundle/test/suite.go +++ b/pkg/bundle/test/suite.go @@ -24,6 +24,7 @@ import ( . "github.com/onsi/gomega/gstruct" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" @@ -101,7 +102,7 @@ var _ = Describe("Integration", func() { By("Creating Bundle for test") testData = testenv.DefaultTrustData() testBundle = testenv.NewTestBundle(ctx, cl, opts, testData) - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) Expect(cl.Get(ctx, client.ObjectKeyFromObject(testBundle), testBundle)).ToNot(HaveOccurred()) }) @@ -134,7 +135,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has synced the new 'D' value", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nC\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nC\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -156,7 +157,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has synced the new 'D' value", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nC\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nC\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -166,7 +167,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has synced the new 'D' value", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nC\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nC\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -175,7 +176,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has removed the old 'A' value", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "B\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "B\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -184,7 +185,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has removed the old 'B' value", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -193,7 +194,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has removed the old 'C' value", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -204,7 +205,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, &configMap)).NotTo(HaveOccurred()) Context("should observe Bundle has changed the value 'A' -> 'D'", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "D\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "D\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -215,7 +216,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, &secret)).NotTo(HaveOccurred()) Context("should observe Bundle has changed the value 'B' -> 'D'", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nD\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nD\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -225,7 +226,7 @@ var _ = Describe("Integration", func() { Expect(cl.Update(ctx, testBundle)).ToNot(HaveOccurred()) Context("should observe Bundle has changed the value 'C' -> 'D'", func() { - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nD\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) @@ -234,7 +235,7 @@ var _ = Describe("Integration", func() { ConfigMap: &trustapi.KeySelector{Key: "changed-target-key"}, } Expect(cl.Update(ctx, testBundle)).ToNot(HaveOccurred()) - Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return testenv.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) Context("should observe that all targets have changed the key written", func() { var namespaceList corev1.NamespaceList @@ -300,4 +301,36 @@ var _ = Describe("Integration", func() { }, eventuallyTimeout, "100ms").Should(BeTrue()) }) }) + + It("should only write to Namespaces where the namespace selector matches", func() { + testNamespace := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "trust-test-smoke-random-namespace", + }, + } + Expect(cl.Create(ctx, &testNamespace)).NotTo(HaveOccurred()) + Context("should observe ConfigMap written to new Namespace", func() { + Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, testNamespace.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + }) + + testBundle.Spec.Target.NamespaceSelector = &trustapi.NamespaceSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar"}, + }, + } + Expect(cl.Update(ctx, testBundle)).ToNot(HaveOccurred()) + Context("should observe ConfigMap deleted from Namespace", func() { + Eventually(func() bool { + var cm corev1.ConfigMap + return apierrors.IsNotFound(cl.Get(ctx, client.ObjectKey{Namespace: "trust-test-smoke-random-namespace", Name: testBundle.Name}, &cm)) + }, eventuallyTimeout, "100ms").Should(BeTrue()) + }) + + Expect(cl.Get(ctx, client.ObjectKeyFromObject(&testNamespace), &testNamespace)).ToNot(HaveOccurred()) + testNamespace.Labels["foo"] = "bar" + Expect(cl.Update(ctx, &testNamespace)).ToNot(HaveOccurred()) + Context("should observe ConfigMap written to Namespace with matching Labels", func() { + Eventually(func() bool { return testenv.BundleHasSynced(ctx, cl, testBundle.Name, testNamespace.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + }) + }) }) From ee9cea0571680377614d2db46222cd9ced30d7c7 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 3 May 2022 18:42:30 +0100 Subject: [PATCH 03/13] Adds webhook validation to namespaceSelector Signed-off-by: joshvanl --- pkg/webhook/validation.go | 6 +++++ pkg/webhook/validation_test.go | 41 +++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/pkg/webhook/validation.go b/pkg/webhook/validation.go index 432a095a..39b01c81 100644 --- a/pkg/webhook/validation.go +++ b/pkg/webhook/validation.go @@ -174,6 +174,12 @@ func (v *validator) validateBundle(ctx context.Context, bundle *trustapi.Bundle) } } + if namespaceSelector := bundle.Spec.Target.NamespaceSelector; namespaceSelector != nil && namespaceSelector.LabelSelector != nil { + if _, err := metav1.LabelSelectorAsSelector(bundle.Spec.Target.NamespaceSelector.LabelSelector); err != nil { + el = append(el, field.Invalid(path.Child("target", "namespaceSelector"), namespaceSelector.LabelSelector, err.Error())) + } + } + path = field.NewPath("status") conditionTypes := make(map[trustapi.BundleConditionType]struct{}) diff --git a/pkg/webhook/validation_test.go b/pkg/webhook/validation_test.go index afbf7066..359b7bef 100644 --- a/pkg/webhook/validation_test.go +++ b/pkg/webhook/validation_test.go @@ -190,6 +190,9 @@ func Test_Handle(t *testing.T) { "target": { "configMap": { "key": "bar" + }, + "namespaceSelector": { + "foo": "bar" } } } @@ -388,6 +391,35 @@ func Test_validateBundle(t *testing.T) { field.Invalid(field.NewPath("status", "conditions", "[1]"), trustapi.BundleCondition{Type: "A", Reason: "C"}, "condition type already present on Bundle"), }, }, + "invalid namespace selector": { + bundle: &trustapi.Bundle{ + ObjectMeta: metav1.ObjectMeta{Name: "test-bundle-1"}, + Spec: trustapi.BundleSpec{ + Sources: []trustapi.BundleSource{ + {InLine: pointer.String("test-1")}, + }, + Target: trustapi.BundleTarget{ + ConfigMap: &trustapi.KeySelector{Key: "test-1"}, + NamespaceSelector: &trustapi.NamespaceSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"@@@@": ""}, + }, + }, + }, + }, + Status: trustapi.BundleStatus{ + Conditions: []trustapi.BundleCondition{ + { + Type: "A", + Reason: "C", + }, + }, + }, + }, + expEl: field.ErrorList{ + field.Invalid(field.NewPath("spec", "target", "namespaceSelector"), &metav1.LabelSelector{MatchLabels: map[string]string{"@@@@": ""}}, `key: Invalid value: "@@@@": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`), + }, + }, "valid bundle": { bundle: &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: "test-bundle-1"}, @@ -395,7 +427,14 @@ func Test_validateBundle(t *testing.T) { Sources: []trustapi.BundleSource{ {InLine: pointer.String("test-1")}, }, - Target: trustapi.BundleTarget{ConfigMap: &trustapi.KeySelector{Key: "test-1"}}, + Target: trustapi.BundleTarget{ + ConfigMap: &trustapi.KeySelector{Key: "test-1"}, + NamespaceSelector: &trustapi.NamespaceSelector{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar"}, + }, + }, + }, }, Status: trustapi.BundleStatus{ Conditions: []trustapi.BundleCondition{ From 83a216361f4dd202d5f09fc97a072f3156a3f535 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 3 May 2022 18:42:45 +0100 Subject: [PATCH 04/13] Adds NamespaceSelector tests to smoke test Signed-off-by: joshvanl --- test/env/data.go | 45 ++++++++++++++++++++++++++------------------- test/gen/bundle.go | 10 ++++++++++ test/smoke/suite.go | 41 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 23 deletions(-) diff --git a/test/env/data.go b/test/env/data.go index abfac6b7..7588fd13 100644 --- a/test/env/data.go +++ b/test/env/data.go @@ -131,29 +131,19 @@ func NewTestBundle(ctx context.Context, cl client.Client, opts bundle.Options, t // Ensures the Bundle status has been updated with the appropriate target. // Ensures the Bundle has the correct status condition with the same // ObservedGeneration as the current Generation. -func BundleHasSynced(ctx context.Context, cl client.Client, name, expectedData string) bool { +func BundleHasSynced(ctx context.Context, cl client.Client, name, namespace, expectedData string) bool { var bundle trustapi.Bundle Expect(cl.Get(ctx, client.ObjectKey{Name: name}, &bundle)).NotTo(HaveOccurred()) - var namespaceList corev1.NamespaceList - Expect(cl.List(ctx, &namespaceList)).NotTo(HaveOccurred()) - - for _, namespace := range namespaceList.Items { - // Skip terminating namespaces since Bundle won't be synced there - if namespace.Status.Phase == corev1.NamespaceTerminating { - continue - } - - var configMap corev1.ConfigMap - Eventually(func() error { - return cl.Get(ctx, client.ObjectKey{Namespace: namespace.Name, Name: bundle.Name}, &configMap) - }, "1s", "100ms").Should(BeNil(), "Waiting for ConfigMap to be created") + var configMap corev1.ConfigMap + Eventually(func() error { + return cl.Get(ctx, client.ObjectKey{Namespace: namespace, Name: bundle.Name}, &configMap) + }, "1s", "100ms").Should(BeNil(), "Waiting for ConfigMap to be created") - if configMap.Data[bundle.Spec.Target.ConfigMap.Key] != expectedData { - By(fmt.Sprintf("ConfigMap does not have expected data: %s/%s: EXPECTED[%q] GOT[%q]", - namespace.Name, bundle.Name, expectedData, configMap.Data[bundle.Spec.Target.ConfigMap.Key])) - return false - } + if configMap.Data[bundle.Spec.Target.ConfigMap.Key] != expectedData { + By(fmt.Sprintf("ConfigMap does not have expected data: %s/%s: EXPECTED[%q] GOT[%q]", + namespace, bundle.Name, expectedData, configMap.Data[bundle.Spec.Target.ConfigMap.Key])) + return false } if bundle.Status.Target == nil || !apiequality.Semantic.DeepEqual(*bundle.Status.Target, bundle.Spec.Target) { @@ -168,3 +158,20 @@ func BundleHasSynced(ctx context.Context, cl client.Client, name, expectedData s return false } + +// BundleHasSyncedAllNamespaces calls BundleHasSynced for all namespaces. +func BundleHasSyncedAllNamespaces(ctx context.Context, cl client.Client, name, expectedData string) bool { + var namespaceList corev1.NamespaceList + Expect(cl.List(ctx, &namespaceList)).NotTo(HaveOccurred()) + + for _, namespace := range namespaceList.Items { + // Skip terminating namespaces since Bundle won't be synced there + if namespace.Status.Phase == corev1.NamespaceTerminating { + continue + } + + return BundleHasSynced(ctx, cl, name, namespace.Name, expectedData) + } + + return true +} diff --git a/test/gen/bundle.go b/test/gen/bundle.go index 4abe286e..26aa895a 100644 --- a/test/gen/bundle.go +++ b/test/gen/bundle.go @@ -67,3 +67,13 @@ func SetBundleResourceVersion(resourceVersion string) BundleModifier { bundle.ResourceVersion = resourceVersion } } + +// SetBundleTargetNamespaceSelector sets the Bundle object's spec target +// namespace selector. +func SetBundleTargetNamespaceSelectorLabelSelector(selector *metav1.LabelSelector) BundleModifier { + return func(bundle *trustapi.Bundle) { + bundle.Spec.Target.NamespaceSelector = &trustapi.NamespaceSelector{ + LabelSelector: selector, + } + } +} diff --git a/test/smoke/suite.go b/test/smoke/suite.go index 7c71b165..f67ec186 100644 --- a/test/smoke/suite.go +++ b/test/smoke/suite.go @@ -22,6 +22,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2/klogr" "k8s.io/utils/pointer" @@ -55,7 +56,7 @@ var _ = Describe("Smoke", func() { }, testData) By("Ensuring the Bundle has Synced") - Eventually(func() bool { return env.BundleHasSynced(ctx, cl, testBundle.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return env.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "A\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) By("Ensuring targets update when a ConfigMap source is updated") var configMap corev1.ConfigMap @@ -63,7 +64,7 @@ var _ = Describe("Smoke", func() { configMap.Data[testData.Sources.ConfigMap.Key] = "D" Expect(cl.Update(ctx, &configMap)).NotTo(HaveOccurred()) Context("should observe Bundle has changed the value 'A' -> 'D'", func() { - Eventually(func() bool { return env.BundleHasSynced(ctx, cl, testBundle.Name, "D\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return env.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "D\nB\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) By("Ensuring targets update when a Secret source is updated") @@ -72,7 +73,7 @@ var _ = Describe("Smoke", func() { secret.Data[testData.Sources.Secret.Key] = []byte("E") Expect(cl.Update(ctx, &secret)).NotTo(HaveOccurred()) Context("should observe Bundle has changed the value 'B' -> 'E'", func() { - Eventually(func() bool { return env.BundleHasSynced(ctx, cl, testBundle.Name, "D\nE\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return env.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "D\nE\nC\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) By("Ensuring targets update when an InLine source is updated") @@ -80,9 +81,41 @@ var _ = Describe("Smoke", func() { testBundle.Spec.Sources[2].InLine = pointer.String("F") Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should observe Bundle has changed the value 'C' -> 'F'", func() { - Eventually(func() bool { return env.BundleHasSynced(ctx, cl, testBundle.Name, "D\nE\nF\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + Eventually(func() bool { return env.BundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, "D\nE\nF\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) }) + By("Ensuring targets update when a Namespace is created") + testNamespace := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "trust-test-smoke-random-namespace-"}} + Expect(cl.Create(ctx, &testNamespace)).NotTo(HaveOccurred()) + Context("should observe Bundle has created ConfigMap in testNamespace", func() { + Eventually(func() bool { return env.BundleHasSynced(ctx, cl, testBundle.Name, testNamespace.Name, "D\nE\nF\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + }) + + By("Setting Namespace Selector should remove ConfigMaps from Namespaces that do not have a match") + Expect(cl.Get(ctx, client.ObjectKey{Name: testBundle.Name}, testBundle)).NotTo(HaveOccurred()) + testBundle.Spec.Target.NamespaceSelector = &trustapi.NamespaceSelector{ + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + } + Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) + Context("should delete ConfigMap in test Namespace", func() { + Eventually(func() bool { + var cm corev1.ConfigMap + err := cl.Get(ctx, client.ObjectKey{Namespace: testNamespace.Name, Name: testBundle.Name}, &cm) + return apierrors.IsNotFound(err) + }, eventuallyTimeout, "100ms").Should(BeTrue()) + }) + + By("Adding matching label on Namespace should sync ConfigMap to namespace") + Expect(cl.Get(ctx, client.ObjectKey{Name: testNamespace.Name}, &testNamespace)).NotTo(HaveOccurred()) + testNamespace.Labels["foo"] = "bar" + Expect(cl.Update(ctx, &testNamespace)).NotTo(HaveOccurred()) + Context("should create ConfigMap in test Namespace", func() { + Eventually(func() bool { return env.BundleHasSynced(ctx, cl, testBundle.Name, testNamespace.Name, "D\nE\nF\n") }, eventuallyTimeout, "100ms").Should(BeTrue()) + }) + + By("Deleting test Namespace") + Expect(cl.Delete(ctx, &testNamespace)).NotTo(HaveOccurred()) + By("Deleting the Bundle created") Expect(cl.Get(ctx, client.ObjectKeyFromObject(testBundle), testBundle)).ToNot(HaveOccurred()) Expect(cl.Delete(ctx, testBundle)).NotTo(HaveOccurred()) From 8e23793ff953ec82958c9b6774d063fd1c024021 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 3 May 2022 18:43:06 +0100 Subject: [PATCH 05/13] Updates trust bundle CRD with namespaceSelector API Signed-off-by: joshvanl --- .../trust.cert-manager.io_bundles.yaml | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml b/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml index ab8bcd99..190c3068 100644 --- a/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml +++ b/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml @@ -102,6 +102,36 @@ spec: key: description: Key is the key of the entry in the object's `data` field to be used. type: string + namespaceSelector: + description: NamespaceSelector will, if set, only sync the target resource in Namespaces which match the selector. + type: object + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. The requirements are ANDed. + type: array + items: + description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. + type: object + required: + - key + - operator + properties: + key: + description: key is the label key that the selector applies to. + type: string + operator: + description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. + type: array + items: + type: string + matchLabels: + description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + additionalProperties: + type: string status: description: Status of the Bundle. This is set and managed automatically. type: object @@ -149,6 +179,36 @@ spec: key: description: Key is the key of the entry in the object's `data` field to be used. type: string + namespaceSelector: + description: NamespaceSelector will, if set, only sync the target resource in Namespaces which match the selector. + type: object + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. The requirements are ANDed. + type: array + items: + description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. + type: object + required: + - key + - operator + properties: + key: + description: key is the label key that the selector applies to. + type: string + operator: + description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. + type: array + items: + type: string + matchLabels: + description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + type: object + additionalProperties: + type: string served: true storage: true subresources: From d6daeef552a17774a3c0a73b24b5deee926a6025 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 3 May 2022 18:43:24 +0100 Subject: [PATCH 06/13] Adds delete permissions to trust controller to enable deleting ConfigMaps for namespaces which are no longer matching namespaces Signed-off-by: joshvanl --- deploy/charts/trust/templates/clusterrole.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deploy/charts/trust/templates/clusterrole.yaml b/deploy/charts/trust/templates/clusterrole.yaml index 94981a10..0471004c 100644 --- a/deploy/charts/trust/templates/clusterrole.yaml +++ b/deploy/charts/trust/templates/clusterrole.yaml @@ -20,7 +20,7 @@ rules: - "" resources: - "configmaps" - verbs: ["get", "list", "create", "update", "watch"] + verbs: ["get", "list", "create", "update", "watch", "delete"] - apiGroups: - "" resources: From 9e277a7449e5464466b4182a3a23631d9aff16eb Mon Sep 17 00:00:00 2001 From: joshvanl Date: Wed, 8 Jun 2022 17:36:55 +0100 Subject: [PATCH 07/13] Fire event on ConfigMap when not deleted due to ownership Signed-off-by: joshvanl --- pkg/bundle/sync.go | 1 + pkg/bundle/sync_test.go | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/bundle/sync.go b/pkg/bundle/sync.go index 8667abe9..4550303d 100644 --- a/pkg/bundle/sync.go +++ b/pkg/bundle/sync.go @@ -172,6 +172,7 @@ func (b *bundle) syncTarget(ctx context.Context, log logr.Logger, } // The ConfigMap isn't owned by us, so we shouldn't delete it. Return that // we did nothing. + b.recorder.Eventf(&configMap, corev1.EventTypeWarning, "NotOwned", "ConfigMap is not owned by trust.cert-manager.io so ignoring") return false, nil } diff --git a/pkg/bundle/sync_test.go b/pkg/bundle/sync_test.go index 3c403c96..597ce0dd 100644 --- a/pkg/bundle/sync_test.go +++ b/pkg/bundle/sync_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" + "k8s.io/client-go/tools/record" "k8s.io/klog/v2/klogr" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" @@ -53,6 +54,7 @@ func Test_syncTarget(t *testing.T) { selector func(t *testing.T) labels.Selector // Expect the configmap to exist at the end of the sync. expExists bool + expEvent string // Expect the owner reference of the configmap to point to the bundle. expOwnerReference bool expNeedsUpdate bool @@ -315,6 +317,7 @@ func Test_syncTarget(t *testing.T) { expExists: true, expOwnerReference: false, expNeedsUpdate: false, + expEvent: "Warning NotOwned ConfigMap is not owned by trust.cert-manager.io so ignoring", }, } @@ -326,8 +329,9 @@ func Test_syncTarget(t *testing.T) { clientBuilder.WithRuntimeObjects(test.object) } fakeclient := clientBuilder.Build() + fakerecorder := record.NewFakeRecorder(1) - b := &bundle{client: fakeclient} + b := &bundle{client: fakeclient, recorder: fakerecorder} needsUpdate, err := b.syncTarget(context.TODO(), klogr.New(), &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: bundleName}, @@ -357,6 +361,13 @@ func Test_syncTarget(t *testing.T) { assert.NotContains(t, configMap.OwnerReferences, expectedOwnerReference) } } + + var event string + select { + case event = <-fakerecorder.Events: + default: + } + assert.Equal(t, test.expEvent, event) }) } } From b08a660e96cb7a91eea0f6906770b43cd79a8da1 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 18 Jul 2022 13:57:37 +0100 Subject: [PATCH 08/13] Change LabelSelector API to use explicit map[string]string Signed-off-by: joshvanl --- pkg/apis/trust/v1alpha1/types_bundle.go | 7 +++++-- pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go | 11 ++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/apis/trust/v1alpha1/types_bundle.go b/pkg/apis/trust/v1alpha1/types_bundle.go index fa601e45..7ad0f41c 100644 --- a/pkg/apis/trust/v1alpha1/types_bundle.go +++ b/pkg/apis/trust/v1alpha1/types_bundle.go @@ -87,13 +87,16 @@ type BundleTarget struct { // NamespaceSelector will, if set, only sync the target resource in // Namespaces which match the selector. + // +optional NamespaceSelector *NamespaceSelector `json:"namespaceSelector,omitempty"` } // NamespaceSelector defines selectors to match on Namespaces. type NamespaceSelector struct { - // LabelSelector defines a selector which matches on the Namespace labels. - *metav1.LabelSelector `json:",inline,omitempty"` + // MatchLabels matches on the set of labels that must be present on a + // Namespace for the Bundle target to be synced there. + // +optional + MatchLabels map[string]string `json:"matchLabels,omitempty"` } // SourceObjectKeySelector is a reference to a source object and its `data` key diff --git a/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go index 67013e5e..46b8e2c9 100644 --- a/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.go @@ -22,7 +22,6 @@ limitations under the License. package v1alpha1 import ( - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -235,10 +234,12 @@ func (in *KeySelector) DeepCopy() *KeySelector { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NamespaceSelector) DeepCopyInto(out *NamespaceSelector) { *out = *in - if in.LabelSelector != nil { - in, out := &in.LabelSelector, &out.LabelSelector - *out = new(v1.LabelSelector) - (*in).DeepCopyInto(*out) + if in.MatchLabels != nil { + in, out := &in.MatchLabels, &out.MatchLabels + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } } return } From 34587a0aabb46b194bcdea64c17d5dd267b8a5f5 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 18 Jul 2022 13:58:16 +0100 Subject: [PATCH 09/13] Updates CRD with new description and selector API Signed-off-by: joshvanl --- .../trust.cert-manager.io_bundles.yaml | 48 ++----------------- 1 file changed, 3 insertions(+), 45 deletions(-) diff --git a/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml b/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml index 190c3068..cddec59f 100644 --- a/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml +++ b/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml @@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.6.1 + controller-gen.kubebuilder.io/version: v0.8.0 creationTimestamp: null name: bundles.trust.cert-manager.io spec: @@ -106,29 +106,8 @@ spec: description: NamespaceSelector will, if set, only sync the target resource in Namespaces which match the selector. type: object properties: - matchExpressions: - description: matchExpressions is a list of label selector requirements. The requirements are ANDed. - type: array - items: - description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. - type: object - required: - - key - - operator - properties: - key: - description: key is the label key that the selector applies to. - type: string - operator: - description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. - type: string - values: - description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. - type: array - items: - type: string matchLabels: - description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + description: MatchLabels matches on the set of labels that must be present on a Namespace for the Bundle target to be synced there. type: object additionalProperties: type: string @@ -183,29 +162,8 @@ spec: description: NamespaceSelector will, if set, only sync the target resource in Namespaces which match the selector. type: object properties: - matchExpressions: - description: matchExpressions is a list of label selector requirements. The requirements are ANDed. - type: array - items: - description: A label selector requirement is a selector that contains values, a key, and an operator that relates the key and values. - type: object - required: - - key - - operator - properties: - key: - description: key is the label key that the selector applies to. - type: string - operator: - description: operator represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists and DoesNotExist. - type: string - values: - description: values is an array of string values. If the operator is In or NotIn, the values array must be non-empty. If the operator is Exists or DoesNotExist, the values array must be empty. This array is replaced during a strategic merge patch. - type: array - items: - type: string matchLabels: - description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. + description: MatchLabels matches on the set of labels that must be present on a Namespace for the Bundle target to be synced there. type: object additionalProperties: type: string From 82017bc0445e7320bd594c7503d692d20b67337a Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 18 Jul 2022 13:59:26 +0100 Subject: [PATCH 10/13] Updates webhook validator for new namespace selector API Signed-off-by: joshvanl --- pkg/webhook/validation.go | 6 +++--- pkg/webhook/validation_test.go | 10 +++------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/pkg/webhook/validation.go b/pkg/webhook/validation.go index 39b01c81..50274bd7 100644 --- a/pkg/webhook/validation.go +++ b/pkg/webhook/validation.go @@ -174,9 +174,9 @@ func (v *validator) validateBundle(ctx context.Context, bundle *trustapi.Bundle) } } - if namespaceSelector := bundle.Spec.Target.NamespaceSelector; namespaceSelector != nil && namespaceSelector.LabelSelector != nil { - if _, err := metav1.LabelSelectorAsSelector(bundle.Spec.Target.NamespaceSelector.LabelSelector); err != nil { - el = append(el, field.Invalid(path.Child("target", "namespaceSelector"), namespaceSelector.LabelSelector, err.Error())) + if nsSel := bundle.Spec.Target.NamespaceSelector; nsSel != nil && len(nsSel.MatchLabels) > 0 { + if _, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{MatchLabels: nsSel.MatchLabels}); err != nil { + el = append(el, field.Invalid(path.Child("target", "namespaceSelector", "matchLabels"), nsSel.MatchLabels, err.Error())) } } diff --git a/pkg/webhook/validation_test.go b/pkg/webhook/validation_test.go index 359b7bef..4913b1fb 100644 --- a/pkg/webhook/validation_test.go +++ b/pkg/webhook/validation_test.go @@ -401,9 +401,7 @@ func Test_validateBundle(t *testing.T) { Target: trustapi.BundleTarget{ ConfigMap: &trustapi.KeySelector{Key: "test-1"}, NamespaceSelector: &trustapi.NamespaceSelector{ - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"@@@@": ""}, - }, + MatchLabels: map[string]string{"@@@@": ""}, }, }, }, @@ -417,7 +415,7 @@ func Test_validateBundle(t *testing.T) { }, }, expEl: field.ErrorList{ - field.Invalid(field.NewPath("spec", "target", "namespaceSelector"), &metav1.LabelSelector{MatchLabels: map[string]string{"@@@@": ""}}, `key: Invalid value: "@@@@": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`), + field.Invalid(field.NewPath("spec", "target", "namespaceSelector", "matchLabels"), map[string]string{"@@@@": ""}, `key: Invalid value: "@@@@": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`), }, }, "valid bundle": { @@ -430,9 +428,7 @@ func Test_validateBundle(t *testing.T) { Target: trustapi.BundleTarget{ ConfigMap: &trustapi.KeySelector{Key: "test-1"}, NamespaceSelector: &trustapi.NamespaceSelector{ - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"foo": "bar"}, - }, + MatchLabels: map[string]string{"foo": "bar"}, }, }, }, From 3d32cc77c08c8d4b442013d6d5430e01a5bc33d0 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 18 Jul 2022 14:01:05 +0100 Subject: [PATCH 11/13] Update bundle controller for new namespace selector API Signed-off-by: joshvanl --- pkg/bundle/bundle.go | 22 ++++++++-------------- pkg/bundle/bundle_test.go | 20 ++++++++++---------- pkg/bundle/test/suite.go | 4 +--- 3 files changed, 19 insertions(+), 27 deletions(-) diff --git a/pkg/bundle/bundle.go b/pkg/bundle/bundle.go index e9d4e7c8..ee9a3868 100644 --- a/pkg/bundle/bundle.go +++ b/pkg/bundle/bundle.go @@ -86,16 +86,13 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, return ctrl.Result{}, fmt.Errorf("failed to get %q: %s", req.NamespacedName, err) } - var namespaceSelector labels.Selector - if nsSelector := bundle.Spec.Target.NamespaceSelector; nsSelector != nil && nsSelector.LabelSelector != nil { - var err error - namespaceSelector, err = metav1.LabelSelectorAsSelector(nsSelector.LabelSelector) + namespaceSelector := labels.Everything() + if nsSelector := bundle.Spec.Target.NamespaceSelector; nsSelector != nil && nsSelector.MatchLabels != nil { + namespaceSelector, err = metav1.LabelSelectorAsSelector(&metav1.LabelSelector{MatchLabels: nsSelector.MatchLabels}) if err != nil { - b.recorder.Eventf(&bundle, corev1.EventTypeWarning, "NamespaceSelectorError", "Failed to build namespace selector: %s", err) + b.recorder.Eventf(&bundle, corev1.EventTypeWarning, "NamespaceSelectorError", "Failed to build namespace match labels selector: %s", err) return ctrl.Result{}, fmt.Errorf("failed to build NamespaceSelector: %w", err) } - } else { - namespaceSelector = labels.Everything() } var namespaceList corev1.NamespaceList @@ -205,13 +202,10 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, needsUpdate = true } - var message string - - if nsSelector := bundle.Spec.Target.NamespaceSelector; nsSelector != nil && nsSelector.LabelSelector != nil { - message = fmt.Sprintf("Successfully synced Bundle to namespaces with selector [matchLabels:%v matchExpressions: %v]", - nsSelector.MatchLabels, nsSelector.MatchExpressions) - } else { - message = "Successfully synced Bundle to all namespaces" + message := "Successfully synced Bundle to all namespaces" + if nsSelector := bundle.Spec.Target.NamespaceSelector; nsSelector != nil && nsSelector.MatchLabels != nil { + message = fmt.Sprintf("Successfully synced Bundle to namespaces with selector [matchLabels:%v]", + nsSelector.MatchLabels) } syncedCondition := trustapi.BundleCondition{ diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index 27d3d946..686eefef 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -333,7 +333,7 @@ func Test_Reconcile(t *testing.T) { }, "if Bundle not synced everywhere, sync except Namespaces that don't match labels and update Synced": { existingObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, - gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + gen.SetBundleTargetNamespaceSelectorMatchLabels(map[string]string{"foo": "bar"}), ), &corev1.Namespace{ TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, @@ -355,12 +355,12 @@ func Test_Reconcile(t *testing.T) { expObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, gen.SetBundleResourceVersion("1001"), - gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + gen.SetBundleTargetNamespaceSelectorMatchLabels(map[string]string{"foo": "bar"}), gen.SetBundleStatus(trustapi.BundleStatus{ Target: &trustapi.BundleTarget{ ConfigMap: &trustapi.KeySelector{Key: targetKey}, NamespaceSelector: &trustapi.NamespaceSelector{ - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + MatchLabels: map[string]string{"foo": "bar"}, }, }, Conditions: []trustapi.BundleCondition{{ @@ -368,7 +368,7 @@ func Test_Reconcile(t *testing.T) { Status: corev1.ConditionTrue, LastTransitionTime: &metav1.Time{Time: fixedclock.Now().Local()}, Reason: "Synced", - Message: "Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + Message: "Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar]]", ObservedGeneration: bundleGeneration, }}, }), @@ -384,11 +384,11 @@ func Test_Reconcile(t *testing.T) { Data: map[string]string{targetKey: "A\nB\nC\n"}, }, ), - expEvent: "Normal Synced Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + expEvent: "Normal Synced Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar]]", }, "if Bundle not synced everywhere, sync except Namespaces that don't match labels and update Synced. Should delete ConfigMaps in wrong namespaces.": { existingObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, - gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + gen.SetBundleTargetNamespaceSelectorMatchLabels(map[string]string{"foo": "bar"}), ), &corev1.ConfigMap{ TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, @@ -411,12 +411,12 @@ func Test_Reconcile(t *testing.T) { expObjects: append(namespaces, sourceConfigMap, sourceSecret, gen.BundleFrom(baseBundle, gen.SetBundleResourceVersion("1001"), - gen.SetBundleTargetNamespaceSelectorLabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}), + gen.SetBundleTargetNamespaceSelectorMatchLabels(map[string]string{"foo": "bar"}), gen.SetBundleStatus(trustapi.BundleStatus{ Target: &trustapi.BundleTarget{ ConfigMap: &trustapi.KeySelector{Key: targetKey}, NamespaceSelector: &trustapi.NamespaceSelector{ - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + MatchLabels: map[string]string{"foo": "bar"}, }, }, Conditions: []trustapi.BundleCondition{{ @@ -424,13 +424,13 @@ func Test_Reconcile(t *testing.T) { Status: corev1.ConditionTrue, LastTransitionTime: &metav1.Time{Time: fixedclock.Now().Local()}, Reason: "Synced", - Message: "Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + Message: "Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar]]", ObservedGeneration: bundleGeneration, }}, }), ), ), - expEvent: "Normal Synced Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar] matchExpressions: []]", + expEvent: "Normal Synced Successfully synced Bundle to namespaces with selector [matchLabels:map[foo:bar]]", }, "if Bundle synced but doesn't have owner reference, should sync and update": { existingObjects: append(namespaces, sourceConfigMap, sourceSecret, diff --git a/pkg/bundle/test/suite.go b/pkg/bundle/test/suite.go index a03caf78..0e10b868 100644 --- a/pkg/bundle/test/suite.go +++ b/pkg/bundle/test/suite.go @@ -314,9 +314,7 @@ var _ = Describe("Integration", func() { }) testBundle.Spec.Target.NamespaceSelector = &trustapi.NamespaceSelector{ - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"foo": "bar"}, - }, + MatchLabels: map[string]string{"foo": "bar"}, } Expect(cl.Update(ctx, testBundle)).ToNot(HaveOccurred()) Context("should observe ConfigMap deleted from Namespace", func() { From aeed85b08b2002c4c06dfdb8705a7d7f7935c94b Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 18 Jul 2022 14:01:24 +0100 Subject: [PATCH 12/13] Update namespace selector in smoke test Signed-off-by: joshvanl --- test/gen/bundle.go | 8 ++++---- test/smoke/suite.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/gen/bundle.go b/test/gen/bundle.go index 26aa895a..c20c3078 100644 --- a/test/gen/bundle.go +++ b/test/gen/bundle.go @@ -68,12 +68,12 @@ func SetBundleResourceVersion(resourceVersion string) BundleModifier { } } -// SetBundleTargetNamespaceSelector sets the Bundle object's spec target -// namespace selector. -func SetBundleTargetNamespaceSelectorLabelSelector(selector *metav1.LabelSelector) BundleModifier { +// SetBundleTargetNamespaceSelectorMatchLabels sets the Bundle object's spec +// target namespace selector. +func SetBundleTargetNamespaceSelectorMatchLabels(matchLabels map[string]string) BundleModifier { return func(bundle *trustapi.Bundle) { bundle.Spec.Target.NamespaceSelector = &trustapi.NamespaceSelector{ - LabelSelector: selector, + MatchLabels: matchLabels, } } } diff --git a/test/smoke/suite.go b/test/smoke/suite.go index f67ec186..dcd25881 100644 --- a/test/smoke/suite.go +++ b/test/smoke/suite.go @@ -94,7 +94,7 @@ var _ = Describe("Smoke", func() { By("Setting Namespace Selector should remove ConfigMaps from Namespaces that do not have a match") Expect(cl.Get(ctx, client.ObjectKey{Name: testBundle.Name}, testBundle)).NotTo(HaveOccurred()) testBundle.Spec.Target.NamespaceSelector = &trustapi.NamespaceSelector{ - LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + MatchLabels: map[string]string{"foo": "bar"}, } Expect(cl.Update(ctx, testBundle)).NotTo(HaveOccurred()) Context("should delete ConfigMap in test Namespace", func() { From 9a99ed812596f82c2c07429d9c1b9d974ff6f337 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Mon, 18 Jul 2022 14:49:39 +0100 Subject: [PATCH 13/13] Fix comment about target ConfigMap Signed-off-by: joshvanl --- .../charts/trust/templates/trust.cert-manager.io_bundles.yaml | 4 ++-- pkg/apis/trust/v1alpha1/types_bundle.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml b/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml index cddec59f..66969460 100644 --- a/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml +++ b/deploy/charts/trust/templates/trust.cert-manager.io_bundles.yaml @@ -94,7 +94,7 @@ spec: type: object properties: configMap: - description: ConfigMap is the target ConfigMap in all Namespaces that all Bundle source data will be synced to. + description: ConfigMap is the target ConfigMap in Namespaces that all Bundle source data will be synced to. type: object required: - key @@ -150,7 +150,7 @@ spec: type: object properties: configMap: - description: ConfigMap is the target ConfigMap in all Namespaces that all Bundle source data will be synced to. + description: ConfigMap is the target ConfigMap in Namespaces that all Bundle source data will be synced to. type: object required: - key diff --git a/pkg/apis/trust/v1alpha1/types_bundle.go b/pkg/apis/trust/v1alpha1/types_bundle.go index 7ad0f41c..f9ea48b7 100644 --- a/pkg/apis/trust/v1alpha1/types_bundle.go +++ b/pkg/apis/trust/v1alpha1/types_bundle.go @@ -81,7 +81,7 @@ type BundleSource struct { // BundleTarget is the target resource that the Bundle will sync all source // data to. type BundleTarget struct { - // ConfigMap is the target ConfigMap in all Namespaces that all Bundle source + // ConfigMap is the target ConfigMap in Namespaces that all Bundle source // data will be synced to. ConfigMap *KeySelector `json:"configMap,omitempty"`