Skip to content

Commit 2248cc6

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1428 from ecordell/44-fix-nilref
[release-4.4] Bug 1818850: Add spec validation and improved status for CatalogSources
2 parents 3f7675c + e423979 commit 2248cc6

File tree

6 files changed

+281
-28
lines changed

6 files changed

+281
-28
lines changed

‎pkg/api/apis/operators/v1alpha1/catalogsource_types.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,11 @@ const (
2929
)
3030

3131
const (
32-
CatalogSourceConfigMapError ConditionReason = "ConfigMapError"
32+
// CatalogSourceSpecInvalidError denotes when fields on the spec of the CatalogSource are not valid.
33+
CatalogSourceSpecInvalidError ConditionReason = "SpecInvalidError"
34+
// CatalogSourceConfigMapError denotes when there is an issue extracting manifests from the specified ConfigMap.
35+
CatalogSourceConfigMapError ConditionReason = "ConfigMapError"
36+
// CatalogSourceRegistryServerError denotes when there is an issue querying the specified registry server.
3337
CatalogSourceRegistryServerError ConditionReason = "RegistryServerError"
3438
)
3539

@@ -104,10 +108,10 @@ type GRPCConnectionState struct {
104108
}
105109

106110
type CatalogSourceStatus struct {
107-
// A human readable message indicating details about why the ClusterServiceVersion is in this condition.
111+
// A human readable message indicating details about why the CatalogSource is in this condition.
108112
// +optional
109113
Message string `json:"message,omitempty"`
110-
// Reason is the reason the Subscription was transitioned to its current state.
114+
// Reason is the reason the CatalogSource was transitioned to its current state.
111115
// +optional
112116
Reason ConditionReason `json:"reason,omitempty"`
113117

@@ -177,7 +181,6 @@ func (c *CatalogSource) Update() bool {
177181
logrus.WithField("CatalogSource", c.Name).Debugf("latest poll %v", *c.Status.LatestImageRegistryPoll)
178182
}
179183

180-
181184
if c.Status.LatestImageRegistryPoll.IsZero() {
182185
logrus.WithField("CatalogSource", c.Name).Debugf("creation timestamp plus interval before now %t", c.CreationTimestamp.Add(interval).Before(time.Now()))
183186
if c.CreationTimestamp.Add(interval).Before(time.Now()) {

‎pkg/controller/operators/catalog/operator.go

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,35 @@ func (o *Operator) handleCatSrcDeletion(obj interface{}) {
485485
o.logger.WithField("source", sourceKey).Info("removed client for deleted catalogsource")
486486
}
487487

488+
func validateSourceType(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, _ error) {
489+
out = in
490+
var err error
491+
switch sourceType := out.Spec.SourceType; sourceType {
492+
case v1alpha1.SourceTypeInternal, v1alpha1.SourceTypeConfigmap:
493+
if out.Spec.ConfigMap == "" {
494+
err = fmt.Errorf("configmap name unset: must be set for sourcetype: %s", sourceType)
495+
}
496+
case v1alpha1.SourceTypeGrpc:
497+
if out.Spec.Image == "" && out.Spec.Address == "" {
498+
err = fmt.Errorf("image and address unset: at least one must be set for sourcetype: %s", sourceType)
499+
}
500+
default:
501+
err = fmt.Errorf("unknown sourcetype: %s", sourceType)
502+
}
503+
if err != nil {
504+
out.SetError(v1alpha1.CatalogSourceSpecInvalidError, err)
505+
return
506+
}
507+
508+
// The sourceType is valid, clear (all) status if there's existing invalid spec reason
509+
if out.Status.Reason == v1alpha1.CatalogSourceSpecInvalidError {
510+
out.Status = v1alpha1.CatalogSourceStatus{}
511+
}
512+
continueSync = true
513+
514+
return
515+
}
516+
488517
func (o *Operator) syncConfigMap(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, syncError error) {
489518
out = in
490519
if !(in.Spec.SourceType == v1alpha1.SourceTypeInternal || in.Spec.SourceType == v1alpha1.SourceTypeConfigmap) {
@@ -705,6 +734,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
705734
}
706735

707736
chain := []CatalogSourceSyncFunc{
737+
validateSourceType,
708738
o.syncConfigMap,
709739
o.syncRegistryServer,
710740
o.syncConnection,
@@ -715,8 +745,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
715745

716746
out, syncError := syncFunc(in, chain)
717747

718-
if equalFunc(&in.Status, &out.Status) {
719-
logger.Debug("no change in status, skipping status update")
748+
if equalFunc(&catsrc.Status, &out.Status) {
720749
return
721750
}
722751

‎pkg/controller/operators/catalog/operator_test.go

Lines changed: 63 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -719,23 +719,13 @@ func TestSyncCatalogSources(t *testing.T) {
719719
UID: types.UID("catalog-uid"),
720720
},
721721
Spec: v1alpha1.CatalogSourceSpec{
722-
ConfigMap: "cool-configmap",
723722
SourceType: "nope",
724723
},
725724
},
726-
k8sObjs: []runtime.Object{
727-
&corev1.ConfigMap{
728-
ObjectMeta: metav1.ObjectMeta{
729-
Name: "cool-configmap",
730-
Namespace: "cool-namespace",
731-
UID: types.UID("configmap-uid"),
732-
ResourceVersion: "resource-version",
733-
},
734-
Data: fakeConfigMapData(),
735-
},
725+
expectedStatus: &v1alpha1.CatalogSourceStatus{
726+
Message: "unknown sourcetype: nope",
727+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
736728
},
737-
expectedStatus: nil,
738-
expectedError: fmt.Errorf("no reconciler for source type nope"),
739729
},
740730
{
741731
testName: "CatalogSourceWithBackingConfigMap",
@@ -877,6 +867,66 @@ func TestSyncCatalogSources(t *testing.T) {
877867
pod(*grpcCatalog),
878868
},
879869
},
870+
{
871+
testName: "CatalogSourceWithGrpcType/EnsuresImageOrAddressIsSet",
872+
namespace: "cool-namespace",
873+
catalogSource: &v1alpha1.CatalogSource{
874+
ObjectMeta: metav1.ObjectMeta{
875+
Name: "invalid-spec-catalog",
876+
Namespace: "cool-namespace",
877+
UID: types.UID("catalog-uid"),
878+
Labels: map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
879+
},
880+
Spec: v1alpha1.CatalogSourceSpec{
881+
SourceType: v1alpha1.SourceTypeGrpc,
882+
},
883+
},
884+
expectedStatus: &v1alpha1.CatalogSourceStatus{
885+
Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
886+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
887+
},
888+
expectedError: nil,
889+
},
890+
{
891+
testName: "CatalogSourceWithInternalType/EnsuresConfigMapIsSet",
892+
namespace: "cool-namespace",
893+
catalogSource: &v1alpha1.CatalogSource{
894+
ObjectMeta: metav1.ObjectMeta{
895+
Name: "invalid-spec-catalog",
896+
Namespace: "cool-namespace",
897+
UID: types.UID("catalog-uid"),
898+
Labels: map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
899+
},
900+
Spec: v1alpha1.CatalogSourceSpec{
901+
SourceType: v1alpha1.SourceTypeInternal,
902+
},
903+
},
904+
expectedStatus: &v1alpha1.CatalogSourceStatus{
905+
Message: fmt.Sprintf("configmap name unset: must be set for sourcetype: %s", v1alpha1.SourceTypeInternal),
906+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
907+
},
908+
expectedError: nil,
909+
},
910+
{
911+
testName: "CatalogSourceWithConfigMapType/EnsuresConfigMapIsSet",
912+
namespace: "cool-namespace",
913+
catalogSource: &v1alpha1.CatalogSource{
914+
ObjectMeta: metav1.ObjectMeta{
915+
Name: "invalid-spec-catalog",
916+
Namespace: "cool-namespace",
917+
UID: types.UID("catalog-uid"),
918+
Labels: map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
919+
},
920+
Spec: v1alpha1.CatalogSourceSpec{
921+
SourceType: v1alpha1.SourceTypeConfigmap,
922+
},
923+
},
924+
expectedStatus: &v1alpha1.CatalogSourceStatus{
925+
Message: fmt.Sprintf("configmap name unset: must be set for sourcetype: %s", v1alpha1.SourceTypeConfigmap),
926+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
927+
},
928+
expectedError: nil,
929+
},
880930
}
881931
for _, tt := range tests {
882932
t.Run(tt.testName, func(t *testing.T) {

‎pkg/controller/operators/catalog/subscription/reconciler.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,18 @@ func (c *catalogHealthReconciler) health(now *metav1.Time, catalog *v1alpha1.Cat
182182
// healthy returns true if the given catalog is healthy, false otherwise, and any error encountered
183183
// while checking the catalog's registry server.
184184
func (c *catalogHealthReconciler) healthy(catalog *v1alpha1.CatalogSource) (bool, error) {
185-
return c.registryReconcilerFactory.ReconcilerForSource(catalog).CheckRegistryServer(catalog)
185+
if catalog.Status.Reason == v1alpha1.CatalogSourceSpecInvalidError {
186+
// The catalog's spec is bad, mark unhealthy
187+
return false, nil
188+
}
189+
190+
// Check connection health
191+
rec := c.registryReconcilerFactory.ReconcilerForSource(catalog)
192+
if rec == nil {
193+
return false, fmt.Errorf("could not get reconciler for catalog: %#v", catalog)
194+
}
195+
196+
return rec.CheckRegistryServer(catalog)
186197
}
187198

188199
// installPlanReconciler reconciles InstallPlan status for Subscriptions.

‎pkg/controller/operators/catalog/subscription/reconciler_test.go

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package subscription
22

33
import (
44
"context"
5+
"fmt"
56
"testing"
67
"time"
78

@@ -100,6 +101,76 @@ func TestCatalogHealthReconcile(t *testing.T) {
100101
),
101102
},
102103
},
104+
{
105+
description: "ExistsToUnhealthy/InvalidCatalogSpec",
106+
fields: fields{
107+
config: &fakeReconcilerConfig{
108+
now: nowFunc,
109+
globalCatalogNamespace: "global",
110+
existingObjs: existingObjs{
111+
clientObjs: []runtime.Object{
112+
func() *v1alpha1.CatalogSource {
113+
cs := catalogSource("ns", "cs-0")
114+
cs.Spec = v1alpha1.CatalogSourceSpec{
115+
SourceType: v1alpha1.SourceTypeGrpc,
116+
}
117+
cs.Status = v1alpha1.CatalogSourceStatus{
118+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
119+
Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
120+
}
121+
122+
return cs
123+
}(),
124+
&v1alpha1.Subscription{
125+
ObjectMeta: metav1.ObjectMeta{
126+
Name: "sub",
127+
Namespace: "ns",
128+
},
129+
Spec: &v1alpha1.SubscriptionSpec{
130+
CatalogSourceNamespace: "ns",
131+
CatalogSource: "cs-0",
132+
},
133+
},
134+
},
135+
},
136+
},
137+
},
138+
args: args{
139+
in: newSubscriptionExistsState(
140+
&v1alpha1.Subscription{
141+
ObjectMeta: metav1.ObjectMeta{
142+
Name: "sub",
143+
Namespace: "ns",
144+
},
145+
Spec: &v1alpha1.SubscriptionSpec{
146+
CatalogSourceNamespace: "ns",
147+
CatalogSource: "cs-0",
148+
},
149+
},
150+
),
151+
},
152+
want: want{
153+
out: newCatalogUnhealthyState(&v1alpha1.Subscription{
154+
ObjectMeta: metav1.ObjectMeta{
155+
Name: "sub",
156+
Namespace: "ns",
157+
},
158+
Spec: &v1alpha1.SubscriptionSpec{
159+
CatalogSourceNamespace: "ns",
160+
CatalogSource: "cs-0",
161+
},
162+
Status: v1alpha1.SubscriptionStatus{
163+
CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
164+
catalogHealth("ns", "cs-0", &now, false),
165+
},
166+
Conditions: []v1alpha1.SubscriptionCondition{
167+
catalogUnhealthyCondition(corev1.ConditionTrue, v1alpha1.CatalogSourcesAdded, "targeted catalogsource ns/cs-0 unhealthy", &now),
168+
},
169+
LastUpdated: now,
170+
},
171+
}),
172+
},
173+
},
103174
{
104175
description: "ExistsToUnhealthy/Catalogs/NoChanges",
105176
fields: fields{
@@ -342,6 +413,94 @@ func TestCatalogHealthReconcile(t *testing.T) {
342413
}),
343414
},
344415
},
416+
{
417+
description: "HealthyToUnhealthy/InvalidCatalogSpec",
418+
fields: fields{
419+
config: &fakeReconcilerConfig{
420+
now: nowFunc,
421+
globalCatalogNamespace: "global",
422+
existingObjs: existingObjs{
423+
clientObjs: []runtime.Object{
424+
func() *v1alpha1.CatalogSource {
425+
cs := catalogSource("ns", "cs-0")
426+
cs.Spec = v1alpha1.CatalogSourceSpec{
427+
SourceType: v1alpha1.SourceTypeGrpc,
428+
}
429+
cs.Status = v1alpha1.CatalogSourceStatus{
430+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
431+
Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
432+
}
433+
434+
return cs
435+
}(),
436+
&v1alpha1.Subscription{
437+
ObjectMeta: metav1.ObjectMeta{
438+
Name: "sub",
439+
Namespace: "ns",
440+
},
441+
Spec: &v1alpha1.SubscriptionSpec{
442+
CatalogSourceNamespace: "ns",
443+
CatalogSource: "cs-0",
444+
},
445+
Status: v1alpha1.SubscriptionStatus{
446+
CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
447+
catalogHealth("ns", "cs-0", &earlier, true),
448+
},
449+
Conditions: []v1alpha1.SubscriptionCondition{
450+
catalogUnhealthyCondition(corev1.ConditionFalse, v1alpha1.AllCatalogSourcesHealthy, "all available catalogsources are healthy", &earlier),
451+
},
452+
LastUpdated: earlier,
453+
},
454+
},
455+
},
456+
},
457+
},
458+
},
459+
args: args{
460+
in: newSubscriptionExistsState(
461+
&v1alpha1.Subscription{
462+
ObjectMeta: metav1.ObjectMeta{
463+
Name: "sub",
464+
Namespace: "ns",
465+
},
466+
Spec: &v1alpha1.SubscriptionSpec{
467+
CatalogSourceNamespace: "ns",
468+
CatalogSource: "cs-0",
469+
},
470+
Status: v1alpha1.SubscriptionStatus{
471+
CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
472+
catalogHealth("ns", "cs-0", &earlier, true),
473+
},
474+
Conditions: []v1alpha1.SubscriptionCondition{
475+
catalogUnhealthyCondition(corev1.ConditionFalse, v1alpha1.AllCatalogSourcesHealthy, "all available catalogsources are healthy", &earlier),
476+
},
477+
LastUpdated: earlier,
478+
},
479+
},
480+
),
481+
},
482+
want: want{
483+
out: newCatalogUnhealthyState(&v1alpha1.Subscription{
484+
ObjectMeta: metav1.ObjectMeta{
485+
Name: "sub",
486+
Namespace: "ns",
487+
},
488+
Spec: &v1alpha1.SubscriptionSpec{
489+
CatalogSourceNamespace: "ns",
490+
CatalogSource: "cs-0",
491+
},
492+
Status: v1alpha1.SubscriptionStatus{
493+
CatalogHealth: []v1alpha1.SubscriptionCatalogHealth{
494+
catalogHealth("ns", "cs-0", &now, false),
495+
},
496+
Conditions: []v1alpha1.SubscriptionCondition{
497+
catalogUnhealthyCondition(corev1.ConditionTrue, v1alpha1.CatalogSourcesUpdated, "targeted catalogsource ns/cs-0 unhealthy", &now),
498+
},
499+
LastUpdated: now,
500+
},
501+
}),
502+
},
503+
},
345504
}
346505
for _, tt := range tests {
347506
t.Run(tt.description, func(t *testing.T) {

0 commit comments

Comments
 (0)