Repository navigation
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tjungblu: This pull request references CNTRLPLANE-1617 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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. |
|
@tjungblu: This pull request references CNTRLPLANE-1617 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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. |
WalkthroughThis PR introduces configurable Kubernetes API Server event TTL for HostedClusters. It adds a new constant, implements annotation-based parameter handling to convert minutes to duration format, updates config generation to use the dynamic value instead of hardcoded "3h", and propagates the annotation through controller reconciliation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@tjungblu: This pull request references CNTRLPLANE-1617 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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
added
area/api
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if eventTTLMinutes := hcp.Annotations[hyperv1.KubeAPIServerEventTTLMinutes]; eventTTLMinutes != "" { | ||
| // Convert minutes to duration format (e.g., "180" -> "180m", "60" -> "60m") | ||
| kasConfig.EventTTL = fmt.Sprintf("%sm", eventTTLMinutes) | ||
| } |
There was a problem hiding this comment.
Bug: Unvalidated input threatens system configuration.
The EventTTL conversion from the annotation value doesn't validate that the input is a valid number or within the documented range of 5-180 minutes. Invalid values like "abc" or out-of-range values like "1" or "500" will be passed directly to the kube-apiserver configuration, potentially causing startup failures or configuration rejection.
|
@tjungblu: This pull request references CNTRLPLANE-1617 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
api/hypershift/v1beta1/hostedcluster_types.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/kas/params_test.go(3 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/params.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/config.goapi/hypershift/v1beta1/hostedcluster_types.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/params_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
🔇 Additional comments (5)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
2232-2232: LGTM! Annotation mirroring properly configured.The addition of
KubeAPIServerEventTTLMinutesto the mirrored annotations list is correct and follows the established pattern. This enables the event TTL configuration to flow from HostedCluster to HostedControlPlane, consistent with how other KAS annotations likeKubeAPIServerGoAwayChanceare handled.control-plane-operator/controllers/hostedcontrolplane/v2/kas/params_test.go (1)
132-132: LGTM! Comprehensive test coverage for EventTTL feature.The test changes properly validate the new EventTTL functionality:
- Default value is correctly set (line 132)
- Annotation-based override is tested with value "60" minutes → "60m" (lines 275-289)
- Dedicated test case covers the annotation in isolation with value "180" minutes → "180m" (lines 291-306)
The tests follow established patterns and provide good coverage for both default behavior and annotation-driven configuration.
Also applies to: 275-306
hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
685-685: LGTM: Test correctly verifies EventTTL annotation propagation.The test properly validates that the
KubeAPIServerEventTTLMinutesannotation with value "180" is mirrored from HostedCluster to HostedControlPlane during initial reconciliation.Also applies to: 694-694
control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go (2)
25-25: LGTM: Default EventTTL value is reasonable.The default of "3h" provides a sensible event retention period.
54-54: LGTM: EventTTL field appropriately added to config params.
| // KubeAPIServerEventTTLMinutes allows the --event-ttl parameter of the kube-apiserver to be overridden from its default of 3h (180 minutes) | ||
| // The value should be specified in minutes (e.g., "60", "180"). Valid range is 5-180 minutes. | ||
| KubeAPIServerEventTTLMinutes = "hypershift.openshift.io/event-ttl-minutes" |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Verify range validation is enforced.
The constant definition is clear and follows the established annotation naming pattern. The documentation clearly specifies the expected format (minutes as numeric string) and valid range (5-180 minutes).
Please verify that the 5-180 minute range validation is enforced in the controller code (likely in params.go) that consumes this annotation:
🏁 Script executed:
#!/bin/bash
# Search for validation of the event-ttl-minutes annotation
rg -n -A 10 -B 2 'KubeAPIServerEventTTLMinutes|event-ttl-minutes' --type=goLength of output: 13671
🏁 Script executed:
#!/bin/bash
# Search for validation rules or CEL rules that might validate the range
rg -n 'event-ttl|EventTTL' --type=yaml --type=go -g '!vendor/**' | head -50Length of output: 33726
🏁 Script executed:
#!/bin/bash
# Look for validation in the HostedControlPlane CRD or validation code
rg -n 'validat.*event|CEL.*event|MinProperties|MaxProperties' --type=yaml -g '!vendor/**'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any validation functions related to this annotation
ast-grep --pattern 'func $_($_, $_) {
$$$
KubeAPIServerEventTTLMinutes
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's validation in webhook or admission code
fd -e go -type f | xargs rg -l 'webhook|admission|validation' -g '!vendor/**' | xargs rg -n 'KubeAPIServerEventTTLMinutes|event-ttl' 2>/dev/null || echo "No webhook validation found"Length of output: 32900
Add input validation for the event-ttl annotation range (5-180 minutes).
The annotation is documented to require a valid range of 5-180 minutes, but the implementation in control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go (lines 128-131) accepts any string value without validation. Invalid inputs like "1", "200", or "abc" would be passed directly to the kube-apiserver argument, potentially causing configuration errors at runtime.
Add range validation in the params.go code to enforce the documented 5-180 minute constraint before converting the annotation value to a duration string.
🤖 Prompt for AI Agents
In control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go around
lines 128-131, the code currently accepts the KubeAPIServerEventTTLMinutes
annotation string blindly; change it to parse the annotation as an integer (use
strconv.Atoi), validate that the parsed minutes are between 5 and 180 inclusive,
and only then convert to the kube-apiserver duration form (e.g.,
fmt.Sprintf("%dm", minutes)) and add the flag. If parsing fails or the value is
out of range, do not add the flag and emit a clear log or return an error
(consistent with surrounding error-handling), so invalid inputs like "1", "200",
or non-numeric values are rejected instead of passed through.
| args.Set("etcd-prefix", "kubernetes.io") | ||
| args.Set("etcd-servers", p.EtcdURL) | ||
| args.Set("event-ttl", "3h") | ||
| args.Set("event-ttl", p.EventTTL) |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Verify params.go handles conversion and defaults.
The change correctly uses the dynamic p.EventTTL parameter instead of a hardcoded value. The separation of concerns (config generation vs. parameter parsing/validation) is appropriate.
Please verify that the params.go file (referenced in dependencies) properly:
- Converts the minutes annotation value to duration format (e.g., "60" → "60m")
- Provides a default value (e.g., "3h") when the annotation is absent
- Validates the 5-180 minute range
🏁 Script executed:
#!/bin/bash
# Search for EventTTL parameter handling in params.go
rg -n -A 15 -B 5 'EventTTL|defaultEventTTL' control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.goLength of output: 3040
Range validation missing in EventTTL parameter handling.
The conversion and default value implementations are correct, but the code lacks validation for the 5-180 minute range. Line 130 in control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go directly converts the annotation value to duration format without bounds checking:
kasConfig.EventTTL = fmt.Sprintf("%sm", eventTTLMinutes)Add validation to ensure eventTTLMinutes is between 5 and 180, otherwise invalid values will be passed to kube-apiserver and cause runtime failures.
🤖 Prompt for AI Agents
In control-plane-operator/controllers/hostedcontrolplane/v2/kas/config.go around
line 214, the EventTTL value is being set without validating that the minutes
value is within the required 5–180 minute range; add a range check on the parsed
eventTTLMinutes and if eventTTLMinutes < 5 or > 180, return an error (or
propagate an existing returned error) indicating the annotation is invalid
instead of calling args.Set, so invalid values are rejected before being passed
to kube-apiserver.
| kasConfig.EventTTL = defaultEventTTL | ||
| if eventTTLMinutes := hcp.Annotations[hyperv1.KubeAPIServerEventTTLMinutes]; eventTTLMinutes != "" { | ||
| // Convert minutes to duration format (e.g., "180" -> "180m", "60" -> "60m") | ||
| kasConfig.EventTTL = fmt.Sprintf("%sm", eventTTLMinutes) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify input validation for EventTTL annotation.
The annotation value is converted to duration format without validation. If the annotation contains non-numeric values (e.g., "abc") or already includes a unit suffix (e.g., "180m"), the resulting duration string would be invalid (e.g., "abcm" or "180mm").
While this pattern is consistent with other annotation handling in this file (e.g., MaxRequestsInflight), the additional transformation increases the risk of invalid configuration.
Run the following script to check if validation exists elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for validation of KubeAPIServerEventTTLMinutes annotation
rg -n "KubeAPIServerEventTTLMinutes" -A 5 -B 5
# Search for validation or usage of EventTTL field
rg -n "\.EventTTL" -A 5 -B 5
# Look for duration parsing/validation in the KAS config code
rg -n "time\.ParseDuration|ParseDuration" --type=go -g "*/kas/*"Length of output: 17624
Add input validation for EventTTL annotation value.
The annotation documentation specifies a valid range of 5-180 minutes, but the code doesn't validate this constraint. Invalid values like "abc", "180m", "1000", or "0" would produce invalid durations ("abcm", "180mm", "1000m", "0m") that fail at runtime when passed to kube-apiserver.
Add validation to verify:
- The value is numeric
- The value is within the documented range (5-180 minutes)
|
@tjungblu: This pull request references CNTRLPLANE-1617 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
/approve
Thanks for the PR!, Some things:
- The PR needs rebase
- Looks good overall but I've mentioned some required changes to be done in the PR review
|
|
||
| // KubeAPIServerEventTTLMinutes allows the --event-ttl parameter of the kube-apiserver to be overridden from its default of 3h (180 minutes) | ||
| // The value should be specified in minutes (e.g., "60", "180"). Valid range is 5-180 minutes. | ||
| KubeAPIServerEventTTLMinutes = "hypershift.openshift.io/event-ttl-minutes" |
There was a problem hiding this comment.
Please include the Annotation in the name
| KubeAPIServerGoAwayChance = "hypershift.openshift.io/kube-apiserver-goaway-chance" | ||
|
|
||
| // KubeAPIServerEventTTLMinutes allows the --event-ttl parameter of the kube-apiserver to be overridden from its default of 3h (180 minutes) | ||
| // The value should be specified in minutes (e.g., "60", "180"). Valid range is 5-180 minutes. |
There was a problem hiding this comment.
How is the range validated?
| }, | ||
| }, | ||
| { | ||
| name: "with event-ttl annotation", |
There was a problem hiding this comment.
Add tests with range limits
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, tjungblu 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
|
@tjungblu: 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. |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 5 Prow job failures on PR #7202 are caused by git merge conflicts between the PR branch and the Root CauseThe PR branch (
The original Recommendations
Evidence
|
|
/close |
4 participants
What this PR does / why we need it:
This implements what was proposed in openshift/enhancements#1857 - adding the event-ttl as a configurable value to the control plane.
This is done like #6019, as there is no KAS Operator in hypershift.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-1617
Special notes for your reviewer:
This has been entirely coded by Cursor, given the above PR as an example input.
Checklist: