Skip to content

Commit 0757e50

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

File tree

2 files changed

+281
-20
lines changed

2 files changed

+281
-20
lines changed

‎pkg/manifest/manifest.go‎

Lines changed: 62 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]{}
@@ -171,6 +172,16 @@ func getFeatureSets(annotations map[string]string) (sets.Set[string], bool, erro
171172
return ret, specified, nil
172173
}
173174

175+
func hasFeatureSetAnnotation(annotations map[string]string) bool {
176+
_, ok := annotations[featureSetAnnotation]
177+
return ok
178+
}
179+
180+
func hasFeatureGateAnnotation(annotations map[string]string) bool {
181+
_, ok := annotations[featureGateAnnotation]
182+
return ok
183+
}
184+
174185
func checkFeatureSets(requiredFeatureSet string, annotations map[string]string) error {
175186
requiredAnnotationValue := requiredFeatureSet
176187
if len(requiredFeatureSet) == 0 {
@@ -187,12 +198,46 @@ func checkFeatureSets(requiredFeatureSet string, annotations map[string]string)
187198
return nil
188199
}
189200

201+
// checkFeatureGates validates if manifest should be included based on feature gate requirements
202+
func checkFeatureGates(enabledGates sets.Set[string], annotations map[string]string) error {
203+
if annotations == nil {
204+
return nil // No annotations, include by default
205+
}
206+
gateRequirements, ok := annotations[featureGateAnnotation]
207+
if !ok {
208+
return nil // No requirements, include by default
209+
}
210+
211+
requirements := strings.Split(gateRequirements, ",")
212+
for _, req := range requirements {
213+
req = strings.TrimSpace(req)
214+
if req == "" {
215+
continue
216+
}
217+
218+
if strings.HasPrefix(req, "-") {
219+
// Exclusion: gate must NOT be enabled
220+
gate := req[1:]
221+
if enabledGates.Has(gate) {
222+
return fmt.Errorf("feature gate %s is enabled but manifest requires it to be disabled", gate)
223+
}
224+
} else {
225+
// Inclusion: gate must be enabled
226+
if !enabledGates.Has(req) {
227+
return fmt.Errorf("feature gate %s is required but not enabled", req)
228+
}
229+
}
230+
}
231+
232+
return nil
233+
}
234+
190235
// Include returns an error if the manifest fails an inclusion filter and should be excluded from further
191236
// processing by cluster version operator. Pointer arguments can be set nil to avoid excluding based on that
192237
// filter. For example, setting profile non-nil and capabilities nil will return an error if the manifest's
193238
// 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)
239+
func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string]) error {
240+
return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, enabledFeatureGates, false)
196241
}
197242

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

207252
annotations := m.Obj.GetAnnotations()
208253
if annotations == nil {
@@ -216,13 +261,27 @@ func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, re
216261
}
217262
}
218263

264+
if requiredFeatureSet != nil && enabledFeatureGates != nil {
265+
if hasFeatureSetAnnotation(annotations) && hasFeatureGateAnnotation(annotations) {
266+
return fmt.Errorf("both feature set and feature gate annotations are present: manifests may specify either a feature set or a feature gate, but not both")
267+
}
268+
}
269+
219270
if requiredFeatureSet != nil {
220271
err := checkFeatureSets(*requiredFeatureSet, annotations)
221272
if err != nil {
222273
return err
223274
}
224275
}
225276

277+
// Feature gate filtering
278+
if enabledFeatureGates != nil {
279+
err := checkFeatureGates(enabledFeatureGates, annotations)
280+
if err != nil {
281+
return err
282+
}
283+
}
284+
226285
if profile != nil {
227286
profileAnnotation := fmt.Sprintf("include.release.openshift.io/%s", *profile)
228287
if val, ok := annotations[profileAnnotation]; ok && val != "true" {

‎pkg/manifest/manifest_test.go‎

Lines changed: 219 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,39 @@ 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+
},
837+
{
838+
name: "feature set required and feature gate specified",
839+
requiredFeatureSet: ptr.To("FeatureSet1"),
840+
enabledFeatureGates: sets.New("FeatureGate1"),
841+
annotations: map[string]interface{}{
842+
"release.openshift.io/feature-set": "FeatureSet1",
843+
"release.openshift.io/feature-gate": "FeatureGate1",
844+
},
845+
expected: fmt.Errorf("both feature set and feature gate annotations are present: manifests may specify either a feature set or a feature gate, but not both"),
846+
},
812847
{
813848
name: "all nil",
814849
profile: nil,
@@ -838,7 +873,7 @@ func Test_include(t *testing.T) {
838873
err := m.populateFromObj()
839874
assert.Equal(t, nil, err)
840875

841-
err = m.Include(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides)
876+
err = m.Include(tt.exclude, tt.requiredFeatureSet, tt.profile, tt.caps, tt.overrides, tt.enabledFeatureGates)
842877
assert.Equal(t, tt.expected, err)
843878
})
844879
}
@@ -847,14 +882,15 @@ func Test_include(t *testing.T) {
847882
func TestIncludeAllowUnknownCapabilities(t *testing.T) {
848883

849884
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
885+
name string
886+
exclude *string
887+
requiredFeatureSet *string
888+
profile *string
889+
annotations map[string]interface{}
890+
caps *configv1.ClusterVersionCapabilitiesStatus
891+
overrides []configv1.ComponentOverride
892+
enabledFeatureGates sets.Set[string]
893+
allowUnknown bool
858894

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

0 commit comments

Comments
 (0)