OCPBUGS-94251: Add CEL duration validation to NodePool drain/detach timeouts - #9117
OCPBUGS-94251: Add CEL duration validation to NodePool drain/detach timeouts#9117vismishr wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@vismishr: This pull request references Jira Issue OCPBUGS-94251, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded kubebuilder validation annotations to Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vismishr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@vismishr: This pull request references Jira Issue OCPBUGS-94251, which is valid. 3 validation(s) were run on this bug
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. |
0223375 to
3ed7210
Compare
…ach timeouts Add XValidation CEL rules to nodeDrainTimeout and nodeVolumeDetachTimeout fields on NodePoolSpec to reject invalid duration strings at admission time. Previously, malformed values like "1" (no unit suffix) passed CRD validation, got stored in etcd, and crashed the cluster-wide NodePool informer on deserialization. Follows the HTTPKeepAliveTimeout precedent from openshift/api for the CEL regex pattern matching Go duration syntax.
3ed7210 to
0b792c4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/hypershift/v1beta1/nodepool_types.go`:
- Around line 187-197: Update the NodeDrainTimeout and nodeVolumeDetachTimeout
validations to use Kubernetes duration format validation with a non-negative
constraint instead of the current regex-only XValidation, preserving their
string representation and documented fields. Ensure validation matches
time.ParseDuration behavior, including rejecting overflow values while accepting
valid forms such as .5s and the ASCII us suffix. Add envtest coverage for
overflow rejection and these edge-valid duration forms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: db181a01-10cf-463b-bba3-46866ffd925d
⛔ Files ignored due to path filters (9)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/stable.nodepools.validation.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
api/hypershift/v1beta1/nodepool_types.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9117 +/- ##
=======================================
Coverage 44.51% 44.51%
=======================================
Files 774 774
Lines 96997 97003 +6
=======================================
+ Hits 43179 43185 +6
Misses 50830 50830
Partials 2988 2988 see 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ection Update NodePool nodeDrainTimeout and nodeVolumeDetachTimeout CEL validation to use duration() function for overflow detection and accept leading-dot decimals like .5s. Follows the HTTPKeepAliveTimeout two-rule pattern: regex for format + guarded duration() for range check. Add envtest coverage for .5s, us suffix, and overflow edge cases.
|
@vismishr: all tests passed! 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
What this PR does / why we need it:
nodeDrainTimeout and nodeVolumeDetachTimeout on NodePoolSpec are *metav1.Duration with no CRD-level validation. Invalid strings like "1" (no unit suffix) pass admission, get stored in etcd, and crash the cluster-wide NodePool informer on deserialization (time.ParseDuration failure).
This PR adds XValidation CEL rules to both fields that validate the value matches Go duration syntax at admission time. Follows the HTTPKeepAliveTimeout precedent from openshift/api (operator/v1/types_ingress.go) for the CEL regex pattern and escaping.
Changes:
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-94251
Special notes for your reviewer:
Follows the HTTPKeepAliveTimeout precedent from openshift/api for CEL regex escaping (four backslashes in the marker). MinLength/MaxLength omitted to avoid CRD schema check errors — the CEL regex already rejects empty/invalid strings.
Checklist:
Summary by CodeRabbit
30s,5m,1h).