Skip to content

Commit ebae212

Browse files
(fix): report Unknown for deprecation status when catalog unavailable and prevent error leak
When a catalog is removed or unreachable, deprecation conditions now correctly show Unknown instead of False. BundleDeprecated now reflects the installed bundle and uses the correct reason. Key changes: - Defer deprecation status updates until end of reconciliation - Track hadCatalogDeprecationData to distinguish catalog absence from "not deprecated" - Report installed bundle deprecation, not the bundle being installed - BundleDeprecated uses Deprecated reason when bundle exists, Absent only when none - Prevent install/validation errors from leaking into deprecation conditions Scenarios fixed: - No catalog + no bundle → BundleDeprecated: Unknown/Absent - No catalog + bundle installed → BundleDeprecated: Unknown/Deprecated (was False/Absent) - During upgrade v1→v2 → shows v1 status, not v2 - Resolution fails with warnings → deprecation warnings still reach users Assisted-by: Cursor
1 parent b3f85d5 commit ebae212

File tree

14 files changed

+821
-144
lines changed

14 files changed

+821
-144
lines changed

PR_DESCRIPTION.md

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# Fix deprecation conditions when catalog is unavailable
2+
3+
Fixes #2008
4+
5+
## Description
6+
7+
This PR fixes how deprecation conditions are reported when the catalog is unavailable or resolution fails:
8+
9+
- **Show Unknown when catalog is unavailable**: When a catalog is removed or unreachable, deprecation conditions now correctly show `Unknown` instead of `False`, so users know "we don't know if it's deprecated" rather than "definitely not deprecated"
10+
11+
- **Report installed bundle deprecation, not resolved**: BundleDeprecated now reflects the currently installed bundle's status, not the bundle being installed during an upgrade
12+
13+
- **Keep deprecation data separate from errors**: Install and validation errors stay in Progressing/Installed conditions, never leak into deprecation conditions
14+
15+
- **Handle catalog deprecation data on failures**: When resolution fails but the catalog still returns deprecation warnings, those warnings now reach users
16+
17+
## TL;DR
18+
19+
`SetDeprecationStatus` now properly handles three scenarios:
20+
21+
**1. No catalog data (catalog removed or unreachable)**
22+
- All deprecation conditions → `Unknown`
23+
- BundleDeprecated reason → `Absent` only when no bundle installed, `Deprecated` when bundle exists
24+
25+
**2. Catalog available with deprecation warnings**
26+
- PackageDeprecated → `True` if catalog marks package deprecated
27+
- ChannelDeprecated → `True` if requested channel marked deprecated
28+
- BundleDeprecated → reflects the **installed** bundle (not the one being installed)
29+
- Deprecated (rollup) → `True` if any deprecation signal present
30+
31+
**3. Resolution fails but returns deprecation data**
32+
- Deprecation warnings shown even when installation cannot proceed
33+
- Progressing condition shows the resolution error
34+
- Deprecation conditions show catalog warnings separately
35+
36+
## Key Changes
37+
38+
### Controller Logic
39+
- Added defer to update deprecation status at the end of reconciliation
40+
- Track `hadCatalogDeprecationData` flag to distinguish "catalog absent" from "catalog says not deprecated"
41+
- Use installed bundle name from `revisionStates.Installed`, not the resolved bundle
42+
43+
### SetDeprecationStatus Function
44+
- Simplified from ~90 lines to ~50 lines using helper function
45+
- Handle `hasCatalogData=false` case to set all conditions Unknown
46+
- BundleDeprecated uses correct reason based on whether bundle exists
47+
48+
### Tests
49+
- Added `TestClusterExtensionResolutionFailsWithoutCatalogDeprecationData` - verifies Unknown status when catalog unavailable
50+
- Enhanced `TestSetDeprecationStatus` with 3 new test cases for catalog absence scenarios
51+
- Added scenario descriptions to all related tests for clarity
52+
53+
## Examples
54+
55+
### Before (incorrect behavior)
56+
```yaml
57+
# Bundle v1.0.0 installed, catalog removed
58+
Deprecated: False/Deprecated # ❌ Wrong - should be Unknown
59+
BundleDeprecated: False/Absent # ❌ Wrong - bundle exists but shows Absent
60+
```
61+
62+
### After (correct behavior)
63+
```yaml
64+
# Bundle v1.0.0 installed, catalog removed
65+
Deprecated: Unknown/Deprecated # ✅ Correct - we don't know
66+
BundleDeprecated: Unknown/Deprecated # ✅ Correct - bundle exists, catalog unknown
67+
```
68+
69+
### During Upgrade (fixed)
70+
```yaml
71+
# Upgrading from v1.0.0 to v1.0.1 (v1.0.1 not installed yet)
72+
BundleDeprecated: shows v1.0.0 status # ✅ Correct - reflects installed bundle
73+
```
74+
75+
## Testing
76+
77+
All tests pass:
78+
- ✅ `TestSetDeprecationStatus` - 11 test cases covering all deprecation scenarios
79+
- ✅ `TestClusterExtensionResolutionFailsWithoutCatalogDeprecationData` - catalog unavailable
80+
- ✅ `TestClusterExtensionResolutionFailsWithDeprecationData` - resolution fails with warnings
81+
- ✅ `TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors` - errors don't leak
82+
- ✅ All existing controller tests
83+
84+
## Files Changed
85+
86+
```
87+
13 files changed, 723 insertions(+), 144 deletions(-)
88+
```
89+
90+
**Controller:**
91+
- `clusterextension_controller.go` - defer logic, simplified SetDeprecationStatus
92+
- `clusterextension_controller_test.go` - new tests and scenario descriptions
93+
94+
**API/Docs:**
95+
- `clusterextension_types.go` - updated deprecation condition documentation
96+
- `olmv1-api-reference.md` - updated API docs
97+
- CRD manifests - regenerated with updated descriptions
98+

