From 0f96d135f4d0423a63296812ee5bead51ac74d47 Mon Sep 17 00:00:00 2001 From: njhale Date: Tue, 30 Apr 2019 15:14:04 -0400 Subject: [PATCH 1/3] test(clientfakes): add k8s client decorator and shared options - Add k8s fake client decorator - Add shared options for k8s and olm decorators - Add simple name generator option --- .../clientset/versioned/fake/decorator.go | 9 ++- .../operators/catalog/operator_test.go | 63 +++------------- .../operators/catalog/subscriptions_test.go | 13 ++-- pkg/lib/clientfake/client_options.go | 53 ++++++++++++++ pkg/lib/clientfake/decorator.go | 72 +++++++++++++++++++ pkg/lib/clientfake/meta.go | 45 ++++++++++++ 6 files changed, 194 insertions(+), 61 deletions(-) create mode 100644 pkg/lib/clientfake/client_options.go create mode 100644 pkg/lib/clientfake/decorator.go create mode 100644 pkg/lib/clientfake/meta.go diff --git a/pkg/api/client/clientset/versioned/fake/decorator.go b/pkg/api/client/clientset/versioned/fake/decorator.go index 4135dc6a9e..5f73950762 100644 --- a/pkg/api/client/clientset/versioned/fake/decorator.go +++ b/pkg/api/client/clientset/versioned/fake/decorator.go @@ -18,6 +18,8 @@ package fake import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/testing" + + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" ) // ClientsetDecorator defines decorator methods for a Clientset. @@ -38,7 +40,7 @@ type ReactionForwardingClientsetDecorator struct { // NewReactionForwardingClientsetDecorator returns the ReactionForwardingClientsetDecorator wrapped Clientset result // of calling NewSimpleClientset with the given objects. -func NewReactionForwardingClientsetDecorator(objects ...runtime.Object) *ReactionForwardingClientsetDecorator { +func NewReactionForwardingClientsetDecorator(objects []runtime.Object, options ...clientfake.Option) *ReactionForwardingClientsetDecorator { decorator := &ReactionForwardingClientsetDecorator{ Clientset: *NewSimpleClientset(objects...), } @@ -46,6 +48,11 @@ func NewReactionForwardingClientsetDecorator(objects ...runtime.Object) *Reactio // Swap out the embedded ReactionChain with a Reactor that reduces over the decorator's ReactionChain. decorator.ReactionChain = decorator.Clientset.ReactionChain decorator.Clientset.ReactionChain = []testing.Reactor{&testing.SimpleReactor{"*", "*", decorator.reduceReactions}} + + // Apply options + for _, option := range options { + option(decorator) + } return decorator } diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go index 775aad83b9..2fa8b1e3d1 100644 --- a/pkg/controller/operators/catalog/operator_test.go +++ b/pkg/controller/operators/catalog/operator_test.go @@ -16,13 +16,11 @@ import ( rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/informers" k8sfake "k8s.io/client-go/kubernetes/fake" - clitesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" @@ -34,6 +32,7 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" "github.com/operator-framework/operator-lifecycle-manager/pkg/fakes" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" @@ -379,8 +378,6 @@ func TestSyncCatalogSources(t *testing.T) { } } -// TODO: CatalogSource tests for RegistryServiceStatus - func TestCompetingCRDOwnersExist(t *testing.T) { testNamespace := "default" @@ -465,43 +462,13 @@ func fakeConfigMapData() map[string]string { return data } -// fakeClientOption configures a fake option -type fakeClientOption func(fake.ClientsetDecorator) - -// withSelfLinks returns a fakeClientOption that configures a ClientsetDecorator to write selfLinks to all OLM types on create. -func withSelfLinks(t *testing.T) fakeClientOption { - return func(c fake.ClientsetDecorator) { - c.PrependReactor("create", "*", func(a clitesting.Action) (bool, runtime.Object, error) { - ca, ok := a.(clitesting.CreateAction) - if !ok { - t.Fatalf("expected CreateAction") - } - - obj := ca.GetObject() - accessor, err := meta.Accessor(obj) - if err != nil { - return false, nil, err - } - if accessor.GetSelfLink() != "" { - // SelfLink is already set - return false, nil, nil - } - - gvr := ca.GetResource() - accessor.SetSelfLink(buildSelfLink(gvr.GroupVersion().String(), gvr.Resource, accessor.GetNamespace(), accessor.GetName())) - - return false, obj, nil - }) - } -} - // fakeOperatorConfig is the configuration for a fake operator. type fakeOperatorConfig struct { - clientObjs []runtime.Object - k8sObjs []runtime.Object - extObjs []runtime.Object - regObjs []runtime.Object - fakeClientOptions []fakeClientOption + clientObjs []runtime.Object + k8sObjs []runtime.Object + extObjs []runtime.Object + regObjs []runtime.Object + clientOptions []clientfake.Option } // fakeOperatorOption applies an option to the given fake operator configuration. @@ -525,9 +492,9 @@ func extObjs(extObjs ...runtime.Object) fakeOperatorOption { } } -func withFakeClientOptions(options ...fakeClientOption) fakeOperatorOption { +func withFakeClientOptions(options ...clientfake.Option) fakeOperatorOption { return func(config *fakeOperatorConfig) { - config.fakeClientOptions = options + config.clientOptions = options } } @@ -540,11 +507,7 @@ func NewFakeOperator(namespace string, watchedNamespaces []string, stopCh <-chan } // Create client fakes - clientFake := fake.NewReactionForwardingClientsetDecorator(config.clientObjs...) - for _, option := range config.fakeClientOptions { - option(clientFake) - } - + clientFake := fake.NewReactionForwardingClientsetDecorator(config.clientObjs, config.clientOptions...) opClientFake := operatorclient.NewClient(k8sfake.NewSimpleClientset(config.k8sObjs...), apiextensionsfake.NewSimpleClientset(config.extObjs...), apiregistrationfake.NewSimpleClientset(config.regObjs...)) // Create operator namespace @@ -707,11 +670,3 @@ func toManifest(obj runtime.Object) string { raw, _ := json.Marshal(obj) return string(raw) } - -// selfLink returns a selfLink. -func buildSelfLink(groupVersion, plural, namespace, name string) string { - if namespace == metav1.NamespaceAll { - return fmt.Sprintf("/apis/%s/%s/%s", groupVersion, plural, name) - } - return fmt.Sprintf("/apis/%s/namespaces/%s/%s/%s", groupVersion, namespace, plural, name) -} diff --git a/pkg/controller/operators/catalog/subscriptions_test.go b/pkg/controller/operators/catalog/subscriptions_test.go index 83ee6b41c6..90b9b09e96 100644 --- a/pkg/controller/operators/catalog/subscriptions_test.go +++ b/pkg/controller/operators/catalog/subscriptions_test.go @@ -14,15 +14,16 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" "github.com/operator-framework/operator-lifecycle-manager/pkg/fakes" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" ) func TestSyncSubscriptions(t *testing.T) { now := metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC) timeNow = func() metav1.Time { return now } - testNamespace := "testNamespace" + type fields struct { - fakeClientOptions []fakeClientOption + clientOptions []clientfake.Option sourcesLastUpdate metav1.Time resolveSteps []*v1alpha1.Step resolveSubs []*v1alpha1.Subscription @@ -51,7 +52,7 @@ func TestSyncSubscriptions(t *testing.T) { { name: "NoStatus/NoCurrentCSV/FoundInCatalog", fields: fields{ - fakeClientOptions: []fakeClientOption{withSelfLinks(t)}, + clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)}, existingOLMObjs: []runtime.Object{ &v1alpha1.Subscription{ ObjectMeta: metav1.ObjectMeta{ @@ -182,7 +183,7 @@ func TestSyncSubscriptions(t *testing.T) { { name: "Status/HaveCurrentCSV/UpdateFoundInCatalog", fields: fields{ - fakeClientOptions: []fakeClientOption{withSelfLinks(t)}, + clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)}, existingOLMObjs: []runtime.Object{ &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -322,7 +323,7 @@ func TestSyncSubscriptions(t *testing.T) { { name: "Status/HaveCurrentCSV/UpdateFoundInCatalog/UpdateRequiresDependency", fields: fields{ - fakeClientOptions: []fakeClientOption{withSelfLinks(t)}, + clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)}, existingOLMObjs: []runtime.Object{ &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -514,7 +515,7 @@ func TestSyncSubscriptions(t *testing.T) { // Create test operator stopCh := make(chan struct{}) defer func() { stopCh <- struct{}{} }() - o, err := NewFakeOperator(testNamespace, []string{testNamespace}, stopCh, withClientObjs(tt.fields.existingOLMObjs...), withK8sObjs(tt.fields.existingObjects...), withFakeClientOptions(tt.fields.fakeClientOptions...)) + o, err := NewFakeOperator(testNamespace, []string{testNamespace}, stopCh, withClientObjs(tt.fields.existingOLMObjs...), withK8sObjs(tt.fields.existingObjects...), withFakeClientOptions(tt.fields.clientOptions...)) require.NoError(t, err) o.reconciler = &fakes.FakeRegistryReconcilerFactory{ diff --git a/pkg/lib/clientfake/client_options.go b/pkg/lib/clientfake/client_options.go new file mode 100644 index 0000000000..5f1b9ede95 --- /dev/null +++ b/pkg/lib/clientfake/client_options.go @@ -0,0 +1,53 @@ +package clientfake + +import ( + "testing" + + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + clitesting "k8s.io/client-go/testing" +) + +// Option configures a ClientsetDecorator +type Option func(ClientsetDecorator) + +// WithSelfLinks returns a fakeClientOption that configures a ClientsetDecorator to write selfLinks to all OLM types on create. +func WithSelfLinks(t *testing.T) Option { + return func(c ClientsetDecorator) { + c.PrependReactor("create", "*", func(a clitesting.Action) (bool, runtime.Object, error) { + ca, ok := a.(clitesting.CreateAction) + if !ok { + t.Fatalf("expected CreateAction") + } + + obj := ca.GetObject() + accessor, err := meta.Accessor(obj) + if err != nil { + return false, nil, err + } + if accessor.GetSelfLink() != "" { + // SelfLink is already set + return false, nil, nil + } + + gvr := ca.GetResource() + accessor.SetSelfLink(BuildSelfLink(gvr.GroupVersion().String(), gvr.Resource, accessor.GetNamespace(), accessor.GetName())) + + return false, obj, nil + }) + } +} + +// WithNameGeneration returns a fakeK8sClientOption that configures a Clientset to write generated names to all types on create. +func WithNameGeneration(t *testing.T) Option { + return func(c ClientsetDecorator) { + c.PrependReactor("create", "*", func(a clitesting.Action) (bool, runtime.Object, error) { + ca, ok := a.(clitesting.CreateAction) + if !ok { + t.Fatalf("expected CreateAction") + } + + return false, AddSimpleGeneratedName(ca.GetObject()), nil + }) + } +} diff --git a/pkg/lib/clientfake/decorator.go b/pkg/lib/clientfake/decorator.go new file mode 100644 index 0000000000..d0b4a6e32d --- /dev/null +++ b/pkg/lib/clientfake/decorator.go @@ -0,0 +1,72 @@ +package clientfake + +import ( + "k8s.io/apimachinery/pkg/runtime" + fake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/testing" +) + +// This is used to prepend reactors to the k8s fake client. should be removed when client go is updated. +// TODO: see if we can merge the OLM ClientsetDecorator and this one. + +// ClientsetDecorator defines decorator methods for a Clientset. +type ClientsetDecorator interface { + // PrependReactor adds a reactor to the beginning of the chain. + PrependReactor(verb, resource string, reaction testing.ReactionFunc) +} + +// ReactionForwardingClientsetDecorator wraps a Clientset and "forwards" Action object mutations +// from all successful non-handling Reactors along the chain to the first handling Reactor. This is +// is a stopgap until we can upgrade to client-go v11.0, where the behavior is the default +// (see https://github.com/kubernetes/client-go/blob/6ee68ca5fd8355d024d02f9db0b3b667e8357a0f/testing/fake.go#L130). +type ReactionForwardingClientsetDecorator struct { + fake.Clientset + ReactionChain []testing.Reactor // shadow embedded ReactionChain + actions []testing.Action // these may be castable to other types, but "Action" is the minimum +} + +// NewReactionForwardingClientsetDecorator returns the ReactionForwardingClientsetDecorator wrapped Clientset result +// of calling NewSimpleClientset with the given objects. +func NewReactionForwardingClientsetDecorator(objects []runtime.Object, options ...Option) *ReactionForwardingClientsetDecorator { + decorator := &ReactionForwardingClientsetDecorator{ + Clientset: *fake.NewSimpleClientset(objects...), + } + + // Swap out the embedded ReactionChain with a Reactor that reduces over the decorator's ReactionChain. + decorator.ReactionChain = decorator.Clientset.ReactionChain + decorator.Clientset.ReactionChain = []testing.Reactor{&testing.SimpleReactor{"*", "*", decorator.reduceReactions}} + + // Apply options + for _, option := range options { + option(decorator) + } + + return decorator +} + +// reduceReactions reduces over all reactions in the chain while "forwarding" Action object mutations +// from all successful non-handling Reactors along the chain to the first handling Reactor. +func (c *ReactionForwardingClientsetDecorator) reduceReactions(action testing.Action) (handled bool, ret runtime.Object, err error) { + // The embedded Client set is already locked, so there's no need to lock again + actionCopy := action.DeepCopy() + c.actions = append(c.actions, action.DeepCopy()) + for _, reactor := range c.ReactionChain { + if !reactor.Handles(actionCopy) { + continue + } + + handled, ret, err = reactor.React(actionCopy) + if !handled { + continue + } + + return + } + + return +} + +// PrependReactor adds a reactor to the beginning of the chain. +func (c *ReactionForwardingClientsetDecorator) PrependReactor(verb, resource string, reaction testing.ReactionFunc) { + c.ReactionChain = append([]testing.Reactor{&testing.SimpleReactor{verb, resource, reaction}}, c.ReactionChain...) +} diff --git a/pkg/lib/clientfake/meta.go b/pkg/lib/clientfake/meta.go new file mode 100644 index 0000000000..2a218ec9b3 --- /dev/null +++ b/pkg/lib/clientfake/meta.go @@ -0,0 +1,45 @@ +package clientfake + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/storage/names" +) + +// BuildSelfLink returns a selflink for the given group version, plural, namespace, and name. +func BuildSelfLink(groupVersion, plural, namespace, name string) string { + if namespace == metav1.NamespaceAll { + return fmt.Sprintf("/apis/%s/%s/%s", groupVersion, plural, name) + } + return fmt.Sprintf("/apis/%s/namespaces/%s/%s/%s", groupVersion, namespace, plural, name) +} + +// AddSimpleGeneratedName returns the given object with a simple generated name added to its metadata. +// If a name already exists, there is no GenerateName field set, or there is an issue accessing the object's metadata +// the object is returned unmodified. +func AddSimpleGeneratedName(obj runtime.Object) runtime.Object { + accessor, err := meta.Accessor(obj) + if err != nil { + return obj + } + if accessor.GetName() == "" && accessor.GetGenerateName() != "" { + // TODO: for tests, it would be nice to be able to retrieve this name later + accessor.SetName(names.SimpleNameGenerator.GenerateName(accessor.GetGenerateName())) + } + + return obj +} + +// AddSimpleGeneratedNames returns the list objects with simple generated names added to their metadata. +// If a name already exists, there is no GenerateName field set, or there is an issue accessing the object's metadata +// the object is returned unmodified. +func AddSimpleGeneratedNames(objs ...runtime.Object) []runtime.Object { + for i, obj := range objs { + objs[i] = AddSimpleGeneratedName(obj) + } + + return objs +} From 727dff5f349543f822d7364467982a112ee5fd8a Mon Sep 17 00:00:00 2001 From: njhale Date: Tue, 30 Apr 2019 16:14:09 -0400 Subject: [PATCH 2/3] test(reconciler): refactor reconciler tests with k8s decorator - Use fake k8s decorator for reconciler tests - Consolidate fake reconciler factory constructors - Add gRPC registry pod check unit tests - Refactor gRPC registry pod reconcile unit tests --- .../registry/reconciler/configmap.go | 31 +- .../registry/reconciler/configmap_test.go | 142 +++++++-- pkg/controller/registry/reconciler/grpc.go | 13 +- .../registry/reconciler/grpc_test.go | 290 +++++++++++------- .../registry/reconciler/reconciler.go | 5 + 5 files changed, 325 insertions(+), 156 deletions(-) diff --git a/pkg/controller/registry/reconciler/configmap.go b/pkg/controller/registry/reconciler/configmap.go index a4fcb63a1d..46d875a190 100644 --- a/pkg/controller/registry/reconciler/configmap.go +++ b/pkg/controller/registry/reconciler/configmap.go @@ -26,26 +26,36 @@ type configMapCatalogSourceDecorator struct { *v1alpha1.CatalogSource } +const ( + // ConfigMapServerPostfix is a postfix appended to the names of resources generated for a ConfigMap server. + ConfigMapServerPostfix string = "-configmap-server" +) + func (s *configMapCatalogSourceDecorator) serviceAccountName() string { - return s.GetName() + "-configmap-server" + return s.GetName() + ConfigMapServerPostfix } func (s *configMapCatalogSourceDecorator) roleName() string { - return s.GetName() + "-configmap-reader" + return s.GetName() + ConfigMapServerPostfix } func (s *configMapCatalogSourceDecorator) Selector() map[string]string { return map[string]string{ - "olm.catalogSource": s.GetName(), + CatalogSourceLabelKey: s.GetName(), } } +const ( + // ConfigMapRVLabelKey is the key for a label used to track the resource version of a related ConfigMap. + ConfigMapRVLabelKey string = "olm.configMapResourceVersion" +) + func (s *configMapCatalogSourceDecorator) Labels() map[string]string { labels := map[string]string{ - "olm.catalogSource": s.GetName(), + CatalogSourceLabelKey: s.GetName(), } if s.Spec.SourceType == v1alpha1.SourceTypeInternal || s.Spec.SourceType == v1alpha1.SourceTypeConfigmap { - labels["olm.configMapResourceVersion"] = s.Status.ConfigMapResource.ResourceVersion + labels[ConfigMapRVLabelKey] = s.Status.ConfigMapResource.ResourceVersion } return labels } @@ -123,7 +133,7 @@ func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod { Operator: v1.TolerationOpExists, }, }, - ServiceAccountName: s.GetName() + "-configmap-server", + ServiceAccountName: s.GetName() + ConfigMapServerPostfix, }, } ownerutil.AddOwner(pod, s.CatalogSource, false, false) @@ -160,10 +170,15 @@ func (s *configMapCatalogSourceDecorator) Role() *rbacv1.Role { return role } +const ( + // ConfigMapReaderPostfix is the postfix applied to the name of reader RoleBinding generated for a ConfigMap server. + ConfigMapReaderPostfix string = ConfigMapServerPostfix + "-reader" +) + func (s *configMapCatalogSourceDecorator) RoleBinding() *rbacv1.RoleBinding { rb := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: s.GetName() + "-server-configmap-reader", + Name: s.GetName() + ConfigMapReaderPostfix, Namespace: s.GetNamespace(), }, Subjects: []rbacv1.Subject{ @@ -442,7 +457,7 @@ func (c *ConfigMapRegistryReconciler) CheckRegistryServer(catalogSource *v1alpha c.currentRole(source) == nil || c.currentRoleBinding(source) == nil || c.currentService(source) == nil || - len(c.currentPods(source, c.Image)) != 1 { + len(c.currentPods(source, c.Image)) < 1 { healthy = false return } diff --git a/pkg/controller/registry/reconciler/configmap_test.go b/pkg/controller/registry/reconciler/configmap_test.go index 3b78e07d52..bcc4d7c8d7 100644 --- a/pkg/controller/registry/reconciler/configmap_test.go +++ b/pkg/controller/registry/reconciler/configmap_test.go @@ -2,23 +2,29 @@ package reconciler import ( "fmt" + "reflect" "testing" "time" "github.com/ghodss/yaml" - "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/informers" - k8sfake "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/tools/cache" + k8slabels "k8s.io/kubernetes/pkg/util/labels" + + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" ) const ( @@ -26,8 +32,43 @@ const ( testNamespace = "testns" ) -func cmReconciler(t *testing.T, k8sObjs []runtime.Object, stopc <-chan struct{}) (*ConfigMapRegistryReconciler, operatorclient.ClientInterface) { - opClientFake := operatorclient.NewClient(k8sfake.NewSimpleClientset(k8sObjs...), nil, nil) +type fakeReconcilerConfig struct { + k8sObjs []runtime.Object + k8sClientOptions []clientfake.Option + configMapServerImage string +} + +type fakeReconcilerOption func(*fakeReconcilerConfig) + +func withK8sObjs(k8sObjs ...runtime.Object) fakeReconcilerOption { + return func(config *fakeReconcilerConfig) { + config.k8sObjs = k8sObjs + } +} + +func withK8sClientOptions(options ...clientfake.Option) fakeReconcilerOption { + return func(config *fakeReconcilerConfig) { + config.k8sClientOptions = options + } +} + +func withConfigMapServerImage(configMapServerImage string) fakeReconcilerOption { + return func(config *fakeReconcilerConfig) { + config.configMapServerImage = configMapServerImage + } +} + +func fakeReconcilerFactory(t *testing.T, stopc <-chan struct{}, options ...fakeReconcilerOption) (RegistryReconcilerFactory, operatorclient.ClientInterface) { + config := &fakeReconcilerConfig{ + configMapServerImage: registryImageName, + } + + // Apply all config options + for _, option := range options { + option(config) + } + + opClientFake := operatorclient.NewClient(clientfake.NewReactionForwardingClientsetDecorator(config.k8sObjs, config.k8sClientOptions...), nil, nil) // Creates registry pods in response to configmaps informerFactory := informers.NewSharedInformerFactory(opClientFake.KubernetesInterface(), 5*time.Second) @@ -55,10 +96,10 @@ func cmReconciler(t *testing.T, k8sObjs []runtime.Object, stopc <-chan struct{}) lister.CoreV1().RegisterPodLister(testNamespace, podInformer.Lister()) lister.CoreV1().RegisterConfigMapLister(testNamespace, configMapInformer.Lister()) - rec := &ConfigMapRegistryReconciler{ - Image: registryImageName, - OpClient: opClientFake, - Lister: lister, + rec := ®istryReconcilerFactory{ + OpClient: opClientFake, + Lister: lister, + ConfigMapServerImage: config.configMapServerImage, } var hasSyncedCheckFns []cache.InformerSynced @@ -137,14 +178,27 @@ func validConfigMapCatalogSource(configMap *corev1.ConfigMap) *v1alpha1.CatalogS } func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object { - decorated := configMapCatalogSourceDecorator{catsrc} - objs := []runtime.Object{ - decorated.Pod(registryImageName), - decorated.Service(), - decorated.ServiceAccount(), - decorated.Role(), - decorated.RoleBinding(), + var objs []runtime.Object + switch catsrc.Spec.SourceType { + case v1alpha1.SourceTypeInternal, v1alpha1.SourceTypeConfigmap: + decorated := configMapCatalogSourceDecorator{catsrc} + objs = clientfake.AddSimpleGeneratedNames( + clientfake.AddSimpleGeneratedName(decorated.Pod(registryImageName)), + decorated.Service(), + decorated.ServiceAccount(), + decorated.Role(), + decorated.RoleBinding(), + ) + case v1alpha1.SourceTypeGrpc: + if catsrc.Spec.Image != "" { + decorated := grpcCatalogSourceDecorator{catsrc} + objs = clientfake.AddSimpleGeneratedNames( + decorated.Pod(), + decorated.Service(), + ) + } } + blockOwnerDeletion := false isController := false for _, o := range objs { @@ -161,14 +215,30 @@ func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object { return objs } -func modifyObjName(objs []runtime.Object, kind, newName string) []runtime.Object { +func modifyObjName(objs []runtime.Object, kind runtime.Object, newName string) []runtime.Object { out := []runtime.Object{} - for _, o := range objs { - if o.GetObjectKind().GroupVersionKind().Kind == kind { - mo := o.(metav1.Object) - mo.SetName(newName) - out = append(out, mo.(runtime.Object)) - continue + t := reflect.TypeOf(kind) + for _, obj := range objs { + o := obj.DeepCopyObject() + if reflect.TypeOf(o) == t { + if accessor, err := meta.Accessor(o); err == nil { + accessor.SetName(newName) + } + } + out = append(out, o) + } + return out +} + +func setLabel(objs []runtime.Object, kind runtime.Object, label, value string) []runtime.Object { + out := []runtime.Object{} + t := reflect.TypeOf(kind) + for _, obj := range objs { + o := obj.DeepCopyObject() + if reflect.TypeOf(o) == t { + if accessor, err := meta.Accessor(o); err == nil { + k8slabels.AddLabel(accessor.GetLabels(), label, value) + } } out = append(out, o) } @@ -235,7 +305,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) { testName: "ExistingRegistry/BadServiceAccount", in: in{ cluster: cluster{ - k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "ServiceAccount", "badName"), validConfigMap), + k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &corev1.ServiceAccount{}, "badName"), validConfigMap), }, catsrc: validCatalogSource, }, @@ -253,7 +323,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) { testName: "ExistingRegistry/BadService", in: in{ cluster: cluster{ - k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "Service", "badName"), validConfigMap), + k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &corev1.Service{}, "badName"), validConfigMap), }, catsrc: validCatalogSource, }, @@ -271,7 +341,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) { testName: "ExistingRegistry/BadPod", in: in{ cluster: cluster{ - k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "Pod", "badName"), validConfigMap), + k8sObjs: append(setLabel(objectsForCatalogSource(validCatalogSource), &corev1.Pod{}, CatalogSourceLabelKey, "badValue"), validConfigMap), }, catsrc: validCatalogSource, }, @@ -289,7 +359,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) { testName: "ExistingRegistry/BadRole", in: in{ cluster: cluster{ - k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "Role", "badName"), validConfigMap), + k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &rbacv1.Role{}, "badName"), validConfigMap), }, catsrc: validCatalogSource, }, @@ -307,7 +377,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) { testName: "ExistingRegistry/BadRoleBinding", in: in{ cluster: cluster{ - k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), "RoleBinding", "badName"), validConfigMap), + k8sObjs: append(modifyObjName(objectsForCatalogSource(validCatalogSource), &rbacv1.RoleBinding{}, "badName"), validConfigMap), }, catsrc: validCatalogSource, }, @@ -345,7 +415,8 @@ func TestConfigMapRegistryReconciler(t *testing.T) { stopc := make(chan struct{}) defer close(stopc) - rec, client := cmReconciler(t, tt.in.cluster.k8sObjs, stopc) + factory, client := fakeReconcilerFactory(t, stopc, withK8sObjs(tt.in.cluster.k8sObjs...), withK8sClientOptions(clientfake.WithNameGeneration(t))) + rec := factory.ReconcilerForSource(tt.in.catsrc) err := rec.EnsureRegistryServer(tt.in.catsrc) @@ -360,9 +431,14 @@ func TestConfigMapRegistryReconciler(t *testing.T) { decorated := configMapCatalogSourceDecorator{tt.in.catsrc} pod := decorated.Pod(registryImageName) - outPod, err := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).Get(pod.GetName(), metav1.GetOptions{}) + listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()} + outPods, err := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(listOptions) require.NoError(t, err) - require.Equal(t, pod, outPod) + require.Len(t, outPods.Items, 1) + outPod := outPods.Items[0] + require.Equal(t, pod.GetGenerateName(), outPod.GetGenerateName()) + require.Equal(t, pod.GetLabels(), outPod.GetLabels()) + require.Equal(t, pod.Spec, outPod.Spec) service := decorated.Service() outService, err := client.KubernetesInterface().CoreV1().Services(service.GetNamespace()).Get(service.GetName(), metav1.GetOptions{}) diff --git a/pkg/controller/registry/reconciler/grpc.go b/pkg/controller/registry/reconciler/grpc.go index 8d5a1a2594..66a0b25310 100644 --- a/pkg/controller/registry/reconciler/grpc.go +++ b/pkg/controller/registry/reconciler/grpc.go @@ -3,16 +3,17 @@ package reconciler import ( "fmt" - "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" "github.com/pkg/errors" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" ) // grpcCatalogSourceDecorator wraps CatalogSource to add additional methods @@ -22,13 +23,13 @@ type grpcCatalogSourceDecorator struct { func (s *grpcCatalogSourceDecorator) Selector() labels.Selector { return labels.SelectorFromValidatedSet(map[string]string{ - "olm.catalogSource": s.GetName(), + CatalogSourceLabelKey: s.GetName(), }) } func (s *grpcCatalogSourceDecorator) Labels() map[string]string { return map[string]string{ - "olm.catalogSource": s.GetName(), + CatalogSourceLabelKey: s.GetName(), } } diff --git a/pkg/controller/registry/reconciler/grpc_test.go b/pkg/controller/registry/reconciler/grpc_test.go index 92ab1d6b58..4cf951f9e4 100644 --- a/pkg/controller/registry/reconciler/grpc_test.go +++ b/pkg/controller/registry/reconciler/grpc_test.go @@ -5,64 +5,17 @@ import ( "time" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/informers" - k8sfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/tools/cache" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" ) -func grpcReconcilerFactory(t *testing.T, k8sObjs []runtime.Object, stopc <-chan struct{}) (RegistryReconcilerFactory, operatorclient.ClientInterface) { - opClientFake := operatorclient.NewClient(k8sfake.NewSimpleClientset(k8sObjs...), nil, nil) - - // Creates registry pods in response to configmaps - informerFactory := informers.NewSharedInformerFactory(opClientFake.KubernetesInterface(), 5*time.Second) - roleInformer := informerFactory.Rbac().V1().Roles() - roleBindingInformer := informerFactory.Rbac().V1().RoleBindings() - serviceAccountInformer := informerFactory.Core().V1().ServiceAccounts() - serviceInformer := informerFactory.Core().V1().Services() - podInformer := informerFactory.Core().V1().Pods() - configMapInformer := informerFactory.Core().V1().ConfigMaps() - - registryInformers := []cache.SharedIndexInformer{ - roleInformer.Informer(), - roleBindingInformer.Informer(), - serviceAccountInformer.Informer(), - serviceInformer.Informer(), - podInformer.Informer(), - configMapInformer.Informer(), - } - - lister := operatorlister.NewLister() - lister.RbacV1().RegisterRoleLister(testNamespace, roleInformer.Lister()) - lister.RbacV1().RegisterRoleBindingLister(testNamespace, roleBindingInformer.Lister()) - lister.CoreV1().RegisterServiceAccountLister(testNamespace, serviceAccountInformer.Lister()) - lister.CoreV1().RegisterServiceLister(testNamespace, serviceInformer.Lister()) - lister.CoreV1().RegisterPodLister(testNamespace, podInformer.Lister()) - lister.CoreV1().RegisterConfigMapLister(testNamespace, configMapInformer.Lister()) - - rec := ®istryReconcilerFactory{ - OpClient: opClientFake, - Lister: lister, - } - - var hasSyncedCheckFns []cache.InformerSynced - for _, informer := range registryInformers { - hasSyncedCheckFns = append(hasSyncedCheckFns, informer.HasSynced) - go informer.Run(stopc) - } - - require.True(t, cache.WaitForCacheSync(stopc, hasSyncedCheckFns...), "caches failed to sync") - - return rec, opClientFake -} - func validGrpcCatalogSource(image, address string) *v1alpha1.CatalogSource { return &v1alpha1.CatalogSource{ ObjectMeta: metav1.ObjectMeta{ @@ -82,10 +35,6 @@ func TestGrpcRegistryReconciler(t *testing.T) { nowTime := metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC) timeNow = func() metav1.Time { return nowTime } - validConfigMap := validConfigMap() - validCatalogSource := validConfigMapCatalogSource(validConfigMap) - outdatedCatalogSource := validCatalogSource.DeepCopy() - outdatedCatalogSource.Status.ConfigMapResource.ResourceVersion = "old" type cluster struct { k8sObjs []runtime.Object } @@ -118,39 +67,10 @@ func TestGrpcRegistryReconciler(t *testing.T) { }, }, { - testName: "Grpc/Address/CreateSuccessful", - in: in{ - cluster: cluster{}, - catsrc: validGrpcCatalogSource("", "catalog.svc.cluster.local:50001"), - }, - out: out{ - status: &v1alpha1.RegistryServiceStatus{ - CreatedAt: timeNow(), - Protocol: "grpc", - }, - }, - }, - { - testName: "Grpc/AddressAndImage/CreateSuccessful", - in: in{ - cluster: cluster{}, - catsrc: validGrpcCatalogSource("img-catalog", "catalog.svc.cluster.local:50001"), - }, - out: out{ - status: &v1alpha1.RegistryServiceStatus{ - CreatedAt: timeNow(), - Protocol: "grpc", - ServiceName: "img-catalog", - ServiceNamespace: testNamespace, - Port: "50051", - }, - }, - }, - { - testName: "Grpc/ExistingRegistry/BadServiceAccount", + testName: "Grpc/ExistingRegistry/CreateSuccessful", in: in{ cluster: cluster{ - k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), "ServiceAccount", "badName"), + k8sObjs: objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), }, catsrc: validGrpcCatalogSource("test-img", ""), }, @@ -165,30 +85,23 @@ func TestGrpcRegistryReconciler(t *testing.T) { }, }, { - testName: "Grpc/ExistingRegistry/BadService", + testName: "Grpc/Address/CreateSuccessful", in: in{ - cluster: cluster{ - k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), "Service", "badName"), - }, - catsrc: validGrpcCatalogSource("test-img", ""), + cluster: cluster{}, + catsrc: validGrpcCatalogSource("", "catalog.svc.cluster.local:50001"), }, out: out{ status: &v1alpha1.RegistryServiceStatus{ - CreatedAt: timeNow(), - Protocol: "grpc", - ServiceName: "img-catalog", - ServiceNamespace: testNamespace, - Port: "50051", + CreatedAt: timeNow(), + Protocol: "grpc", }, }, }, { - testName: "Grpc/ExistingRegistry/BadPod", + testName: "Grpc/AddressAndImage/CreateSuccessful", in: in{ - cluster: cluster{ - k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), "Pod", "badName"), - }, - catsrc: validGrpcCatalogSource("test-img", ""), + cluster: cluster{}, + catsrc: validGrpcCatalogSource("img-catalog", "catalog.svc.cluster.local:50001"), }, out: out{ status: &v1alpha1.RegistryServiceStatus{ @@ -201,10 +114,10 @@ func TestGrpcRegistryReconciler(t *testing.T) { }, }, { - testName: "Grpc/ExistingRegistry/BadRole", + testName: "Grpc/ExistingRegistry/BadService", in: in{ cluster: cluster{ - k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), "Role", "badName"), + k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), &corev1.Service{}, "badName"), }, catsrc: validGrpcCatalogSource("test-img", ""), }, @@ -219,10 +132,10 @@ func TestGrpcRegistryReconciler(t *testing.T) { }, }, { - testName: "Grpc/ExistingRegistry/BadRoleBinding", + testName: "Grpc/ExistingRegistry/BadPod", in: in{ cluster: cluster{ - k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), "RoleBinding", "badName"), + k8sObjs: setLabel(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), &corev1.Pod{}, CatalogSourceLabelKey, ""), }, catsrc: validGrpcCatalogSource("test-img", ""), }, @@ -260,7 +173,7 @@ func TestGrpcRegistryReconciler(t *testing.T) { stopc := make(chan struct{}) defer close(stopc) - factory, client := grpcReconcilerFactory(t, tt.in.cluster.k8sObjs, stopc) + factory, client := fakeReconcilerFactory(t, stopc, withK8sObjs(tt.in.cluster.k8sObjs...), withK8sClientOptions(clientfake.WithNameGeneration(t))) rec := factory.ReconcilerForSource(tt.in.catsrc) err := rec.EnsureRegistryServer(tt.in.catsrc) @@ -276,20 +189,25 @@ func TestGrpcRegistryReconciler(t *testing.T) { decorated := grpcCatalogSourceDecorator{tt.in.catsrc} pod := decorated.Pod() service := decorated.Service() - outPod, podErr := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).Get(pod.GetName(), metav1.GetOptions{}) + listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()} + outPods, podErr := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(listOptions) outService, serviceErr := client.KubernetesInterface().CoreV1().Services(service.GetNamespace()).Get(service.GetName(), metav1.GetOptions{}) switch rec.(type) { case *GrpcRegistryReconciler: // Should be created by a GrpcRegistryReconciler require.NoError(t, podErr) - require.Equal(t, pod, outPod) + require.Len(t, outPods.Items, 1) + outPod := outPods.Items[0] + require.Equal(t, pod.GetGenerateName(), outPod.GetGenerateName()) + require.Equal(t, pod.GetLabels(), outPod.GetLabels()) + require.Equal(t, pod.Spec, outPod.Spec) require.NoError(t, serviceErr) require.Equal(t, service, outService) case *GrpcAddressRegistryReconciler: // Should not be created by a GrpcAddressRegistryReconciler - require.Error(t, podErr) - require.True(t, k8serrors.IsNotFound(podErr)) + require.NoError(t, podErr) + require.Len(t, outPods.Items, 0) require.NoError(t, err) require.True(t, k8serrors.IsNotFound(serviceErr)) } @@ -297,3 +215,157 @@ func TestGrpcRegistryReconciler(t *testing.T) { }) } } + +func TestGrpcRegistryChecker(t *testing.T) { + nowTime := metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC) + timeNow = func() metav1.Time { return nowTime } + + type cluster struct { + k8sObjs []runtime.Object + } + type in struct { + cluster cluster + catsrc *v1alpha1.CatalogSource + } + type out struct { + healthy bool + err error + } + tests := []struct { + testName string + in in + out out + }{ + { + testName: "Grpc/ExistingRegistry/Image/Healthy", + in: in{ + cluster: cluster{ + k8sObjs: objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), + }, + catsrc: validGrpcCatalogSource("test-img", ""), + }, + out: out{ + healthy: true, + }, + }, + { + testName: "Grpc/NoExistingRegistry/Image/NotHealthy", + in: in{ + catsrc: validGrpcCatalogSource("test-img", ""), + }, + out: out{ + healthy: false, + }, + }, + { + testName: "Grpc/ExistingRegistry/Image/BadService", + in: in{ + cluster: cluster{ + k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), &corev1.Service{}, "badName"), + }, + catsrc: validGrpcCatalogSource("test-img", ""), + }, + out: out{ + healthy: false, + }, + }, + { + testName: "Grpc/ExistingRegistry/Image/BadPod", + in: in{ + cluster: cluster{ + k8sObjs: setLabel(objectsForCatalogSource(validGrpcCatalogSource("test-img", "")), &corev1.Pod{}, CatalogSourceLabelKey, ""), + }, + catsrc: validGrpcCatalogSource("test-img", ""), + }, + out: out{ + healthy: false, + }, + }, + { + testName: "Grpc/ExistingRegistry/Image/OldPod/NotHealthy", + in: in{ + cluster: cluster{ + k8sObjs: objectsForCatalogSource(validGrpcCatalogSource("old-img", "")), + }, + catsrc: validGrpcCatalogSource("new-img", ""), + }, + out: out{ + healthy: false, + }, + }, + { + testName: "Grpc/NoExistingRegistry/Address/Healthy", + in: in{ + catsrc: validGrpcCatalogSource("", "catalog.svc.cluster.local:50001"), + }, + out: out{ + healthy: true, + }, + }, + { + testName: "Grpc/ExistingRegistry/AddressAndImage/Healthy", + in: in{ + cluster: cluster{ + k8sObjs: objectsForCatalogSource(validGrpcCatalogSource("img-catalog", "catalog.svc.cluster.local:50001")), + }, + catsrc: validGrpcCatalogSource("img-catalog", "catalog.svc.cluster.local:50001"), + }, + out: out{ + healthy: true, + }, + }, + { + testName: "Grpc/NoExistingRegistry/AddressAndImage/NotHealthy", + in: in{ + cluster: cluster{}, + catsrc: validGrpcCatalogSource("img-catalog", "catalog.svc.cluster.local:50001"), + }, + out: out{ + healthy: false, + }, + }, + { + testName: "Grpc/ExistingRegistry/AddressAndImage/BadService/NotHealthy", + in: in{ + cluster: cluster{ + k8sObjs: modifyObjName(objectsForCatalogSource(validGrpcCatalogSource("test-img", "catalog.svc.cluster.local:50001")), &corev1.Service{}, "badName"), + }, + catsrc: validGrpcCatalogSource("test-img", "catalog.svc.cluster.local:50001"), + }, + out: out{ + healthy: false, + }, + }, + { + testName: "Grpc/ExistingRegistry/AddressAndImage/OldPod/NotHealthy", + in: in{ + cluster: cluster{ + k8sObjs: objectsForCatalogSource(validGrpcCatalogSource("old-img", "catalog.svc.cluster.local:50001")), + }, + catsrc: validGrpcCatalogSource("new-img", "catalog.svc.cluster.local:50001"), + }, + out: out{ + healthy: false, + }, + }, + } + for _, tt := range tests { + t.Run(tt.testName, func(t *testing.T) { + stopc := make(chan struct{}) + defer close(stopc) + + factory, _ := fakeReconcilerFactory(t, stopc, withK8sObjs(tt.in.cluster.k8sObjs...)) + rec := factory.ReconcilerForSource(tt.in.catsrc) + + healthy, err := rec.CheckRegistryServer(tt.in.catsrc) + + require.Equal(t, tt.out.err, err) + if tt.out.err != nil { + return + } + + require.Equal(t, tt.out.healthy, healthy) + + }) + } +} diff --git a/pkg/controller/registry/reconciler/reconciler.go b/pkg/controller/registry/reconciler/reconciler.go index 18d2a77b28..a6d450f103 100644 --- a/pkg/controller/registry/reconciler/reconciler.go +++ b/pkg/controller/registry/reconciler/reconciler.go @@ -7,6 +7,11 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" ) +const ( + // CatalogSourceLabelKey is the key for a label containing a CatalogSource name. + CatalogSourceLabelKey string = "olm.catalogSource" +) + // RegistryEnsurer describes methods for ensuring a registry exists. type RegistryEnsurer interface { // EnsureRegistryServer ensures a registry server exists for the given CatalogSource. From 5fff1e960c36ba7774b9878a96d656e104b88a8e Mon Sep 17 00:00:00 2001 From: njhale Date: Tue, 30 Apr 2019 23:06:15 -0400 Subject: [PATCH 3/3] fix(catalogs): make grpc registry check for < 1 pods --- .../registry/reconciler/configmap.go | 9 +- pkg/controller/registry/reconciler/grpc.go | 2 +- test/e2e/catalog_e2e_test.go | 104 ++++++++++++++++-- test/e2e/setup_bare_test.go | 9 ++ test/e2e/setup_test.go | 13 ++- 5 files changed, 116 insertions(+), 21 deletions(-) diff --git a/pkg/controller/registry/reconciler/configmap.go b/pkg/controller/registry/reconciler/configmap.go index 46d875a190..e9fd258fc9 100644 --- a/pkg/controller/registry/reconciler/configmap.go +++ b/pkg/controller/registry/reconciler/configmap.go @@ -36,7 +36,7 @@ func (s *configMapCatalogSourceDecorator) serviceAccountName() string { } func (s *configMapCatalogSourceDecorator) roleName() string { - return s.GetName() + ConfigMapServerPostfix + return s.GetName() + "-configmap-reader" } func (s *configMapCatalogSourceDecorator) Selector() map[string]string { @@ -170,15 +170,10 @@ func (s *configMapCatalogSourceDecorator) Role() *rbacv1.Role { return role } -const ( - // ConfigMapReaderPostfix is the postfix applied to the name of reader RoleBinding generated for a ConfigMap server. - ConfigMapReaderPostfix string = ConfigMapServerPostfix + "-reader" -) - func (s *configMapCatalogSourceDecorator) RoleBinding() *rbacv1.RoleBinding { rb := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: s.GetName() + ConfigMapReaderPostfix, + Name: s.GetName() + "-server-configmap-reader", Namespace: s.GetNamespace(), }, Subjects: []rbacv1.Subject{ diff --git a/pkg/controller/registry/reconciler/grpc.go b/pkg/controller/registry/reconciler/grpc.go index 66a0b25310..31788afd62 100644 --- a/pkg/controller/registry/reconciler/grpc.go +++ b/pkg/controller/registry/reconciler/grpc.go @@ -214,7 +214,7 @@ func (c *GrpcRegistryReconciler) CheckRegistryServer(catalogSource *v1alpha1.Cat // Check on registry resources // TODO: add gRPC health check - if len(c.currentPodsWithCorrectImage(source)) == 1 || + if len(c.currentPodsWithCorrectImage(source)) < 1 || c.currentService(source) == nil { healthy = false return diff --git a/test/e2e/catalog_e2e_test.go b/test/e2e/catalog_e2e_test.go index 47d6b6bb49..ccba19e796 100644 --- a/test/e2e/catalog_e2e_test.go +++ b/test/e2e/catalog_e2e_test.go @@ -49,8 +49,8 @@ func TestCatalogLoadingBetweenRestarts(t *testing.T) { crc := newCRClient(t) catalogSourceName := genName("mock-ocs-") - _, cleanupCatalogSource := createInternalCatalogSource(t, c, crc, catalogSourceName, operatorNamespace, manifests, []apiextensions.CustomResourceDefinition{crd}, []v1alpha1.ClusterServiceVersion{csv}) - defer cleanupCatalogSource() + _, cleanupSource := createInternalCatalogSource(t, c, crc, catalogSourceName, operatorNamespace, manifests, []apiextensions.CustomResourceDefinition{crd}, []v1alpha1.ClusterServiceVersion{csv}) + defer cleanupSource() // ensure the mock catalog exists and has been synced by the catalog operator catalogSource, err := fetchCatalogSource(t, crc, catalogSourceName, operatorNamespace, catalogSourceRegistryPodSynced) @@ -264,7 +264,7 @@ func TestConfigMapReplaceTriggersRegistryPodRollout(t *testing.T) { } // Create the initial catalogsource - _, cleanupCatalog := createInternalCatalogSource(t, c, crc, mainCatalogName, testNamespace, mainManifests, nil, []v1alpha1.ClusterServiceVersion{mainCSV}) + _, cleanupSource := createInternalCatalogSource(t, c, crc, mainCatalogName, testNamespace, mainManifests, nil, []v1alpha1.ClusterServiceVersion{mainCSV}) // Attempt to get the catalog source before creating install plan fetchedInitialCatalog, err := fetchCatalogSource(t, crc, mainCatalogName, testNamespace, catalogSourceRegistryPodSynced) @@ -279,7 +279,7 @@ func TestConfigMapReplaceTriggersRegistryPodRollout(t *testing.T) { require.Equal(t, 1, len(initialPods.Items)) // delete the first catalog - cleanupCatalog() + cleanupSource() // create a catalog with the same name createInternalCatalogSource(t, c, crc, mainCatalogName, testNamespace, append(mainManifests, dependentManifests...), []apiextensions.CustomResourceDefinition{dependentCRD}, []v1alpha1.ClusterServiceVersion{mainCSV, dependentCSV}) @@ -436,7 +436,7 @@ func TestGrpcAddressCatalogSource(t *testing.T) { require.NoError(t, err) } -func TestDeleteRegistryPodTriggersRecreation(t *testing.T) { +func TestDeleteInternalRegistryPodTriggersRecreation(t *testing.T) { // Create internal CatalogSource containing csv in package // Wait for a registry pod to be created // Create a Subscription for package @@ -452,7 +452,7 @@ func TestDeleteRegistryPodTriggersRecreation(t *testing.T) { packageStable := fmt.Sprintf("%s-stable", packageName) stableChannel := "stable" namedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil) - catalogName := genName("catsrc-") + sourceName := genName("catalog-") crd := newCRD(genName("ins-")) csv := newCSV(packageStable, testNamespace, "", *semver.New("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, namedStrategy) manifests := []registry.PackageManifest{ @@ -467,11 +467,11 @@ func TestDeleteRegistryPodTriggersRecreation(t *testing.T) { c := newKubeClient(t) crc := newCRClient(t) - _, cleanupCatalog := createInternalCatalogSource(t, c, crc, catalogName, testNamespace, manifests, []apiextensions.CustomResourceDefinition{crd}, []v1alpha1.ClusterServiceVersion{csv}) - defer cleanupCatalog() + _, cleanupSource := createInternalCatalogSource(t, c, crc, sourceName, testNamespace, manifests, []apiextensions.CustomResourceDefinition{crd}, []v1alpha1.ClusterServiceVersion{csv}) + defer cleanupSource() // Wait for a new registry pod to be created - selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": catalogName}) + selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": sourceName}) singlePod := podCount(1) registryPods, err := awaitPods(t, c, testNamespace, selector.String(), singlePod) require.NoError(t, err, "error awaiting registry pod") @@ -484,7 +484,7 @@ func TestDeleteRegistryPodTriggersRecreation(t *testing.T) { // Create a Subscription for package subscriptionName := genName("sub-") - cleanupSubscription := createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, catalogName, packageName, stableChannel, "", v1alpha1.ApprovalAutomatic) + cleanupSubscription := createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, sourceName, packageName, stableChannel, "", v1alpha1.ApprovalAutomatic) defer cleanupSubscription() // Wait for the Subscription to succeed @@ -516,6 +516,90 @@ func TestDeleteRegistryPodTriggersRecreation(t *testing.T) { require.Equal(t, 1, len(registryPods.Items), "unexpected number of replacement registry pods found") } +func TestDeleteGRPCRegistryPodTriggersRecreation(t *testing.T) { + // Create gRPC CatalogSource using an external registry image (community-operators) + // Wait for a registry pod to be created + // Create a Subscription for package + // Wait for the Subscription to succeed + // Wait for csv to succeed + // Delete the registry pod + // Wait for a new registry pod to be created + + defer cleaner.NotifyTestComplete(t, true) + + sourceName := genName("catalog-") + packageName := "etcd" + channelName := "clusterwide-alpha" + + // Create gRPC CatalogSource using an external registry image (community-operators) + source := &v1alpha1.CatalogSource{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.CatalogSourceKind, + APIVersion: v1alpha1.CatalogSourceCRDAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: sourceName, + Namespace: testNamespace, + }, + Spec: v1alpha1.CatalogSourceSpec{ + SourceType: v1alpha1.SourceTypeGrpc, + Image: communityOperatorsImage, + }, + } + + crc := newCRClient(t) + source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(source) + require.NoError(t, err) + defer func() { + require.NoError(t, crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(source.GetName(), &metav1.DeleteOptions{})) + }() + + // Wait for a new registry pod to be created + c := newKubeClient(t) + selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": source.GetName()}) + singlePod := podCount(1) + registryPods, err := awaitPods(t, c, source.GetNamespace(), selector.String(), singlePod) + require.NoError(t, err, "error awaiting registry pod") + require.NotNil(t, registryPods, "nil registry pods") + require.Equal(t, 1, len(registryPods.Items), "unexpected number of registry pods found") + + // Store the UID for later comparison + uid := registryPods.Items[0].GetUID() + name := registryPods.Items[0].GetName() + + // Create a Subscription for package + subscriptionName := genName("sub-") + cleanupSubscription := createSubscriptionForCatalog(t, crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic) + defer cleanupSubscription() + + // Wait for the Subscription to succeed + subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + + // Wait for csv to succeed + _, err = fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker) + require.NoError(t, err) + + // Delete the registry pod + require.NoError(t, c.KubernetesInterface().CoreV1().Pods(testNamespace).Delete(name, &metav1.DeleteOptions{})) + + // Wait for a new registry pod to be created + notUID := func(pods *corev1.PodList) bool { + for _, pod := range pods.Items { + if pod.GetUID() == uid { + return false + } + } + + return true + } + registryPods, err = awaitPods(t, c, testNamespace, selector.String(), unionPodsCheck(singlePod, notUID)) + require.NoError(t, err, "error waiting for replacement registry pod") + require.NotNil(t, registryPods, "nil replacement registry pods") + require.Equal(t, 1, len(registryPods.Items), "unexpected number of replacement registry pods found") +} + func getOperatorDeployment(c operatorclient.ClientInterface, namespace string, operatorLabels labels.Set) (*appsv1.Deployment, error) { deployments, err := c.ListDeploymentsWithLabels(namespace, operatorLabels) if err != nil || deployments == nil || len(deployments.Items) != 1 { diff --git a/test/e2e/setup_bare_test.go b/test/e2e/setup_bare_test.go index ffed218f83..7ea1054099 100644 --- a/test/e2e/setup_bare_test.go +++ b/test/e2e/setup_bare_test.go @@ -37,8 +37,15 @@ var ( olmNamespace = flag.String( "olmNamespace", "", "namespace where olm is running") + communityOperators = flag.String( + "communityOperators", + "quay.io/operator-framework/upstream-community-operators@sha256:098457dc5e0b6ca9599bd0e7a67809f8eca397907ca4d93597380511db478fec", + "reference to upstream-community-operators image") + + testNamespace = "" operatorNamespace = "" + communityOperatorsImage = "" ) func TestMain(m *testing.M) { @@ -56,6 +63,8 @@ func TestMain(m *testing.M) { testNamespace = string(testNamespaceBytes) } operatorNamespace = *olmNamespace + communityOperatorsImage = *communityOperators + cleaner = newNamespaceCleaner(testNamespace) namespaces := strings.Split(*watchedNamespaces, ",") diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index a820e65e92..ccd46c2e95 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -23,8 +23,14 @@ var ( olmNamespace = flag.String( "olmNamespace", "", "namespace where olm is running") - testNamespace = "" - operatorNamespace = "" + communityOperators = flag.String( + "communityOperators", + "quay.io/operator-framework/upstream-community-operators@sha256:098457dc5e0b6ca9599bd0e7a67809f8eca397907ca4d93597380511db478fec", + "reference to upstream-community-operators image") + + testNamespace = "" + operatorNamespace = "" + communityOperatorsImage = "" ) func TestMain(m *testing.M) { @@ -35,8 +41,9 @@ func TestMain(m *testing.M) { testNamespace = *namespace operatorNamespace = *olmNamespace - cleaner = newNamespaceCleaner(testNamespace) + communityOperatorsImage = *communityOperators + cleaner = newNamespaceCleaner(testNamespace) c, err := client.NewClient(*kubeConfigPath) if err != nil { panic(err)