operator/v1: replace anyOf with CEL XValidation for maxConnections

[asadawar]

Summary

• Replace anyOf OpenAPI schema validation with CEL XValidation rule for IngressControllerTuningOptions.maxConnections
• The anyOf pattern (enum [-1, 0] OR range [2000, 2000000]) produced incomplete error messages when both branches failed: the API server only reported "must be one of: -1, 0", omitting the valid range
• The new CEL rule produces a clear, unified error message: "maxConnections must be 0, -1, or between 2000 and 2000000"
• Add 8 onCreate tests (valid and invalid values) and 3 onUpdate ratcheting tests

Before (anyOf error)

spec.tuningOptions: Invalid value: "object": spec.tuningOptions in body
must validate at least one schema (anyOf)
spec.tuningOptions.maxConnections: Unsupported value: 500:
supported values: "-1", "0"

After (CEL error)

spec.tuningOptions.maxConnections: Invalid value: "integer":
maxConnections must be 0, -1, or between 2000 and 2000000

Context

The original anyOf was added in PR #1161 (April 2022) before CEL XValidation was widely adopted in this repo. The file now has 30 XValidation rules, including 3 on the adjacent httpKeepAliveTimeout field in the same struct. CEL and OpenAPI structural validation ratchet identically under CRDValidationRatcheting (GA since Kubernetes 1.30), so there is no behavioral change for existing stored objects.

Test plan

• make update (full codegen) completes successfully
• Integration tests pass (make -C tests test), including:
  • Valid values: -1, 2000, 50000, 2000000
  • Invalid values: 1, 500, 1999, 3000000
  • Ratcheting: update invalid to valid, update invalid to another invalid (rejected), retain invalid while updating other fields (ratcheted)
• anyOf removed from all 5 generated CRD variants (Default, CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade, OKD)
• CEL XValidation rule present in all 5 generated CRD variants

Bug: https://issues.redhat.com/browse/OCPBUGS-86570

[openshift-merge-bot]

Pipeline controller notification
This repo 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. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci]

Hello @asadawar! 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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign everettraven for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci]

Hi @asadawar. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR implements validation constraints for the MaxConnections field in IngressControllerTuningOptions. A CEL-based validation rule is added to the Go type definition, restricting values to 0, -1, or the range 2000–2000000. The corresponding OpenAPI schema constraints are removed from the CRD manifest. Test coverage is added for both onCreate and onUpdate scenarios, verifying valid and invalid value acceptance, rejection with appropriate error messages, and field ratcheting behavior.

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Microshift Test Compatibility	⚠️ Warning	New Ginkgo tests use operator.openshift.io API (IngressController), unavailable on MicroShift, without protection mechanisms ([apigroup] tag or [Skipped:MicroShift] label).	Add [apigroup:operator.openshift.io] tag to test names or use [Skipped:MicroShift] label to skip on MicroShift clusters.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly describes the main change: replacing anyOf validation with CEL XValidation for maxConnections, which is the core objective of this PR.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the rationale, error message improvements, test coverage, and validation of codegen outcomes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	All 11 test names for maxConnections are stable and deterministic with only descriptive static strings, no dynamic content.
Test Structure And Quality	✅ Passed	Tests implement Ginkgo best practices: BeforeEach/AfterEach setup/cleanup, 5-sec timeouts, clear assertions. 11 maxConnections tests each test one behavior with proper resource cleanup.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests are added. PR contains only CRD validation rule addition and YAML schema test cases, not Ginkgo tests. Check not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies only validation rules for maxConnections via CEL XValidation, not scheduling constraints. No pod affinity, topology spread, nodeSelector, tolerations, or replica changes.
Ote Binary Stdout Contract	✅ Passed	PR contains no executable process-level code (main/init/BeforeSuite) or stdout writes; changes are type metadata and YAML configuration data only.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests were added in this PR. Changes consist of CRD manifests, a declarative YAML validation test file (non-Ginkgo), and Go type definitions with validation rules.
No-Weak-Crypto	✅ Passed	PR contains no weak cryptography, custom crypto implementations, or non-constant-time secret comparisons. Changes are validation rule modifications for ingress controller tuning options.
Container-Privileges	✅ Passed	PR modifies CRD schemas and test definitions, not container manifests. No privileged container configurations present in changed files.
No-Sensitive-Data-In-Logs	✅ Passed	PR adds CEL XValidation rule for maxConnections with message containing only field name and numeric bounds; no logging of sensitive data introduced.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml (1)

