From 4ff6f5f87df90bfec039ec257976cf7600d7a163 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Wed, 20 Sep 2023 08:47:45 -0600 Subject: [PATCH 01/17] test/e2e: improve waits, logs, etc Signed-off-by: Steve Kuznetsov --- .../templates/0000_50_olm_07-olm-operator.deployment.yaml | 4 ++++ .../0000_50_olm_08-catalog-operator.deployment.yaml | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/deploy/chart/templates/0000_50_olm_07-olm-operator.deployment.yaml b/deploy/chart/templates/0000_50_olm_07-olm-operator.deployment.yaml index 825d39f0df..76f7ca1ee0 100644 --- a/deploy/chart/templates/0000_50_olm_07-olm-operator.deployment.yaml +++ b/deploy/chart/templates/0000_50_olm_07-olm-operator.deployment.yaml @@ -110,6 +110,10 @@ spec: fieldPath: metadata.namespace - name: OPERATOR_NAME value: olm-operator + {{- if .Values.debug }} + - name: CI + value: "true" + {{- end }} {{- if .Values.olm.resources }} resources: {{ toYaml .Values.olm.resources | indent 12 }} diff --git a/deploy/chart/templates/0000_50_olm_08-catalog-operator.deployment.yaml b/deploy/chart/templates/0000_50_olm_08-catalog-operator.deployment.yaml index 48e882d4ee..2b2d4534d3 100644 --- a/deploy/chart/templates/0000_50_olm_08-catalog-operator.deployment.yaml +++ b/deploy/chart/templates/0000_50_olm_08-catalog-operator.deployment.yaml @@ -90,6 +90,11 @@ spec: - --set-workload-user-id=false {{ end }} image: {{ .Values.catalog.image.ref }} + {{- if .Values.debug }} + env: + - name: CI + value: "true" + {{- end }} imagePullPolicy: {{ .Values.catalog.image.pullPolicy }} ports: - containerPort: {{ .Values.olm.service.internalPort }} From f690c77a18b779f51cb195a69cb1a0946511582c Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 7 Sep 2023 10:20:09 -0600 Subject: [PATCH 02/17] *: detect when all objects are labelled, restart When all of the k8s objects that need labels have them, we are good to exit the process. The next Pod that start up will detect that all labels are present and be able to filter informers going forward. Signed-off-by: Steve Kuznetsov --- cmd/olm/main.go | 2 +- pkg/controller/install/apiservice.go | 1 + pkg/controller/install/deployment.go | 2 +- pkg/controller/install/deployment_test.go | 1 + pkg/controller/operators/catalog/operator.go | 40 ++++- pkg/controller/operators/labeller/filters.go | 3 +- pkg/controller/operators/labeller/labels.go | 163 +++++++++++------- pkg/controller/operators/labeller/rbac.go | 70 +++++--- pkg/controller/operators/olm/operator.go | 86 ++++++++- pkg/controller/operators/olm/operator_test.go | 34 +++- pkg/controller/operators/olm/operatorgroup.go | 15 +- pkg/controller/operators/olm/requirements.go | 125 ++++++++++++-- .../operators/olm/requirements_test.go | 34 +++- pkg/lib/scoped/syncer.go | 12 +- 14 files changed, 461 insertions(+), 127 deletions(-) diff --git a/cmd/olm/main.go b/cmd/olm/main.go index 4473496787..6d76606dc3 100644 --- a/cmd/olm/main.go +++ b/cmd/olm/main.go @@ -173,7 +173,7 @@ func main() { olm.WithExternalClient(crClient), olm.WithMetadataClient(metadataClient), olm.WithOperatorClient(opClient), - olm.WithRestConfig(config), + olm.WithRestConfig(validatingConfig), olm.WithConfigClient(versionedConfigClient), olm.WithProtectedCopiedCSVNamespaces(*protectedCopiedCSVNamespaces), ) diff --git a/pkg/controller/install/apiservice.go b/pkg/controller/install/apiservice.go index 9d17e1664c..09e3b52371 100644 --- a/pkg/controller/install/apiservice.go +++ b/pkg/controller/install/apiservice.go @@ -61,6 +61,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateAPIService(caPEM []byte, des if err := ownerutil.AddOwnerLabels(apiService, i.owner); err != nil { return err } + apiService.Labels[OLMManagedLabelKey] = OLMManagedLabelValue // Create a service for the deployment containerPort := int32(443) diff --git a/pkg/controller/install/deployment.go b/pkg/controller/install/deployment.go index 12e8044b25..8d5e726261 100644 --- a/pkg/controller/install/deployment.go +++ b/pkg/controller/install/deployment.go @@ -152,11 +152,11 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1 dep.Spec.Template.SetAnnotations(annotations) // Set custom labels before CSV owner labels + dep.SetLabels(specLabels) if dep.Labels == nil { dep.Labels = map[string]string{} } dep.Labels[OLMManagedLabelKey] = OLMManagedLabelValue - dep.SetLabels(specLabels) ownerutil.AddNonBlockingOwner(dep, i.owner) ownerutil.AddOwnerLabelsForKind(dep, i.owner, v1alpha1.ClusterServiceVersionKind) diff --git a/pkg/controller/install/deployment_test.go b/pkg/controller/install/deployment_test.go index d9ec66cc2a..cceea7deb9 100644 --- a/pkg/controller/install/deployment_test.go +++ b/pkg/controller/install/deployment_test.go @@ -353,6 +353,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) { dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"}) dep.Spec.RevisionHistoryLimit = &revisionHistoryLimit dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec))) + dep.Labels[OLMManagedLabelKey] = OLMManagedLabelValue dep.Status.Conditions = append(dep.Status.Conditions, appsv1.DeploymentCondition{ Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue, diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 615f2267e3..613bfd38a7 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -207,10 +207,10 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo ogQueueSet: queueinformer.NewEmptyResourceQueueSet(), catalogSubscriberIndexer: map[string]cache.Indexer{}, serviceAccountQuerier: scoped.NewUserDefinedServiceAccountQuerier(logger, crClient), - clientAttenuator: scoped.NewClientAttenuator(logger, config, opClient), + clientAttenuator: scoped.NewClientAttenuator(logger, validatingConfig, opClient), installPlanTimeout: installPlanTimeout, bundleUnpackTimeout: bundleUnpackTimeout, - clientFactory: clients.NewFactory(config), + clientFactory: clients.NewFactory(validatingConfig), } op.sources = grpc.NewSourceStore(logger, 10*time.Second, 10*time.Minute, op.syncSourceState) op.sourceInvalidator = resolver.SourceProviderFromRegistryClientProvider(op.sources, logger) @@ -380,10 +380,22 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo op.lister.RbacV1().RegisterRoleLister(metav1.NamespaceAll, roleInformer.Lister()) sharedIndexInformers = append(sharedIndexInformers, roleInformer.Informer()) - labelObjects := func(gvr schema.GroupVersionResource, informer cache.SharedIndexInformer, sync queueinformer.LegacySyncHandler) error { + complete := map[schema.GroupVersionResource][]bool{} + completeLock := &sync.Mutex{} + + labelObjects := func(gvr schema.GroupVersionResource, informer cache.SharedIndexInformer, sync func(done func() bool) queueinformer.LegacySyncHandler) error { if canFilter { return nil } + var idx int + if _, exists := complete[gvr]; exists { + idx = len(complete[gvr]) + complete[gvr] = append(complete[gvr], false) + } else { + idx = 0 + complete[gvr] = []bool{false} + } + queue := workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{ Name: gvr.String(), }) @@ -392,7 +404,18 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo queueinformer.WithQueue(queue), queueinformer.WithLogger(op.logger), queueinformer.WithInformer(informer), - queueinformer.WithSyncer(sync.ToSyncer()), + queueinformer.WithSyncer(sync(func() bool { + completeLock.Lock() + complete[gvr][idx] = true + allDone := true + for _, items := range complete { + for _, done := range items { + allDone = allDone && done + } + } + completeLock.Unlock() + return allDone + }).ToSyncer()), ) if err != nil { return err @@ -408,6 +431,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo rolesgvk := rbacv1.SchemeGroupVersion.WithResource("roles") if err := labelObjects(rolesgvk, roleInformer.Informer(), labeller.ObjectLabeler[*rbacv1.Role, *rbacv1applyconfigurations.RoleApplyConfiguration]( ctx, op.logger, labeller.Filter(rolesgvk), + roleInformer.Lister().List, rbacv1applyconfigurations.Role, func(namespace string, ctx context.Context, cfg *rbacv1applyconfigurations.RoleApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.Role, error) { return op.opClient.KubernetesInterface().RbacV1().Roles(namespace).Apply(ctx, cfg, opts) @@ -420,6 +444,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo func(role *rbacv1.Role) (string, error) { return resolver.PolicyRuleHashLabelValue(role.Rules) }, + roleInformer.Lister().List, rbacv1applyconfigurations.Role, func(namespace string, ctx context.Context, cfg *rbacv1applyconfigurations.RoleApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.Role, error) { return op.opClient.KubernetesInterface().RbacV1().Roles(namespace).Apply(ctx, cfg, opts) @@ -436,6 +461,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo rolebindingsgvk := rbacv1.SchemeGroupVersion.WithResource("rolebindings") if err := labelObjects(rolebindingsgvk, roleBindingInformer.Informer(), labeller.ObjectLabeler[*rbacv1.RoleBinding, *rbacv1applyconfigurations.RoleBindingApplyConfiguration]( ctx, op.logger, labeller.Filter(rolebindingsgvk), + roleBindingInformer.Lister().List, rbacv1applyconfigurations.RoleBinding, func(namespace string, ctx context.Context, cfg *rbacv1applyconfigurations.RoleBindingApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.RoleBinding, error) { return op.opClient.KubernetesInterface().RbacV1().RoleBindings(namespace).Apply(ctx, cfg, opts) @@ -448,6 +474,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo func(roleBinding *rbacv1.RoleBinding) (string, error) { return resolver.RoleReferenceAndSubjectHashLabelValue(roleBinding.RoleRef, roleBinding.Subjects) }, + roleBindingInformer.Lister().List, rbacv1applyconfigurations.RoleBinding, func(namespace string, ctx context.Context, cfg *rbacv1applyconfigurations.RoleBindingApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.RoleBinding, error) { return op.opClient.KubernetesInterface().RbacV1().RoleBindings(namespace).Apply(ctx, cfg, opts) @@ -464,6 +491,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo serviceaccountsgvk := corev1.SchemeGroupVersion.WithResource("serviceaccounts") if err := labelObjects(serviceaccountsgvk, serviceAccountInformer.Informer(), labeller.ObjectLabeler[*corev1.ServiceAccount, *corev1applyconfigurations.ServiceAccountApplyConfiguration]( ctx, op.logger, labeller.Filter(serviceaccountsgvk), + serviceAccountInformer.Lister().List, corev1applyconfigurations.ServiceAccount, func(namespace string, ctx context.Context, cfg *corev1applyconfigurations.ServiceAccountApplyConfiguration, opts metav1.ApplyOptions) (*corev1.ServiceAccount, error) { return op.opClient.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Apply(ctx, cfg, opts) @@ -480,6 +508,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo servicesgvk := corev1.SchemeGroupVersion.WithResource("services") if err := labelObjects(servicesgvk, serviceInformer.Informer(), labeller.ObjectLabeler[*corev1.Service, *corev1applyconfigurations.ServiceApplyConfiguration]( ctx, op.logger, labeller.Filter(servicesgvk), + serviceInformer.Lister().List, corev1applyconfigurations.Service, func(namespace string, ctx context.Context, cfg *corev1applyconfigurations.ServiceApplyConfiguration, opts metav1.ApplyOptions) (*corev1.Service, error) { return op.opClient.KubernetesInterface().CoreV1().Services(namespace).Apply(ctx, cfg, opts) @@ -505,6 +534,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo podsgvk := corev1.SchemeGroupVersion.WithResource("pods") if err := labelObjects(podsgvk, csPodInformer.Informer(), labeller.ObjectLabeler[*corev1.Pod, *corev1applyconfigurations.PodApplyConfiguration]( ctx, op.logger, labeller.Filter(podsgvk), + csPodInformer.Lister().List, corev1applyconfigurations.Pod, func(namespace string, ctx context.Context, cfg *corev1applyconfigurations.PodApplyConfiguration, opts metav1.ApplyOptions) (*corev1.Pod, error) { return op.opClient.KubernetesInterface().CoreV1().Pods(namespace).Apply(ctx, cfg, opts) @@ -542,6 +572,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo ctx, op.logger, labeller.JobFilter(func(namespace, name string) (metav1.Object, error) { return configMapInformer.Lister().ConfigMaps(namespace).Get(name) }), + jobInformer.Lister().List, batchv1applyconfigurations.Job, func(namespace string, ctx context.Context, cfg *batchv1applyconfigurations.JobApplyConfiguration, opts metav1.ApplyOptions) (*batchv1.Job, error) { return op.opClient.KubernetesInterface().BatchV1().Jobs(namespace).Apply(ctx, cfg, opts) @@ -617,6 +648,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo customresourcedefinitionsgvk := apiextensionsv1.SchemeGroupVersion.WithResource("customresourcedefinitions") if err := labelObjects(customresourcedefinitionsgvk, crdInformer, labeller.ObjectPatchLabeler( ctx, op.logger, labeller.Filter(customresourcedefinitionsgvk), + crdLister.List, op.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Patch, )); err != nil { return nil, err diff --git a/pkg/controller/operators/labeller/filters.go b/pkg/controller/operators/labeller/filters.go index 4bdbfece31..c5f5010622 100644 --- a/pkg/controller/operators/labeller/filters.go +++ b/pkg/controller/operators/labeller/filters.go @@ -75,7 +75,7 @@ var filters = map[schema.GroupVersionResource]func(metav1.Object) bool{ func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface) (bool, error) { okLock := sync.Mutex{} - var ok bool + ok := true g, ctx := errgroup.WithContext(ctx) allFilters := map[schema.GroupVersionResource]func(metav1.Object) bool{} for gvr, filter := range filters { @@ -124,5 +124,6 @@ func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadat if err := g.Wait(); err != nil { return false, err } + logger.WithField("canFilter", ok).Info("detected ability to filter informers") return ok, nil } diff --git a/pkg/controller/operators/labeller/labels.go b/pkg/controller/operators/labeller/labels.go index 8c2af67c9d..dabf6dd258 100644 --- a/pkg/controller/operators/labeller/labels.go +++ b/pkg/controller/operators/labeller/labels.go @@ -4,12 +4,14 @@ import ( "context" "encoding/json" "fmt" + "os" "strings" jsonpatch "github.com/evanphx/json-patch" "github.com/sirupsen/logrus" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -40,28 +42,50 @@ func ObjectLabeler[T metav1.Object, A ApplyConfig[A]]( ctx context.Context, logger *logrus.Logger, check func(metav1.Object) bool, + list func(options labels.Selector) ([]T, error), applyConfigFor func(name, namespace string) A, apply func(namespace string, ctx context.Context, cfg A, opts metav1.ApplyOptions) (T, error), -) queueinformer.LegacySyncHandler { - return func(obj interface{}) error { - cast, ok := obj.(T) - if !ok { - err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(T), obj) - logger.WithError(err).Error("casting failed") - return fmt.Errorf("casting failed: %w", err) - } +) func(done func() bool) queueinformer.LegacySyncHandler { + return func(done func() bool) queueinformer.LegacySyncHandler { + return func(obj interface{}) error { + cast, ok := obj.(T) + if !ok { + err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(T), obj) + logger.WithError(err).Error("casting failed") + return fmt.Errorf("casting failed: %w", err) + } - if !check(cast) || hasLabel(cast) { - return nil - } + if !check(cast) || hasLabel(cast) { + // if the object we're processing does not need us to label it, it's possible that every object that requires + // the label already has it; in which case we should exit the process, so the Pod that succeeds us can filter + // the informers used to drive the controller and stop having to track extraneous objects + items, err := list(labels.Everything()) + if err != nil { + logger.WithError(err).Warn("failed to list all objects to check for labelling completion") + return nil + } + gvrFullyLabelled := true + for _, item := range items { + gvrFullyLabelled = gvrFullyLabelled && (!check(item) || hasLabel(item)) + } + if gvrFullyLabelled { + allObjectsLabelled := done() + if allObjectsLabelled { + logrus.Info("detected that every object is labelled, exiting to re-start the process...") + os.Exit(0) + } + } + return nil + } - cfg := applyConfigFor(cast.GetName(), cast.GetNamespace()) - cfg.WithLabels(map[string]string{ - install.OLMManagedLabelKey: install.OLMManagedLabelValue, - }) + cfg := applyConfigFor(cast.GetName(), cast.GetNamespace()) + cfg.WithLabels(map[string]string{ + install.OLMManagedLabelKey: install.OLMManagedLabelValue, + }) - _, err := apply(cast.GetNamespace(), ctx, cfg, metav1.ApplyOptions{}) - return err + _, err := apply(cast.GetNamespace(), ctx, cfg, metav1.ApplyOptions{}) + return err + } } } @@ -71,58 +95,77 @@ func ObjectPatchLabeler( ctx context.Context, logger *logrus.Logger, check func(metav1.Object) bool, + list func(selector labels.Selector) (ret []*metav1.PartialObjectMetadata, err error), patch func(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (result *apiextensionsv1.CustomResourceDefinition, err error), -) func( - obj interface{}, -) error { - return func(obj interface{}) error { - cast, ok := obj.(*apiextensionsv1.CustomResourceDefinition) - if !ok { - err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(*apiextensionsv1.CustomResourceDefinition), obj) - logger.WithError(err).Error("casting failed") - return fmt.Errorf("casting failed: %w", err) - } +) func(done func() bool) queueinformer.LegacySyncHandler { + return func(done func() bool) queueinformer.LegacySyncHandler { + return func(obj interface{}) error { + cast, ok := obj.(*apiextensionsv1.CustomResourceDefinition) + if !ok { + err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(*apiextensionsv1.CustomResourceDefinition), obj) + logger.WithError(err).Error("casting failed") + return fmt.Errorf("casting failed: %w", err) + } - if !check(cast) || hasLabel(cast) { - return nil - } + if !check(cast) || hasLabel(cast) { + // if the object we're processing does not need us to label it, it's possible that every object that requires + // the label already has it; in which case we should exit the process, so the Pod that succeeds us can filter + // the informers used to drive the controller and stop having to track extraneous objects + items, err := list(labels.Everything()) + if err != nil { + logger.WithError(err).Warn("failed to list all objects to check for labelling completion") + return nil + } + gvrFullyLabelled := true + for _, item := range items { + gvrFullyLabelled = gvrFullyLabelled && (!check(item) || hasLabel(item)) + } + if gvrFullyLabelled { + allObjectsLabelled := done() + if allObjectsLabelled { + logrus.Fatal("detected that every object is labelled, exiting...") + } + } + return nil + } - uid := cast.GetUID() - rv := cast.GetResourceVersion() + uid := cast.GetUID() + rv := cast.GetResourceVersion() - // to ensure they appear in the patch as preconditions - previous := cast.DeepCopy() - previous.SetUID("") - previous.SetResourceVersion("") + // to ensure they appear in the patch as preconditions + previous := cast.DeepCopy() + previous.SetUID("") + previous.SetResourceVersion("") - oldData, err := json.Marshal(previous) - if err != nil { - return fmt.Errorf("failed to Marshal old data for %s/%s: %w", previous.GetNamespace(), previous.GetName(), err) - } + oldData, err := json.Marshal(previous) + if err != nil { + return fmt.Errorf("failed to Marshal old data for %s/%s: %w", previous.GetNamespace(), previous.GetName(), err) + } - // to ensure they appear in the patch as preconditions - updated := cast.DeepCopy() - updated.SetUID(uid) - updated.SetResourceVersion(rv) - labels := updated.GetLabels() - if labels == nil { - labels = map[string]string{} - } - labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue - updated.SetLabels(labels) + // to ensure they appear in the patch as preconditions + updated := cast.DeepCopy() + updated.SetUID(uid) + updated.SetResourceVersion(rv) + labels := updated.GetLabels() + if labels == nil { + labels = map[string]string{} + } + labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue + updated.SetLabels(labels) - newData, err := json.Marshal(updated) - if err != nil { - return fmt.Errorf("failed to Marshal old data for %s/%s: %w", updated.GetNamespace(), updated.GetName(), err) - } + newData, err := json.Marshal(updated) + if err != nil { + return fmt.Errorf("failed to Marshal old data for %s/%s: %w", updated.GetNamespace(), updated.GetName(), err) + } - patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) - if err != nil { - return fmt.Errorf("failed to create patch for %s/%s: %w", cast.GetNamespace(), cast.GetName(), err) - } + patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) + if err != nil { + return fmt.Errorf("failed to create patch for %s/%s: %w", cast.GetNamespace(), cast.GetName(), err) + } - _, err = patch(ctx, cast.GetName(), types.MergePatchType, patchBytes, metav1.PatchOptions{}) - return err + _, err = patch(ctx, cast.GetName(), types.MergePatchType, patchBytes, metav1.PatchOptions{}) + return err + } } } diff --git a/pkg/controller/operators/labeller/rbac.go b/pkg/controller/operators/labeller/rbac.go index 6e64791ca7..395e19592a 100644 --- a/pkg/controller/operators/labeller/rbac.go +++ b/pkg/controller/operators/labeller/rbac.go @@ -10,6 +10,7 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" ) func hasHashLabel(obj metav1.Object) bool { @@ -22,35 +23,56 @@ func ContentHashLabeler[T metav1.Object, A ApplyConfig[A]]( logger *logrus.Logger, check func(metav1.Object) bool, hasher func(object T) (string, error), + list func(options labels.Selector) ([]T, error), applyConfigFor func(name, namespace string) A, apply func(namespace string, ctx context.Context, cfg A, opts metav1.ApplyOptions) (T, error), -) queueinformer.LegacySyncHandler { - return func(obj interface{}) error { - cast, ok := obj.(T) - if !ok { - err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(T), obj) - logger.WithError(err).Error("casting failed") - return fmt.Errorf("casting failed: %w", err) - } +) func(done func() bool) queueinformer.LegacySyncHandler { + return func(done func() bool) queueinformer.LegacySyncHandler { + return func(obj interface{}) error { + cast, ok := obj.(T) + if !ok { + err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(T), obj) + logger.WithError(err).Error("casting failed") + return fmt.Errorf("casting failed: %w", err) + } - if _, _, ok := ownerutil.GetOwnerByKindLabel(cast, v1alpha1.ClusterServiceVersionKind); !ok { - return nil - } + if _, _, ok := ownerutil.GetOwnerByKindLabel(cast, v1alpha1.ClusterServiceVersionKind); !ok { + // if the object we're processing does not need us to label it, it's possible that every object that requires + // the label already has it; in which case we should exit the process, so the Pod that succeeds us can filter + // the informers used to drive the controller and stop having to track extraneous objects + items, err := list(labels.Everything()) + if err != nil { + logger.WithError(err).Warn("failed to list all objects to check for labelling completion") + return nil + } + gvrFullyLabelled := true + for _, item := range items { + gvrFullyLabelled = gvrFullyLabelled && (!check(item) || hasLabel(item)) + } + if gvrFullyLabelled { + allObjectsLabelled := done() + if allObjectsLabelled { + logrus.Fatal("detected that every object is labelled, exiting...") + } + } + return nil + } - if !check(cast) || hasHashLabel(cast) { - return nil - } + if !check(cast) || hasHashLabel(cast) { + return nil + } - hash, err := hasher(cast) - if err != nil { - return fmt.Errorf("failed to calculate hash: %w", err) - } + hash, err := hasher(cast) + if err != nil { + return fmt.Errorf("failed to calculate hash: %w", err) + } - cfg := applyConfigFor(cast.GetName(), cast.GetNamespace()) - cfg.WithLabels(map[string]string{ - resolver.ContentHashLabelKey: hash, - }) - _, err = apply(cast.GetNamespace(), ctx, cfg, metav1.ApplyOptions{}) - return err + cfg := applyConfigFor(cast.GetName(), cast.GetNamespace()) + cfg.WithLabels(map[string]string{ + resolver.ContentHashLabelKey: hash, + }) + _, err = apply(cast.GetNamespace(), ctx, cfg, metav1.ApplyOptions{}) + return err + } } } diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 9855e8d106..2ba02129a9 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "sync" "time" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" @@ -102,12 +103,57 @@ type Operator struct { clientFactory clients.Factory plugins []plugins.OperatorPlugin informersByNamespace map[string]*plugins.Informers + informersFiltered bool + + ruleChecker func(*v1alpha1.ClusterServiceVersion) *install.CSVRuleChecker + ruleCheckerLock *sync.RWMutex + resyncPeriod func() time.Duration + ctx context.Context } func (a *Operator) Informers() map[string]*plugins.Informers { return a.informersByNamespace } +func (a *Operator) getRuleChecker() func(*v1alpha1.ClusterServiceVersion) *install.CSVRuleChecker { + var ruleChecker func(*v1alpha1.ClusterServiceVersion) *install.CSVRuleChecker + a.ruleCheckerLock.RLock() + ruleChecker = a.ruleChecker + a.ruleCheckerLock.RUnlock() + if ruleChecker != nil { + return ruleChecker + } + + a.ruleCheckerLock.Lock() + defer a.ruleCheckerLock.Unlock() + if a.ruleChecker != nil { + return a.ruleChecker + } + + sif := informers.NewSharedInformerFactoryWithOptions(a.opClient.KubernetesInterface(), a.resyncPeriod()) + rolesLister := sif.Rbac().V1().Roles().Lister() + roleBindingsLister := sif.Rbac().V1().RoleBindings().Lister() + clusterRolesLister := sif.Rbac().V1().ClusterRoles().Lister() + clusterRoleBindingsLister := sif.Rbac().V1().ClusterRoleBindings().Lister() + + done := make(chan struct{}) + go func() { + <-a.ctx.Done() + done <- struct{}{} + }() + sif.Start(done) + sif.WaitForCacheSync(done) + + a.ruleChecker = func(csv *v1alpha1.ClusterServiceVersion) *install.CSVRuleChecker { + return install.NewCSVRuleChecker( + rolesLister, roleBindingsLister, + clusterRolesLister, clusterRoleBindingsLister, + csv, + ) + } + return a.ruleChecker +} + func NewOperator(ctx context.Context, options ...OperatorOption) (*Operator, error) { config := defaultOperatorConfig() config.apply(options) @@ -171,6 +217,10 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat serviceAccountQuerier: scoped.NewUserDefinedServiceAccountQuerier(config.logger, config.externalClient), clientFactory: clients.NewFactory(config.restConfig), protectedCopiedCSVNamespaces: config.protectedCopiedCSVNamespaces, + resyncPeriod: config.resyncPeriod, + ruleCheckerLock: &sync.RWMutex{}, + ctx: ctx, + informersFiltered: canFilter, } informersByNamespace := map[string]*plugins.Informers{} @@ -447,10 +497,21 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat } } - labelObjects := func(gvr schema.GroupVersionResource, informer cache.SharedIndexInformer, sync queueinformer.LegacySyncHandler) error { + complete := map[schema.GroupVersionResource][]bool{} + completeLock := &sync.Mutex{} + + labelObjects := func(gvr schema.GroupVersionResource, informer cache.SharedIndexInformer, sync func(done func() bool) queueinformer.LegacySyncHandler) error { if canFilter { return nil } + var idx int + if _, exists := complete[gvr]; exists { + idx = len(complete[gvr]) + complete[gvr] = append(complete[gvr], false) + } else { + idx = 0 + complete[gvr] = []bool{false} + } queue := workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{ Name: gvr.String(), }) @@ -459,7 +520,18 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat queueinformer.WithQueue(queue), queueinformer.WithLogger(op.logger), queueinformer.WithInformer(informer), - queueinformer.WithSyncer(sync.ToSyncer()), + queueinformer.WithSyncer(sync(func() bool { + completeLock.Lock() + complete[gvr][idx] = true + allDone := true + for _, items := range complete { + for _, done := range items { + allDone = allDone && done + } + } + completeLock.Unlock() + return allDone + }).ToSyncer()), ) if err != nil { return err @@ -475,6 +547,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat deploymentsgvk := appsv1.SchemeGroupVersion.WithResource("deployments") if err := labelObjects(deploymentsgvk, informersByNamespace[metav1.NamespaceAll].DeploymentInformer.Informer(), labeller.ObjectLabeler[*appsv1.Deployment, *appsv1applyconfigurations.DeploymentApplyConfiguration]( ctx, op.logger, labeller.Filter(deploymentsgvk), + informersByNamespace[metav1.NamespaceAll].DeploymentInformer.Lister().List, appsv1applyconfigurations.Deployment, func(namespace string, ctx context.Context, cfg *appsv1applyconfigurations.DeploymentApplyConfiguration, opts metav1.ApplyOptions) (*appsv1.Deployment, error) { return op.opClient.KubernetesInterface().AppsV1().Deployments(namespace).Apply(ctx, cfg, opts) @@ -547,6 +620,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat clusterrolesgvk := rbacv1.SchemeGroupVersion.WithResource("clusterroles") if err := labelObjects(clusterrolesgvk, clusterRoleInformer.Informer(), labeller.ObjectLabeler[*rbacv1.ClusterRole, *rbacv1applyconfigurations.ClusterRoleApplyConfiguration]( ctx, op.logger, labeller.Filter(clusterrolesgvk), + clusterRoleInformer.Lister().List, func(name, _ string) *rbacv1applyconfigurations.ClusterRoleApplyConfiguration { return rbacv1applyconfigurations.ClusterRole(name) }, @@ -561,6 +635,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat func(clusterRole *rbacv1.ClusterRole) (string, error) { return resolver.PolicyRuleHashLabelValue(clusterRole.Rules) }, + clusterRoleInformer.Lister().List, func(name, _ string) *rbacv1applyconfigurations.ClusterRoleApplyConfiguration { return rbacv1applyconfigurations.ClusterRole(name) }, @@ -590,6 +665,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat clusterrolebindingssgvk := rbacv1.SchemeGroupVersion.WithResource("clusterrolebindings") if err := labelObjects(clusterrolebindingssgvk, clusterRoleBindingInformer.Informer(), labeller.ObjectLabeler[*rbacv1.ClusterRoleBinding, *rbacv1applyconfigurations.ClusterRoleBindingApplyConfiguration]( ctx, op.logger, labeller.Filter(clusterrolebindingssgvk), + clusterRoleBindingInformer.Lister().List, func(name, _ string) *rbacv1applyconfigurations.ClusterRoleBindingApplyConfiguration { return rbacv1applyconfigurations.ClusterRoleBinding(name) }, @@ -604,6 +680,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat func(clusterRoleBinding *rbacv1.ClusterRoleBinding) (string, error) { return resolver.RoleReferenceAndSubjectHashLabelValue(clusterRoleBinding.RoleRef, clusterRoleBinding.Subjects) }, + clusterRoleBindingInformer.Lister().List, func(name, _ string) *rbacv1applyconfigurations.ClusterRoleBindingApplyConfiguration { return rbacv1applyconfigurations.ClusterRoleBinding(name) }, @@ -614,8 +691,9 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat return nil, err } - // register namespace queueinformer - namespaceInformer := k8sInformerFactory.Core().V1().Namespaces() + // register namespace queueinformer using a new informer factory - since namespaces won't have the labels + // that other k8s objects will + namespaceInformer := informers.NewSharedInformerFactory(op.opClient.KubernetesInterface(), config.resyncPeriod()).Core().V1().Namespaces() informersByNamespace[metav1.NamespaceAll].NamespaceInformer = namespaceInformer op.lister.CoreV1().RegisterNamespaceLister(namespaceInformer.Lister()) op.nsQueueSet = workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "resolver") diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index 99dca3fd98..20e1038fb9 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -383,6 +383,7 @@ func deployment(deploymentName, namespace, serviceAccountName string, templateAn ObjectMeta: metav1.ObjectMeta{ Name: deploymentName, Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Spec: appsv1.DeploymentSpec{ Selector: &metav1.LabelSelector{ @@ -432,6 +433,7 @@ func serviceAccount(name, namespace string) *corev1.ServiceAccount { ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, } @@ -440,6 +442,9 @@ func serviceAccount(name, namespace string) *corev1.ServiceAccount { func service(name, namespace, deploymentName string, targetPort int, ownerReferences ...metav1.OwnerReference) *corev1.Service { service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, + }, Spec: corev1.ServiceSpec{ Ports: []corev1.ServicePort{ { @@ -461,6 +466,9 @@ func service(name, namespace, deploymentName string, targetPort int, ownerRefere func clusterRoleBinding(name, clusterRoleName, serviceAccountName, serviceAccountNamespace string) *rbacv1.ClusterRoleBinding { clusterRoleBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, + }, Subjects: []rbacv1.Subject{ { Kind: "ServiceAccount", @@ -482,6 +490,9 @@ func clusterRoleBinding(name, clusterRoleName, serviceAccountName, serviceAccoun func clusterRole(name string, rules []rbacv1.PolicyRule) *rbacv1.ClusterRole { clusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, + }, Rules: rules, } clusterRole.SetName(name) @@ -491,6 +502,9 @@ func clusterRole(name string, rules []rbacv1.PolicyRule) *rbacv1.ClusterRole { func role(name, namespace string, rules []rbacv1.PolicyRule) *rbacv1.Role { role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, + }, Rules: rules, } role.SetName(name) @@ -501,6 +515,9 @@ func role(name, namespace string, rules []rbacv1.PolicyRule) *rbacv1.Role { func roleBinding(name, namespace, roleName, serviceAccountName, serviceAccountNamespace string) *rbacv1.RoleBinding { roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, + }, Subjects: []rbacv1.Subject{ { Kind: "ServiceAccount", @@ -523,6 +540,9 @@ func roleBinding(name, namespace, roleName, serviceAccountName, serviceAccountNa func tlsSecret(name, namespace string, certPEM, privPEM []byte) *corev1.Secret { secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, + }, Data: map[string][]byte{ "tls.crt": certPEM, "tls.key": privPEM, @@ -846,7 +866,8 @@ func apiService(group, version, serviceName, serviceNamespace, deploymentName st func crd(name, version, group string) *apiextensionsv1.CustomResourceDefinition { return &apiextensionsv1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ - Name: name + "." + group, + Name: name + "." + group, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: group, @@ -4371,6 +4392,7 @@ func TestSyncOperatorGroups(t *testing.T) { }, Rules: permissions[0].Rules, } + role.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue roleBinding := &rbacv1.RoleBinding{ TypeMeta: metav1.TypeMeta{ @@ -4397,6 +4419,7 @@ func TestSyncOperatorGroups(t *testing.T) { Name: role.GetName(), }, } + roleBinding.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue type initial struct { operatorGroup *operatorsv1.OperatorGroup @@ -4985,6 +5008,7 @@ func TestSyncOperatorGroups(t *testing.T) { Name: "csv-role", Namespace: targetNamespace, Labels: map[string]string{ + "olm.managed": "true", "olm.copiedFrom": "operator-ns", "olm.owner": "csv1", "olm.owner.namespace": "target-ns", @@ -5006,6 +5030,7 @@ func TestSyncOperatorGroups(t *testing.T) { Name: "csv-rolebinding", Namespace: targetNamespace, Labels: map[string]string{ + "olm.managed": "true", "olm.copiedFrom": "operator-ns", "olm.owner": "csv1", "olm.owner.namespace": "target-ns", @@ -5088,6 +5113,7 @@ func TestSyncOperatorGroups(t *testing.T) { Name: "csv-role", Namespace: targetNamespace, Labels: map[string]string{ + "olm.managed": "true", "olm.copiedFrom": "operator-ns", "olm.owner": "csv1", "olm.owner.namespace": "target-ns", @@ -5109,6 +5135,7 @@ func TestSyncOperatorGroups(t *testing.T) { Name: "csv-rolebinding", Namespace: targetNamespace, Labels: map[string]string{ + "olm.managed": "true", "olm.copiedFrom": "operator-ns", "olm.owner": "csv1", "olm.owner.namespace": "target-ns", @@ -5188,6 +5215,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "csv-role", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "csv1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "ClusterServiceVersion", @@ -5207,6 +5235,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "csv-rolebinding", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "csv1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "ClusterServiceVersion", @@ -5507,17 +5536,14 @@ func TestSyncOperatorGroups(t *testing.T) { } for _, csv := range csvs.Items { - t.Logf("%s/%s", csv.Namespace, csv.Name) if csv.Status.Phase == v1alpha1.CSVPhaseInstalling { simulateSuccessfulRollout(&csv) } - t.Log("op.syncClusterServiceVersion") if err := op.syncClusterServiceVersion(&csv); err != nil { return false, fmt.Errorf("failed to syncClusterServiceVersion: %w", err) } - t.Log("op.syncCopyCSV") if err := op.syncCopyCSV(&csv); err != nil && !tt.ignoreCopyError { return false, fmt.Errorf("failed to syncCopyCSV: %w", err) } diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 92c70c649f..6eb6dbdce7 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -489,7 +489,6 @@ func (a *Operator) ensureRBACInTargetNamespace(csv *v1alpha1.ClusterServiceVersi if !ok { return fmt.Errorf("could not cast install strategy as type %T", strategyDetailsDeployment) } - ruleChecker := install.NewCSVRuleChecker(a.lister.RbacV1().RoleLister(), a.lister.RbacV1().RoleBindingLister(), a.lister.RbacV1().ClusterRoleLister(), a.lister.RbacV1().ClusterRoleBindingLister(), csv) logger := a.logger.WithField("opgroup", operatorGroup.GetName()).WithField("csv", csv.GetName()) @@ -500,7 +499,7 @@ func (a *Operator) ensureRBACInTargetNamespace(csv *v1alpha1.ClusterServiceVersi // synthesize cluster permissions to verify rbac strategyDetailsDeployment.ClusterPermissions = append(strategyDetailsDeployment.ClusterPermissions, strategyDetailsDeployment.Permissions...) strategyDetailsDeployment.Permissions = nil - permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, corev1.NamespaceAll, csv) + permMet, _, err := a.permissionStatus(strategyDetailsDeployment, corev1.NamespaceAll, csv) if err != nil { return err } @@ -655,6 +654,7 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c return err } targetRole.SetLabels(utillabels.AddLabel(targetRole.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace)) + targetRole.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue if _, err := a.opClient.CreateRole(targetRole); err != nil { return err } @@ -699,6 +699,7 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c return err } ownedRoleBinding.SetLabels(utillabels.AddLabel(ownedRoleBinding.GetLabels(), v1alpha1.CopiedLabelKey, operatorNamespace)) + ownedRoleBinding.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue if _, err := a.opClient.CreateRoleBinding(ownedRoleBinding); err != nil { return err } @@ -713,8 +714,6 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o } strategyDetailsDeployment := &csv.Spec.InstallStrategy.StrategySpec - ruleChecker := install.NewCSVRuleChecker(a.lister.RbacV1().RoleLister(), a.lister.RbacV1().RoleBindingLister(), a.lister.RbacV1().ClusterRoleLister(), a.lister.RbacV1().ClusterRoleBindingLister(), csv) - logger := a.logger.WithField("opgroup", operatorGroup.GetName()).WithField("csv", csv.GetName()) targetCSVs := make(map[string]*v1alpha1.ClusterServiceVersion) @@ -730,20 +729,20 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o if targets.Contains(ns.GetName()) { var targetCSV *v1alpha1.ClusterServiceVersion if targetCSV, err = a.copyToNamespace(©Prototype, csv.GetNamespace(), ns.GetName(), nonstatus, status); err != nil { - a.logger.WithError(err).Debug("error copying to target") + logger.WithError(err).Debug("error copying to target") continue } targetCSVs[ns.GetName()] = targetCSV } else { if err := a.pruneFromNamespace(operatorGroup.GetName(), ns.GetName()); err != nil { - a.logger.WithError(err).Debug("error pruning from old target") + logger.WithError(err).Debug("error pruning from old target") } } } targetNamespaces := operatorGroup.Status.Namespaces if targetNamespaces == nil { - a.logger.Errorf("operatorgroup '%v' should have non-nil status", operatorGroup.GetName()) + logger.Errorf("operatorgroup '%v' should have non-nil status", operatorGroup.GetName()) return nil } if len(targetNamespaces) == 1 && targetNamespaces[0] == corev1.NamespaceAll { @@ -752,7 +751,7 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o } for _, ns := range targetNamespaces { // create roles/rolebindings for each target namespace - permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, ns, csv) + permMet, _, err := a.permissionStatus(strategyDetailsDeployment, ns, csv) if err != nil { logger.WithError(err).Debug("permission status") return err diff --git a/pkg/controller/operators/olm/requirements.go b/pkg/controller/operators/olm/requirements.go index 0c288b8e36..448f7ce27f 100644 --- a/pkg/controller/operators/olm/requirements.go +++ b/pkg/controller/operators/olm/requirements.go @@ -7,15 +7,17 @@ import ( "strings" "github.com/coreos/go-semver/semver" - "github.com/sirupsen/logrus" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/operator-framework/api/pkg/operators/v1alpha1" listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/alongside" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" + "github.com/sirupsen/logrus" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" ) func (a *Operator) minKubeVersionStatus(name string, minKubeVersion string) (met bool, statuses []v1alpha1.RequirementStatus) { @@ -254,7 +256,7 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *v1alpha1.Strateg } // permissionStatus checks whether the given CSV's RBAC requirements are met in its namespace -func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.StrategyDetailsDeployment, ruleChecker install.RuleChecker, targetNamespace string, csv *v1alpha1.ClusterServiceVersion) (bool, []v1alpha1.RequirementStatus, error) { +func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.StrategyDetailsDeployment, targetNamespace string, csv *v1alpha1.ClusterServiceVersion) (bool, []v1alpha1.RequirementStatus, error) { statusesSet := map[string]v1alpha1.RequirementStatus{} checkPermissions := func(permissions []v1alpha1.StrategyDeploymentPermissions, namespace string) (bool, error) { @@ -296,6 +298,57 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy } // Check if PolicyRules are satisfied + if a.informersFiltered { + // we don't hold the whole set of RBAC in memory, so we can't use the authorizer: + // check for rules we would have created ourselves first + var err error + var permissionMet bool + if namespace == metav1.NamespaceAll { + permissionMet, err = permissionsPreviouslyCreated[*rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding]( + perm, csv, + a.lister.RbacV1().ClusterRoleLister().List, a.lister.RbacV1().ClusterRoleBindingLister().List, + ) + } else { + permissionMet, err = permissionsPreviouslyCreated[*rbacv1.Role, *rbacv1.RoleBinding]( + perm, csv, + a.lister.RbacV1().RoleLister().List, a.lister.RbacV1().RoleBindingLister().List, + ) + } + if err != nil { + return false, err + } + if permissionMet { + // OLM previously made all the permissions we need, exit early + for _, rule := range perm.Rules { + dependent := v1alpha1.DependentStatus{ + Group: "rbac.authorization.k8s.io", + Kind: "PolicyRule", + Version: "v1", + Status: v1alpha1.DependentStatusReasonSatisfied, + } + marshalled, err := json.Marshal(rule) + if err != nil { + dependent.Status = v1alpha1.DependentStatusReasonNotSatisfied + dependent.Message = "rule unmarshallable" + status.Dependents = append(status.Dependents, dependent) + continue + } + + var scope string + if namespace == metav1.NamespaceAll { + scope = "cluster" + } else { + scope = "namespaced" + } + dependent.Message = fmt.Sprintf("%s rule:%s", scope, marshalled) + status.Dependents = append(status.Dependents, dependent) + } + continue + } + } + // if we have not filtered our informers or if we were unable to detect the correct permissions, we have + // no choice but to page in the world and see if the user pre-created permissions for this CSV + ruleChecker := a.getRuleChecker()(csv) for _, rule := range perm.Rules { dependent := v1alpha1.DependentStatus{ Group: "rbac.authorization.k8s.io", @@ -358,6 +411,59 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy return permMet && clusterPermMet, statuses, nil } +func permissionsPreviouslyCreated[T, U metav1.Object]( + permission v1alpha1.StrategyDeploymentPermissions, + csv *v1alpha1.ClusterServiceVersion, + listRoles func(labels.Selector) ([]T, error), + listBindings func(labels.Selector) ([]U, error), +) (bool, error) { + // first, find the (cluster)role + ruleHash, err := resolver.PolicyRuleHashLabelValue(permission.Rules) + if err != nil { + return false, fmt.Errorf("failed to hash permission rules: %w", err) + } + roleSelectorMap := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind) + roleSelectorMap[resolver.ContentHashLabelKey] = ruleHash + roleSelectorSet := labels.Set{} + for key, value := range roleSelectorMap { + roleSelectorSet[key] = value + } + roleSelector := labels.SelectorFromSet(roleSelectorSet) + roles, err := listRoles(roleSelector) + if err != nil { + return false, err + } + + if len(roles) == 0 { + return false, nil + } + + // then, find the (cluster)rolebinding, if we found the role + bindingHash, err := resolver.RoleReferenceAndSubjectHashLabelValue(rbacv1.RoleRef{ + Kind: "Role", + Name: roles[0].GetName(), + APIGroup: rbacv1.GroupName, + }, + []rbacv1.Subject{{ + Kind: "ServiceAccount", + Name: permission.ServiceAccountName, + Namespace: csv.GetNamespace(), + }}, + ) + if err != nil { + return false, fmt.Errorf("failed to hash binding content: %w", err) + } + bindingSelectorMap := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind) + bindingSelectorMap[resolver.ContentHashLabelKey] = bindingHash + bindingSelectorSet := labels.Set{} + for key, value := range bindingSelectorMap { + bindingSelectorSet[key] = value + } + bindingSelector := labels.SelectorFromSet(bindingSelectorSet) + bindings, err := listBindings(bindingSelector) + return len(roles) > 0 && len(bindings) > 0, err +} + // requirementAndPermissionStatus returns the aggregate requirement and permissions statuses for the given CSV func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVersion) (bool, []v1alpha1.RequirementStatus, error) { allReqStatuses := []v1alpha1.RequirementStatus{} @@ -383,14 +489,7 @@ func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVe reqMet, reqStatuses := a.requirementStatus(strategyDetailsDeployment, csv) allReqStatuses = append(allReqStatuses, reqStatuses...) - rbacLister := a.lister.RbacV1() - roleLister := rbacLister.RoleLister() - roleBindingLister := rbacLister.RoleBindingLister() - clusterRoleLister := rbacLister.ClusterRoleLister() - clusterRoleBindingLister := rbacLister.ClusterRoleBindingLister() - - ruleChecker := install.NewCSVRuleChecker(roleLister, roleBindingLister, clusterRoleLister, clusterRoleBindingLister, csv) - permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace(), csv) + permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, csv.GetNamespace(), csv) if err != nil { return false, nil, err } diff --git a/pkg/controller/operators/olm/requirements_test.go b/pkg/controller/operators/olm/requirements_test.go index e3712d695d..32239963ea 100644 --- a/pkg/controller/operators/olm/requirements_test.go +++ b/pkg/controller/operators/olm/requirements_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -80,6 +81,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { Name: "sa", Namespace: namespace, UID: types.UID("sa"), + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, OwnerReferences: []metav1.OwnerReference{ { Kind: v1alpha1.ClusterServiceVersionKind, @@ -92,6 +94,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "role", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -105,6 +108,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "roleBinding", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -122,7 +126,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: "clusterRole", + Name: "clusterRole", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -133,7 +138,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) { }, &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: "clusterRoleBinding", + Name: "clusterRoleBinding", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -224,6 +230,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { Name: "sa", Namespace: namespace, UID: types.UID("sa"), + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, OwnerReferences: []metav1.OwnerReference{ { Kind: v1alpha1.ClusterServiceVersionKind, @@ -236,6 +243,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "role", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -249,6 +257,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "roleBinding", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -266,7 +275,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: "clusterRole", + Name: "clusterRole", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -277,7 +287,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) { }, &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: "clusterRoleBinding", + Name: "clusterRoleBinding", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -368,6 +379,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { Name: "sa", Namespace: namespace, UID: types.UID("sa"), + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, OwnerReferences: []metav1.OwnerReference{ { Kind: v1alpha1.ClusterServiceVersionKind, @@ -380,6 +392,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "role", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -393,6 +406,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "roleBinding", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -410,7 +424,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) { }, &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: "clusterRole", + Name: "clusterRole", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -421,7 +436,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) { }, &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: "clusterRoleBinding", + Name: "clusterRoleBinding", + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -491,6 +507,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { Name: "sa", Namespace: namespace, UID: types.UID("sa"), + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, OwnerReferences: []metav1.OwnerReference{ { Kind: v1alpha1.ClusterServiceVersionKind, @@ -503,6 +520,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "role", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Rules: []rbacv1.PolicyRule{ { @@ -516,6 +534,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "roleBinding", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, }, Subjects: []rbacv1.Subject{ { @@ -770,6 +789,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { Name: "sa", Namespace: namespace, UID: types.UID("sa"), + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, OwnerReferences: []metav1.OwnerReference{ { Kind: v1alpha1.ClusterServiceVersionKind, @@ -824,6 +844,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { Name: "sa", Namespace: namespace, UID: types.UID("sa"), + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, OwnerReferences: []metav1.OwnerReference{ { Kind: v1alpha1.SubscriptionKind, // arbitrary non-CSV kind @@ -877,6 +898,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "sa", Namespace: namespace, + Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}, UID: types.UID("sa"), }, }, diff --git a/pkg/lib/scoped/syncer.go b/pkg/lib/scoped/syncer.go index dec0a0d0e7..6a8921b2ea 100644 --- a/pkg/lib/scoped/syncer.go +++ b/pkg/lib/scoped/syncer.go @@ -5,12 +5,14 @@ import ( "fmt" "reflect" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - meta "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + corev1applyconfigurations "k8s.io/client-go/applyconfigurations/core/v1" "k8s.io/client-go/tools/reference" "k8s.io/utils/clock" @@ -76,6 +78,14 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup return } + // A service account has been specified, but likely does not have the labels we expect it to have so it will + // show up in our listers, so let's add that and queue again later + config := corev1applyconfigurations.ServiceAccount(serviceAccountName, namespace) + config.Labels = map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue} + if _, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Apply(context.TODO(), config, metav1.ApplyOptions{FieldManager: "operator-lifecycle-manager"}); err != nil { + return out, fmt.Errorf("failed to apply labels[%s]=%s to serviceaccount %s/%s: %w", install.OLMManagedLabelKey, install.OLMManagedLabelValue, namespace, serviceAccountName, err) + } + // A service account has been specified, we need to update the status. sa, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(context.TODO(), serviceAccountName, metav1.GetOptions{}) if err != nil { From 7b31c001ee8a3b7577996e47e5ec53574bca97ee Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 11 Sep 2023 07:27:04 -0600 Subject: [PATCH 03/17] *: track and label user-provided service-accounts Signed-off-by: Steve Kuznetsov --- pkg/controller/operators/catalog/operator.go | 15 ++++++-- pkg/controller/operators/labeller/filters.go | 35 ++++++++++++++++--- pkg/controller/operators/olm/operator.go | 2 +- pkg/controller/operators/olm/operator_test.go | 2 ++ pkg/lib/scoped/syncer.go | 16 ++++----- 5 files changed, 55 insertions(+), 15 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 613bfd38a7..e486dc8752 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -185,7 +185,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo return nil, err } - canFilter, err := labeller.Validate(ctx, logger, metadataClient) + canFilter, err := labeller.Validate(ctx, logger, metadataClient, crClient) if err != nil { return nil, err } @@ -490,7 +490,18 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo serviceaccountsgvk := corev1.SchemeGroupVersion.WithResource("serviceaccounts") if err := labelObjects(serviceaccountsgvk, serviceAccountInformer.Informer(), labeller.ObjectLabeler[*corev1.ServiceAccount, *corev1applyconfigurations.ServiceAccountApplyConfiguration]( - ctx, op.logger, labeller.Filter(serviceaccountsgvk), + ctx, op.logger, labeller.ServiceAccountFilter(func(namespace, name string) bool { + operatorGroups, err := operatorGroupInformer.Lister().OperatorGroups(namespace).List(labels.Everything()) + if err != nil { + return false + } + for _, operatorGroup := range operatorGroups { + if operatorGroup.Spec.ServiceAccountName == name { + return true + } + } + return false + }), serviceAccountInformer.Lister().List, corev1applyconfigurations.ServiceAccount, func(namespace string, ctx context.Context, cfg *corev1applyconfigurations.ServiceAccountApplyConfiguration, opts metav1.ApplyOptions) (*corev1.ServiceAccount, error) { diff --git a/pkg/controller/operators/labeller/filters.go b/pkg/controller/operators/labeller/filters.go index c5f5010622..11e04a24d8 100644 --- a/pkg/controller/operators/labeller/filters.go +++ b/pkg/controller/operators/labeller/filters.go @@ -6,6 +6,7 @@ import ( "strings" "sync" + operators "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler" "github.com/sirupsen/logrus" "golang.org/x/sync/errgroup" @@ -16,6 +17,8 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/metadata" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/alongside" @@ -49,15 +52,18 @@ func JobFilter(getConfigMap func(namespace, name string) (metav1.Object, error)) } } +func ServiceAccountFilter(isServiceAccountReferenced func(namespace, name string) bool) func(object metav1.Object) bool { + return func(object metav1.Object) bool { + return HasOLMOwnerRef(object) || HasOLMLabel(object) || isServiceAccountReferenced(object.GetNamespace(), object.GetName()) + } +} + var filters = map[schema.GroupVersionResource]func(metav1.Object) bool{ corev1.SchemeGroupVersion.WithResource("services"): HasOLMOwnerRef, corev1.SchemeGroupVersion.WithResource("pods"): func(object metav1.Object) bool { _, ok := object.GetLabels()[reconciler.CatalogSourceLabelKey] return ok }, - corev1.SchemeGroupVersion.WithResource("serviceaccounts"): func(object metav1.Object) bool { - return HasOLMOwnerRef(object) || HasOLMLabel(object) - }, appsv1.SchemeGroupVersion.WithResource("deployments"): HasOLMOwnerRef, rbacv1.SchemeGroupVersion.WithResource("roles"): HasOLMOwnerRef, rbacv1.SchemeGroupVersion.WithResource("rolebindings"): HasOLMOwnerRef, @@ -73,7 +79,7 @@ var filters = map[schema.GroupVersionResource]func(metav1.Object) bool{ }, } -func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface) (bool, error) { +func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface, operatorClient operators.Interface) (bool, error) { okLock := sync.Mutex{} ok := true g, ctx := errgroup.WithContext(ctx) @@ -96,6 +102,27 @@ func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadat return previous != nil && previous(object) && ContentHashFilter(object) } } + + operatorGroups, err := operatorClient.OperatorsV1().OperatorGroups(metav1.NamespaceAll).List(ctx, metav1.ListOptions{}) + if err != nil { + return false, err + } + userProvidedServiceAccounts := sets.New[types.NamespacedName]() + for _, operatorGroup := range operatorGroups.Items { + if operatorGroup.Spec.ServiceAccountName != "" { + userProvidedServiceAccounts.Insert(types.NamespacedName{ + Namespace: operatorGroup.Namespace, + Name: operatorGroup.Spec.ServiceAccountName, + }) + } + } + allFilters[corev1.SchemeGroupVersion.WithResource("serviceaccounts")] = ServiceAccountFilter(func(namespace, name string) bool { + return userProvidedServiceAccounts.Has(types.NamespacedName{ + Namespace: namespace, + Name: name, + }) + }) + for gvr, filter := range allFilters { gvr, filter := gvr, filter g.Go(func() error { diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 2ba02129a9..de028b69f7 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -186,7 +186,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat return nil, err } - canFilter, err := labeller.Validate(ctx, config.logger, config.metadataClient) + canFilter, err := labeller.Validate(ctx, config.logger, config.metadataClient, config.externalClient) if err != nil { return nil, err } diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index 20e1038fb9..bd374e3b06 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -4364,6 +4364,7 @@ func TestSyncOperatorGroups(t *testing.T) { annotatedDeployment := ownedDeployment.DeepCopy() annotatedDeployment.Spec.Template.SetAnnotations(map[string]string{operatorsv1.OperatorGroupTargetsAnnotationKey: operatorNamespace + "," + targetNamespace, operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}) annotatedDeployment.SetLabels(map[string]string{ + "olm.managed": "true", "olm.owner": "csv1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "ClusterServiceVersion", @@ -4373,6 +4374,7 @@ func TestSyncOperatorGroups(t *testing.T) { annotatedGlobalDeployment := ownedDeployment.DeepCopy() annotatedGlobalDeployment.Spec.Template.SetAnnotations(map[string]string{operatorsv1.OperatorGroupTargetsAnnotationKey: "", operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}) annotatedGlobalDeployment.SetLabels(map[string]string{ + "olm.managed": "true", "olm.owner": "csv1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "ClusterServiceVersion", diff --git a/pkg/lib/scoped/syncer.go b/pkg/lib/scoped/syncer.go index 6a8921b2ea..f69505beff 100644 --- a/pkg/lib/scoped/syncer.go +++ b/pkg/lib/scoped/syncer.go @@ -78,14 +78,6 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup return } - // A service account has been specified, but likely does not have the labels we expect it to have so it will - // show up in our listers, so let's add that and queue again later - config := corev1applyconfigurations.ServiceAccount(serviceAccountName, namespace) - config.Labels = map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue} - if _, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Apply(context.TODO(), config, metav1.ApplyOptions{FieldManager: "operator-lifecycle-manager"}); err != nil { - return out, fmt.Errorf("failed to apply labels[%s]=%s to serviceaccount %s/%s: %w", install.OLMManagedLabelKey, install.OLMManagedLabelValue, namespace, serviceAccountName, err) - } - // A service account has been specified, we need to update the status. sa, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(context.TODO(), serviceAccountName, metav1.GetOptions{}) if err != nil { @@ -108,6 +100,14 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup return } + // A service account has been specified, but likely does not have the labels we expect it to have so it will + // show up in our listers, so let's add that and queue again later + config := corev1applyconfigurations.ServiceAccount(serviceAccountName, namespace) + config.Labels = map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue} + if _, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Apply(context.TODO(), config, metav1.ApplyOptions{FieldManager: "operator-lifecycle-manager"}); err != nil { + return out, fmt.Errorf("failed to apply labels[%s]=%s to serviceaccount %s/%s: %w", install.OLMManagedLabelKey, install.OLMManagedLabelValue, namespace, serviceAccountName, err) + } + ref, err := reference.GetReference(s.scheme, sa) if err != nil { return From 0ddd69757fe9a911c174f14f7c36dc1d63b52b54 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 21 Sep 2023 06:48:34 -0600 Subject: [PATCH 04/17] tests: label RBAC as needed Signed-off-by: Steve Kuznetsov --- pkg/controller/operators/olm/operator_test.go | 24 +++++++++++++++++++ pkg/controller/operators/olm/operatorgroup.go | 1 + 2 files changed, 25 insertions(+) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index bd374e3b06..e6dbe50146 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -4587,6 +4587,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4597,6 +4598,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4607,6 +4609,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4648,6 +4651,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1-admin", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4658,6 +4662,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1-view", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4668,6 +4673,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1-edit", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4686,6 +4692,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4696,6 +4703,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4706,6 +4714,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4716,6 +4725,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1-admin", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4726,6 +4736,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1-view", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4736,6 +4747,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "operator-group-1-edit", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4777,6 +4789,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns-bob", "olm.owner.kind": "OperatorGroup", @@ -4788,6 +4801,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-5", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4800,6 +4814,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroupKind", @@ -4818,6 +4833,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4829,6 +4845,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4839,6 +4856,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4882,6 +4900,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4892,6 +4911,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4902,6 +4922,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4919,6 +4940,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.admin-8rdAjL0E35JMMAkOqYmoorzjpIIihfnj3DcgDU", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4929,6 +4951,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.edit-9lBEUxqAYE7CX7wZfFEPYutTfQTo43WarB08od", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", @@ -4939,6 +4962,7 @@ func TestSyncOperatorGroups(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "olm.og.operator-group-1.view-1l6ymczPK5SceF4d0DCtAnWZuvmKn6s8oBUxHr", Labels: map[string]string{ + "olm.managed": "true", "olm.owner": "operator-group-1", "olm.owner.namespace": "operator-ns", "olm.owner.kind": "OperatorGroup", diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 6eb6dbdce7..bd82c860af 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -1052,6 +1052,7 @@ func (a *Operator) ensureOpGroupClusterRole(op *operatorsv1.OperatorGroup, suffi if err := ownerutil.AddOwnerLabels(clusterRole, op); err != nil { return err } + clusterRole.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue a.logger.Infof("creating cluster role: %s owned by operator group: %s/%s", clusterRole.GetName(), op.GetNamespace(), op.GetName()) _, err = a.opClient.KubernetesInterface().RbacV1().ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}) From fcbe1883b4111b6b1e694f8e7d56b9f1df29846c Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 21 Sep 2023 10:18:57 -0600 Subject: [PATCH 05/17] olm: look for rbac we create literally Signed-off-by: Steve Kuznetsov --- pkg/controller/install/certresources.go | 12 ++++- pkg/controller/operators/olm/apiservices.go | 59 +++++---------------- 2 files changed, 22 insertions(+), 49 deletions(-) diff --git a/pkg/controller/install/certresources.go b/pkg/controller/install/certresources.go index 1a2dcbca7d..8d50b96643 100644 --- a/pkg/controller/install/certresources.go +++ b/pkg/controller/install/certresources.go @@ -456,7 +456,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo Name: "system:auth-delegator", }, } - authDelegatorClusterRoleBinding.SetName(service.GetName() + "-system:auth-delegator") + authDelegatorClusterRoleBinding.SetName(AuthDelegatorClusterRoleBindingName(service.GetName())) authDelegatorClusterRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) existingAuthDelegatorClusterRoleBinding, err := i.strategyClient.GetOpLister().RbacV1().ClusterRoleBindingLister().Get(authDelegatorClusterRoleBinding.GetName()) @@ -504,7 +504,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo Name: "extension-apiserver-authentication-reader", }, } - authReaderRoleBinding.SetName(service.GetName() + "-auth-reader") + authReaderRoleBinding.SetName(AuthReaderRolebindingName(service.GetName())) authReaderRoleBinding.SetNamespace(KubeSystem) authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) @@ -543,6 +543,14 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo return &depSpec, caPEM, nil } +func AuthDelegatorClusterRoleBindingName(serviceName string) string { + return serviceName + "-system:auth-delegator" +} + +func AuthReaderRolebindingName(serviceName string) string { + return serviceName + "-auth-reader" +} + func SetCAAnnotation(depSpec *appsv1.DeploymentSpec, caHash string) { if len(depSpec.Template.ObjectMeta.GetAnnotations()) == 0 { depSpec.Template.ObjectMeta.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash}) diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go index cb052f510f..23f9d8751e 100644 --- a/pkg/controller/operators/olm/apiservices.go +++ b/pkg/controller/operators/olm/apiservices.go @@ -6,7 +6,6 @@ import ( log "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" - rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -47,7 +46,6 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion, }) errs := []error{} - ruleChecker := install.NewCSVRuleChecker(a.lister.RbacV1().RoleLister(), a.lister.RbacV1().RoleBindingLister(), a.lister.RbacV1().ClusterRoleLister(), a.lister.RbacV1().ClusterRoleBindingLister(), csv) for _, desc := range csv.GetOwnedAPIServiceDescriptions() { apiServiceName := desc.GetName() logger := logger.WithFields(log.Fields{ @@ -164,60 +162,27 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion, if serviceAccountName == "" { serviceAccountName = "default" } - serviceAccount, err := a.lister.CoreV1().ServiceAccountLister().ServiceAccounts(deployment.GetNamespace()).Get(serviceAccountName) + _, err = a.opClient.KubernetesInterface().CoreV1().ServiceAccounts(deployment.GetNamespace()).Get(context.TODO(), serviceAccountName, metav1.GetOptions{}) if err != nil { - logger.WithField("serviceaccount", serviceAccountName).Warnf("could not retrieve ServiceAccount") + logger.WithError(err).WithField("serviceaccount", serviceAccountName).Warnf("could not retrieve ServiceAccount") errs = append(errs, err) - continue } - // Ensure RBAC permissions for the APIService are correct - rulesMap := map[string][]rbacv1.PolicyRule{ - // Serving cert Secret Rule - csv.GetNamespace(): { - { - Verbs: []string{"get"}, - APIGroups: []string{""}, - Resources: []string{"secrets"}, - ResourceNames: []string{secret.GetName()}, - }, - }, - install.KubeSystem: {}, - metav1.NamespaceAll: {}, + if _, err := a.lister.RbacV1().RoleLister().Roles(secret.GetNamespace()).Get(secret.GetName()); err != nil { + logger.WithError(err).Warnf("could not retrieve role %s/%s", secret.GetNamespace(), secret.GetName()) + errs = append(errs, err) } - - // extension-apiserver-authentication-reader - authReaderRole, err := a.lister.RbacV1().RoleLister().Roles(install.KubeSystem).Get("extension-apiserver-authentication-reader") - if err != nil { - logger.Warnf("could not retrieve Role extension-apiserver-authentication-reader") + if _, err := a.lister.RbacV1().RoleBindingLister().RoleBindings(secret.GetNamespace()).Get(secret.GetName()); err != nil { + logger.WithError(err).Warnf("could not retrieve role binding %s/%s", secret.GetNamespace(), secret.GetName()) errs = append(errs, err) - continue } - rulesMap[install.KubeSystem] = append(rulesMap[install.KubeSystem], authReaderRole.Rules...) - - // system:auth-delegator - authDelegatorClusterRole, err := a.lister.RbacV1().ClusterRoleLister().Get("system:auth-delegator") - if err != nil { - logger.Warnf("could not retrieve ClusterRole system:auth-delegator") + if _, err := a.lister.RbacV1().ClusterRoleBindingLister().Get(install.AuthDelegatorClusterRoleBindingName(service.GetName())); err != nil { + logger.WithError(err).Warnf("could not retrieve auth delegator cluster role binding %s", install.AuthDelegatorClusterRoleBindingName(service.GetName())) errs = append(errs, err) - continue } - rulesMap[metav1.NamespaceAll] = append(rulesMap[metav1.NamespaceAll], authDelegatorClusterRole.Rules...) - - for namespace, rules := range rulesMap { - for _, rule := range rules { - satisfied, err := ruleChecker.RuleSatisfied(serviceAccount, namespace, rule) - if err != nil { - logger.WithField("rule", fmt.Sprintf("%+v", rule)).Warnf("error checking Rule") - errs = append(errs, err) - continue - } - if !satisfied { - logger.WithField("rule", fmt.Sprintf("%+v", rule)).Warnf("Rule not satisfied") - errs = append(errs, fmt.Errorf("rule %+v not satisfied", rule)) - continue - } - } + if _, err := a.lister.RbacV1().RoleBindingLister().RoleBindings(install.KubeSystem).Get(install.AuthReaderRolebindingName(service.GetName())); err != nil { + logger.WithError(err).Warnf("could not retrieve role binding %s/%s", install.KubeSystem, install.AuthReaderRolebindingName(service.GetName())) + errs = append(errs, err) } } From 7bd684f4fb2ce3d0df7386cec9f73fa78a46d01e Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 21 Sep 2023 10:19:13 -0600 Subject: [PATCH 06/17] test/e2e: more fixes Signed-off-by: Steve Kuznetsov --- test/e2e/subscription_e2e_test.go | 6 ++++++ test/e2e/user_defined_sa_test.go | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index a3421b12dd..f4804ce651 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "os" "path/filepath" "reflect" "strings" @@ -2904,6 +2905,11 @@ func fetchSubscription(crc versioned.Interface, namespace, name string, checker func buildSubscriptionCleanupFunc(crc versioned.Interface, subscription *operatorsv1alpha1.Subscription) cleanupFunc { return func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of install plan for subscription %s/%s...\n", subscription.GetNamespace(), subscription.GetName()) + return + } + if installPlanRef := subscription.Status.InstallPlanRef; installPlanRef != nil { installPlan, err := crc.OperatorsV1alpha1().InstallPlans(subscription.GetNamespace()).Get(context.Background(), installPlanRef.Name, metav1.GetOptions{}) if err == nil { diff --git a/test/e2e/user_defined_sa_test.go b/test/e2e/user_defined_sa_test.go index 1e05304b9a..51358fea9c 100644 --- a/test/e2e/user_defined_sa_test.go +++ b/test/e2e/user_defined_sa_test.go @@ -3,6 +3,7 @@ package e2e import ( "context" "fmt" + "os" "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" @@ -242,6 +243,10 @@ func newServiceAccount(client operatorclient.ClientInterface, namespace, name st Expect(sa).ToNot(BeNil()) cleanup = func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of service account %s/%s...\n", sa.GetNamespace(), sa.GetName()) + return + } err := client.KubernetesInterface().CoreV1().ServiceAccounts(sa.GetNamespace()).Delete(context.TODO(), sa.GetName(), metav1.DeleteOptions{}) Expect(err).ToNot(HaveOccurred()) } @@ -268,6 +273,10 @@ func newOperatorGroupWithServiceAccount(client versioned.Interface, namespace, n Expect(og).ToNot(BeNil()) cleanup = func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of operator group %s/%s...\n", og.GetNamespace(), og.GetName()) + return + } err := client.OperatorsV1().OperatorGroups(og.GetNamespace()).Delete(context.TODO(), og.GetName(), metav1.DeleteOptions{}) Expect(err).ToNot(HaveOccurred()) } @@ -538,6 +547,14 @@ func grantPermission(t GinkgoTInterface, client operatorclient.ClientInterface, require.NoError(t, err) cleanup = func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of role %s/%s...\n", role.GetNamespace(), role.GetName()) + fmt.Printf("Skipping cleanup of role binding %s/%s...\n", binding.GetNamespace(), binding.GetName()) + fmt.Printf("Skipping cleanup of cluster role %s...\n", clusterrole.GetName()) + fmt.Printf("Skipping cleanup of cluster role binding %s...\n", clusterbinding.GetName()) + return + } + err := client.KubernetesInterface().RbacV1().Roles(role.GetNamespace()).Delete(context.TODO(), role.GetName(), metav1.DeleteOptions{}) require.NoError(t, err) From 608e55f159eb8c52cb8662bceb892da87b1d84bc Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 21 Sep 2023 11:29:59 -0600 Subject: [PATCH 07/17] catalog: don't treat permissions errors as terminal Signed-off-by: Steve Kuznetsov --- pkg/controller/operators/catalog/operator.go | 8 +++++--- test/e2e/installplan_e2e_test.go | 21 +++++++++++++++----- test/e2e/user_defined_sa_test.go | 9 ++++----- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index e486dc8752..17cebe0cdf 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -2031,13 +2031,15 @@ func transitionInstallPlanState(log logrus.FieldLogger, transitioner installPlan } log.Debug("attempting to install") if err := transitioner.ExecutePlan(out); err != nil { - if now.Sub(out.Status.StartTime.Time) >= timeout { + if apierrors.IsForbidden(err) || now.Sub(out.Status.StartTime.Time) < timeout { + // forbidden problems are never terminal since we don't know when a user might provide + // the service account they specified with more permissions + out.Status.Message = fmt.Sprintf("retrying execution due to error: %s", err.Error()) + } else { out.Status.SetCondition(v1alpha1.ConditionFailed(v1alpha1.InstallPlanInstalled, v1alpha1.InstallPlanReasonComponentFailed, err.Error(), &now)) out.Status.Phase = v1alpha1.InstallPlanPhaseFailed out.Status.Message = err.Error() - } else { - out.Status.Message = fmt.Sprintf("retrying execution due to error: %s", err.Error()) } return out, err } else if !out.Status.NeedsRequeue() { diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 2de113cb25..c56bf19f11 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -3638,11 +3638,9 @@ var _ = Describe("Install Plan", func() { } Expect(ctx.Ctx().Client().Status().Update(context.Background(), newPlan)).To(Succeed()) - newKey := client.ObjectKeyFromObject(newPlan) - - Eventually(func() (*operatorsv1alpha1.InstallPlan, error) { - return newPlan, ctx.Ctx().Client().Get(context.Background(), newKey, newPlan) - }).Should(HavePhase(operatorsv1alpha1.InstallPlanPhaseFailed)) + ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource "services" in API group`) + _, err = fetchInstallPlanWithNamespace(GinkgoT(), crc, newPlan.Name, newPlan.Namespace, ipPhaseCheckerFunc) + require.NoError(GinkgoT(), err) Expect(client.IgnoreNotFound(ctx.Ctx().Client().Delete(context.Background(), &crd))).To(Succeed()) Eventually(func() error { @@ -3928,6 +3926,19 @@ func validateCRDVersions(t GinkgoTInterface, c operatorclient.ClientInterface, n require.Equal(t, 0, len(expectedVersions), "Actual CRD versions do not match expected") } +func buildInstallPlanMessageCheckFunc(substring string) checkInstallPlanFunc { + var lastMessage string + lastTime := time.Now() + return func(fip *operatorsv1alpha1.InstallPlan) bool { + if fip.Status.Message != lastMessage { + ctx.Ctx().Logf("waiting %s for installplan %s/%s to have message substring %s, have message %s", time.Since(lastTime), fip.Namespace, fip.Name, substring, fip.Status.Phase) + lastMessage = fip.Status.Message + lastTime = time.Now() + } + return strings.Contains(fip.Status.Message, substring) + } +} + func buildInstallPlanPhaseCheckFunc(phases ...operatorsv1alpha1.InstallPlanPhase) checkInstallPlanFunc { var lastPhase operatorsv1alpha1.InstallPlanPhase lastTime := time.Now() diff --git a/test/e2e/user_defined_sa_test.go b/test/e2e/user_defined_sa_test.go index 51358fea9c..1dce545163 100644 --- a/test/e2e/user_defined_sa_test.go +++ b/test/e2e/user_defined_sa_test.go @@ -83,7 +83,7 @@ var _ = Describe("User defined service account", func() { By("We expect the InstallPlan to be in status: Failed.") ipName := subscription.Status.Install.Name - ipPhaseCheckerFunc := buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseFailed) + ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource`) ipGot, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, ipName, generatedNamespace.GetName(), ipPhaseCheckerFunc) require.NoError(GinkgoT(), err) @@ -186,12 +186,11 @@ var _ = Describe("User defined service account", func() { require.NoError(GinkgoT(), err) require.NotNil(GinkgoT(), subscription) - By("We expect the InstallPlan to be in status: Failed.") + By("We expect the InstallPlan to expose the permissions error.") ipNameOld := subscription.Status.InstallPlanRef.Name - ipPhaseCheckerFunc := buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseFailed) - ipGotOld, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, ipNameOld, generatedNamespace.GetName(), ipPhaseCheckerFunc) + ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource "clusterserviceversions" in API group "operators.coreos.com" in the namespace`) + _, err = fetchInstallPlanWithNamespace(GinkgoT(), crc, ipNameOld, generatedNamespace.GetName(), ipPhaseCheckerFunc) require.NoError(GinkgoT(), err) - require.Equal(GinkgoT(), v1alpha1.InstallPlanPhaseFailed, ipGotOld.Status.Phase) By("Grant permission now and this should trigger an retry of InstallPlan.") cleanupPerm := grantPermission(GinkgoT(), c, generatedNamespace.GetName(), saName) From a30fcf2e53cfd64636b5ade33645182c03f48c50 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Sat, 23 Sep 2023 07:19:37 -0600 Subject: [PATCH 08/17] test/e2e: stop failing on teardown The issue for this is open for years and it's not super interesting to go debug it. The test threads will exit when the test process does. Having teardown fail means none of the other tests run for me. Signed-off-by: Steve Kuznetsov --- pkg/controller/operators/openshift/suite_test.go | 2 +- pkg/controller/operators/suite_test.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/controller/operators/openshift/suite_test.go b/pkg/controller/operators/openshift/suite_test.go index da10439784..e639720b5c 100644 --- a/pkg/controller/operators/openshift/suite_test.go +++ b/pkg/controller/operators/openshift/suite_test.go @@ -108,5 +108,5 @@ var _ = BeforeSuite(func() { var _ = AfterSuite(func() { By("tearing down the test environment") close(syncCh) - Expect(testEnv.Stop()).To(Succeed()) + testEnv.Stop() }) diff --git a/pkg/controller/operators/suite_test.go b/pkg/controller/operators/suite_test.go index 2ddfb18ae8..c75cd69b9f 100644 --- a/pkg/controller/operators/suite_test.go +++ b/pkg/controller/operators/suite_test.go @@ -162,8 +162,7 @@ var _ = AfterSuite(func() { ctx.Done() By("tearing down the test environment") - err := testEnv.Stop() - Expect(err).ToNot(HaveOccurred()) + testEnv.Stop() }) func newOperator(name string) *decorators.Operator { From 0a876efac096c9f148593b9e5872b63760fcee59 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Sat, 23 Sep 2023 07:20:32 -0600 Subject: [PATCH 09/17] olm: apparently there's a whole other controller framework That also starts LIST + WATCH calls and needs labelling ... sigh. Signed-off-by: Steve Kuznetsov --- cmd/olm/manager.go | 34 +++++++++++++++++++ .../operators/adoption_controller_test.go | 7 ++++ 2 files changed, 41 insertions(+) diff --git a/cmd/olm/manager.go b/cmd/olm/manager.go index fbca8e2d92..66f29d991f 100644 --- a/cmd/olm/manager.go +++ b/cmd/olm/manager.go @@ -3,11 +3,15 @@ package main import ( "context" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" @@ -53,6 +57,36 @@ func Manager(ctx context.Context, debug bool) (ctrl.Manager, error) { MetricsBindAddress: "0", // TODO(njhale): Enable metrics on non-conflicting port (not 8080) Cache: cache.Options{ ByObject: map[client.Object]cache.ByObject{ + &appsv1.Deployment{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &corev1.Service{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &apiextensionsv1.CustomResourceDefinition{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &apiregistrationv1.APIService{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &corev1.ConfigMap{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &corev1.ServiceAccount{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.Role{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.RoleBinding{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.ClusterRole{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.ClusterRoleBinding{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, &corev1.Secret{}: { Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), }, diff --git a/pkg/controller/operators/adoption_controller_test.go b/pkg/controller/operators/adoption_controller_test.go index 271e754778..2fd4cb6511 100644 --- a/pkg/controller/operators/adoption_controller_test.go +++ b/pkg/controller/operators/adoption_controller_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -350,6 +351,12 @@ var _ = Describe("Adoption Controller", func() { ), } for _, component := range components { + labels := component.GetLabels() + if labels == nil { + labels = map[string]string{} + } + labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue + component.SetLabels(labels) Eventually(func() error { return k8sClient.Create(ctx, component) }, timeout, interval).Should(Succeed()) From cbf1df58b018ce003a6bc26d4911738c54fa082d Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Sat, 23 Sep 2023 07:21:05 -0600 Subject: [PATCH 10/17] test/e2e: check errors from polling Signed-off-by: Steve Kuznetsov --- test/e2e/operator_groups_e2e_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index 940b06e5e1..e8514e6cd7 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/test/e2e/operator_groups_e2e_test.go @@ -1950,6 +1950,7 @@ var _ = Describe("Operator Group", func() { } return true, nil }) + require.NoError(GinkgoT(), err) require.EqualValues(GinkgoT(), append(role.Rules, rbacv1.PolicyRule{ Verbs: []string{"get", "list", "watch"}, APIGroups: []string{""}, @@ -1966,6 +1967,7 @@ var _ = Describe("Operator Group", func() { } return true, nil }) + require.NoError(GinkgoT(), err) require.EqualValues(GinkgoT(), roleBinding.Subjects, fetchedRoleBinding.Subjects) require.EqualValues(GinkgoT(), roleBinding.RoleRef.Name, fetchedRoleBinding.RoleRef.Name) require.EqualValues(GinkgoT(), "rbac.authorization.k8s.io", fetchedRoleBinding.RoleRef.APIGroup) From 8fccaed14fc200fc0422fdeacde99615d14a2802 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 25 Sep 2023 06:23:06 -0600 Subject: [PATCH 11/17] test/e2e: add management label to internal resources in tests Signed-off-by: Steve Kuznetsov --- test/e2e/operator_groups_e2e_test.go | 31 +++++++++++++++++- test/e2e/operator_test.go | 48 +++++++++++++++++++--------- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index e8514e6cd7..a3575fba59 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/test/e2e/operator_groups_e2e_test.go @@ -3,6 +3,7 @@ package e2e import ( "context" "fmt" + "os" "strings" "time" @@ -1420,19 +1421,31 @@ var _ = Describe("Operator Group", func() { Name: role.GetName(), }, } - _, err = c.CreateServiceAccount(serviceAccount) + serviceAccount, err = c.CreateServiceAccount(serviceAccount) require.NoError(GinkgoT(), err) defer func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of serviceaccount %s/%s...\n", serviceAccount.GetNamespace(), serviceAccount.GetName()) + return + } c.DeleteServiceAccount(serviceAccount.GetNamespace(), serviceAccount.GetName(), metav1.NewDeleteOptions(0)) }() createdRole, err := c.CreateRole(role) require.NoError(GinkgoT(), err) defer func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of role %s/%s...\n", role.GetNamespace(), role.GetName()) + return + } c.DeleteRole(role.GetNamespace(), role.GetName(), metav1.NewDeleteOptions(0)) }() createdRoleBinding, err := c.CreateRoleBinding(roleBinding) require.NoError(GinkgoT(), err) defer func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of role binding %s/%s...\n", roleBinding.GetNamespace(), roleBinding.GetName()) + return + } c.DeleteRoleBinding(roleBinding.GetNamespace(), roleBinding.GetName(), metav1.NewDeleteOptions(0)) }() // Create a new NamedInstallStrategy @@ -1448,11 +1461,13 @@ var _ = Describe("Operator Group", func() { err = ownerutil.AddOwnerLabels(createdRole, createdCSV) require.NoError(GinkgoT(), err) + createdRole.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue _, err = c.UpdateRole(createdRole) require.NoError(GinkgoT(), err) err = ownerutil.AddOwnerLabels(createdRoleBinding, createdCSV) require.NoError(GinkgoT(), err) + createdRoleBinding.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue _, err = c.UpdateRoleBinding(createdRoleBinding) require.NoError(GinkgoT(), err) GinkgoT().Log("wait for CSV to succeed") @@ -1903,16 +1918,28 @@ var _ = Describe("Operator Group", func() { _, err = c.CreateServiceAccount(serviceAccount) require.NoError(GinkgoT(), err) defer func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of serviceaccount %s/%s...\n", serviceAccount.GetNamespace(), serviceAccount.GetName()) + return + } c.DeleteServiceAccount(serviceAccount.GetNamespace(), serviceAccount.GetName(), metav1.NewDeleteOptions(0)) }() createdRole, err := c.CreateRole(role) require.NoError(GinkgoT(), err) defer func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of role %s/%s...\n", role.GetNamespace(), role.GetName()) + return + } c.DeleteRole(role.GetNamespace(), role.GetName(), metav1.NewDeleteOptions(0)) }() createdRoleBinding, err := c.CreateRoleBinding(roleBinding) require.NoError(GinkgoT(), err) defer func() { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of role binding %s/%s...\n", roleBinding.GetNamespace(), roleBinding.GetName()) + return + } c.DeleteRoleBinding(roleBinding.GetNamespace(), roleBinding.GetName(), metav1.NewDeleteOptions(0)) }() // Create a new NamedInstallStrategy @@ -1928,11 +1955,13 @@ var _ = Describe("Operator Group", func() { err = ownerutil.AddOwnerLabels(createdRole, createdCSV) require.NoError(GinkgoT(), err) + createdRole.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue _, err = c.UpdateRole(createdRole) require.NoError(GinkgoT(), err) err = ownerutil.AddOwnerLabels(createdRoleBinding, createdCSV) require.NoError(GinkgoT(), err) + createdRoleBinding.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue _, err = c.UpdateRoleBinding(createdRoleBinding) require.NoError(GinkgoT(), err) GinkgoT().Log("wait for CSV to succeed") diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 6824adcdd2..0ffa06bc10 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -3,12 +3,14 @@ package e2e import ( "context" "fmt" + "os" "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/format" gomegatypes "github.com/onsi/gomega/types" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -74,6 +76,7 @@ var _ = Describe("Operator API", func() { It("should surface components in its status", func() { o := &operatorsv1.Operator{} o.SetName(genName("o-")) + By(fmt.Sprintf("Creating an Operator resource %s", o.GetName())) Consistently(o).ShouldNot(ContainCopiedCSVReferences()) @@ -83,6 +86,10 @@ var _ = Describe("Operator API", func() { defer func() { Eventually(func() error { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of operator %s...\n", o.GetName()) + return nil + } err := client.Delete(clientCtx, o) if apierrors.IsNotFound(err) { return nil @@ -116,11 +123,11 @@ var _ = Describe("Operator API", func() { })) defer w.Stop() - // Create namespaces ns-a and ns-b nsA := &corev1.Namespace{} nsA.SetName(genName("ns-a-")) nsB := &corev1.Namespace{} nsB.SetName(genName("ns-b-")) + By(fmt.Sprintf("Create namespaces ns-a: (%s) and ns-b: (%s)", nsA.GetName(), nsB.GetName())) for _, ns := range []*corev1.Namespace{nsA, nsB} { Eventually(func() error { @@ -128,6 +135,10 @@ var _ = Describe("Operator API", func() { }).Should(Succeed()) defer func(n *corev1.Namespace) { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of namespace %s...\n", n.GetName()) + return + } Eventually(func() error { err := client.Delete(clientCtx, n) if apierrors.IsNotFound(err) { @@ -138,24 +149,27 @@ var _ = Describe("Operator API", func() { }(ns) } - // Label ns-a with o's component label + By(fmt.Sprintf("Label ns-a (%s) with o's (%s) component label (%s)", nsA.GetName(), o.GetName(), expectedKey)) setComponentLabel := func(m metav1.Object) error { - m.SetLabels(map[string]string{expectedKey: ""}) + m.SetLabels(map[string]string{ + install.OLMManagedLabelKey: install.OLMManagedLabelValue, + expectedKey: "", + }) return nil } Eventually(Apply(nsA, setComponentLabel)).Should(Succeed()) - // Ensure o's status.components.refs field eventually contains a reference to ns-a + By("Ensure o's status.components.refs field eventually contains a reference to ns-a") By("eventually listing a single component reference") componentRefEventuallyExists(w, true, getReference(scheme, nsA)) - // Create ServiceAccounts sa-a and sa-b in namespaces ns-a and ns-b respectively saA := &corev1.ServiceAccount{} saA.SetName(genName("sa-a-")) saA.SetNamespace(nsA.GetName()) saB := &corev1.ServiceAccount{} saB.SetName(genName("sa-b-")) saB.SetNamespace(nsB.GetName()) + By(fmt.Sprintf("Create ServiceAccounts sa-a (%s/%s) and sa-b (%s/%s) in namespaces ns-a and ns-b respectively", saA.GetNamespace(), saA.GetName(), saB.GetNamespace(), saB.GetName())) for _, sa := range []*corev1.ServiceAccount{saA, saB} { Eventually(func() error { @@ -163,6 +177,10 @@ var _ = Describe("Operator API", func() { }).Should(Succeed()) defer func(sa *corev1.ServiceAccount) { Eventually(func() error { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of serviceaccount %s/%s...\n", sa.GetNamespace(), sa.GetName()) + return nil + } err := client.Delete(clientCtx, sa) if apierrors.IsNotFound(err) { return nil @@ -172,26 +190,26 @@ var _ = Describe("Operator API", func() { }(sa) } - // Label sa-a and sa-b with o's component label + By("Label sa-a and sa-b with o's component label") Eventually(Apply(saA, setComponentLabel)).Should(Succeed()) Eventually(Apply(saB, setComponentLabel)).Should(Succeed()) - // Ensure o's status.components.refs field eventually contains references to sa-a and sa-b + By("Ensure o's status.components.refs field eventually contains references to sa-a and sa-b") By("eventually listing multiple component references") componentRefEventuallyExists(w, true, getReference(scheme, saA)) componentRefEventuallyExists(w, true, getReference(scheme, saB)) - // Remove the component label from sa-b + By("Remove the component label from sa-b") Eventually(Apply(saB, func(m metav1.Object) error { m.SetLabels(nil) return nil })).Should(Succeed()) - // Ensure the reference to sa-b is eventually removed from o's status.components.refs field + By("Ensure the reference to sa-b is eventually removed from o's status.components.refs field") By("removing a component's reference when it no longer bears the component label") componentRefEventuallyExists(w, false, getReference(scheme, saB)) - // Delete o + By("Delete o") Eventually(func() error { err := client.Delete(clientCtx, o) if err != nil && !apierrors.IsNotFound(err) { @@ -200,13 +218,13 @@ var _ = Describe("Operator API", func() { return nil }).Should(Succeed()) - // Ensure that o is eventually recreated (because some of its components still exist). + By("Ensure that o is eventually recreated (because some of its components still exist).") By("recreating the Operator when any components still exist") Eventually(func() error { return client.Get(clientCtx, types.NamespacedName{Name: o.GetName()}, o) }).Should(Succeed()) - // Delete ns-a + By("Delete ns-a") Eventually(func() error { err := client.Delete(clientCtx, nsA) if apierrors.IsNotFound(err) { @@ -215,11 +233,11 @@ var _ = Describe("Operator API", func() { return err }).Should(Succeed()) - // Ensure the reference to ns-a is eventually removed from o's status.components.refs field + By("Ensure the reference to ns-a is eventually removed from o's status.components.refs field") By("removing a component's reference when it no longer exists") componentRefEventuallyExists(w, false, getReference(scheme, nsA)) - // Delete o + By("Delete o") Eventually(func() error { err := client.Delete(clientCtx, o) if apierrors.IsNotFound(err) { @@ -228,7 +246,7 @@ var _ = Describe("Operator API", func() { return err }).Should(Succeed()) - // Ensure that o is consistently not found + By("Ensure that o is consistently not found") By("verifying the Operator is permanently deleted if it has no components") Consistently(func() error { err := client.Get(clientCtx, types.NamespacedName{Name: o.GetName()}, o) From 6b40c1688499ce2a62f3859c9d67c94f4d71fabd Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 25 Sep 2023 08:12:34 -0600 Subject: [PATCH 12/17] test/e2e: more misc improvements Signed-off-by: Steve Kuznetsov --- test/e2e/subscription_e2e_test.go | 4 ++-- test/e2e/util.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index f4804ce651..a524d74cf1 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -2890,9 +2890,9 @@ func fetchSubscription(crc versioned.Interface, namespace, name string, checker return false, err } thisState, thisCSV, thisInstallPlanRef := fetchedSubscription.Status.State, fetchedSubscription.Status.CurrentCSV, fetchedSubscription.Status.InstallPlanRef - if thisState != lastState || thisCSV != lastCSV || equality.Semantic.DeepEqual(thisInstallPlanRef, lastInstallPlanRef) { + if thisState != lastState || thisCSV != lastCSV || !equality.Semantic.DeepEqual(thisInstallPlanRef, lastInstallPlanRef) { lastState, lastCSV, lastInstallPlanRef = thisState, thisCSV, thisInstallPlanRef - log(fmt.Sprintf("%s (%s): %s", thisState, thisCSV, thisInstallPlanRef)) + log(fmt.Sprintf("subscription %s/%s state: %s (csv %s): installPlanRef: %#v", namespace, name, thisState, thisCSV, thisInstallPlanRef)) } return checker(fetchedSubscription), nil }) diff --git a/test/e2e/util.go b/test/e2e/util.go index 04878b321b..616c5bdb79 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -50,7 +50,7 @@ import ( ) const ( - pollInterval = 1 * time.Second + pollInterval = 100 * time.Millisecond pollDuration = 5 * time.Minute olmConfigMap = "olm-operators" // No-longer used, how long do we keep this around? From 034ba2d5fa6b285e413cc156eb5bf5e820d823a8 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 25 Sep 2023 13:48:34 -0600 Subject: [PATCH 13/17] test/e2e: update expectations for permissions problem on install plan Signed-off-by: Steve Kuznetsov --- test/e2e/installplan_e2e_test.go | 2 +- test/e2e/user_defined_sa_test.go | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index c56bf19f11..c8aa2f7586 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -3931,7 +3931,7 @@ func buildInstallPlanMessageCheckFunc(substring string) checkInstallPlanFunc { lastTime := time.Now() return func(fip *operatorsv1alpha1.InstallPlan) bool { if fip.Status.Message != lastMessage { - ctx.Ctx().Logf("waiting %s for installplan %s/%s to have message substring %s, have message %s", time.Since(lastTime), fip.Namespace, fip.Name, substring, fip.Status.Phase) + ctx.Ctx().Logf("waiting %s for installplan %s/%s to have message substring %q, have message %q", time.Since(lastTime), fip.Namespace, fip.Name, substring, fip.Status.Message) lastMessage = fip.Status.Message lastTime = time.Now() } diff --git a/test/e2e/user_defined_sa_test.go b/test/e2e/user_defined_sa_test.go index 1dce545163..ef82ac6481 100644 --- a/test/e2e/user_defined_sa_test.go +++ b/test/e2e/user_defined_sa_test.go @@ -81,17 +81,12 @@ var _ = Describe("User defined service account", func() { require.NoError(GinkgoT(), err) require.NotNil(GinkgoT(), subscription) - By("We expect the InstallPlan to be in status: Failed.") + By("We expect the InstallPlan to be in status: Installing.") ipName := subscription.Status.Install.Name ipPhaseCheckerFunc := buildInstallPlanMessageCheckFunc(`cannot create resource`) ipGot, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, ipName, generatedNamespace.GetName(), ipPhaseCheckerFunc) require.NoError(GinkgoT(), err) - - conditionGot := mustHaveCondition(GinkgoT(), ipGot, v1alpha1.InstallPlanInstalled) - assert.Equal(GinkgoT(), corev1.ConditionFalse, conditionGot.Status) - assert.Equal(GinkgoT(), v1alpha1.InstallPlanReasonComponentFailed, conditionGot.Reason) - assert.Contains(GinkgoT(), conditionGot.Message, fmt.Sprintf("is forbidden: User \"system:serviceaccount:%s:%s\" cannot create resource", generatedNamespace.GetName(), saName)) - + By("Verify that all step resources are in Unknown state.") for _, step := range ipGot.Status.Plan { assert.Equal(GinkgoT(), v1alpha1.StepStatusUnknown, step.Status) From 6bb58e6f4a4f531483523ebc694c8a89fc01229a Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Wed, 27 Sep 2023 06:32:18 -0600 Subject: [PATCH 14/17] *: address review comments Signed-off-by: Steve Kuznetsov --- pkg/controller/install/certresources.go | 4 ++-- pkg/controller/operators/olm/apiservices.go | 4 ++-- pkg/controller/operators/olm/operator.go | 4 ++-- pkg/controller/operators/olm/requirements.go | 7 +------ 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/pkg/controller/install/certresources.go b/pkg/controller/install/certresources.go index 8d50b96643..45bc78d0c9 100644 --- a/pkg/controller/install/certresources.go +++ b/pkg/controller/install/certresources.go @@ -504,7 +504,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo Name: "extension-apiserver-authentication-reader", }, } - authReaderRoleBinding.SetName(AuthReaderRolebindingName(service.GetName())) + authReaderRoleBinding.SetName(AuthReaderRoleBindingName(service.GetName())) authReaderRoleBinding.SetNamespace(KubeSystem) authReaderRoleBinding.SetLabels(map[string]string{OLMManagedLabelKey: OLMManagedLabelValue}) @@ -547,7 +547,7 @@ func AuthDelegatorClusterRoleBindingName(serviceName string) string { return serviceName + "-system:auth-delegator" } -func AuthReaderRolebindingName(serviceName string) string { +func AuthReaderRoleBindingName(serviceName string) string { return serviceName + "-auth-reader" } diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go index 23f9d8751e..69d49e144e 100644 --- a/pkg/controller/operators/olm/apiservices.go +++ b/pkg/controller/operators/olm/apiservices.go @@ -180,8 +180,8 @@ func (a *Operator) checkAPIServiceResources(csv *v1alpha1.ClusterServiceVersion, logger.WithError(err).Warnf("could not retrieve auth delegator cluster role binding %s", install.AuthDelegatorClusterRoleBindingName(service.GetName())) errs = append(errs, err) } - if _, err := a.lister.RbacV1().RoleBindingLister().RoleBindings(install.KubeSystem).Get(install.AuthReaderRolebindingName(service.GetName())); err != nil { - logger.WithError(err).Warnf("could not retrieve role binding %s/%s", install.KubeSystem, install.AuthReaderRolebindingName(service.GetName())) + if _, err := a.lister.RbacV1().RoleBindingLister().RoleBindings(install.KubeSystem).Get(install.AuthReaderRoleBindingName(service.GetName())); err != nil { + logger.WithError(err).Warnf("could not retrieve role binding %s/%s", install.KubeSystem, install.AuthReaderRoleBindingName(service.GetName())) errs = append(errs, err) } } diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index de028b69f7..fb03494d10 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -106,7 +106,7 @@ type Operator struct { informersFiltered bool ruleChecker func(*v1alpha1.ClusterServiceVersion) *install.CSVRuleChecker - ruleCheckerLock *sync.RWMutex + ruleCheckerLock sync.RWMutex resyncPeriod func() time.Duration ctx context.Context } @@ -218,7 +218,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat clientFactory: clients.NewFactory(config.restConfig), protectedCopiedCSVNamespaces: config.protectedCopiedCSVNamespaces, resyncPeriod: config.resyncPeriod, - ruleCheckerLock: &sync.RWMutex{}, + ruleCheckerLock: sync.RWMutex{}, ctx: ctx, informersFiltered: canFilter, } diff --git a/pkg/controller/operators/olm/requirements.go b/pkg/controller/operators/olm/requirements.go index 448f7ce27f..0a20fe8f3c 100644 --- a/pkg/controller/operators/olm/requirements.go +++ b/pkg/controller/operators/olm/requirements.go @@ -424,12 +424,7 @@ func permissionsPreviouslyCreated[T, U metav1.Object]( } roleSelectorMap := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind) roleSelectorMap[resolver.ContentHashLabelKey] = ruleHash - roleSelectorSet := labels.Set{} - for key, value := range roleSelectorMap { - roleSelectorSet[key] = value - } - roleSelector := labels.SelectorFromSet(roleSelectorSet) - roles, err := listRoles(roleSelector) + roles, err := listRoles(labels.SelectorFromSet(roleSelectorMap)) if err != nil { return false, err } From ead44da4173fe353901ffbc1ea61879d70574ef7 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Wed, 27 Sep 2023 15:32:23 -0600 Subject: [PATCH 15/17] controller: add comments to the tricky parts Signed-off-by: Steve Kuznetsov --- pkg/controller/operators/catalog/operator.go | 8 ++++++++ pkg/controller/operators/olm/operator.go | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 17cebe0cdf..b99a19236f 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -387,6 +387,9 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo if canFilter { return nil } + + // for each GVR, we may have more than one labelling controller active; each of which detects + // when it is done; we allocate a space in complete[gvr][idx] to hold that outcome and track it var idx int if _, exists := complete[gvr]; exists { idx = len(complete[gvr]) @@ -405,6 +408,11 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo queueinformer.WithLogger(op.logger), queueinformer.WithInformer(informer), queueinformer.WithSyncer(sync(func() bool { + // this function is called by the processor when it detects that it's work is done - so, for that + // particular labelling action on that particular GVR, all objects are in the correct state. when + // that action is done, we need to further know if that was the last action to be completed, as + // when every action we know about has been completed, we re-start the process to allow the future + // invocation of this process to filter informers (canFilter = true) and elide all this logic completeLock.Lock() complete[gvr][idx] = true allDone := true diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index fb03494d10..c22d53ef64 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -504,6 +504,9 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat if canFilter { return nil } + + // for each GVR, we may have more than one labelling controller active; each of which detects + // when it is done; we allocate a space in complete[gvr][idx] to hold that outcome and track it var idx int if _, exists := complete[gvr]; exists { idx = len(complete[gvr]) @@ -521,6 +524,11 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat queueinformer.WithLogger(op.logger), queueinformer.WithInformer(informer), queueinformer.WithSyncer(sync(func() bool { + // this function is called by the processor when it detects that it's work is done - so, for that + // particular labelling action on that particular GVR, all objects are in the correct state. when + // that action is done, we need to further know if that was the last action to be completed, as + // when every action we know about has been completed, we re-start the process to allow the future + // invocation of this process to filter informers (canFilter = true) and elide all this logic completeLock.Lock() complete[gvr][idx] = true allDone := true From c5e6b63cc48b9c64b196b1dae6fee34e032bc41a Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Thu, 28 Sep 2023 10:13:55 -0600 Subject: [PATCH 16/17] *: address comments Signed-off-by: Steve Kuznetsov --- cmd/olm/manager.go | 39 +------------------- pkg/controller/operators/labeller/labels.go | 3 +- pkg/controller/operators/labeller/rbac.go | 4 +- pkg/controller/operators/olm/operator.go | 14 +++---- pkg/controller/operators/olm/requirements.go | 4 ++ pkg/lib/scoped/syncer.go | 4 +- 6 files changed, 19 insertions(+), 49 deletions(-) diff --git a/cmd/olm/manager.go b/cmd/olm/manager.go index 66f29d991f..953b2ad306 100644 --- a/cmd/olm/manager.go +++ b/cmd/olm/manager.go @@ -3,15 +3,10 @@ package main import ( "context" - appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" - apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" @@ -56,40 +51,8 @@ func Manager(ctx context.Context, debug bool) (ctrl.Manager, error) { Scheme: scheme, MetricsBindAddress: "0", // TODO(njhale): Enable metrics on non-conflicting port (not 8080) Cache: cache.Options{ + DefaultLabelSelector: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), ByObject: map[client.Object]cache.ByObject{ - &appsv1.Deployment{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &corev1.Service{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &apiextensionsv1.CustomResourceDefinition{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &apiregistrationv1.APIService{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &corev1.ConfigMap{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &corev1.ServiceAccount{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &rbacv1.Role{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &rbacv1.RoleBinding{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &rbacv1.ClusterRole{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &rbacv1.ClusterRoleBinding{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, - &corev1.Secret{}: { - Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), - }, &operatorsv1alpha1.ClusterServiceVersion{}: { Label: copiedLabelDoesNotExist, }, diff --git a/pkg/controller/operators/labeller/labels.go b/pkg/controller/operators/labeller/labels.go index dabf6dd258..f131ed1053 100644 --- a/pkg/controller/operators/labeller/labels.go +++ b/pkg/controller/operators/labeller/labels.go @@ -123,7 +123,8 @@ func ObjectPatchLabeler( if gvrFullyLabelled { allObjectsLabelled := done() if allObjectsLabelled { - logrus.Fatal("detected that every object is labelled, exiting...") + logrus.Info("detected that every object is labelled, exiting to re-start the process...") + os.Exit(0) } } return nil diff --git a/pkg/controller/operators/labeller/rbac.go b/pkg/controller/operators/labeller/rbac.go index 395e19592a..939ea82429 100644 --- a/pkg/controller/operators/labeller/rbac.go +++ b/pkg/controller/operators/labeller/rbac.go @@ -3,6 +3,7 @@ package labeller import ( "context" "fmt" + "os" "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" @@ -52,7 +53,8 @@ func ContentHashLabeler[T metav1.Object, A ApplyConfig[A]]( if gvrFullyLabelled { allObjectsLabelled := done() if allObjectsLabelled { - logrus.Fatal("detected that every object is labelled, exiting...") + logrus.Info("detected that every object is labelled, exiting to re-start the process...") + os.Exit(0) } } return nil diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index c22d53ef64..d30005b678 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -136,13 +136,13 @@ func (a *Operator) getRuleChecker() func(*v1alpha1.ClusterServiceVersion) *insta clusterRolesLister := sif.Rbac().V1().ClusterRoles().Lister() clusterRoleBindingsLister := sif.Rbac().V1().ClusterRoleBindings().Lister() - done := make(chan struct{}) - go func() { - <-a.ctx.Done() - done <- struct{}{} - }() - sif.Start(done) - sif.WaitForCacheSync(done) + sif.Start(a.ctx.Done()) + sif.WaitForCacheSync(a.ctx.Done()) + + if a.ctx.Err() != nil { + a.ruleChecker = nil + return nil + } a.ruleChecker = func(csv *v1alpha1.ClusterServiceVersion) *install.CSVRuleChecker { return install.NewCSVRuleChecker( diff --git a/pkg/controller/operators/olm/requirements.go b/pkg/controller/operators/olm/requirements.go index 0a20fe8f3c..890fb06cbf 100644 --- a/pkg/controller/operators/olm/requirements.go +++ b/pkg/controller/operators/olm/requirements.go @@ -3,6 +3,7 @@ package olm import ( "context" "encoding/json" + "errors" "fmt" "strings" @@ -349,6 +350,9 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy // if we have not filtered our informers or if we were unable to detect the correct permissions, we have // no choice but to page in the world and see if the user pre-created permissions for this CSV ruleChecker := a.getRuleChecker()(csv) + if ruleChecker == nil { + return false, errors.New("could not create a rule checker (are we shutting down?)") + } for _, rule := range perm.Rules { dependent := v1alpha1.DependentStatus{ Group: "rbac.authorization.k8s.io", diff --git a/pkg/lib/scoped/syncer.go b/pkg/lib/scoped/syncer.go index f69505beff..15208fd247 100644 --- a/pkg/lib/scoped/syncer.go +++ b/pkg/lib/scoped/syncer.go @@ -100,8 +100,8 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup return } - // A service account has been specified, but likely does not have the labels we expect it to have so it will - // show up in our listers, so let's add that and queue again later + // A service account has been specified, but likely does not have the labels we require it to have to ensure it + // shows up in our listers, so let's add that and queue again later config := corev1applyconfigurations.ServiceAccount(serviceAccountName, namespace) config.Labels = map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue} if _, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Apply(context.TODO(), config, metav1.ApplyOptions{FieldManager: "operator-lifecycle-manager"}); err != nil { From 355219318fbc8685ca42ec0a5bb1bd9dcf086289 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Tue, 3 Oct 2023 09:23:06 -0600 Subject: [PATCH 17/17] olm/manager: don't use a global label selector Signed-off-by: Steve Kuznetsov --- cmd/olm/manager.go | 36 +++++++++++++++++++++++++++++- test/e2e/operator_test.go | 47 ++++++++++++++++++--------------------- 2 files changed, 57 insertions(+), 26 deletions(-) diff --git a/cmd/olm/manager.go b/cmd/olm/manager.go index 953b2ad306..dbb6d0d222 100644 --- a/cmd/olm/manager.go +++ b/cmd/olm/manager.go @@ -3,10 +3,15 @@ package main import ( "context" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" @@ -51,8 +56,37 @@ func Manager(ctx context.Context, debug bool) (ctrl.Manager, error) { Scheme: scheme, MetricsBindAddress: "0", // TODO(njhale): Enable metrics on non-conflicting port (not 8080) Cache: cache.Options{ - DefaultLabelSelector: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), ByObject: map[client.Object]cache.ByObject{ + &appsv1.Deployment{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &corev1.Service{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &apiextensionsv1.CustomResourceDefinition{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &apiregistrationv1.APIService{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &corev1.ConfigMap{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &corev1.ServiceAccount{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.Role{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.RoleBinding{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.ClusterRole{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, + &rbacv1.ClusterRoleBinding{}: { + Label: labels.SelectorFromValidatedSet(map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue}), + }, &operatorsv1alpha1.ClusterServiceVersion{}: { Label: copiedLabelDoesNotExist, }, diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 0ffa06bc10..90caea0f1f 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -11,6 +11,7 @@ import ( "github.com/onsi/gomega/format" gomegatypes "github.com/onsi/gomega/types" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" + "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -266,20 +267,22 @@ var _ = Describe("Operator API", func() { ) BeforeEach(func() { - // Subscribe to a package and await a successful install + By("Subscribe to a package and await a successful install") ns = &corev1.Namespace{} ns.SetName(genName("ns-")) Eventually(func() error { return client.Create(clientCtx, ns) }).Should(Succeed()) + By(fmt.Sprintf("created namespace %s", ns.Name)) - // Default to AllNamespaces + By("Default to AllNamespaces") og := &operatorsv1.OperatorGroup{} og.SetNamespace(ns.GetName()) og.SetName(genName("og-")) Eventually(func() error { return client.Create(clientCtx, og) }).Should(Succeed()) + By(fmt.Sprintf("created operator group %s/%s", og.Namespace, og.Name)) cs := &operatorsv1alpha1.CatalogSource{ Spec: operatorsv1alpha1.CatalogSourceSpec{ @@ -295,8 +298,9 @@ var _ = Describe("Operator API", func() { Eventually(func() error { return client.Create(clientCtx, cs) }).Should(Succeed()) + By(fmt.Sprintf("created catalog source %s/%s", cs.Namespace, cs.Name)) - // Wait for the CatalogSource to be ready + By("Wait for the CatalogSource to be ready") _, err := fetchCatalogSourceOnStatus(newCRClient(), cs.GetName(), cs.GetNamespace(), catalogSourceRegistryPodSynced()) Expect(err).ToNot(HaveOccurred()) @@ -314,38 +318,31 @@ var _ = Describe("Operator API", func() { Eventually(func() error { return client.Create(clientCtx, sub) }).Should(Succeed()) + By(fmt.Sprintf("created subscription %s/%s", sub.Namespace, sub.Name)) - Eventually(func() (operatorsv1alpha1.SubscriptionState, error) { - s := sub.DeepCopy() - if err := client.Get(clientCtx, testobj.NamespacedName(s), s); err != nil { - return operatorsv1alpha1.SubscriptionStateNone, err - } - - return s.Status.State, nil - }).Should(BeEquivalentTo(operatorsv1alpha1.SubscriptionStateAtLatest)) + _, err = fetchSubscription(newCRClient(), sub.Namespace, sub.Name, subscriptionStateAtLatestChecker()) + require.NoError(GinkgoT(), err) - var ipRef *corev1.ObjectReference - Eventually(func() (*corev1.ObjectReference, error) { - if err := client.Get(clientCtx, testobj.NamespacedName(sub), sub); err != nil { - return nil, err - } - ipRef = sub.Status.InstallPlanRef + subscriptionWithInstallPLan, err := fetchSubscription(newCRClient(), sub.Namespace, sub.Name, subscriptionHasInstallPlanChecker()) + require.NoError(GinkgoT(), err) + require.NotNil(GinkgoT(), subscriptionWithInstallPLan) + ipRef := subscriptionWithInstallPLan.Status.InstallPlanRef - return ipRef, nil - }).ShouldNot(BeNil()) - - ip = &operatorsv1alpha1.InstallPlan{} - Eventually(func() error { - return client.Get(clientCtx, types.NamespacedName{Namespace: ipRef.Namespace, Name: ipRef.Name}, ip) - }).Should(Succeed()) + ip, err = fetchInstallPlan(GinkgoT(), newCRClient(), ipRef.Name, ipRef.Namespace, buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseComplete)) + Expect(err).To(BeNil()) operator, err := operatorFactory.NewPackageOperator(sub.Spec.Package, sub.GetNamespace()) Expect(err).ToNot(HaveOccurred()) operatorName = testobj.NamespacedName(operator) + By(fmt.Sprintf("waiting for operator %s/%s to exist", operator.Namespace, operator.Name)) }) AfterEach(func() { Eventually(func() error { + if env := os.Getenv("SKIP_CLEANUP"); env != "" { + fmt.Printf("Skipping cleanup of namespace %s...\n", ns.Name) + return nil + } err := client.Delete(clientCtx, ns) if apierrors.IsNotFound(err) { return nil @@ -386,7 +383,7 @@ var _ = Describe("Operator API", func() { var newNs *corev1.Namespace BeforeEach(func() { - // Subscribe to a package and await a successful install + By("Subscribe to a package and await a successful install") newNs = &corev1.Namespace{} newNs.SetName(genName("ns-")) Eventually(func() error {