Repository: operator-framework/operator-lifecycle-manager
Pull request #1428 (2020-04-06 14:16:07)

[release-4.4] Bug 1818850: Add spec validation and improved status for CatalogSources
<!--

Before making a PR, please read our contributing guidelines https://github.com/operator-framework/operator-lifecycle-manager/blob/master/CONTRIBUTING.MD

Note: Make sure your branch is rebased to the latest upstream master.

-->

**Description of the change:**
Cherry pick of #1414

**Motivation for the change:**

**Reviewer Checklist**
- [ ] Implementation matches the proposed design, or proposal is updated to match implementation
- [ ] Sufficient unit test coverage 
- [ ] Sufficient end-to-end test coverage
- [ ] Docs updated or added to `/docs` 
- [ ] Commit messages sensible and descriptive


<!--

Note: If this PR is fixing an issue make sure to add a note saying:
Closes #<ISSUE_NUMBER>

-->



Patch:
diff --git a/pkg/api/apis/operators/v1alpha1/catalogsource_types.go b/pkg/api/apis/operators/v1alpha1/catalogsource_types.go
index 00f45c11a..b89412dd3 100644
--- a/pkg/api/apis/operators/v1alpha1/catalogsource_types.go
+++ b/pkg/api/apis/operators/v1alpha1/catalogsource_types.go
@@ -29,7 +29,11 @@ const (
 )
 
 const (
-	CatalogSourceConfigMapError      ConditionReason = "ConfigMapError"
+	// CatalogSourceSpecInvalidError denotes when fields on the spec of the CatalogSource are not valid.
+	CatalogSourceSpecInvalidError ConditionReason = "SpecInvalidError"
+	// CatalogSourceConfigMapError denotes when there is an issue extracting manifests from the specified ConfigMap.
+	CatalogSourceConfigMapError ConditionReason = "ConfigMapError"
+	// CatalogSourceRegistryServerError denotes when there is an issue querying the specified registry server.
 	CatalogSourceRegistryServerError ConditionReason = "RegistryServerError"
 )
 
