Skip to content

Commit 1702292

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1508 from benluddy/bz-1818851-status
Bug 1818851: feat(catalogs): add spec validation for sourcetypes
2 parents 502b8a0 + a202f6e commit 1702292

File tree

7 files changed

+277
-28
lines changed

7 files changed

+277
-28
lines changed

‎deploy/chart/templates/0000_50_olm_06-catalogsource.crd.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,10 @@ spec:
146146
type: string
147147
message:
148148
description: A human readable message indicating details about why the
149-
ClusterServiceVersion is in this condition.
149+
CatalogSource is in this condition.
150150
type: string
151151
reason:
152-
description: Reason is the reason the Subscription was transitioned
152+
description: Reason is the reason the CatalogSource was transitioned
153153
to its current state.
154154
type: string
155155
registryService:

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,11 @@ const (
2828
)
2929

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

@@ -85,10 +89,10 @@ type GRPCConnectionState struct {
8589
}
8690

8791
type CatalogSourceStatus struct {
88-
// A human readable message indicating details about why the ClusterServiceVersion is in this condition.
92+
// A human readable message indicating details about why the CatalogSource is in this condition.
8993
// +optional
9094
Message string `json:"message,omitempty"`
91-
// Reason is the reason the Subscription was transitioned to its current state.
95+
// Reason is the reason the CatalogSource was transitioned to its current state.
9296
// +optional
9397
Reason ConditionReason `json:"reason,omitempty"`
9498

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

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

464+
func validateSourceType(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, _ error) {
465+
out = in
466+
var err error
467+
switch sourceType := out.Spec.SourceType; sourceType {
468+
case v1alpha1.SourceTypeInternal, v1alpha1.SourceTypeConfigmap:
469+
if out.Spec.ConfigMap == "" {
470+
err = fmt.Errorf("configmap name unset: must be set for sourcetype: %s", sourceType)
471+
}
472+
case v1alpha1.SourceTypeGrpc:
473+
if out.Spec.Image == "" && out.Spec.Address == "" {
474+
err = fmt.Errorf("image and address unset: at least one must be set for sourcetype: %s", sourceType)
475+
}
476+
default:
477+
err = fmt.Errorf("unknown sourcetype: %s", sourceType)
478+
}
479+
if err != nil {
480+
out.SetError(v1alpha1.CatalogSourceSpecInvalidError, err)
481+
return
482+
}
483+
484+
// The sourceType is valid, clear (all) status if there's existing invalid spec reason
485+
if out.Status.Reason == v1alpha1.CatalogSourceSpecInvalidError {
486+
out.Status = v1alpha1.CatalogSourceStatus{}
487+
}
488+
continueSync = true
489+
490+
return
491+
}
492+
464493
func (o *Operator) syncConfigMap(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, syncError error) {
465494
out = in
466495
if !(in.Spec.SourceType == v1alpha1.SourceTypeInternal || in.Spec.SourceType == v1alpha1.SourceTypeConfigmap) {
@@ -678,6 +707,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
678707
}
679708

680709
chain := []CatalogSourceSyncFunc{
710+
validateSourceType,
681711
o.syncConfigMap,
682712
o.syncRegistryServer,
683713
o.syncConnection,
@@ -688,8 +718,7 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
688718

689719
out, syncError := syncFunc(in, chain)
690720

691-
if equalFunc(&in.Status, &out.Status) {
692-
logger.Debug("no change in status, skipping status update")
721+
if equalFunc(&catsrc.Status, &out.Status) {
693722
return
694723
}
695724

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

Lines changed: 63 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -565,23 +565,13 @@ func TestSyncCatalogSources(t *testing.T) {
565565
UID: types.UID("catalog-uid"),
566566
},
567567
Spec: v1alpha1.CatalogSourceSpec{
568-
ConfigMap: "cool-configmap",
569568
SourceType: "nope",
570569
},
571570
},
572-
k8sObjs: []runtime.Object{
573-
&corev1.ConfigMap{
574-
ObjectMeta: metav1.ObjectMeta{
575-
Name: "cool-configmap",
576-
Namespace: "cool-namespace",
577-
UID: types.UID("configmap-uid"),
578-
ResourceVersion: "resource-version",
579-
},
580-
Data: fakeConfigMapData(),
581-
},
571+
expectedStatus: &v1alpha1.CatalogSourceStatus{
572+
Message: "unknown sourcetype: nope",
573+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
582574
},
583-
expectedStatus: nil,
584-
expectedError: fmt.Errorf("no reconciler for source type nope"),
585575
},
586576
{
587577
testName: "CatalogSourceWithBackingConfigMap",
@@ -723,6 +713,66 @@ func TestSyncCatalogSources(t *testing.T) {
723713
pod(*grpcCatalog),
724714
},
725715
},
716+
{
717+
testName: "CatalogSourceWithGrpcType/EnsuresImageOrAddressIsSet",
718+
namespace: "cool-namespace",
719+
catalogSource: &v1alpha1.CatalogSource{
720+
ObjectMeta: metav1.ObjectMeta{
721+
Name: "invalid-spec-catalog",
722+
Namespace: "cool-namespace",
723+
UID: types.UID("catalog-uid"),
724+
Labels: map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
725+
},
726+
Spec: v1alpha1.CatalogSourceSpec{
727+
SourceType: v1alpha1.SourceTypeGrpc,
728+
},
729+
},
730+
expectedStatus: &v1alpha1.CatalogSourceStatus{
731+
Message: fmt.Sprintf("image and address unset: at least one must be set for sourcetype: %s", v1alpha1.SourceTypeGrpc),
732+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
733+
},
734+
expectedError: nil,
735+
},
736+
{
737+
testName: "CatalogSourceWithInternalType/EnsuresConfigMapIsSet",
738+
namespace: "cool-namespace",
739+
catalogSource: &v1alpha1.CatalogSource{
740+
ObjectMeta: metav1.ObjectMeta{
741+
Name: "invalid-spec-catalog",
742+
Namespace: "cool-namespace",
743+
UID: types.UID("catalog-uid"),
744+
Labels: map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
745+
},
746+
Spec: v1alpha1.CatalogSourceSpec{
747+
SourceType: v1alpha1.SourceTypeInternal,
748+
},
749+
},
750+
expectedStatus: &v1alpha1.CatalogSourceStatus{
751+
Message: fmt.Sprintf("configmap name unset: must be set for sourcetype: %s", v1alpha1.SourceTypeInternal),
752+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
753+
},
754+
expectedError: nil,
755+
},
756+
{
757+
testName: "CatalogSourceWithConfigMapType/EnsuresConfigMapIsSet",
758+
namespace: "cool-namespace",
759+
catalogSource: &v1alpha1.CatalogSource{
760+
ObjectMeta: metav1.ObjectMeta{
761+
Name: "invalid-spec-catalog",
762+
Namespace: "cool-namespace",
763+
UID: types.UID("catalog-uid"),
764+
Labels: map[string]string{"olm.catalogSource": "invalid-spec-catalog"},
765+
},
766+
Spec: v1alpha1.CatalogSourceSpec{
767+
SourceType: v1alpha1.SourceTypeConfigmap,
768+
},
769+
},
770+
expectedStatus: &v1alpha1.CatalogSourceStatus{
771+
Message: fmt.Sprintf("configmap name unset: must be set for sourcetype: %s", v1alpha1.SourceTypeConfigmap),
772+
Reason: v1alpha1.CatalogSourceSpecInvalidError,
773+
},
774+
expectedError: nil,
775+
},
726776
}
727777
for _, tt := range tests {
728778
t.Run(tt.testName, func(t *testing.T) {

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,12 @@ 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+
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
185191
rec := c.registryReconcilerFactory.ReconcilerForSource(catalog)
186192
if rec == nil {
187193
return false, fmt.Errorf("could not get reconciler for catalog: %#v", catalog)

‎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)