api/v1/clusterextension_types.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -483,12 +483,15 @@ type ClusterExtensionStatus struct {
483483
// When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.
484484
// When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.
485485
//
486-
// When the ClusterExtension is sourced from a catalog, if may also communicate a deprecation condition.
486+
// When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.
487487
// These are indications from a package owner to guide users away from a particular package, channel, or bundle.
488-
// BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.
489-
// ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.
490-
// PackageDeprecated is set if the requested package is marked deprecated in the catalog.
491-
// Deprecated is a rollup condition that is present when any of the deprecated conditions are present.
488+
// PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks
489+
// about the package, the condition stays Unknown instead of claiming False.
490+
// ChannelDeprecated follows the same rules for each requested channel.
491+
// BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog
492+
// data is available, it remains Unknown.
493+
// Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown
494+
// when we have no catalog information to report.
492495
//
493496
// +listType=map
494497
// +listMapKey=type

docs/api-reference/olmv1-api-reference.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ _Appears in:_
359359

360360
| Field | Description | Default | Validation |
361361
| --- | --- | --- | --- |
362-
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | The set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether or not the bundle has been installed for this ClusterExtension.<br />When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br />When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br />When the ClusterExtension is sourced from a catalog, if may also communicate a deprecation condition.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle.<br />BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.<br />ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.<br />PackageDeprecated is set if the requested package is marked deprecated in the catalog.<br />Deprecated is a rollup condition that is present when any of the deprecated conditions are present. | | |
362+
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | The set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether or not the bundle has been installed for this ClusterExtension.<br />When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br />When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br />When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle.<br />PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks<br />about the package, the condition stays Unknown instead of claiming False.<br />ChannelDeprecated follows the same rules for each requested channel.<br />BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog<br />data is available, it remains Unknown.<br />Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown<br />when we have no catalog information to report. | | |
363363
| `install` _[ClusterExtensionInstallStatus](#clusterextensioninstallstatus)_ | install is a representation of the current installation status for this ClusterExtension. | | |
364364

365365

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -518,12 +518,15 @@ spec:
518518
When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.
519519
When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.
520520
521-
When the ClusterExtension is sourced from a catalog, if may also communicate a deprecation condition.
521+
When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.
522522
These are indications from a package owner to guide users away from a particular package, channel, or bundle.
523-
BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.
524-
ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.
525-
PackageDeprecated is set if the requested package is marked deprecated in the catalog.
526-
Deprecated is a rollup condition that is present when any of the deprecated conditions are present.
523+
PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks
524+
about the package, the condition stays Unknown instead of claiming False.
525+
ChannelDeprecated follows the same rules for each requested channel.
526+
BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog
527+
data is available, it remains Unknown.
528+
Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown
529+
when we have no catalog information to report.
527530
items:
528531
description: Condition contains details for one aspect of the current
529532
state of this API Resource.

helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -479,12 +479,15 @@ spec:
479479
When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.
480480
When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.
481481
482-
When the ClusterExtension is sourced from a catalog, if may also communicate a deprecation condition.
482+
When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.
483483
These are indications from a package owner to guide users away from a particular package, channel, or bundle.
484-
BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.
485-
ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.
486-
PackageDeprecated is set if the requested package is marked deprecated in the catalog.
487-
Deprecated is a rollup condition that is present when any of the deprecated conditions are present.
484+
PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks
485+
about the package, the condition stays Unknown instead of claiming False.
486+
ChannelDeprecated follows the same rules for each requested channel.
487+
BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog
488+
data is available, it remains Unknown.
489+
Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown
490+
when we have no catalog information to report.
488491
items:
489492
description: Condition contains details for one aspect of the current
490493
state of this API Resource.

internal/operator-controller/controllers/clusterextension_admission_test.go

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,20 +13,20 @@ import (
1313
)
1414

1515
func TestClusterExtensionSourceConfig(t *testing.T) {
16-
sourceTypeEmptyError := "Invalid value: null"
16+
sourceTypeEmptyErrors := []string{"Invalid value: \"null\"", "Invalid value: null"}
1717
sourceTypeMismatchError := "spec.source.sourceType: Unsupported value"
1818
sourceConfigInvalidError := "spec.source: Invalid value"
1919
// unionField represents the required Catalog or (future) Bundle field required by SourceConfig
2020
testCases := []struct {
2121
name string
2222
sourceType string
2323
unionField string
24-
errMsg string
24+
errMsgs []string
2525
}{
26-
{"sourceType is null", "", "Catalog", sourceTypeEmptyError},
27-
{"sourceType is invalid", "Invalid", "Catalog", sourceTypeMismatchError},
28-
{"catalog field does not exist", "Catalog", "", sourceConfigInvalidError},
29-
{"sourceConfig has required fields", "Catalog", "Catalog", ""},
26+
{"sourceType is null", "", "Catalog", sourceTypeEmptyErrors},
27+
{"sourceType is invalid", "Invalid", "Catalog", []string{sourceTypeMismatchError}},
28+
{"catalog field does not exist", "Catalog", "", []string{sourceConfigInvalidError}},
29+
{"sourceConfig has required fields", "Catalog", "Catalog", nil},
3030
}
3131

3232
t.Parallel()
@@ -62,12 +62,20 @@ func TestClusterExtensionSourceConfig(t *testing.T) {
6262
}))
6363
}
6464

65-
if tc.errMsg == "" {
65+
if len(tc.errMsgs) == 0 {
6666
require.NoError(t, err, "unexpected error for sourceType %q: %w", tc.sourceType, err)
67-
} else {
68-
require.Error(t, err)
69-
require.Contains(t, err.Error(), tc.errMsg)
67+
return
68+
}
69+
70+
require.Error(t, err)
71+
matched := false
72+
for _, msg := range tc.errMsgs {
73+
if strings.Contains(err.Error(), msg) {
74+
matched = true
75+
break
76+
}
7077
}
78+
require.True(t, matched, "expected one of %v in error %q", tc.errMsgs, err)
7179
})
7280
}
7381
}

0 commit comments

Comments
 (0)