681-812: ⚡ Quick win

Consider adding test coverage for maxConnections: 0.

The CEL rule on line 2037 of operator/v1/types_ingress.go explicitly allows self == 0, and the field documentation states that 0 is a permitted value (treated as default). While the current tests comprehensively cover -1, the valid range boundaries (2000, 2000000), mid-range values (50000), and various invalid values, there is no explicit test verifying that maxConnections: 0 is accepted.

Adding a test case for this edge value would complete the coverage of all documented permitted values.

🤖 Prompt for 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.

In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml`
around lines 681 - 812, Add a test case that verifies maxConnections: 0 is
accepted — create a new test entry (e.g. name "Should be able to create an
IngressController with maxConnections of 0") modeled after the existing valid
cases (like the -1 and 2000 cases) with an initial YAML containing
spec.tuningOptions.maxConnections: 0 and an expected YAML that includes the
defaulted policies (httpEmptyRequestsPolicy, idleConnectionTerminationPolicy,
closedClientConnectionPolicy) and tuningOptions.maxConnections: 0; this will
exercise the CEL rule referenced in types_ingress.go that allows self == 0 and
complete coverage of permitted values.

🤖 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.

Nitpick comments:
In `@operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml`:
- Around line 681-812: Add a test case that verifies maxConnections: 0 is
accepted — create a new test entry (e.g. name "Should be able to create an
IngressController with maxConnections of 0") modeled after the existing valid
cases (like the -1 and 2000 cases) with an initial YAML containing
spec.tuningOptions.maxConnections: 0 and an expected YAML that includes the
defaulted policies (httpEmptyRequestsPolicy, idleConnectionTerminationPolicy,
closedClientConnectionPolicy) and tuningOptions.maxConnections: 0; this will
exercise the CEL rule referenced in types_ingress.go that allows self == 0 and
complete coverage of permitted values.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 9937caba-83df-49ba-9bad-95689ab5e295

📥 Commits

Reviewing files that changed from the base of the PR and between efa7fe9 and 751d5ed.

⛔ Files ignored due to path filters (7)

• operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
• operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerDynamicConfigurationManager.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**

📒 Files selected for processing (3)

• operator/v1/manual-override-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
• operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
• operator/v1/types_ingress.go

💤 Files with no reviewable changes (1)

• operator/v1/manual-override-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml

[JoelSpeed]

/ok-to-test

CC @Miciah

This LGTM but would like Miciah to ack

[openshift-ci]

@asadawar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/integration	751d5ed	link	true	/test integration
ci/prow/verify-crdify	751d5ed	link	true	/test verify-crdify
ci/prow/verify-crd-schema	751d5ed	link	true	/test verify-crd-schema

Full PR test history. Your PR dashboard.

Details

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.

[asadawar]

The verify-crd-schema and verify-crdify failures are expected. Removing the anyOf from the manual override removes the structural schema entry for tuningOptions.maxConnections, which the crd-schema-checker flags as NoFieldRemoval.

This is the same pattern as PR #2837, where the approver used /override ci/prow/verify-crd-schema and /override ci/prow/verify-crdify to bypass these checks for an intentional API schema change.

Why the anyOf must be removed (not kept alongside CEL):

When both anyOf and CEL XValidation are present, the API server short-circuits on the anyOf structural validation failure and never evaluates the CEL rule. The error message remains:

"spec.tuningOptions" must validate at least one schema (anyOf)
spec.tuningOptions.maxConnections: Unsupported value: 500: supported values: "-1", "0"
some validation rules were not checked because the object was invalid

The CEL error message never appears. The only way to get a clear error message is to remove the anyOf and let CEL handle the validation.

The integration test failure is likely a cascading effect from the schema change. All integration tests pass locally with make -C tests test.