From dfd59280437d8823ec2ae48a18cd5ba07a06b0bf Mon Sep 17 00:00:00 2001 From: Andrej Krejcir Date: Tue, 28 May 2024 10:34:57 +0200 Subject: [PATCH 1/4] tests: Create SSP in the same namespace as deployment The SSP object has to be in the same namespace where the SSP deployment is. Signed-off-by: Andrej Krejcir --- automation/e2e-upgrade-functests/run.sh | 12 +--------- tests/env/env.go | 5 ---- tests/tests_suite_test.go | 32 ++++++------------------- 3 files changed, 8 insertions(+), 41 deletions(-) diff --git a/automation/e2e-upgrade-functests/run.sh b/automation/e2e-upgrade-functests/run.sh index f121c99ed..ce060dc67 100755 --- a/automation/e2e-upgrade-functests/run.sh +++ b/automation/e2e-upgrade-functests/run.sh @@ -35,18 +35,9 @@ oc apply -n $NAMESPACE -f "https://github.com/kubevirt/ssp-operator/releases/dow oc wait --for=condition=Available --timeout=600s -n ${NAMESPACE} deployments/ssp-operator SSP_NAME="ssp-test" -SSP_NAMESPACE="ssp-operator-functests" +SSP_NAMESPACE="kubevirt" SSP_TEMPLATES_NAMESPACE="ssp-operator-functests-templates" -oc apply -f - < Date: Tue, 28 May 2024 09:43:49 +0200 Subject: [PATCH 2/4] feat: Watch SSP objects only from a single namespace Signed-off-by: Andrej Krejcir --- internal/controllers/controller.go | 15 +++- internal/controllers/services_controller.go | 6 +- internal/controllers/setup.go | 6 +- internal/controllers/ssp_controller.go | 38 +++++----- internal/controllers/vm_controller.go | 11 ++- internal/controllers/webhook_controller.go | 6 +- main.go | 79 +++++++++++++++++---- tests/webhook_test.go | 25 +++++++ webhooks/ssp_webhook.go | 18 +++-- webhooks/ssp_webhook_test.go | 37 ++++++++-- 10 files changed, 190 insertions(+), 51 deletions(-) diff --git a/internal/controllers/controller.go b/internal/controllers/controller.go index 0d0cbdf7f..2854831fe 100644 --- a/internal/controllers/controller.go +++ b/internal/controllers/controller.go @@ -2,6 +2,7 @@ package controllers import ( ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" crd_watch "kubevirt.io/ssp-operator/internal/crd-watch" ) @@ -9,5 +10,17 @@ import ( type Controller interface { Name() string AddToManager(mgr ctrl.Manager, crdList crd_watch.CrdList) error - RequiredCrds() []string + GetWatchObjects() []WatchObject +} + +type WatchObject struct { + // Object is the object that this option applies to. + Object client.Object + + // CrdName is the name of the CRD that defines this object. + CrdName string + + // WatchOnlyOperatorNamespace sets if the cache should only watch the object type + // in the same namespace where the operator is defined. + WatchOnlyOperatorNamespace bool } diff --git a/internal/controllers/services_controller.go b/internal/controllers/services_controller.go index a17246a86..94a54e95e 100644 --- a/internal/controllers/services_controller.go +++ b/internal/controllers/services_controller.go @@ -123,8 +123,10 @@ func (s *serviceReconciler) AddToManager(mgr ctrl.Manager, _ crd_watch.CrdList) })) } -func (s *serviceReconciler) RequiredCrds() []string { - return nil +func (s *serviceReconciler) GetWatchObjects() []WatchObject { + return []WatchObject{{ + Object: &v1.Service{}, + }} } func (s *serviceReconciler) setServiceOwnerReference(service *v1.Service) error { diff --git a/internal/controllers/setup.go b/internal/controllers/setup.go index 189ab998c..bf87a165e 100644 --- a/internal/controllers/setup.go +++ b/internal/controllers/setup.go @@ -105,7 +105,11 @@ func CreateControllers(ctx context.Context, apiReader client.Reader) ([]Controll func setupManager(ctx context.Context, cancel context.CancelFunc, mgr controllerruntime.Manager, controllers []Controller) error { var requiredCrds []string for _, controller := range controllers { - requiredCrds = append(requiredCrds, controller.RequiredCrds()...) + for _, watchObject := range controller.GetWatchObjects() { + if watchObject.CrdName != "" { + requiredCrds = append(requiredCrds, watchObject.CrdName) + } + } } crdWatch := crd_watch.New(mgr.GetCache(), requiredCrds...) diff --git a/internal/controllers/ssp_controller.go b/internal/controllers/ssp_controller.go index b892d48ef..e618597c9 100644 --- a/internal/controllers/ssp_controller.go +++ b/internal/controllers/ssp_controller.go @@ -131,27 +131,31 @@ func (s *sspController) AddToManager(mgr ctrl.Manager, crdList crd_watch.CrdList return builder.Complete(s) } -func (r *sspController) RequiredCrds() []string { - var result []string - for _, operand := range r.operands { - result = append(result, getRequiredCrds(operand)...) - } - return result -} +func (s *sspController) GetWatchObjects() []WatchObject { + var results []WatchObject + + results = append(results, WatchObject{ + Object: &ssp.SSP{}, + CrdName: "ssps.ssp.kubevirt.io", + WatchOnlyOperatorNamespace: true, + }) -func getRequiredCrds(operand operands.Operand) []string { - var result []string - for _, watchType := range operand.WatchTypes() { - if watchType.Crd != "" { - result = append(result, watchType.Crd) + for _, operand := range s.operands { + for _, watchType := range operand.WatchTypes() { + results = append(results, WatchObject{ + Object: watchType.Object, + CrdName: watchType.Crd, + }) } - } - for _, watchType := range operand.WatchClusterTypes() { - if watchType.Crd != "" { - result = append(result, watchType.Crd) + for _, watchType := range operand.WatchClusterTypes() { + results = append(results, WatchObject{ + Object: watchType.Object, + CrdName: watchType.Crd, + }) } } - return result + + return results } // Reconcile is part of the main kubernetes reconciliation loop which aims to diff --git a/internal/controllers/vm_controller.go b/internal/controllers/vm_controller.go index 629064931..f53e224eb 100644 --- a/internal/controllers/vm_controller.go +++ b/internal/controllers/vm_controller.go @@ -57,8 +57,15 @@ func (v *vmController) AddToManager(mgr ctrl.Manager, crdList crd_watch.CrdList) Complete(v) } -func (v *vmController) RequiredCrds() []string { - return []string{getVmCrd()} +func (v *vmController) GetWatchObjects() []WatchObject { + return []WatchObject{{ + Object: &kubevirtv1.VirtualMachine{}, + CrdName: getVmCrd(), + }, { + Object: &corev1.PersistentVolumeClaim{}, + }, { + Object: &corev1.PersistentVolume{}, + }} } func (v *vmController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { diff --git a/internal/controllers/webhook_controller.go b/internal/controllers/webhook_controller.go index de88da606..87ca339fe 100644 --- a/internal/controllers/webhook_controller.go +++ b/internal/controllers/webhook_controller.go @@ -60,8 +60,10 @@ func (w *webhookCtrl) AddToManager(mgr ctrl.Manager, _ crd_watch.CrdList) error Complete(w) } -func (w *webhookCtrl) RequiredCrds() []string { - return nil +func (w *webhookCtrl) GetWatchObjects() []WatchObject { + return []WatchObject{{ + Object: &admissionv1.ValidatingWebhookConfiguration{}, + }} } func (w *webhookCtrl) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { diff --git a/main.go b/main.go index fdd0c9f14..73fa48992 100644 --- a/main.go +++ b/main.go @@ -32,10 +32,12 @@ import ( ocpconfigv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/crypto" "github.com/prometheus/client_golang/prometheus/promhttp" + "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -45,10 +47,10 @@ import ( ssp "kubevirt.io/ssp-operator/api/v1beta2" "kubevirt.io/ssp-operator/internal/common" "kubevirt.io/ssp-operator/internal/controllers" + "kubevirt.io/ssp-operator/internal/env" sspMetrics "kubevirt.io/ssp-operator/pkg/monitoring/metrics/ssp-operator" "kubevirt.io/ssp-operator/pkg/monitoring/rules" "kubevirt.io/ssp-operator/webhooks" - // +kubebuilder:scaffold:imports ) var ( @@ -229,21 +231,26 @@ func main() { os.Exit(1) } - // Using closure so that the temporary client is cleaned up after it is not needed anymore. - ctrls, err := func() ([]controllers.Controller, error) { - apiClient, err := client.New(apiConfig, client.Options{ - Scheme: common.Scheme, - }) - if err != nil { - return nil, err - } - return controllers.CreateControllers(ctx, apiClient) - }() + apiClient, err := client.New(apiConfig, client.Options{ + Scheme: common.Scheme, + }) + if err != nil { + setupLog.Error(err, "error creating API client") + os.Exit(1) + } + + ctrls, err := controllers.CreateControllers(ctx, apiClient) if err != nil { setupLog.Error(err, "error creating controllers") os.Exit(1) } + operatorNamespace, err := env.GetOperatorNamespace() + if err != nil { + setupLog.Error(err, "error getting operator namespace") + os.Exit(1) + } + var mgr ctrl.Manager getTLSOptsFunc := func(cfg *tls.Config) { @@ -252,8 +259,15 @@ func main() { } } + cacheOptions, err := createCacheOptions(ctrls, operatorNamespace) + if err != nil { + setupLog.Error(err, "error creating cache options") + os.Exit(1) + } + mgr, err = ctrl.NewManager(apiConfig, ctrl.Options{ Scheme: common.Scheme, + Cache: cacheOptions, Metrics: metricsserver.Options{ BindAddress: "0", }, @@ -272,7 +286,7 @@ func main() { } if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = webhooks.Setup(mgr); err != nil { + if err = webhooks.Setup(apiClient, operatorNamespace, mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "SSP") os.Exit(1) } @@ -337,3 +351,44 @@ func createCertificateSymlinks() error { return nil } + +func createCacheOptions(ctrls []controllers.Controller, operatorNamespace string) (cache.Options, error) { + watchObjectsMap := map[schema.GroupVersionKind]controllers.WatchObject{} + for _, controller := range ctrls { + watchObjects := controller.GetWatchObjects() + for _, watchObject := range watchObjects { + gvk, err := apiutil.GVKForObject(watchObject.Object, common.Scheme) + if err != nil { + return cache.Options{}, err + } + existingObject, exists := watchObjectsMap[gvk] + if !exists { + watchObjectsMap[gvk] = watchObject + continue + } + + // If one of the objects wants to watch all namespaces, + // then the resulting WatchObject should too. + if !watchObject.WatchOnlyOperatorNamespace { + existingObject.WatchOnlyOperatorNamespace = false + } + watchObjectsMap[gvk] = existingObject + } + } + + cacheOptions := cache.Options{ + ByObject: map[client.Object]cache.ByObject{}, + } + for _, watchObject := range watchObjectsMap { + if !watchObject.WatchOnlyOperatorNamespace { + continue + } + cacheOptions.ByObject[watchObject.Object] = cache.ByObject{ + Namespaces: map[string]cache.Config{ + operatorNamespace: {}, + }, + } + } + + return cacheOptions, nil +} diff --git a/tests/webhook_test.go b/tests/webhook_test.go index be7fd7b74..86cf0dd6b 100644 --- a/tests/webhook_test.go +++ b/tests/webhook_test.go @@ -134,6 +134,31 @@ var _ = Describe("Validation webhook", func() { }) }) + Context("with second namespace", func() { + var ( + namespace *corev1.Namespace + ) + + BeforeEach(func() { + namespace = &corev1.Namespace{ + ObjectMeta: v1.ObjectMeta{ + GenerateName: "test-webhook-namespace-", + }, + } + Expect(apiClient.Create(ctx, namespace)).To(Succeed()) + }) + + AfterEach(func() { + Expect(apiClient.Delete(ctx, namespace)).To(Succeed()) + }) + + It("[test_id:TODO] should fail to create SSP CR in a different namespace", func() { + newSsp.Namespace = namespace.Name + Expect(apiClient.Create(ctx, newSsp, client.DryRunAll)). + To(MatchError(ContainSubstring("SSP object must be in namespace:"))) + }) + }) + It("[test_id:TODO] should fail when DataImportCronTemplate does not have a name", func() { newSsp.Spec.CommonTemplates.DataImportCronTemplates = []sspv1beta2.DataImportCronTemplate{{ ObjectMeta: metav1.ObjectMeta{Name: ""}, diff --git a/webhooks/ssp_webhook.go b/webhooks/ssp_webhook.go index 9d74462b1..1d86de0f1 100644 --- a/webhooks/ssp_webhook.go +++ b/webhooks/ssp_webhook.go @@ -39,7 +39,7 @@ import ( var ssplog = logf.Log.WithName("ssp-resource") -func Setup(mgr ctrl.Manager) error { +func Setup(uncachedClient client.Client, namespace string, mgr ctrl.Manager) error { // This is a hack. Using Unstructured allows the webhook to correctly decode different versions of objects. // Controller-runtime currently does not support a single webhook for multiple versions. @@ -49,7 +49,7 @@ func Setup(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). For(obj). - WithValidator(newSspValidator(mgr.GetClient())). + WithValidator(newSspValidator(uncachedClient, namespace)). Complete() } @@ -57,6 +57,7 @@ func Setup(mgr ctrl.Manager) error { type sspValidator struct { apiClient client.Client + namespace string } var _ admission.CustomValidator = &sspValidator{} @@ -67,10 +68,13 @@ func (s *sspValidator) ValidateCreate(ctx context.Context, obj runtime.Object) ( return nil, err } - var ssps sspv1beta2.SSPList - - // Check if no other SSP resources are present in the cluster ssplog.Info("validate create", "name", sspObj.Name) + + if sspObj.Namespace != s.namespace { + return nil, fmt.Errorf("SSP object must be in namespace: %s", s.namespace) + } + + var ssps sspv1beta2.SSPList err = s.apiClient.List(ctx, &ssps, &client.ListOptions{}) if err != nil { return nil, fmt.Errorf("could not list SSPs for validation, please try again: %v", err) @@ -229,6 +233,6 @@ func validateCommonInstancetypes(ssp *sspv1beta2.SSP) error { return nil } -func newSspValidator(clt client.Client) *sspValidator { - return &sspValidator{apiClient: clt} +func newSspValidator(clt client.Client, namespace string) *sspValidator { + return &sspValidator{apiClient: clt, namespace: namespace} } diff --git a/webhooks/ssp_webhook_test.go b/webhooks/ssp_webhook_test.go index 098d66b13..84563ee93 100644 --- a/webhooks/ssp_webhook_test.go +++ b/webhooks/ssp_webhook_test.go @@ -39,6 +39,10 @@ import ( ) var _ = Describe("SSP Validation", func() { + const ( + testOperatorNamespace = "kubevirt" + ) + var ( client client.Client objects = make([]runtime.Object, 0) @@ -57,7 +61,7 @@ var _ = Describe("SSP Validation", func() { client = fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objects...).Build() - validator = newSspValidator(client) + validator = newSspValidator(client, testOperatorNamespace) ctx = context.Background() }) @@ -79,13 +83,31 @@ var _ = Describe("SSP Validation", func() { objects = make([]runtime.Object, 0) }) + It("should reject if it is not in operator namespace", func() { + ssp := &sspv1beta2.SSP{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ssp2", + Namespace: "test-ns2", + }, + Spec: sspv1beta2.SSPSpec{ + CommonTemplates: sspv1beta2.CommonTemplates{ + Namespace: templatesNamespace, + }, + }, + } + + _, err := validator.ValidateCreate(ctx, toUnstructured(ssp)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("SSP object must be in namespace")) + }) + Context("when one is already present", func() { BeforeEach(func() { // add an SSP CR to fake client objects = append(objects, &sspv1beta2.SSP{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ssp", - Namespace: "test-ns", + Namespace: testOperatorNamespace, ResourceVersion: "1", }, Spec: sspv1beta2.SSPSpec{ @@ -100,7 +122,7 @@ var _ = Describe("SSP Validation", func() { ssp := &sspv1beta2.SSP{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ssp2", - Namespace: "test-ns2", + Namespace: testOperatorNamespace, }, Spec: sspv1beta2.SSPSpec{ CommonTemplates: sspv1beta2.CommonTemplates{ @@ -111,7 +133,7 @@ var _ = Describe("SSP Validation", func() { _, err := validator.ValidateCreate(ctx, toUnstructured(ssp)) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("creation failed, an SSP CR already exists in namespace test-ns: test-ssp")) + Expect(err.Error()).To(ContainSubstring("creation failed, an SSP CR already exists in namespace")) }) }) @@ -119,7 +141,7 @@ var _ = Describe("SSP Validation", func() { ssp := &sspv1beta1.SSP{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ssp", - Namespace: "test-ns", + Namespace: testOperatorNamespace, }, Spec: sspv1beta1.SSPSpec{ TemplateValidator: &sspv1beta1.TemplateValidator{ @@ -170,7 +192,7 @@ var _ = Describe("SSP Validation", func() { oldSSP = &sspv1beta2.SSP{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ssp", - Namespace: "test-ns", + Namespace: testOperatorNamespace, }, Spec: sspv1beta2.SSPSpec{ CommonTemplates: sspv1beta2.CommonTemplates{ @@ -231,7 +253,8 @@ var _ = Describe("SSP Validation", func() { }) sspObj = &sspv1beta2.SSP{ ObjectMeta: metav1.ObjectMeta{ - Name: "ssp", + Name: "ssp", + Namespace: testOperatorNamespace, }, Spec: sspv1beta2.SSPSpec{ CommonTemplates: sspv1beta2.CommonTemplates{ From 83945adf9125df07b707bc2ed0a73377d22c1cdc Mon Sep 17 00:00:00 2001 From: Andrej Krejcir Date: Tue, 4 Jun 2024 14:37:32 +0200 Subject: [PATCH 3/4] WIP - watch non-cluster objects only in single namespace. Signed-off-by: Andrej Krejcir --- internal/controllers/ssp_controller.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/controllers/ssp_controller.go b/internal/controllers/ssp_controller.go index e618597c9..01ff206ef 100644 --- a/internal/controllers/ssp_controller.go +++ b/internal/controllers/ssp_controller.go @@ -143,8 +143,9 @@ func (s *sspController) GetWatchObjects() []WatchObject { for _, operand := range s.operands { for _, watchType := range operand.WatchTypes() { results = append(results, WatchObject{ - Object: watchType.Object, - CrdName: watchType.Crd, + Object: watchType.Object, + CrdName: watchType.Crd, + WatchOnlyOperatorNamespace: true, }) } for _, watchType := range operand.WatchClusterTypes() { From 6640b2a520dd491cd6106a68ad25dafcb87b5c49 Mon Sep 17 00:00:00 2001 From: Andrej Krejcir Date: Tue, 4 Jun 2024 14:03:33 +0200 Subject: [PATCH 4/4] WIP - watch only based on labels Signed-off-by: Andrej Krejcir --- internal/common/labels.go | 2 ++ internal/common/resource.go | 33 +++++++++++++++-- internal/common/resource_test.go | 7 ++++ internal/controllers/controller.go | 4 +++ internal/controllers/setup.go | 2 +- internal/controllers/ssp_controller.go | 10 +++--- .../common-instancetypes/reconcile.go | 19 ++++++---- .../operands/common-templates/reconcile.go | 8 +++-- internal/operands/data-sources/reconcile.go | 36 ++++++++++++++----- internal/operands/metrics/reconcile.go | 17 +++++---- internal/operands/operand.go | 4 +++ internal/operands/tekton-cleanup/reconcile.go | 12 ++++--- .../operands/template-validator/reconcile.go | 21 ++++++----- .../operands/vm-console-proxy/reconcile.go | 21 ++++++----- main.go | 31 +++++++++++++--- 15 files changed, 170 insertions(+), 57 deletions(-) diff --git a/internal/common/labels.go b/internal/common/labels.go index a74e8d20b..5fc5acb59 100644 --- a/internal/common/labels.go +++ b/internal/common/labels.go @@ -8,6 +8,8 @@ import ( ) const ( + WatchedObjectLabel = "ssp.kubevirt.io/watched" + AppKubernetesNameLabel = "app.kubernetes.io/name" AppKubernetesPartOfLabel = "app.kubernetes.io/part-of" AppKubernetesVersionLabel = "app.kubernetes.io/version" diff --git a/internal/common/resource.go b/internal/common/resource.go index 24cb0fa9e..9a7c77a9e 100644 --- a/internal/common/resource.go +++ b/internal/common/resource.go @@ -165,6 +165,7 @@ func (r *reconcileBuilder) Reconcile() (ReconcileResult, error) { found := newEmptyResource(r.resource) found.SetName(r.resource.GetName()) found.SetNamespace(r.resource.GetNamespace()) + // TODO -- consider refactoring this mutateFn out. mutateFn := func() error { if !found.GetDeletionTimestamp().IsZero() { // Skip update, because the resource is being deleted @@ -177,11 +178,22 @@ func (r *reconcileBuilder) Reconcile() (ReconcileResult, error) { UpdateLabels(r.resource, found) updateAnnotations(r.resource, found) + // TODO -- consider how the watch label relates to version cache if r.options.AlwaysCallUpdateFunc || !r.request.VersionCache.Contains(found) { // The generation was updated by other cluster components, // operator needs to update the resource r.updateFunc(r.resource, found) } + + // TODO -- move to a different function, because this label is needed + // by all resources + + // Adding this label after, so that updateFunc cannot remove it. + if found.GetLabels() == nil { + found.SetLabels(map[string]string{}) + } + found.GetLabels()[WatchedObjectLabel] = "true" + return nil } @@ -287,10 +299,27 @@ func (r *reconcileBuilder) createOrUpdateWithImmutableSpec(obj client.Object, f if err := mutate(f, key, obj); err != nil { return OperationResultNone, nil, err } - if err := r.request.Client.Create(r.request.Context, obj); err != nil { + + err := r.request.Client.Create(r.request.Context, obj) + if err == nil { + return OperationResultCreated, nil, nil + } + if !errors.IsAlreadyExists(err) { + return OperationResultNone, nil, err + } + + // If the get operation above does not find the object and + // create returns an error, because the object already exists, + // then it means that cache is stale. This can happen + // if the cache only watches objects with a certain label, + // and the object was created without the label. + // + // Using uncached client to read the object again. + + // TODO -- validate that this is correct. + if err = r.request.UncachedReader.Get(r.request.Context, key, obj); err != nil { return OperationResultNone, nil, err } - return OperationResultCreated, nil, nil } existing := obj.DeepCopyObject().(client.Object) diff --git a/internal/common/resource_test.go b/internal/common/resource_test.go index 39920443b..48d72b3ae 100644 --- a/internal/common/resource_test.go +++ b/internal/common/resource_test.go @@ -28,6 +28,8 @@ const ( name = "test-ssp" ) +// TODO -- add unit tests for the watch label + var _ = Describe("Resource", func() { var ( request Request @@ -340,5 +342,10 @@ func expectEqualResourceExists(resource client.Object, request *Request) { resource.SetResourceVersion(found.GetResourceVersion()) resource.SetOwnerReferences(found.GetOwnerReferences()) + if resource.GetLabels() == nil { + resource.SetLabels(map[string]string{}) + } + resource.GetLabels()[WatchedObjectLabel] = "true" + ExpectWithOffset(1, found).To(Equal(resource)) } diff --git a/internal/controllers/controller.go b/internal/controllers/controller.go index 2854831fe..656733076 100644 --- a/internal/controllers/controller.go +++ b/internal/controllers/controller.go @@ -23,4 +23,8 @@ type WatchObject struct { // WatchOnlyOperatorNamespace sets if the cache should only watch the object type // in the same namespace where the operator is defined. WatchOnlyOperatorNamespace bool + + // WatchOnlyObjectsWithLabel sets if the cache should only watch objets + /// with label "ssp.kubevirt.io/watched". + WatchOnlyObjectsWithLabel bool } diff --git a/internal/controllers/setup.go b/internal/controllers/setup.go index bf87a165e..c7f440e33 100644 --- a/internal/controllers/setup.go +++ b/internal/controllers/setup.go @@ -25,7 +25,7 @@ import ( ) // Need to watch CRDs -// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=list;watch +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch func StartControllers(ctx context.Context, mgr controllerruntime.Manager, controllers []Controller) error { mgrCtx, cancel := context.WithCancel(ctx) diff --git a/internal/controllers/ssp_controller.go b/internal/controllers/ssp_controller.go index 01ff206ef..eea7e4054 100644 --- a/internal/controllers/ssp_controller.go +++ b/internal/controllers/ssp_controller.go @@ -96,11 +96,11 @@ var _ Controller = &sspController{} var _ reconcile.Reconciler = &sspController{} -// +kubebuilder:rbac:groups=ssp.kubevirt.io,resources=ssps,verbs=list;watch;update +// +kubebuilder:rbac:groups=ssp.kubevirt.io,resources=ssps,verbs=get;list;watch;update // +kubebuilder:rbac:groups=ssp.kubevirt.io,resources=ssps/status,verbs=update // +kubebuilder:rbac:groups=ssp.kubevirt.io,resources=ssps/finalizers,verbs=update // +kubebuilder:rbac:groups=config.openshift.io,resources=infrastructures;clusterversions,verbs=get -// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=list +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list func (s *sspController) Name() string { return "ssp-controller" @@ -146,12 +146,14 @@ func (s *sspController) GetWatchObjects() []WatchObject { Object: watchType.Object, CrdName: watchType.Crd, WatchOnlyOperatorNamespace: true, + WatchOnlyObjectsWithLabel: watchType.WatchOnlyWithLabel, }) } for _, watchType := range operand.WatchClusterTypes() { results = append(results, WatchObject{ - Object: watchType.Object, - CrdName: watchType.Crd, + Object: watchType.Object, + CrdName: watchType.Crd, + WatchOnlyObjectsWithLabel: watchType.WatchOnlyWithLabel, }) } } diff --git a/internal/operands/common-instancetypes/reconcile.go b/internal/operands/common-instancetypes/reconcile.go index 30b99bd33..7b22155e9 100644 --- a/internal/operands/common-instancetypes/reconcile.go +++ b/internal/operands/common-instancetypes/reconcile.go @@ -24,8 +24,8 @@ import ( ) // Define RBAC rules needed by this operand: -// +kubebuilder:rbac:groups=instancetype.kubevirt.io,resources=virtualmachineclusterinstancetypes,verbs=list;watch;create;update;delete -// +kubebuilder:rbac:groups=instancetype.kubevirt.io,resources=virtualmachineclusterpreferences,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=instancetype.kubevirt.io,resources=virtualmachineclusterinstancetypes,verbs=get;list;watch;create;update;delete +// +kubebuilder:rbac:groups=instancetype.kubevirt.io,resources=virtualmachineclusterpreferences,verbs=get;list;watch;create;update;delete const ( operandName = "common-instancetypes" @@ -57,10 +57,17 @@ func (c *CommonInstancetypes) Name() string { } func WatchClusterTypes() []operands.WatchType { - return []operands.WatchType{ - {Object: &instancetypev1beta1.VirtualMachineClusterInstancetype{}, Crd: virtualMachineClusterInstancetypeCrd, WatchFullObject: true}, - {Object: &instancetypev1beta1.VirtualMachineClusterPreference{}, Crd: virtualMachineClusterPreferenceCrd, WatchFullObject: true}, - } + return []operands.WatchType{{ + Object: &instancetypev1beta1.VirtualMachineClusterInstancetype{}, + Crd: virtualMachineClusterInstancetypeCrd, + WatchFullObject: true, + WatchOnlyWithLabel: true, + }, { + Object: &instancetypev1beta1.VirtualMachineClusterPreference{}, + Crd: virtualMachineClusterPreferenceCrd, + WatchFullObject: true, + WatchOnlyWithLabel: true, + }} } func (c *CommonInstancetypes) WatchClusterTypes() []operands.WatchType { diff --git a/internal/operands/common-templates/reconcile.go b/internal/operands/common-templates/reconcile.go index 65b85bbbb..a56fa77ba 100644 --- a/internal/operands/common-templates/reconcile.go +++ b/internal/operands/common-templates/reconcile.go @@ -30,9 +30,11 @@ func init() { } func WatchClusterTypes() []operands.WatchType { - return []operands.WatchType{ - {Object: &templatev1.Template{}}, - } + return []operands.WatchType{{ + Object: &templatev1.Template{}, + // TODO -- consider setting to "true" in the future + WatchOnlyWithLabel: false, + }} } type commonTemplates struct { diff --git a/internal/operands/data-sources/reconcile.go b/internal/operands/data-sources/reconcile.go index fa9dfedeb..1aed3fe61 100644 --- a/internal/operands/data-sources/reconcile.go +++ b/internal/operands/data-sources/reconcile.go @@ -19,7 +19,7 @@ import ( // Define RBAC rules needed by this operand: // Define RBAC rules needed by this operand: // +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch;create;update;delete -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;roles;rolebindings,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;roles;rolebindings,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups=cdi.kubevirt.io,resources=datasources,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cdi.kubevirt.io,resources=dataimportcrons,verbs=get;list;watch;create;update;patch;delete @@ -47,15 +47,29 @@ func init() { } func WatchClusterTypes() []operands.WatchType { - return []operands.WatchType{ - {Object: &rbac.ClusterRole{}}, - {Object: &rbac.Role{}}, - {Object: &rbac.RoleBinding{}}, - {Object: &core.Namespace{}}, + return []operands.WatchType{{ + Object: &rbac.ClusterRole{}, + WatchOnlyWithLabel: true, + }, { + Object: &rbac.Role{}, + WatchOnlyWithLabel: true, + }, { + Object: &rbac.RoleBinding{}, + WatchOnlyWithLabel: true, + }, { + Object: &core.Namespace{}, + WatchOnlyWithLabel: true, + }, { + Object: &cdiv1beta1.DataSource{}, + Crd: dataSourceCrd, // Need to watch status of DataSource to notice if referenced PVC was deleted. - {Object: &cdiv1beta1.DataSource{}, Crd: dataSourceCrd, WatchFullObject: true}, - {Object: &cdiv1beta1.DataImportCron{}, Crd: dataImportCronCrd}, - } + WatchFullObject: true, + WatchOnlyWithLabel: true, + }, { + Object: &cdiv1beta1.DataImportCron{}, + Crd: dataImportCronCrd, + WatchOnlyWithLabel: true, + }} } type dataSources struct { @@ -384,6 +398,10 @@ func reconcileDataSource(dsInfo dataSourceInfo, request *common.Request) (common ClusterResource(dsInfo.dataSource). Options(common.ReconcileOptions{AlwaysCallUpdateFunc: true}). UpdateFunc(func(newRes, foundRes client.Object) { + // TODO -- think about data sources and the watched label + // - if DS is created by CDI, it should not have the watched label + // - how does this method work with stale cache? + if dsInfo.autoUpdateEnabled { if foundRes.GetLabels() == nil { foundRes.SetLabels(make(map[string]string)) diff --git a/internal/operands/metrics/reconcile.go b/internal/operands/metrics/reconcile.go index 48241d73b..418243eee 100644 --- a/internal/operands/metrics/reconcile.go +++ b/internal/operands/metrics/reconcile.go @@ -10,8 +10,8 @@ import ( ) // Define RBAC rules needed by this operand: -// +kubebuilder:rbac:groups=monitoring.coreos.com,resources=prometheusrules;servicemonitors,verbs=list;watch;create;update -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=monitoring.coreos.com,resources=prometheusrules;servicemonitors,verbs=get;list;watch;create;update +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups="",resources=pods;endpoints,verbs=get;list;watch const prometheusRulesCrd = "prometheusrules.monitoring.coreos.com" @@ -28,10 +28,15 @@ func WatchTypes() []operands.WatchType { } func WatchClusterTypes() []operands.WatchType { - return []operands.WatchType{ - {Object: &rbac.ClusterRole{}, Crd: prometheusRulesCrd}, - {Object: &rbac.ClusterRoleBinding{}, Crd: prometheusRulesCrd}, - } + return []operands.WatchType{{ + Object: &rbac.ClusterRole{}, + Crd: prometheusRulesCrd, + WatchOnlyWithLabel: true, + }, { + Object: &rbac.ClusterRoleBinding{}, + Crd: prometheusRulesCrd, + WatchOnlyWithLabel: true, + }} } type metrics struct{} diff --git a/internal/operands/operand.go b/internal/operands/operand.go index a7802e974..cb37935bb 100644 --- a/internal/operands/operand.go +++ b/internal/operands/operand.go @@ -34,4 +34,8 @@ type WatchType struct { // Otherwise, only these changes in spec, labels, and annotations. // If an object does not have spec field, the full object is watched by default. WatchFullObject bool + + // WatchOnlyWithLabel sets if the cache should only watch objets + /// with label "ssp.kubevirt.io/watched". + WatchOnlyWithLabel bool } diff --git a/internal/operands/tekton-cleanup/reconcile.go b/internal/operands/tekton-cleanup/reconcile.go index 53003d2fb..1f06d28cb 100644 --- a/internal/operands/tekton-cleanup/reconcile.go +++ b/internal/operands/tekton-cleanup/reconcile.go @@ -16,11 +16,11 @@ import ( "kubevirt.io/ssp-operator/internal/operands" ) -// +kubebuilder:rbac:groups=tekton.dev,resources=pipelines,verbs=list;watch;create;update -// +kubebuilder:rbac:groups=tekton.dev,resources=tasks,verbs=list;watch;update -// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=list;watch;update -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;rolebindings,verbs=list;watch;update -// +kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch;create;update +// +kubebuilder:rbac:groups=tekton.dev,resources=pipelines,verbs=get;list;watch;create;update +// +kubebuilder:rbac:groups=tekton.dev,resources=tasks,verbs=get;list;watch;update +// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=get;list;watch;update +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;rolebindings,verbs=get;list;watch;update +// +kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;list;watch;create;update // Below are RBAC for deployed ClusterRoles. We still need these permissions so we can update annotations on existing ClusterRoles @@ -50,6 +50,8 @@ func init() { func WatchClusterTypes() []operands.WatchType { return []operands.WatchType{ + // Watch resources without label, because this is for cleanup. + // TODO -- think about adding the label to old resources too. {Object: &v1.ConfigMap{}}, {Object: &rbac.ClusterRole{}}, {Object: &rbac.RoleBinding{}}, diff --git a/internal/operands/template-validator/reconcile.go b/internal/operands/template-validator/reconcile.go index 23c20dfba..c0fdb92c8 100644 --- a/internal/operands/template-validator/reconcile.go +++ b/internal/operands/template-validator/reconcile.go @@ -13,11 +13,11 @@ import ( ) // Define RBAC rules needed by this operand: -// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;delete -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=list;watch;create;update;delete -// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;watch;create;update;delete +// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch;create;update;delete // RBAC for created roles // +kubebuilder:rbac:groups=template.openshift.io,resources=templates,verbs=list;watch @@ -32,11 +32,16 @@ func WatchTypes() []operands.WatchType { } func WatchClusterTypes() []operands.WatchType { - return []operands.WatchType{ - {Object: &rbac.ClusterRole{}}, - {Object: &rbac.ClusterRoleBinding{}}, - {Object: &admission.ValidatingWebhookConfiguration{}}, - } + return []operands.WatchType{{ + Object: &rbac.ClusterRole{}, + WatchOnlyWithLabel: true, + }, { + Object: &rbac.ClusterRoleBinding{}, + WatchOnlyWithLabel: true, + }, { + Object: &admission.ValidatingWebhookConfiguration{}, + WatchOnlyWithLabel: true, + }} } type templateValidator struct{} diff --git a/internal/operands/vm-console-proxy/reconcile.go b/internal/operands/vm-console-proxy/reconcile.go index 42bb0b564..2aeb91db1 100644 --- a/internal/operands/vm-console-proxy/reconcile.go +++ b/internal/operands/vm-console-proxy/reconcile.go @@ -27,13 +27,13 @@ const ( // Define RBAC rules needed by this operand: // +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch;create;update;delete -// +kubebuilder:rbac:groups=core,resources=configmaps;serviceaccounts,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=core,resources=configmaps;serviceaccounts,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;delete -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;rolebindings,verbs=list;watch;create;update;delete +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;rolebindings,verbs=get;list;watch;create;update;delete // +kubebuilder:rbac:groups=apiregistration.k8s.io,resources=apiservices,verbs=get;list;watch;create;update;delete // Deprecated: -// +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=list;watch;delete +// +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch;delete // RBAC for created roles // +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch @@ -52,11 +52,16 @@ func init() { } func WatchClusterTypes() []operands.WatchType { - return []operands.WatchType{ - {Object: &rbac.ClusterRole{}}, - {Object: &rbac.ClusterRoleBinding{}}, - {Object: &apiregv1.APIService{}}, - } + return []operands.WatchType{{ + Object: &rbac.ClusterRole{}, + WatchOnlyWithLabel: true, + }, { + Object: &rbac.ClusterRoleBinding{}, + WatchOnlyWithLabel: true, + }, { + Object: &apiregv1.APIService{}, + WatchOnlyWithLabel: true, + }} } func WatchTypes() []operands.WatchType { diff --git a/main.go b/main.go index 73fa48992..73d4cccf3 100644 --- a/main.go +++ b/main.go @@ -32,7 +32,9 @@ import ( ocpconfigv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/crypto" "github.com/prometheus/client_golang/prometheus/promhttp" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/selection" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/certwatcher" @@ -367,11 +369,16 @@ func createCacheOptions(ctrls []controllers.Controller, operatorNamespace string continue } - // If one of the objects wants to watch all namespaces, + // If one of the objects wants to watch all namespaces or + // objects without labels, // then the resulting WatchObject should too. if !watchObject.WatchOnlyOperatorNamespace { existingObject.WatchOnlyOperatorNamespace = false } + if !watchObject.WatchOnlyObjectsWithLabel { + existingObject.WatchOnlyObjectsWithLabel = false + } + watchObjectsMap[gvk] = existingObject } } @@ -380,14 +387,28 @@ func createCacheOptions(ctrls []controllers.Controller, operatorNamespace string ByObject: map[client.Object]cache.ByObject{}, } for _, watchObject := range watchObjectsMap { - if !watchObject.WatchOnlyOperatorNamespace { + if !watchObject.WatchOnlyOperatorNamespace && !watchObject.WatchOnlyObjectsWithLabel { continue } - cacheOptions.ByObject[watchObject.Object] = cache.ByObject{ - Namespaces: map[string]cache.Config{ + + byObject := cache.ByObject{} + if watchObject.WatchOnlyOperatorNamespace { + byObject.Namespaces = map[string]cache.Config{ + // TODO -- verify that the label selector is defaulted to the below selector operatorNamespace: {}, - }, + } } + + if watchObject.WatchOnlyObjectsWithLabel { + requirement, err := labels.NewRequirement(common.WatchedObjectLabel, selection.Equals, []string{"true"}) + if err != nil { + // It is ok to panic, because the above function has constant arguments, so any error is a programmer's mistake. + panic(fmt.Sprintf("Could not create label selector: %v", err)) + } + byObject.Label = labels.NewSelector().Add(*requirement) + } + + cacheOptions.ByObject[watchObject.Object] = byObject } return cacheOptions, nil