@@ -104,10 +108,10 @@ type GRPCConnectionState struct {
 }
 
 type CatalogSourceStatus struct {
-	// A human readable message indicating details about why the ClusterServiceVersion is in this condition.
+	// A human readable message indicating details about why the CatalogSource is in this condition.
 	// +optional
 	Message string `json:"message,omitempty"`
-	// Reason is the reason the Subscription was transitioned to its current state.
+	// Reason is the reason the CatalogSource was transitioned to its current state.
 	// +optional
 	Reason ConditionReason `json:"reason,omitempty"`
 
@@ -177,7 +181,6 @@ func (c *CatalogSource) Update() bool {
 		logrus.WithField("CatalogSource", c.Name).Debugf("latest poll %v", *c.Status.LatestImageRegistryPoll)
 	}
 
-
 	if c.Status.LatestImageRegistryPoll.IsZero() {
 		logrus.WithField("CatalogSource", c.Name).Debugf("creation timestamp plus interval before now %t", c.CreationTimestamp.Add(interval).Before(time.Now()))
 		if c.CreationTimestamp.Add(interval).Before(time.Now()) {
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index bf2be8bc5..330977850 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/pkg/controller/operators/catalog/operator.go
@@ -485,6 +485,35 @@ func (o *Operator) handleCatSrcDeletion(obj interface{}) {
 	o.logger.WithField("source", sourceKey).Info("removed client for deleted catalogsource")
 }
 
+func validateSourceType(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, _ error) {
+	out = in
+	var err error
+	switch sourceType := out.Spec.SourceType; sourceType {
+	case v1alpha1.SourceTypeInternal, v1alpha1.SourceTypeConfigmap:
+		if out.Spec.ConfigMap == "" {
+			err = fmt.Errorf("configmap name unset: must be set for sourcetype: %s", sourceType)
+		}
+	case v1alpha1.SourceTypeGrpc:
+		if out.Spec.Image == "" && out.Spec.Address == "" {
+			err = fmt.Errorf("image and address unset: at least one must be set for sourcetype: %s", sourceType)
+		}
+	default:
+		err = fmt.Errorf("unknown sourcetype: %s", sourceType)
+	}
+	if err != nil {
+		out.SetError(v1alpha1.CatalogSourceSpecInvalidError, err)
+		return
+	}
+
+	// The sourceType is valid, clear (all) status if there's existing invalid spec reason
+	if out.Status.Reason == v1alpha1.CatalogSourceSpecInvalidError {
+		out.Status = v1alpha1.CatalogSourceStatus{}
+	}
+	continueSync = true
+
+	return
+}
+
 func (o *Operator) syncConfigMap(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, syncError error) {
 	out = in
 	if !(in.Spec.SourceType == v1alpha1.SourceTypeInternal || in.Spec.SourceType == v1alpha1.SourceTypeConfigmap) {
@@ -705,6 +734,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
 	}
 
 	chain := []CatalogSourceSyncFunc{
+		validateSourceType,
 		o.syncConfigMap,
 		o.syncRegistryServer,
 		o.syncConnection,
@@ -715,8 +745,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
 
 	out, syncError := syncFunc(in, chain)
 
-	if equalFunc(&in.Status, &out.Status) {
-		logger.Debug("no change in status, skipping status update")
+	if equalFunc(&catsrc.Status, &out.Status) {
 		return
 	}
 
diff --git a/pkg/controller/operators/catalog/subscription/reconciler.go b/pkg/controller/operators/catalog/subscription/reconciler.go
index 6cb9b2cb3..d716e6dd0 100644
--- a/pkg/controller/operators/catalog/subscription/reconciler.go
+++ b/pkg/controller/operators/catalog/subscription/reconciler.go
@@ -182,7 +182,18 @@ func (c *catalogHealthReconciler) health(now *metav1.Time, catalog *v1alpha1.Cat
 // healthy returns true if the given catalog is healthy, false otherwise, and any error encountered
 // while checking the catalog's registry server.
 func (c *catalogHealthReconciler) healthy(catalog *v1alpha1.CatalogSource) (bool, error) {
-	return c.registryReconcilerFactory.ReconcilerForSource(catalog).CheckRegistryServer(catalog)
+	if catalog.Status.Reason == v1alpha1.CatalogSourceSpecInvalidError {
+		// The catalog's spec is bad, mark unhealthy
+		return false, nil
+	}
+
+	// Check connection health
+	rec := c.registryReconcilerFactory.ReconcilerForSource(catalog)
+	if rec == nil {
+		return false, fmt.Errorf("could not get reconciler for catalog: %#v", catalog)
+	}
+
+	return rec.CheckRegistryServer(catalog)
 }
 
 // installPlanReconciler reconciles InstallPlan status for Subscriptions.


Test patch:
diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
index 36a886863..4c9f22149 100644
--- a/pkg/controller/operators/catalog/operator_test.go
+++ b/pkg/controller/operators/catalog/operator_test.go
@@ -719,23 +719,13 @@ func TestSyncCatalogSources(t *testing.T) {
 					UID:       types.UID("catalog-uid"),
 				},
 				Spec: v1alpha1.CatalogSourceSpec{
-					ConfigMap:  "cool-configmap",
 					SourceType: "nope",
 				},
 			},
-			k8sObjs: []runtime.Object{
-				&corev1.ConfigMap{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:            "cool-configmap",
-						Namespace:       "cool-namespace",
-						UID:             types.UID("configmap-uid"),
-						ResourceVersion: "resource-version",
-					},
-					Data: fakeConfigMapData(),
-				},
+			expectedStatus: &v1alpha1.CatalogSourceStatus{
+				Message: "unknown sourcetype: nope",
+				Reason:  v1alpha1.CatalogSourceSpecInvalidError,
 			},
-			expectedStatus: nil,
-			expectedError:  fmt.Errorf("no reconciler for source type nope"),
 		},
 		{
 			testName:      "CatalogSourceWithBackingConfigMap",
@@ -877,6 +867,66 @@ func TestSyncCatalogSources(t *testing.T) {
 				pod(*grpcCatalog),
 			},
 		},
+		{
+			testName:  "CatalogSourceWithGrpcType/EnsuresImageOrAddressIsSet",
+			namespace: "cool-namespace",
+			catalogSource: &v1alpha1.CatalogSource{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "invalid-spec-catalog",
+					Namespace: "cool-namespace",
+					UID:       types.UID("catalog-uid"),
+					Labels:    map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
+				},
+				Spec: v1alpha1.CatalogSourceSpec{
+					SourceType: v1alpha1.SourceTypeGrpc,
+				},
+			},
+			expectedStatus: &v1alpha1.CatalogSourceStatus{
+				Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
+				Reason:  v1alpha1.CatalogSourceSpecInvalidError,
+			},
+			expectedError: nil,
+		},
+		{
+			testName:  "CatalogSourceWithInternalType/EnsuresConfigMapIsSet",
+			namespace: "cool-namespace",
+			catalogSource: &v1alpha1.CatalogSource{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "invalid-spec-catalog",
+					Namespace: "cool-namespace",
+					UID:       types.UID("catalog-uid"),
+					Labels:    map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
+				},
+				Spec: v1alpha1.CatalogSourceSpec{
+					SourceType: v1alpha1.SourceTypeInternal,
+				},
+			},
+			expectedStatus: &v1alpha1.CatalogSourceStatus{
+				Message: fmt.Sprintf("configmap name unset: must be set for sourcetype: %s", v1alpha1.SourceTypeInternal),
+				Reason:  v1alpha1.CatalogSourceSpecInvalidError,
+			},
+			expectedError: nil,
+		},
+		{
+			testName:  "CatalogSourceWithConfigMapType/EnsuresConfigMapIsSet",
+			namespace: "cool-namespace",
+			catalogSource: &v1alpha1.CatalogSource{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "invalid-spec-catalog",
+					Namespace: "cool-namespace",
+					UID:       types.UID("catalog-uid"),
+					Labels:    map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
+				},
+				Spec: v1alpha1.CatalogSourceSpec{
+					SourceType: v1alpha1.SourceTypeConfigmap,
+				},
+			},
+			expectedStatus: &v1alpha1.CatalogSourceStatus{
+				Message: fmt.Sprintf("configmap name unset: must be set for sourcetype: %s", v1alpha1.SourceTypeConfigmap),
+				Reason:  v1alpha1.CatalogSourceSpecInvalidError,
+			},
+			expectedError: nil,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.testName, func(t *testing.T) {
diff --git a/pkg/controller/operators/catalog/subscription/reconciler_test.go b/pkg/controller/operators/catalog/subscription/reconciler_test.go
index 215d7356f..953e1f2d8 100644
--- a/pkg/controller/operators/catalog/subscription/reconciler_test.go
+++ b/pkg/controller/operators/catalog/subscription/reconciler_test.go
@@ -2,6 +2,7 @@ package subscription
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -100,6 +101,76 @@ func TestCatalogHealthReconcile(t *testing.T) {
 				),
 			},
 		},
+		{
+			description: "ExistsToUnhealthy/InvalidCatalogSpec",
+			fields: fields{
+				config: &fakeReconcilerConfig{
+					now:                    nowFunc,
+					globalCatalogNamespace: "global",
+					existingObjs: existingObjs{
+						clientObjs: []runtime.Object{
+							func() *v1alpha1.CatalogSource {
+								cs := catalogSource("ns", "cs-0")
+								cs.Spec = v1alpha1.CatalogSourceSpec{
+									SourceType: v1alpha1.SourceTypeGrpc,
+								}
+								cs.Status = v1alpha1.CatalogSourceStatus{
+									Reason:  v1alpha1.CatalogSourceSpecInvalidError,
+									Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
+								}
+
+								return cs
+							}(),
+							&v1alpha1.Subscription{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "sub",
+									Namespace: "ns",
+								},
+								Spec: &v1alpha1.SubscriptionSpec{
+									CatalogSourceNamespace: "ns",
+									CatalogSource:          "cs-0",
+								},
+							},
+						},
+					},
+				},
+			},
+			args: args{
+				in: newSubscriptionExistsState(
+					&v1alpha1.Subscription{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "sub",
+							Namespace: "ns",
+						},
+						Spec: &v1alpha1.SubscriptionSpec{
+							CatalogSourceNamespace: "ns",
+							CatalogSource:          "cs-0",
+						},
+					},
+				),
+			},
+			want: want{
+				out: newCatalogUnhealthyState(&v1alpha1.Subscription{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: "ns",
+					},
+					Spec: &v1alpha1.SubscriptionSpec{
+						CatalogSourceNamespace: "ns",
+						CatalogSource:          "cs-0",
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
+							catalogHealth("ns", "cs-0", &now, false),
+						},
+						Conditions: []v1alpha1.SubscriptionCondition{
+							catalogUnhealthyCondition(corev1.ConditionTrue, v1alpha1.CatalogSourcesAdded, "targeted catalogsource ns/cs-0 unhealthy", &now),
+						},
+						LastUpdated: now,
+					},
+				}),
+			},
+		},
 		{
 			description: "ExistsToUnhealthy/Catalogs/NoChanges",
 			fields: fields{
@@ -342,6 +413,94 @@ func TestCatalogHealthReconcile(t *testing.T) {
 				}),
 			},
 		},
