OCPBUGS-61858: Use pointer type for HTTPKeepAliveTimeout in IngressController API

[alebedev87]

This commit addresses the problem of the creation of the default IngressController without HTTPKeepAliveTimeout tuning option by the cluster ingress operator:

"IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.httpKeepAliveTimeout: Invalid value: \"string\": httpKeepAliveTimeout must be greater than or equal to 1 millisecond"

The CEL expression which checks the minimal value prevents the creation of IngressController with unspecified (zero value) HTTPKeepAliveTimeout.

Follow-up of #2547.

[openshift-ci-robot]

Pipeline controller notification
This repository is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. Review these jobs and use /test <job> to manually trigger optional jobs most likely to be impacted by the proposed changes.

[openshift-ci-robot]

@alebedev87: This pull request references Jira Issue OCPBUGS-61858, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.21.0) matches configured target version for branch (4.21.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This commit addresses the problem of the creation of the default IngressController without HTTPKeepAliveTimeout tuning option by the cluster ingress operator:

"IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.httpKeepAliveTimeout: Invalid value: \"string\": httpKeepAliveTimeout must be greater than or equal to 1 millisecond"

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]

Hello @alebedev87! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

[coderabbitai]

Walkthrough

The HTTPKeepAliveTimeout field in IngressControllerTuningOptions was converted from a non-pointer to a pointer type (metav1.Duration to *metav1.Duration) to support optional/unset values, with corresponding deep-copy logic updated to handle nil-safety.

Changes

Cohort / File(s)	Summary
Type definition update operator/v1/types_ingress.go	Changed HTTPKeepAliveTimeout field signature from metav1.Duration to *metav1.Duration in IngressControllerTuningOptions, allowing nil representation for unset values.
Deep-copy implementation operator/v1/zz_generated.deepcopy.go	Updated DeepCopyInto method to conditionally deep-copy HTTPKeepAliveTimeout only when non-nil, replacing previous unconditional shallow copy with nil-aware logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify all callers of HTTPKeepAliveTimeout are updated to handle pointer type and perform nil-checks where appropriate
• Confirm deep-copy logic correctly preserves zero-value behavior when the field is nil
• Check for any validation logic dependent on the field type that may need updates

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent 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

📥 Commits

Reviewing files that changed from the base of the PR and between 916c700 and 8542335.

📒 Files selected for processing (2)

• operator/v1/types_ingress.go (1 hunks)
• operator/v1/zz_generated.deepcopy.go (1 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:

• operator/v1/types_ingress.go
• operator/v1/zz_generated.deepcopy.go

🔇 Additional comments (2)

operator/v1/types_ingress.go (1)

1887-1915: Pointer type here correctly makes httpKeepAliveTimeout truly optional

Changing HTTPKeepAliveTimeout to *metav1.Duration with omitempty aligns this field with the other tuning timeouts (which are already pointers) and finally matches the documented “when omitted, the platform chooses a default” behavior. It also prevents the zero value from being serialized (e.g. "0s") and tripping the lower-bound XValidations that caused the reported bug.

The only thing to be aware of is that this is a Go‑level source change for callers using this field directly (they’ll need to pass/handle a pointer), but at the JSON/schema level this is the right, backward‑compatible semantics fix.

operator/v1/zz_generated.deepcopy.go (1)

2557-2571: Deep copy logic for HTTPKeepAliveTimeout matches existing pointer duration fields

The new if in.HTTPKeepAliveTimeout != nil { ... } block correctly allocates a fresh metav1.Duration and copies the value, consistent with ClientTimeout, ConnectTimeout, etc. No aliasing or nil-handling issues here.

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: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented
The command is terminated due to an error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[alebedev87]

The problem was indeed in the non pointer type: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-ingress-operator/1297/pull-ci-openshift-cluster-ingress-operator-master-e2e-azure-operator/1991445858800898048.

[everettraven]

We try to avoid pointers where possible.

If the zero value is not a valid value, there isn't really any need for a pointer. It looks like metav1.Duration is a struct and not a type alias.

Does adding omitzero resolve this issue?

Suggested change

	HTTPKeepAliveTimeout *metav1.Duration `json:"httpKeepAliveTimeout,omitempty"`
	HTTPKeepAliveTimeout metav1.Duration `json:"httpKeepAliveTimeout,omitempty,omitzero"`

[everettraven]

@alebedev87 reminded me that this needs to be backported to 4.16 which will not support the omitzero tag.

Pointer is correct here.

[everettraven]

/lgtm

/override ci/prow/lint

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [everettraven]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@everettraven: Overrode contexts on behalf of everettraven: ci/prow/lint

In response to this:

/lgtm

/override ci/prow/lint

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 kubernetes-sigs/prow repository.

[openshift-ci]

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[alebedev87]

As discussed with NI&D QE lead (@lihongan), the API change is fine to be validated by the CI.

/verified by ci-jobs

Similar example.

[openshift-ci-robot]

@alebedev87: This PR has been marked as verified by ci-jobs.

In response to this:

As discussed with NI&D QE lead (@lihongan), the API change is fine to be validated by the CI.

/verified by ci-jobs

Similar example.

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-robot]

@alebedev87: Jira Issue OCPBUGS-61858: Some pull requests linked via external trackers have merged:

The following pull request, linked via external tracker, has not merged:

• openshift/cluster-ingress-operator#1297 is open

All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-61858 has not been moved to the MODIFIED state.

This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload.

In response to this:

This commit addresses the problem of the creation of the default IngressController without HTTPKeepAliveTimeout tuning option by the cluster ingress operator:

"IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.httpKeepAliveTimeout: Invalid value: \"string\": httpKeepAliveTimeout must be greater than or equal to 1 millisecond"

The CEL expression which checks the minimal value prevents the creation of IngressController with unspecified (zero value) HTTPKeepAliveTimeout.

Follow-up of #2547.

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.