diff --git a/README.md b/README.md index 1ef98fde3..71e99d6d6 100644 --- a/README.md +++ b/README.md @@ -45,11 +45,10 @@ Removing the top-level application ensures that Argo won't try to put back anyth ## Watch the logs -Note that when installing via UI the namespace will be `openshift-operators` -and not `patterns-operator-system` +When installing via UI the namespace will be `patterns-operator` (recommended) ``` -oc logs -npatterns-operator-system `oc get -npatterns-operator-system pods -o name --field-selector status.phase=Running | grep patterns` -c manager -f +oc logs -n patterns-operator `oc get -n patterns-operator pods -o name --field-selector status.phase=Running | grep patterns` -c manager -f ``` ## Development diff --git a/cmd/main.go b/cmd/main.go index b6edf5582..31f332c1a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -78,6 +78,8 @@ func main() { printVersion() + setupLog.Info("detected operator namespace", "namespace", controllers.DetectOperatorNamespace()) + // Create initial config map for gitops err := createGitOpsConfigMap() if err != nil { @@ -160,14 +162,14 @@ func createGitOpsConfigMap() error { }, ObjectMeta: metav1.ObjectMeta{ Name: controllers.OperatorConfigMap, - Namespace: controllers.OperatorNamespace, + Namespace: controllers.DetectOperatorNamespace(), }, } - _, err = clientset.CoreV1().ConfigMaps(controllers.OperatorNamespace).Get(context.Background(), controllers.OperatorConfigMap, metav1.GetOptions{}) + _, err = clientset.CoreV1().ConfigMaps(controllers.DetectOperatorNamespace()).Get(context.Background(), controllers.OperatorConfigMap, metav1.GetOptions{}) if err != nil && errors.IsNotFound(err) { // if the configmap does not exist we create an empty one - _, err = clientset.CoreV1().ConfigMaps(controllers.OperatorNamespace).Create(context.Background(), &configMap, metav1.CreateOptions{}) + _, err = clientset.CoreV1().ConfigMaps(controllers.DetectOperatorNamespace()).Create(context.Background(), &configMap, metav1.CreateOptions{}) if err != nil { return err } @@ -193,7 +195,7 @@ func areAnalyticsEnabled(reader crclient.Reader) bool { enabled := strings.ToLower(os.Getenv("ANALYTICS")) != "false" var cm corev1.ConfigMap - err := reader.Get(context.Background(), crclient.ObjectKey{Namespace: controllers.OperatorNamespace, Name: controllers.OperatorConfigMap}, &cm) + err := reader.Get(context.Background(), crclient.ObjectKey{Namespace: controllers.DetectOperatorNamespace(), Name: controllers.OperatorConfigMap}, &cm) if err != nil { setupLog.Error(err, "error reading operator configmap for analytics setting") return enabled diff --git a/cmd/main_test.go b/cmd/main_test.go index fa9a7e4dd..d88937be4 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -26,7 +26,7 @@ func newFakeReader(objs ...crclient.Object) crclient.Reader { func newOperatorConfigMap(analyticsEnabled string) *corev1.ConfigMap { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Namespace: controllers.OperatorNamespace, + Namespace: controllers.DetectOperatorNamespace(), Name: controllers.OperatorConfigMap, }, Data: map[string]string{ diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml index 4185b6887..5e3da2a8b 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -1,5 +1,5 @@ # Adds namespace to all resources. -namespace: patterns-operator-system +namespace: patterns-operator # Value of this field is prepended to the # names of all resources, e.g. a deployment named diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index b6c58235c..6827b0d16 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -13,4 +13,4 @@ kind: Kustomization images: - name: controller newName: quay.io/validatedpatterns/patterns-operator - newTag: 0.0.65 + newTag: 0.0.4 diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 9ad7350d0..de264e617 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -36,6 +36,11 @@ spec: securityContext: allowPrivilegeEscalation: false imagePullPolicy: IfNotPresent + env: + - name: OPERATOR_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace livenessProbe: httpGet: path: /healthz diff --git a/config/manifests/bases/patterns-operator.clusterserviceversion.yaml b/config/manifests/bases/patterns-operator.clusterserviceversion.yaml index e90e56a0d..6214cca6f 100644 --- a/config/manifests/bases/patterns-operator.clusterserviceversion.yaml +++ b/config/manifests/bases/patterns-operator.clusterserviceversion.yaml @@ -17,6 +17,7 @@ metadata: features.operators.openshift.io/token-auth-aws: "false" features.operators.openshift.io/token-auth-azure: "false" features.operators.openshift.io/token-auth-gcp: "false" + operatorframework.io/suggested-namespace: patterns-operator operators.openshift.io/must-gather-image: quay.io/validatedpatterns/must-gather:latest repository: https://github.com/validatedpatterns/patterns-operator support: validatedpatterns@googlegroups.com @@ -204,7 +205,7 @@ spec: deployments: null strategy: "" installModes: - - supported: false + - supported: true type: OwnNamespace - supported: false type: SingleNamespace diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index f135bb68f..98c40b58b 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -9,8 +9,12 @@ rules: resources: - configmaps verbs: + - create + - delete - get - list + - patch + - update - apiGroups: - "" resources: @@ -103,6 +107,7 @@ rules: - apiGroups: - operators.coreos.com resources: + - operatorgroups - subscriptions verbs: - create diff --git a/hack/operator-build-deploy.sh b/hack/operator-build-deploy.sh index 82fd83825..f33b531f7 100755 --- a/hack/operator-build-deploy.sh +++ b/hack/operator-build-deploy.sh @@ -2,7 +2,7 @@ set -e -o pipefail CATALOGSOURCE="test-pattern-operator" -NS="openshift-operators" +DEFAULT_NS="patterns-operator" OPERATOR="patterns-operator" VERSION="${VERSION:-6.6.6}" UPLOADREGISTRY="${UPLOADREGISTRY:-kuemper.int.rhx/bandini}" @@ -78,7 +78,34 @@ make VERSION=${VERSION} UPLOADREGISTRY="${UPLOADREGISTRY}" CHANNELS=fast USE_IMA manifests bundle generate docker-build docker-push bundle-build bundle-push catalog-build \ catalog-push catalog-install +# If the operator already exists in openshift-operators, keep using that namespace; +# otherwise use the new dedicated namespace. +if oc get subscriptions.operators.coreos.com "${OPERATOR}" -n openshift-operators &>/dev/null; then + NS="openshift-operators" + echo "Existing installation found in openshift-operators, upgrading in place" +else + NS="${DEFAULT_NS}" + echo "No existing installation found, installing in ${NS}" +fi + wait_for_resource "packagemanifest" "${OPERATOR}" "" "${CATALOGSOURCE}" + +# Create namespace and OperatorGroup for dedicated namespace install +# (openshift-operators already has a global OperatorGroup, so skip for that namespace) +if [[ "${NS}" != "openshift-operators" ]]; then + oc create namespace ${NS} --dry-run=client -o yaml | oc apply -f - + oc apply -f - < 0 { + return strings.TrimSpace(string(data)) + } + return LegacyOperatorNamespace +} + // Below are the default constants that we will // use throughout the patterns operator code const ( - // Default Operator Namespace - OperatorNamespace = "openshift-operators" + LegacyOperatorNamespace = "openshift-operators" // Default Operator Config Map Name OperatorConfigMap = "patterns-operator-config" - // Default Subscription Namespace - SubscriptionNamespace = "openshift-operators" // Default Application Namespace ApplicationNamespace = "openshift-gitops" // ClusterWide Argo Name @@ -17,21 +31,15 @@ const ( // GitOps Subscription const ( + GitOpsDefaultSubscriptionNamespace = "openshift-gitops-operator" + GitOpsLegacySubscriptionNamespace = LegacyOperatorNamespace GitOpsDefaultChannel = "gitops-1.18" GitOpsDefaultPackageName = "openshift-gitops-operator" GitOpsDefaultCatalogSource = "redhat-operators" GitOpsDefaultCatalogSourceNamespace = "openshift-marketplace" GitOpsDefaultApprovalPlan = "Automatic" -) - -// GitOps Configuration -const ( - // Require manual intervention before Argo will sync new content. Default: False - GitOpsDefaultManualSync = "false" - // Require manual confirmation before installing and upgrading operators. Default: False - GitOpsDefaultManualApproval = "false" - // Dangerous. Force a specific version to be installed. Default: False - GitOpsDefaultUseCSV = "false" + // Dangerous. Force a specific version to be installed. Default: "" + GitOpsDefaultCSV = "" ) // Gitea chart defaults @@ -67,11 +75,10 @@ const ( var DefaultPatternOperatorConfig = map[string]string{ "gitops.catalogSource": GitOpsDefaultCatalogSource, - "gitops.name": GitOpsDefaultPackageName, "gitops.channel": GitOpsDefaultChannel, "gitops.sourceNamespace": GitOpsDefaultCatalogSourceNamespace, "gitops.installApprovalPlan": GitOpsDefaultApprovalPlan, - "gitops.ManualSync": GitOpsDefaultManualSync, + "gitops.csv": GitOpsDefaultCSV, "gitea.chartName": GiteaChartName, "gitea.helmRepoUrl": GiteaHelmRepoUrl, "gitea.chartVersion": GiteaDefaultChartVersion, diff --git a/internal/controller/defaults_test.go b/internal/controller/defaults_test.go index 5fdbead34..338746d2f 100644 --- a/internal/controller/defaults_test.go +++ b/internal/controller/defaults_test.go @@ -26,11 +26,6 @@ var _ = Describe("GitOpsConfig getValueWithDefault", func() { Expect(config.getValueWithDefault("gitops.catalogSource")).To(Equal(GitOpsDefaultCatalogSource)) }) - It("should return the default value for gitops.name", func() { - config := GitOpsConfig{} - Expect(config.getValueWithDefault("gitops.name")).To(Equal(GitOpsDefaultPackageName)) - }) - It("should return the default value for gitops.sourceNamespace", func() { config := GitOpsConfig{} Expect(config.getValueWithDefault("gitops.sourceNamespace")).To(Equal(GitOpsDefaultCatalogSourceNamespace)) @@ -90,11 +85,9 @@ var _ = Describe("DefaultPatternOperatorConfig", func() { It("should contain all expected keys", func() { expectedKeys := []string{ "gitops.catalogSource", - "gitops.name", "gitops.channel", "gitops.sourceNamespace", "gitops.installApprovalPlan", - "gitops.ManualSync", "gitea.chartName", "gitea.helmRepoUrl", "gitea.chartVersion", @@ -109,7 +102,6 @@ var _ = Describe("DefaultPatternOperatorConfig", func() { Expect(DefaultPatternOperatorConfig["gitops.catalogSource"]).To(Equal("redhat-operators")) Expect(DefaultPatternOperatorConfig["gitops.sourceNamespace"]).To(Equal("openshift-marketplace")) Expect(DefaultPatternOperatorConfig["gitops.installApprovalPlan"]).To(Equal("Automatic")) - Expect(DefaultPatternOperatorConfig["gitops.ManualSync"]).To(Equal("false")) Expect(DefaultPatternOperatorConfig["analytics.enabled"]).To(Equal("true")) }) }) diff --git a/internal/controller/operatorgroup.go b/internal/controller/operatorgroup.go new file mode 100644 index 000000000..f6a79e12c --- /dev/null +++ b/internal/controller/operatorgroup.go @@ -0,0 +1,49 @@ +/* +Copyright 2022. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "context" + + v1 "github.com/operator-framework/api/pkg/operators/v1" + olmclient "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func getOperatorGroup(client olmclient.Interface, name string) (*v1.OperatorGroup, error) { //nolint:unparam + var og *v1.OperatorGroup + var err error + if og, err = client.OperatorsV1().OperatorGroups(name).Get(context.Background(), name, metav1.GetOptions{}); err != nil { + if errors.IsNotFound(err) { + return nil, nil + } + return nil, err + } + return og, nil +} + +func createOperatorGroup(client olmclient.Interface, name string) error { //nolint:unparam + _, err := client.OperatorsV1().OperatorGroups(name).Create( + context.Background(), + &v1.OperatorGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: name}, + Spec: v1.OperatorGroupSpec{}}, + metav1.CreateOptions{}) + return err +} diff --git a/internal/controller/operatorgroup_test.go b/internal/controller/operatorgroup_test.go new file mode 100644 index 000000000..cc89eaa99 --- /dev/null +++ b/internal/controller/operatorgroup_test.go @@ -0,0 +1,118 @@ +/* +Copyright 2022. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "context" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v1 "github.com/operator-framework/api/pkg/operators/v1" + olmclient "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/testing" +) + +var _ = Describe("OperatorGroup Functions", func() { + Context("getOperatorGroup", func() { + var fakeOlmClient *olmclient.Clientset + const testNamespace = "openshift-gitops-operator" + + BeforeEach(func() { + fakeOlmClient = olmclient.NewSimpleClientset() + }) + + It("should return nil, nil when OperatorGroup does not exist", func() { + og, err := getOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).ToNot(HaveOccurred()) + Expect(og).To(BeNil()) + }) + + It("should return the OperatorGroup when it exists", func() { + existing := &v1.OperatorGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: testNamespace, + Namespace: testNamespace, + }, + Spec: v1.OperatorGroupSpec{}, + } + _, err := fakeOlmClient.OperatorsV1().OperatorGroups(testNamespace).Create( + context.Background(), existing, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) + + og, err := getOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).ToNot(HaveOccurred()) + Expect(og).ToNot(BeNil()) + Expect(og.Name).To(Equal(testNamespace)) + Expect(og.Namespace).To(Equal(testNamespace)) + Expect(og.Spec).To(Equal(v1.OperatorGroupSpec{})) + }) + + It("should return error on non-NotFound API errors", func() { + fakeOlmClient.PrependReactor("get", "operatorgroups", func(action testing.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("server error") + }) + + og, err := getOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("server error")) + Expect(og).To(BeNil()) + }) + }) + + Context("createOperatorGroup", func() { + var fakeOlmClient *olmclient.Clientset + const testNamespace = "openshift-gitops-operator" + + BeforeEach(func() { + fakeOlmClient = olmclient.NewSimpleClientset() + }) + + It("should create an OperatorGroup with the given name in the same namespace", func() { + err := createOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).ToNot(HaveOccurred()) + + og, err := fakeOlmClient.OperatorsV1().OperatorGroups(testNamespace).Get( + context.Background(), testNamespace, metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(og).ToNot(BeNil()) + Expect(og.Name).To(Equal(testNamespace)) + Expect(og.Namespace).To(Equal(testNamespace)) + Expect(og.Spec).To(Equal(v1.OperatorGroupSpec{})) + }) + + It("should return error when create fails", func() { + fakeOlmClient.PrependReactor("create", "operatorgroups", func(action testing.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("create failed") + }) + + err := createOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("create failed")) + }) + + It("should return error when OperatorGroup already exists", func() { + err := createOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).ToNot(HaveOccurred()) + + err = createOperatorGroup(fakeOlmClient, testNamespace) + Expect(err).To(HaveOccurred()) + }) + }) +}) diff --git a/internal/controller/pattern_controller.go b/internal/controller/pattern_controller.go index b30b5302c..fee2e71a5 100644 --- a/internal/controller/pattern_controller.go +++ b/internal/controller/pattern_controller.go @@ -47,6 +47,9 @@ import ( argoclient "github.com/argoproj/argo-cd/v3/pkg/client/clientset/versioned" configclient "github.com/openshift/client-go/config/clientset/versioned" routeclient "github.com/openshift/client-go/route/clientset/versioned" + v1 "github.com/operator-framework/api/pkg/operators/v1" + operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + olmclient "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "k8s.io/client-go/kubernetes" @@ -88,7 +91,8 @@ type PatternReconciler struct { //+kubebuilder:rbac:groups=argoproj.io,resources=argocds,verbs=list;get;create;update;patch;delete //+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=list;get;create;update;patch;delete //+kubebuilder:rbac:groups=operators.coreos.com,resources=subscriptions,verbs=list;get;create;update;patch;delete -//+kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list +//+kubebuilder:rbac:groups=operators.coreos.com,resources=operatorgroups,verbs=list;get;create;update;patch;delete +//+kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;create;update;patch;delete //+kubebuilder:rbac:groups="operator.open-cluster-management.io",resources=multiclusterhubs,verbs=get;list //+kubebuilder:rbac:groups=operator.openshift.io,resources="openshiftcontrollermanagers",resources=openshiftcontrollermanagers,verbs=get;list //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update;watch @@ -105,7 +109,7 @@ type PatternReconciler struct { // // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.10.0/pkg/reconcile -func (r *PatternReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *PatternReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { //nolint:funlen // Reconcile() should perform at most one action in any invocation // in order to simplify testing. r.logger = klog.FromContext(ctx) @@ -168,45 +172,71 @@ func (r *PatternReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } // -- GitOps Subscription - targetSub, _ := newSubscriptionFromConfigMap(r.fullClient) - operatorConfigMap, err := GetOperatorConfigmap() - if err == nil { - if err := controllerutil.SetOwnerReference(operatorConfigMap, targetSub, r.Scheme); err != nil { - return r.actionPerformed(qualifiedInstance, "error setting owner of gitops subscription", err) + targetSub, err := newSubscriptionFromConfigMap(r.fullClient) + + if err != nil { + return r.actionPerformed(qualifiedInstance, "error creating new subscription from configmap", err) + } + subscriptionNamespace := GitOpsLegacySubscriptionNamespace + subscriptionName := GitOpsDefaultPackageName + // If the pattern operator is installed to the new vp namespace we need to create a ns, operatorgroup for the new sub + if DetectOperatorNamespace() != LegacyOperatorNamespace { + subscriptionNamespace = GitOpsDefaultSubscriptionNamespace + subscriptionName = GitOpsDefaultPackageName + + // Create namespace for gitops subscription + if err := createNamespace(r.fullClient, subscriptionNamespace); err != nil { + return r.actionPerformed(qualifiedInstance, "error creating namespace for gitops subscription", err) + } + + // Create operatorgroup for gitops subscription + var og *v1.OperatorGroup + if og, err = getOperatorGroup(r.olmClient, subscriptionNamespace); err != nil { + return r.actionPerformed(qualifiedInstance, "error getting operatorgroup for gitops subscription", err) + } + if og == nil { + if err := createOperatorGroup(r.olmClient, subscriptionNamespace); err != nil { + return r.actionPerformed(qualifiedInstance, "error creating operatorgroup for gitops subscription", err) + } } - } else { - return r.actionPerformed(qualifiedInstance, "error getting operator configmap", err) } - sub, _ := getSubscription(r.olmClient, targetSub.Name) - if sub == nil { - err = createSubscription(r.olmClient, targetSub) - return r.actionPerformed(qualifiedInstance, "create gitops subscription", err) - } else if ownedBySame(targetSub, sub) { - // Check version/channel etc - // Dangerous if multiple patterns do not agree, or automatic upgrades are in place... - changed, errSub := updateSubscription(r.olmClient, targetSub, sub) - if changed { - return r.actionPerformed(qualifiedInstance, "update gitops subscription", errSub) + var currentSub *operatorv1alpha1.Subscription + + if currentSub, err = getSubscription(r.olmClient, subscriptionName, subscriptionNamespace); err != nil { + return r.actionPerformed(qualifiedInstance, "error getting gitops subscription", err) + } + + if currentSub == nil { + if err = createSubscription(r.olmClient, targetSub); err != nil { + return r.actionPerformed(qualifiedInstance, "error creating gitops subscription", err) } } else { - // Historically the subscription was owned by the pattern, not the operator. If this is the case, - // we update the owner reference to the operator itself. When the subscription is owned by the pattern, - // deleting the pattern removes the subscription and some, but not all, argo resources. This causes - // subsequent pattern installations to try to start argo in namespaced mode and any charts requiring - // cluster-wide access, like Vault, will fail to install. Having the subscription owned by the operator - // allows subsequent pattern installations to reuse the openshift gitops operator already on the cluster. - if err := controllerutil.RemoveOwnerReference(qualifiedInstance, sub, r.Scheme); err == nil { - if err := controllerutil.SetOwnerReference(operatorConfigMap, sub, r.Scheme); err != nil { - return r.actionPerformed(qualifiedInstance, "error setting patterns operator owner reference of gitops subscription", err) + // Remove any stale owner references from the subscription (historically set by + // the pattern or the operator configmap). Cross-namespace owner references are + // not allowed, so we clean them up and rely on the subscription persisting + // independently. + changed := false + if err := controllerutil.RemoveOwnerReference(qualifiedInstance, currentSub, r.Scheme); err == nil { + changed = true + } + operatorConfigMap, cmErr := GetOperatorConfigmap() + if cmErr == nil { + if err := controllerutil.RemoveOwnerReference(operatorConfigMap, currentSub, r.Scheme); err == nil { + changed = true } - // Persist the updated ownerReferences on the Subscription - if _, err := r.olmClient.OperatorsV1alpha1().Subscriptions(SubscriptionNamespace).Update(context.Background(), sub, metav1.UpdateOptions{}); err != nil { - return r.actionPerformed(qualifiedInstance, "error updating gitops subscription owner references", err) + } + if changed { + if _, err := r.olmClient.OperatorsV1alpha1().Subscriptions(currentSub.Namespace).Update(context.Background(), currentSub, metav1.UpdateOptions{}); err != nil { + return r.actionPerformed(qualifiedInstance, "error removing stale owner references from gitops subscription", err) } - return r.actionPerformed(qualifiedInstance, "updated patterns operator owner reference of gitops subscription", nil) - } else { - logOnce("The gitops subscription is not owned by us, leaving untouched") + return r.actionPerformed(qualifiedInstance, "removed stale owner references from gitops subscription", nil) + } + + // Check version/channel etc + updatedSub, errSub := updateSubscription(r.olmClient, targetSub, currentSub) + if updatedSub { + return r.actionPerformed(qualifiedInstance, "update gitops subscription", errSub) } } logOnce("subscription found") diff --git a/internal/controller/pattern_controller_test.go b/internal/controller/pattern_controller_test.go index 8dd0e25f5..7deaa54d5 100644 --- a/internal/controller/pattern_controller_test.go +++ b/internal/controller/pattern_controller_test.go @@ -44,7 +44,6 @@ import ( ) const ( - namespace = "openshift-operators" defaultNamespace = "default" foo = "foo" originURL = "https://origin.url" @@ -52,6 +51,7 @@ const ( ) var ( + namespace = suggestedOperatorNamespace patternNamespaced = types.NamespacedName{Name: foo, Namespace: namespace} mockGitOps *MockGitOperations gitOptions *git.CloneOptions diff --git a/internal/controller/subscription.go b/internal/controller/subscription.go index c099d48c9..c9684f4ca 100644 --- a/internal/controller/subscription.go +++ b/internal/controller/subscription.go @@ -34,7 +34,7 @@ func newSubscriptionFromConfigMap(r kubernetes.Interface) (*operatorv1alpha1.Sub var newSubscription *operatorv1alpha1.Subscription // Check if the config map exists and read the config map values - cm, err := r.CoreV1().ConfigMaps(OperatorNamespace).Get(context.Background(), OperatorConfigMap, metav1.GetOptions{}) + cm, err := r.CoreV1().ConfigMaps(DetectOperatorNamespace()).Get(context.Background(), OperatorConfigMap, metav1.GetOptions{}) // If we hit an error that is not related to the configmap not existing bubble it up if err != nil && !apierrors.IsNotFound(err) { return nil, err @@ -55,7 +55,7 @@ func newSubscriptionFromConfigMap(r kubernetes.Interface) (*operatorv1alpha1.Sub spec := &operatorv1alpha1.SubscriptionSpec{ CatalogSource: PatternsOperatorConfig.getValueWithDefault("gitops.catalogSource"), CatalogSourceNamespace: PatternsOperatorConfig.getValueWithDefault("gitops.sourceNamespace"), - Package: PatternsOperatorConfig.getValueWithDefault("gitops.name"), + Package: GitOpsDefaultPackageName, Channel: PatternsOperatorConfig.getValueWithDefault("gitops.channel"), StartingCSV: PatternsOperatorConfig.getValueWithDefault("gitops.csv"), InstallPlanApproval: installPlanApproval, @@ -68,11 +68,20 @@ func newSubscriptionFromConfigMap(r kubernetes.Interface) (*operatorv1alpha1.Sub }, }, } + var subscriptionName, subscriptionNamespace string + + subscriptionName = GitOpsDefaultPackageName + subscriptionNamespace = GitOpsDefaultSubscriptionNamespace + + if DetectOperatorNamespace() == LegacyOperatorNamespace { + subscriptionName = GitOpsDefaultPackageName + subscriptionNamespace = GitOpsLegacySubscriptionNamespace + } newSubscription = &operatorv1alpha1.Subscription{ ObjectMeta: metav1.ObjectMeta{ - Name: GitOpsDefaultPackageName, - Namespace: SubscriptionNamespace, + Name: subscriptionName, + Namespace: subscriptionNamespace, }, Spec: spec, } @@ -80,16 +89,20 @@ func newSubscriptionFromConfigMap(r kubernetes.Interface) (*operatorv1alpha1.Sub return newSubscription, nil } -func getSubscription(client olmclient.Interface, name string) (*operatorv1alpha1.Subscription, error) { - sub, err := client.OperatorsV1alpha1().Subscriptions(SubscriptionNamespace).Get(context.Background(), name, metav1.GetOptions{}) - if err != nil { +func getSubscription(client olmclient.Interface, name, namespace string) (*operatorv1alpha1.Subscription, error) { + var subscription *operatorv1alpha1.Subscription + var err error + if subscription, err = client.OperatorsV1alpha1().Subscriptions(namespace).Get(context.Background(), name, metav1.GetOptions{}); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } return nil, err } - return sub, nil + return subscription, nil } func createSubscription(client olmclient.Interface, sub *operatorv1alpha1.Subscription) error { - _, err := client.OperatorsV1alpha1().Subscriptions(SubscriptionNamespace).Create(context.Background(), sub, metav1.CreateOptions{}) + _, err := client.OperatorsV1alpha1().Subscriptions(sub.Namespace).Create(context.Background(), sub, metav1.CreateOptions{}) return err } @@ -133,7 +146,7 @@ func updateSubscription(client olmclient.Interface, target, current *operatorv1a if changed { target.Spec.DeepCopyInto(current.Spec) - _, err := client.OperatorsV1alpha1().Subscriptions(SubscriptionNamespace).Update(context.Background(), current, metav1.UpdateOptions{}) + _, err := client.OperatorsV1alpha1().Subscriptions(current.Namespace).Update(context.Background(), current, metav1.UpdateOptions{}) return changed, err } diff --git a/internal/controller/subscription_test.go b/internal/controller/subscription_test.go index 00e678d4d..2153abe5d 100644 --- a/internal/controller/subscription_test.go +++ b/internal/controller/subscription_test.go @@ -3,6 +3,7 @@ package controllers import ( "context" "fmt" + "os" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -15,41 +16,44 @@ import ( "k8s.io/client-go/testing" ) -var defaultTestSubscription = operatorv1alpha1.Subscription{ - Spec: &operatorv1alpha1.SubscriptionSpec{ - CatalogSource: "foosource", - CatalogSourceNamespace: "foosourcenamespace", - Package: "foooperator", - Channel: "foochannel", - InstallPlanApproval: operatorv1alpha1.ApprovalAutomatic, - Config: &operatorv1alpha1.SubscriptionConfig{ - Env: []corev1.EnvVar{ - { - Name: "foo", - Value: "bar", +func newDefaultTestSubscription() operatorv1alpha1.Subscription { + return operatorv1alpha1.Subscription{ + Spec: &operatorv1alpha1.SubscriptionSpec{ + CatalogSource: "foosource", + CatalogSourceNamespace: "foosourcenamespace", + Package: "foooperator", + Channel: "foochannel", + InstallPlanApproval: operatorv1alpha1.ApprovalAutomatic, + Config: &operatorv1alpha1.SubscriptionConfig{ + Env: []corev1.EnvVar{ + { + Name: "foo", + Value: "bar", + }, }, }, }, - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "foosubscription", - Namespace: OperatorNamespace, - }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foosubscription", + Namespace: LegacyOperatorNamespace, + }, + } } -var defaultTestSubConfigMap = corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: OperatorConfigMap, - Namespace: OperatorNamespace, - }, - Data: map[string]string{ - "gitops.installApprovalPlan": "Manual", - "gitops.catalogSource": "foo-source", - "gitops.sourceNamespace": "foo-source-namespace", - "gitops.name": "foo-name", - "gitops.channel": "foo-channel", - "gitops.csv": "1.2.3", - }, +func newDefaultTestSubConfigMap() corev1.ConfigMap { + return corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: OperatorConfigMap, + Namespace: DetectOperatorNamespace(), + }, + Data: map[string]string{ + "gitops.installApprovalPlan": "Manual", + "gitops.catalogSource": "foo-source", + "gitops.sourceNamespace": "foo-source-namespace", + "gitops.channel": "foo-channel", + "gitops.csv": "1.2.3", + }, + } } var _ = Describe("Subscription Functions", func() { @@ -58,39 +62,39 @@ var _ = Describe("Subscription Functions", func() { var fakeOlmClientSet *olmclient.Clientset BeforeEach(func() { - testSubscription = defaultTestSubscription.DeepCopy() + s := newDefaultTestSubscription() + testSubscription = s.DeepCopy() fakeOlmClientSet = olmclient.NewSimpleClientset() }) - It("should error out with a non existing a Subscription", func() { + It("should not error out with a non existing a Subscription", func() { err := createSubscription(fakeOlmClientSet, testSubscription) Expect(err).ToNot(HaveOccurred()) - _, err = getSubscription(fakeOlmClientSet, "foo") - Expect(err).To(HaveOccurred()) + + sub, err := getSubscription(fakeOlmClientSet, "non-existing-sub", GitOpsLegacySubscriptionNamespace) + Expect(err).ToNot(HaveOccurred()) + Expect(sub).To(BeNil()) + }) It("should return a proper Subscription", func() { err := createSubscription(fakeOlmClientSet, testSubscription) Expect(err).ToNot(HaveOccurred()) - sub, err := getSubscription(fakeOlmClientSet, "foosubscription") + sub, err := getSubscription(fakeOlmClientSet, testSubscription.Name, testSubscription.Namespace) Expect(err).ToNot(HaveOccurred()) - Expect(sub.Spec.Channel).To(Equal("foochannel")) - Expect(sub.Spec.CatalogSource).To(Equal("foosource")) - Expect(sub.Spec.CatalogSourceNamespace).To(Equal("foosourcenamespace")) - Expect(sub.Spec.Package).To(Equal("foooperator")) - Expect(sub.Spec.Channel).To(Equal("foochannel")) - Expect(sub.Spec.StartingCSV).To(BeEmpty()) - Expect(sub.Spec.InstallPlanApproval).To(Equal(operatorv1alpha1.ApprovalAutomatic)) + Expect(sub).ToNot(BeNil()) + Expect(sub).To(Equal(testSubscription)) }) }) - Context("newSubscriptionFromConfigMap", func() { + Context("newSubscriptionFromConfigMap (operator in suggested namespace patterns-operator)", func() { var testConfigMap *corev1.ConfigMap var fakeClientSet *kubeclient.Clientset BeforeEach(func() { fakeClientSet = kubeclient.NewSimpleClientset() - testConfigMap = defaultTestSubConfigMap.DeepCopy() + cm := newDefaultTestSubConfigMap() + testConfigMap = cm.DeepCopy() }) It("should handle the absence of the ConfigMap gracefully", func() { @@ -103,20 +107,70 @@ var _ = Describe("Subscription Functions", func() { Expect(sub.Spec.Channel).To(Equal(GitOpsDefaultChannel)) Expect(sub.Spec.StartingCSV).To(BeEmpty()) Expect(sub.Spec.InstallPlanApproval).To(Equal(operatorv1alpha1.ApprovalAutomatic)) + Expect(sub.Namespace).To(Equal(GitOpsDefaultSubscriptionNamespace)) + Expect(sub.Name).To(Equal(GitOpsDefaultPackageName)) }) It("should create a Subscription from a configmap", func() { - _, err := fakeClientSet.CoreV1().ConfigMaps(OperatorNamespace).Create(context.Background(), testConfigMap, metav1.CreateOptions{}) + _, err := fakeClientSet.CoreV1().ConfigMaps(DetectOperatorNamespace()).Create(context.Background(), testConfigMap, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) + sub, err := newSubscriptionFromConfigMap(fakeClientSet) + Expect(err).ToNot(HaveOccurred()) + Expect(sub).NotTo(BeNil()) + Expect(sub.Spec.CatalogSource).To(Equal("foo-source")) + Expect(sub.Spec.CatalogSourceNamespace).To(Equal("foo-source-namespace")) + Expect(sub.Spec.Package).To(Equal(GitOpsDefaultPackageName)) + Expect(sub.Spec.Channel).To(Equal("foo-channel")) + Expect(sub.Spec.StartingCSV).To(Equal("1.2.3")) + Expect(sub.Spec.InstallPlanApproval).To(Equal(operatorv1alpha1.ApprovalManual)) + Expect(sub.Namespace).To(Equal(GitOpsDefaultSubscriptionNamespace)) + Expect(sub.Name).To(Equal(GitOpsDefaultPackageName)) + }) + }) + + Context("newSubscriptionFromConfigMap (operator in legacy namespace openshift-operators)", func() { + var testConfigMap *corev1.ConfigMap + var fakeClientSet *kubeclient.Clientset + + BeforeEach(func() { + os.Setenv("OPERATOR_NAMESPACE", LegacyOperatorNamespace) + fakeClientSet = kubeclient.NewSimpleClientset() + cm := newDefaultTestSubConfigMap() + testConfigMap = cm.DeepCopy() + }) + + AfterEach(func() { + os.Setenv("OPERATOR_NAMESPACE", suggestedOperatorNamespace) + }) + + It("should create a Subscription to legacy operator ns", func() { + _, err := fakeClientSet.CoreV1().ConfigMaps(DetectOperatorNamespace()).Create(context.Background(), testConfigMap, metav1.CreateOptions{}) Expect(err).ToNot(HaveOccurred()) sub, err := newSubscriptionFromConfigMap(fakeClientSet) Expect(err).ToNot(HaveOccurred()) Expect(sub).NotTo(BeNil()) Expect(sub.Spec.CatalogSource).To(Equal("foo-source")) Expect(sub.Spec.CatalogSourceNamespace).To(Equal("foo-source-namespace")) - Expect(sub.Spec.Package).To(Equal("foo-name")) + Expect(sub.Spec.Package).To(Equal(GitOpsDefaultPackageName)) Expect(sub.Spec.Channel).To(Equal("foo-channel")) Expect(sub.Spec.StartingCSV).To(Equal("1.2.3")) Expect(sub.Spec.InstallPlanApproval).To(Equal(operatorv1alpha1.ApprovalManual)) + Expect(sub.Namespace).To(Equal(GitOpsLegacySubscriptionNamespace)) + Expect(sub.Name).To(Equal(GitOpsDefaultPackageName)) + }) + + It("should handle the absence of the ConfigMap gracefully (legacy operator ns)", func() { + sub, err := newSubscriptionFromConfigMap(fakeClientSet) + Expect(err).ToNot(HaveOccurred()) + Expect(sub).NotTo(BeNil()) + Expect(sub.Spec.CatalogSource).To(Equal(GitOpsDefaultCatalogSource)) + Expect(sub.Spec.CatalogSourceNamespace).To(Equal(GitOpsDefaultCatalogSourceNamespace)) + Expect(sub.Spec.Package).To(Equal(GitOpsDefaultPackageName)) + Expect(sub.Spec.Channel).To(Equal(GitOpsDefaultChannel)) + Expect(sub.Spec.StartingCSV).To(BeEmpty()) + Expect(sub.Spec.InstallPlanApproval).To(Equal(operatorv1alpha1.ApprovalAutomatic)) + Expect(sub.Namespace).To(Equal(GitOpsLegacySubscriptionNamespace)) + Expect(sub.Name).To(Equal(GitOpsDefaultPackageName)) }) }) }) @@ -131,7 +185,7 @@ var _ = Describe("UpdateSubscription", func() { BeforeEach(func() { client = olmclient.NewSimpleClientset() - subscriptionNs = "openshift-operators" + subscriptionNs = GitOpsLegacySubscriptionNamespace current = &operatorv1alpha1.Subscription{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index a0c19e6b6..3f4e2f1e4 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -42,6 +42,10 @@ var testEnv *envtest.Environment var tempDir string var gitOpsImpl *GitOperationsImpl +// suggestedOperatorNamespace is the namespace used when testing the "new" install path +// (operator in patterns-operator ns, subscription in openshift-gitops-operator). +const suggestedOperatorNamespace = "patterns-operator" + func TestAPIs(t *testing.T) { RegisterFailHandler(Fail) @@ -62,6 +66,9 @@ func cleanupTempDir(tempDir string) { var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + // Set OPERATOR_NAMESPACE env var so DetectOperatorNamespace() returns the test namespace + os.Setenv("OPERATOR_NAMESPACE", suggestedOperatorNamespace) + By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, diff --git a/internal/controller/utils.go b/internal/controller/utils.go index b827633ae..212a4d3d6 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -428,5 +428,5 @@ func GetOperatorConfigmap() (*corev1.ConfigMap, error) { return nil, fmt.Errorf("failed to call NewForConfig: %s", err) } - return clientset.CoreV1().ConfigMaps(OperatorNamespace).Get(context.Background(), OperatorConfigMap, metav1.GetOptions{}) + return clientset.CoreV1().ConfigMaps(DetectOperatorNamespace()).Get(context.Background(), OperatorConfigMap, metav1.GetOptions{}) }