Skip to content

Commit e5e7381

Browse files
JoelSpeedJoelSpeed
committed
Add feature gate inclusion/exclusion to pkg/manifest
1 parent ab8d518 commit e5e7381

File tree

2 files changed

+255
-20
lines changed

2 files changed

+255
-20
lines changed

‎pkg/manifest/manifest.go‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ const (
2626
CapabilityAnnotation = "capability.openshift.io/name"
2727
DefaultClusterProfile = "self-managed-high-availability"
2828
featureSetAnnotation = "release.openshift.io/feature-set"
29+
featureGateAnnotation = "release.openshift.io/feature-gate"
2930
)
3031

3132
var knownFeatureSets = sets.Set[string]{}
@@ -187,12 +188,46 @@ func checkFeatureSets(requiredFeatureSet string, annotations map[string]string)
187188
return nil
188189
}
189190

191+
// checkFeatureGates validates if manifest should be included based on feature gate requirements
192+
func checkFeatureGates(enabledGates sets.Set[string], annotations map[string]string) error {
193+
if annotations == nil {
194+
return nil // No annotations, include by default
195+
}
196+
gateRequirements, ok := annotations[featureGateAnnotation]
197+
if !ok {
198+
return nil // No requirements, include by default
199+
}
200+
201+
requirements := strings.Split(gateRequirements, ",")
202+
for _, req := range requirements {
203+
req = strings.TrimSpace(req)
204+
if req == "" {
205+
continue
206+
}
207+
208+
if strings.HasPrefix(req, "-") {
209+
// Exclusion: gate must NOT be enabled
210+
gate := req[1:]
211+
if enabledGates.Has(gate) {
212+
return fmt.Errorf("feature gate %s is enabled but manifest requires it to be disabled", gate)
213+
}
214+
} else {
215+
// Inclusion: gate must be enabled
216+
if !enabledGates.Has(req) {
217+
return fmt.Errorf("feature gate %s is required but not enabled", req)
218+
}
219+
}
220+
}
221+
222+
return nil
223+
}
224+
190225
// Include returns an error if the manifest fails an inclusion filter and should be excluded from further
191226
// processing by cluster version operator. Pointer arguments can be set nil to avoid excluding based on that
192227
// filter. For example, setting profile non-nil and capabilities nil will return an error if the manifest's
193228
// profile does not match, but will never return an error about capability issues.
194-
func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride) error {
195-
return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, false)
229+
func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string]) error {
230+
return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, enabledFeatureGates, false)
196231
}
197232

198233
// IncludeAllowUnknownCapabilities returns an error if the manifest fails an inclusion filter and should be excluded from
@@ -202,7 +237,7 @@ func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string
202237
// to capabilities filtering. When set to true a manifest will not be excluded simply because it contains an unknown
203238
// capability. This is necessary to allow updates to an OCP version containing newly defined capabilities.
204239
func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, requiredFeatureSet *string, profile *string,
205-
capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, allowUnknownCapabilities bool) error {
240+
capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string], allowUnknownCapabilities bool) error {
206241

207242
annotations := m.Obj.GetAnnotations()
208243
if annotations == nil {
@@ -223,6 +258,14 @@ func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, re
223258
}
224259
}
225260