+		{
+			description: "HealthyToUnhealthy/InvalidCatalogSpec",
+			fields: fields{
+				config: &fakeReconcilerConfig{
+					now:                    nowFunc,
+					globalCatalogNamespace: "global",
+					existingObjs: existingObjs{
+						clientObjs: []runtime.Object{
+							func() *v1alpha1.CatalogSource {
+								cs := catalogSource("ns", "cs-0")
+								cs.Spec = v1alpha1.CatalogSourceSpec{
+									SourceType: v1alpha1.SourceTypeGrpc,
+								}
+								cs.Status = v1alpha1.CatalogSourceStatus{
+									Reason:  v1alpha1.CatalogSourceSpecInvalidError,
+									Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
+								}
+
+								return cs
+							}(),
+							&v1alpha1.Subscription{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "sub",
+									Namespace: "ns",
+								},
+								Spec: &v1alpha1.SubscriptionSpec{
+									CatalogSourceNamespace: "ns",
+									CatalogSource:          "cs-0",
+								},
+								Status: v1alpha1.SubscriptionStatus{
+									CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
+										catalogHealth("ns", "cs-0", &earlier, true),
+									},
+									Conditions: []v1alpha1.SubscriptionCondition{
+										catalogUnhealthyCondition(corev1.ConditionFalse, v1alpha1.AllCatalogSourcesHealthy, "all available catalogsources are healthy", &earlier),
+									},
+									LastUpdated: earlier,
+								},
+							},
+						},
+					},
+				},
+			},
+			args: args{
+				in: newSubscriptionExistsState(
+					&v1alpha1.Subscription{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "sub",
+							Namespace: "ns",
+						},
+						Spec: &v1alpha1.SubscriptionSpec{
+							CatalogSourceNamespace: "ns",
+							CatalogSource:          "cs-0",
+						},
+						Status: v1alpha1.SubscriptionStatus{
+							CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
+								catalogHealth("ns", "cs-0", &earlier, true),
+							},
+							Conditions: []v1alpha1.SubscriptionCondition{
+								catalogUnhealthyCondition(corev1.ConditionFalse, v1alpha1.AllCatalogSourcesHealthy, "all available catalogsources are healthy", &earlier),
+							},
+							LastUpdated: earlier,
+						},
+					},
+				),
+			},
+			want: want{
+				out: newCatalogUnhealthyState(&v1alpha1.Subscription{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: "ns",
+					},
+					Spec: &v1alpha1.SubscriptionSpec{
+						CatalogSourceNamespace: "ns",
+						CatalogSource:          "cs-0",
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
+							catalogHealth("ns", "cs-0", &now, false),
+						},
+						Conditions: []v1alpha1.SubscriptionCondition{
+							catalogUnhealthyCondition(corev1.ConditionTrue, v1alpha1.CatalogSourcesUpdated, "targeted catalogsource ns/cs-0 unhealthy", &now),
+						},
+						LastUpdated: now,
+					},
+				}),
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.description, func(t *testing.T) {
diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go
index c3190fa71..e290dc1e4 100644
--- a/test/e2e/subscription_e2e_test.go
+++ b/test/e2e/subscription_e2e_test.go
@@ -11,14 +11,6 @@ import (
 	"github.com/blang/semver"
 	"github.com/ghodss/yaml"
 	configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/comparison"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/version"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -27,6 +19,15 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/clientcmd"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/comparison"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/version"
 )
 
 var doubleInstance = int32(2)