UpdateStrategy RegistryPoll with nil Interval (#2920)

Adds protection against a nil-pointer panic when an UpdateStrategy with nil value for RegistryPoll Interval is supplied. Also adds a unit test to ensure that the code does not panic but instead returns an error when this situation is encountered.

Signed-off-by: Daniel Franz <[email protected]>

Author: dtfranz

pkg/controller/operators/catalog/operator.go

@@ -749,16 +749,19 @@ func (o *Operator) syncRegistryServer(logger *logrus.Entry, in *v1alpha1.Catalog
 	logger.Debug("ensured registry server")
 
 	// requeue the catalog sync based on the polling interval, for accurate syncs of catalogs with polling enabled
-	if out.Spec.UpdateStrategy != nil {
-		if out.Spec.UpdateStrategy.RegistryPoll != nil {
-			if out.Spec.UpdateStrategy.RegistryPoll.ParsingError != "" && out.Status.Reason != v1alpha1.CatalogSourceIntervalInvalidError {
-				out.SetError(v1alpha1.CatalogSourceIntervalInvalidError, fmt.Errorf(out.Spec.UpdateStrategy.RegistryPoll.ParsingError))
-			}
-			logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String())
-			resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now())
-			o.catsrcQueueSet.RequeueAfter(out.GetNamespace(), out.GetName(), queueinformer.ResyncWithJitter(resyncPeriod, 0.1)())
+	if out.Spec.UpdateStrategy != nil && out.Spec.UpdateStrategy.RegistryPoll != nil {
+		if out.Spec.UpdateStrategy.Interval == nil {
+			syncError = fmt.Errorf("empty polling interval; cannot requeue registry server sync without a provided polling interval")
+			out.SetError(v1alpha1.CatalogSourceIntervalInvalidError, syncError)
 			return
 		}
+		if out.Spec.UpdateStrategy.RegistryPoll.ParsingError != "" && out.Status.Reason != v1alpha1.CatalogSourceIntervalInvalidError {
+			out.SetError(v1alpha1.CatalogSourceIntervalInvalidError, fmt.Errorf(out.Spec.UpdateStrategy.RegistryPoll.ParsingError))
+		}
+		logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String())
+		resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now())
+		o.catsrcQueueSet.RequeueAfter(out.GetNamespace(), out.GetName(), queueinformer.ResyncWithJitter(resyncPeriod, 0.1)())
+		return
 	}
 
 	if err := o.sources.Remove(sourceKey); err != nil {

pkg/controller/operators/catalog/operator_test.go

@@ -1415,6 +1415,54 @@ func TestValidateExistingCRs(t *testing.T) {
 	}
 }
 
+func TestSyncRegistryServer(t *testing.T) {
+	namespace := "ns"
+
+	tests := []struct {
+		testName   string
+		err        error
+		catSrc     *v1alpha1.CatalogSource
+		clientObjs []runtime.Object
+	}{
+		{
+			testName: "EmptyRegistryPoll",
+			err:      fmt.Errorf("empty polling interval; cannot requeue registry server sync without a provided polling interval"),
+			catSrc: &v1alpha1.CatalogSource{
+				Spec: v1alpha1.CatalogSourceSpec{
+					UpdateStrategy: &v1alpha1.UpdateStrategy{
+						RegistryPoll: &v1alpha1.RegistryPoll{},
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.testName, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.TODO())
+			defer cancel()
+
+			tt.clientObjs = append(tt.clientObjs, tt.catSrc)
+			op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(tt.clientObjs...))
+			require.NoError(t, err)
+
+			op.reconciler = &fakes.FakeRegistryReconcilerFactory{
+				ReconcilerForSourceStub: func(source *v1alpha1.CatalogSource) reconciler.RegistryReconciler {
+					return &fakes.FakeRegistryReconciler{
+						EnsureRegistryServerStub: func(source *v1alpha1.CatalogSource) error {
+							return nil
+						},
+					}
+				},
+			}
+			require.NotPanics(t, func() {
+				_, _, err = op.syncRegistryServer(logrus.NewEntry(op.logger), tt.catSrc)
+			})
+			require.Equal(t, tt.err, err)
+		})
+	}
+}
+
 func fakeConfigMapData() map[string]string {
 	data := make(map[string]string)
 	yaml, err := yaml.Marshal([]apiextensionsv1beta1.CustomResourceDefinition{crd("fake-crd")})