261+
// Feature gate filtering
262+
if enabledFeatureGates != nil {
263+
err := checkFeatureGates(enabledFeatureGates, annotations)
264+
if err != nil {
265+
return err
266+
}
267+
}
268+
226269
if profile != nil {
227270
profileAnnotation := fmt.Sprintf("include.release.openshift.io/%s", *profile)
228271
if val, ok := annotations[profileAnnotation]; ok && val != "true" {

‎pkg/manifest/manifest_test.go‎

Lines changed: 209 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/stretchr/testify/assert"
1717
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1818
"k8s.io/apimachinery/pkg/runtime/schema"
19+
"k8s.io/apimachinery/pkg/util/sets"
1920
"k8s.io/klog/v2"
2021
)
2122

@@ -599,13 +600,14 @@ func Test_include(t *testing.T) {
599600
singleNodeProfile := "single-node"
600601

601602
tests := []struct {
602-
name string
603-
exclude *string
604-
requiredFeatureSet *string
605-
profile *string
606-
annotations map[string]interface{}
607-
caps *configv1.ClusterVersionCapabilitiesStatus
608-
overrides []configv1.ComponentOverride
603+
name string
604+
exclude *string
605+
requiredFeatureSet *string
606+
profile *string
607+
annotations map[string]interface{}
608+
caps *configv1.ClusterVersionCapabilitiesStatus
609+
overrides []configv1.ComponentOverride
610+
enabledFeatureGates sets.Set[string]
609611

610612
expected error
611613
}{
@@ -809,6 +811,29 @@ func Test_include(t *testing.T) {
809811
},
810812
expected: fmt.Errorf("overridden"),
811813
},
814+
{
815+
name: "feature gate required but not enabled",
816+
enabledFeatureGates: sets.New("FeatureGate1"),
817+
annotations: map[string]interface{}{
818+
"release.openshift.io/feature-gate": "FeatureGate2",
819+
},
820+
expected: fmt.Errorf("feature gate FeatureGate2 is required but not enabled"),
821+
},
822+
{
823+
name: "feature gate enabled but manifest requires it to be disabled",
824+
enabledFeatureGates: sets.New("FeatureGate1"),
825+
annotations: map[string]interface{}{
826+
"release.openshift.io/feature-gate": "-FeatureGate1",
827+
},
828+
expected: fmt.Errorf("feature gate FeatureGate1 is enabled but manifest requires it to be disabled"),
829+
},
830+
{
831+
name: "feature gate required and is enabled",
832+
enabledFeatureGates: sets.New("FeatureGate1"),
833+
annotations: map[string]interface{}{
834+
"release.openshift.io/feature-gate": "FeatureGate1",
835+
},
836+
},
812837
{
813838
name: "all nil",
814839
profile: nil,
@@ -838,7 +863,7 @@ func Test_include(t *testing.T) {
838863
err := m.populateFromObj()
839864
assert.Equal(t, nil, err)
840865

841-
err = m.Include(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides)
866+
err = m.Include(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides, tt.enabledFeatureGates)
842867
assert.Equal(t, tt.expected, err)
843868
})
844869
}
@@ -847,14 +872,15 @@ func Test_include(t *testing.T) {
847872
func TestIncludeAllowUnknownCapabilities(t *testing.T) {
848873

849874
tests := []struct {
850-
name string
851-
exclude *string
852-
requiredFeatureSet *string
853-
profile *string
854-
annotations map[string]interface{}
855-
caps *configv1.ClusterVersionCapabilitiesStatus
856-
overrides []configv1.ComponentOverride
857-
allowUnknown bool
875+
name string
876+
exclude *string
877+
requiredFeatureSet *string
878+
profile *string
879+
annotations map[string]interface{}
880+
caps *configv1.ClusterVersionCapabilitiesStatus
881+
overrides []configv1.ComponentOverride
882+
enabledFeatureGates sets.Set[string]
883+
allowUnknown bool
858884

859885
expected error
860886
}{
@@ -897,7 +923,7 @@ func TestIncludeAllowUnknownCapabilities(t *testing.T) {
897923
},
898924
},
899925
}
900-
err := m.IncludeAllowUnknownCapabilities(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides, tt.allowUnknown)
926+
err := m.IncludeAllowUnknownCapabilities(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides, tt.enabledFeatureGates, tt.allowUnknown)
901927
assert.Equal(t, tt.expected, err)
902928
})
903929
}
@@ -1009,3 +1035,169 @@ func TestSameResourceID(t *testing.T) {
10091035
})
10101036
}
10111037
}
1038+
1039+
func TestCheckFeatureGates(t *testing.T) {
1040+
tests := []struct {
1041+
name string
1042+
enabledGates sets.Set[string]
1043+
annotations map[string]string
1044+
expectedErr string
1045+
}{
1046+
{
1047+
name: "no feature gate annotation",
1048+
enabledGates: sets.New("gate1", "gate2"),
1049+
annotations: map[string]string{},
1050+
expectedErr: "",
1051+
},
1052+
{
1053+
name: "empty feature gate annotation",
1054+
enabledGates: sets.New("gate1", "gate2"),
1055+
annotations: map[string]string{featureGateAnnotation: ""},
1056+
expectedErr: "",
1057+
},
1058+
{
1059+
name: "required gate is enabled - should include",
1060+
enabledGates: sets.New("TechPreviewFeatureGate", "gate2"),
1061+
annotations: map[string]string{featureGateAnnotation: "TechPreviewFeatureGate"},
1062+
expectedErr: "",
1063+
},
1064+
{
1065+
name: "required gate is not enabled - should exclude",
1066+
enabledGates: sets.New("gate1", "gate2"),
1067+
annotations: map[string]string{featureGateAnnotation: "TechPreviewFeatureGate"},
1068+
expectedErr: "feature gate TechPreviewFeatureGate is required but not enabled",
1069+
},
1070+
{
1071+
name: "excluded gate is not enabled - should include",
1072+
enabledGates: sets.New("gate1", "gate2"),
1073+
annotations: map[string]string{featureGateAnnotation: "-TechPreviewFeatureGate"},
1074+
expectedErr: "",
1075+
},
1076+
{
1077+
name: "excluded gate is enabled - should exclude",
1078+
enabledGates: sets.New("TechPreviewFeatureGate", "gate2"),
1079+
annotations: map[string]string{featureGateAnnotation: "-TechPreviewFeatureGate"},
1080+
expectedErr: "feature gate TechPreviewFeatureGate is enabled but manifest requires it to be disabled",
1081+
},
1082+
{
1083+
name: "multiple requirements - all satisfied",
1084+
enabledGates: sets.New("gate1", "gate2"),
1085+
annotations: map[string]string{featureGateAnnotation: "gate1,gate2,-gate3"},
1086+
expectedErr: "",
1087+
},
1088+
{
1089+
name: "multiple requirements - inclusion not satisfied",
1090+
enabledGates: sets.New("gate1"),
1091+
annotations: map[string]string{featureGateAnnotation: "gate1,gate2,-gate3"},
1092+
expectedErr: "feature gate gate2 is required but not enabled",
1093+
},
1094+
{
1095+
name: "multiple requirements - exclusion not satisfied",
1096+
enabledGates: sets.New("gate1", "gate2", "gate3"),
1097+
annotations: map[string]string{featureGateAnnotation: "gate1,gate2,-gate3"},
1098+
expectedErr: "feature gate gate3 is enabled but manifest requires it to be disabled",
1099+
},
1100+
{
1101+
name: "whitespace handling",
1102+
enabledGates: sets.New("gate1", "gate2"),
1103+
annotations: map[string]string{featureGateAnnotation: " gate1 , gate2 , -gate3 "},
1104+
expectedErr: "",
1105+
},
1106+
{
1107+
name: "empty requirements in list",
1108+
enabledGates: sets.New("gate1"),
1109+
annotations: map[string]string{featureGateAnnotation: "gate1,,"},
1110+
expectedErr: "",
1111+
},
1112+
{
1113+
name: "complex scenario - OR logic for inclusion",
1114+
enabledGates: sets.New("NewStorageFeature"),
1115+
annotations: map[string]string{featureGateAnnotation: "NewStorageFeature,AlternativeStorageFeature"},
1116+
expectedErr: "feature gate AlternativeStorageFeature is required but not enabled",
1117+
},
1118+
{
1119+
name: "complex scenario - AND logic for mixed requirements",
1120+
enabledGates: sets.New("FeatureA"),
1121+
annotations: map[string]string{featureGateAnnotation: "FeatureA,-FeatureB"},
1122+
expectedErr: "",
1123+
},
1124+
}
1125+
1126+
for _, tt := range tests {
1127+
t.Run(tt.name, func(t *testing.T) {
1128+
err := checkFeatureGates(tt.enabledGates, tt.annotations)
1129+
if tt.expectedErr == "" {
1130+
if err != nil {
1131+
t.Errorf("checkFeatureGates() expected no error, got %v", err)
1132+
}
1133+
} else {
1134+
if err == nil {
1135+
t.Errorf("checkFeatureGates() expected error %q, got nil", tt.expectedErr)
1136+
} else if err.Error() != tt.expectedErr {
1137+
t.Errorf("checkFeatureGates() expected error %q, got %q", tt.expectedErr, err.Error())
1138+
}
1139+
}
1140+
})
1141+
}
1142+
}
1143+
1144+
func TestManifestIncludeWithFeatureGates(t *testing.T) {
1145+
// Test just the feature gate functionality by testing manifest with no other filtering requirements
1146+
tests := []struct {
1147+
name string
1148+
enabledGates sets.Set[string]
1149+
annotations map[string]interface{}
1150+
expectedErr string
1151+
}{
1152+
{
1153+
name: "manifest included when feature gate enabled",
1154+
enabledGates: sets.New("TechPreviewFeatureGate"),
1155+
annotations: map[string]interface{}{
1156+
featureGateAnnotation: "TechPreviewFeatureGate",
1157+
},
1158+
expectedErr: "",
1159+
},
1160+
{
1161+
name: "manifest excluded when feature gate disabled",
1162+
enabledGates: sets.New("SomeOtherGate"),
1163+
annotations: map[string]interface{}{
1164+
featureGateAnnotation: "TechPreviewFeatureGate",
1165+
},
1166+
expectedErr: "feature gate TechPreviewFeatureGate is required but not enabled",
1167+
},
1168+
}
1169+
1170+
for _, tt := range tests {
1171+
metadata := map[string]interface{}{
1172+
"name": "test",
1173+
"namespace": "test",
1174+
}
1175+
1176+
t.Run(tt.name, func(t *testing.T) {
1177+
if tt.annotations != nil {
1178+
metadata["annotations"] = tt.annotations
1179+
}
1180+
manifest := Manifest{
1181+
Obj: &unstructured.Unstructured{
1182+
Object: map[string]interface{}{
1183+
"metadata": metadata,
1184+
},
1185+
},
1186+
}
1187+
1188+
// Test only feature gates, set all other filters to nil
1189+
err := manifest.Include(nil, nil, nil, nil, nil, tt.enabledGates)
1190+
if tt.expectedErr == "" {
1191+
if err != nil {
1192+
t.Errorf("manifest.Include() expected no error, got %v", err)
1193+
}
1194+
} else {
1195+
if err == nil {
1196+
t.Errorf("manifest.Include() expected error containing %q, got nil", tt.expectedErr)
1197+
} else if !strings.Contains(err.Error(), tt.expectedErr) {
1198+
t.Errorf("manifest.Include() expected error containing %q, got %q", tt.expectedErr, err.Error())
1199+
}
1200+
}
1201+
})
1202+
}
1203+
}

0 commit comments

Comments
 (0)