Skip to content

NE-2523: Implement configurationManagement API#1385

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
Miciah:NE-2523-implement-configurationManagement-API
Mar 30, 2026
Merged

NE-2523: Implement configurationManagement API#1385
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
Miciah:NE-2523-implement-configurationManagement-API

Conversation

@Miciah
Copy link
Copy Markdown
Contributor

@Miciah Miciah commented Mar 13, 2026

Bump openshift/api for configurationManagement API

Bump to openshift/api@582dc3d to get the spec.tuningOptions.configurationManagement API field to allow enabling or disabling the Dynamic Configuration Manager.

Implement configurationManagement API

Check the spec.tuningOptions.configurationManagement API field, and only enable the Dynamic Configuration Manager if the field is set to "Dynamic".

For now, the default behavior (when the field is omitted) is not to enable DCM. Once we are more confident that the feature is safe to enable, we will change the default behavior.


This PR depends on openshift/api#2757.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 13, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Mar 13, 2026

@Miciah: This pull request references NE-2523 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.22.0" version, but no target version was set.

Details

In response to this:

Bump openshift/api for configurationManagement API

Bump to github.com/Miciah/api@6a8c07c9cf163d3b5e7964ffb5c7d70f6bdeff5e to get the spec.tuningOptions.configurationManagement API field to allow enabling or disabling the Dynamic Configuration Manager.

Implement configurationManagement API

Check the spec.tuningOptions.configurationManagement API field, and only enable the Dynamic Configuration Manager if the field is set to "Dynamic".

For now, the default behavior (when the field is omitted) is not to enable DCM. Once we are more confident that the feature is safe to enable, we will change the default behavior.


This PR depends on openshift/api#2757.

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 13, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: cdff3452-71cf-49dc-a1f1-e6b5844a75c7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Updates Go module versions for OpenShift APIs and client-go in go.mod. Adds release.openshift.io/feature-set metadata to two CRDs (OKD and Default) and expands toleration/operator schema docs to include Exists, Equal, Lt, and Gt, noting Lt/Gt require the TaintTolerationComparisonOperators feature gate. Changes router deployment logic to enable Dynamic Configuration Manager (DCM) only when Spec.TuningOptions.ConfigurationManagement is Dynamic, unless an unsupported override explicitly sets the dynamic config manager. Adjusts unit tests to cover omitted, ForkAndReload, and Dynamic configuration-management modes and related override behavior.

🚥 Pre-merge checks | ✅ 8
✅ Passed checks (8 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and directly describes the main implementation focus of the PR: adding support for the configurationManagement API field.
Description check ✅ Passed The description is directly related to the changeset, explaining both the API bump and the implementation of the configurationManagement API feature.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Stable And Deterministic Test Names ✅ Passed The test file uses standard Go testing package with table-driven tests, not Ginkgo framework. No Ginkgo test macros are present, and all test names are static descriptive strings without dynamic values.
Test Structure And Quality ✅ Passed The PR modifies standard Go testing code using testing.T, not Ginkgo tests. The codebase does not use Ginkgo framework, so the Ginkgo-specific check is outside the scope of this PR.
Microshift Test Compatibility ✅ Passed PR modifies unit tests in deployment_test.go using Go's standard testing.T framework, not Ginkgo e2e tests. No Ginkgo patterns like It(), Describe(), Context(), or When() are present.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PR modifies only standard Go unit tests, not Ginkgo e2e tests. Check targeting new Ginkgo e2e tests is not applicable.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PR adds only standard Go unit tests in testing package, not Ginkgo e2e tests; no IPv4 assumptions or external connectivity requirements detected.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci openshift-ci Bot requested review from alebedev87 and knobunc March 13, 2026 15:37
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pkg/operator/controller/ingress/deployment.go (1)

587-591: ⚠️ Potential issue | 🟠 Major

Unsupported override can still force-enable DCM outside Dynamic mode

Line 590 assigns enableDCM = v, so dynamicConfigManager: "true" can re-enable DCM even when spec.tuningOptions.configurationManagement is not Dynamic. That bypasses the new gate.

Proposed fix
 dynamicConfigOverride := unsupportedConfigOverrides.DynamicConfigManager
-if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil {
-	// Config override can still be used to opt out from DCM.
-	enableDCM = v
+if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil && !v {
+	// Config override can still be used to opt out from DCM.
+	enableDCM = false
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/operator/controller/ingress/deployment.go` around lines 587 - 591, The
code currently sets enableDCM = v from
unsupportedConfigOverrides.DynamicConfigManager, which lets a "true" override
enable DCM even when spec.tuningOptions.configurationManagement is not
"Dynamic"; change the logic so the parsed override only allows opting out: parse
unsupportedConfigOverrides.DynamicConfigManager (as done in
dynamicConfigOverride) and if parse succeeds and v == false then set enableDCM =
false, but do not set enableDCM = true when v == true; keep all other logic
intact (look for dynamicConfigOverride,
unsupportedConfigOverrides.DynamicConfigManager and enableDCM to update this
conditional).
manifests/00-custom-resource-definition-OKD.yaml (1)

2120-2398: ⚠️ Potential issue | 🔴 Critical

Regenerate this CRD with spec.tuningOptions.configurationManagement.

spec.tuningOptions still does not define configurationManagement anywhere in the schema. On a structural CRD, that means OKD will prune the new field from user objects, so spec.tuningOptions.configurationManagement: Dynamic never reaches the operator.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@manifests/00-custom-resource-definition-OKD.yaml` around lines 2120 - 2398,
The CRD schema for tuningOptions is missing the specification for
spec.tuningOptions.configurationManagement so OKD will prune that field; add a
new property named configurationManagement under the existing
tuningOptions.properties (next to clientTimeout, maxConnections, etc.) with the
proper type/enum (e.g., type: string and enum: ["Dynamic","Manual"] or the exact
allowed values your operator expects), include any necessary
validation/defaults, then regenerate the CRD so
spec.tuningOptions.configurationManagement is persisted and reaches the
operator.
🧹 Nitpick comments (2)
go.mod (1)

160-160: Add a follow-up comment next to the openshift/api fork replace to ensure removal once upstream support is available

The temporary fork replace at line 160 lacks any tracking comment. Add a // TODO: or similar note indicating that this replace should be removed once upstream API changes are merged, so it doesn't get accidentally left in production.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@go.mod` at line 160, Add a short tracking comment next to the replace
statement for the openshift API fork in go.mod (the line containing "replace
github.com/openshift/api => github.com/Miciah/api
v0.0.0-20260312135711-6a8c07c9cf16") — e.g., a "// TODO: remove replace when
upstream openshift/api includes required changes (track PR/issue #...)" — so the
temporary fork is clearly marked for removal once upstream support is available.
pkg/operator/controller/ingress/deployment_test.go (1)

1208-1228: Add the dynamicConfigManager:true + non-Dynamic cases to this matrix.

The new table exercises dynamicConfigManager:false, but not the inverse when configurationManagement is omitted or ForkAndReload. Those are the combinations that actually pin the new gating behavior and would catch regressions where the legacy override still enables DCM without Dynamic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/operator/controller/ingress/deployment_test.go` around lines 1208 - 1228,
Add three mirror test cases to the existing table that use
unsupportedConfigOverrides
`{"dynamicConfigManager":"true","maxDynamicServers":"7"}` for the combinations
where configurationManagement is omitted and where it is ForkAndReload (i.e.,
duplicate the blocks named
"featuregate-enabled-configurationManagement-omitted-dcm-override" and
"featuregate-enabled-configurationManagement-ForkAndReload-dcm-override" but
with dynamicConfigManager set to true), setting dcmEnabled and expectedEnv to
the values that assert DCM is enabled (the opposite of notSet). Ensure you
update the entries that reference unsupportedConfigOverrides, dcmEnabled,
configurationManagement, and expectedEnv so the table exercises the inverse/true
path of the dynamicConfigManager flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@manifests/00-custom-resource-definition-OKD.yaml`:
- Around line 2010-2017: The CRD text wrongly states "TLS 1.3 cipher suites ...
are not configurable" which contradicts ingress behavior that renders and uses
ROUTER_CIPHERSUITES from a custom profile; update the prose under the "ciphers:"
example to accurately reflect behavior: explain that TLS 1.2 suites are
configurable, TLS 1.3 suites are negotiated differently but values provided in
the custom profile are rendered into ROUTER_CIPHERSUITES and will be respected
by the ingress controller where supported, or clearly state if TLS 1.3 suites
are ignored at runtime; adjust the sentence mentioning "not configurable" and
add a note referencing ROUTER_CIPHERSUITES and the custom profile to remove the
contradiction.

---

Outside diff comments:
In `@manifests/00-custom-resource-definition-OKD.yaml`:
- Around line 2120-2398: The CRD schema for tuningOptions is missing the
specification for spec.tuningOptions.configurationManagement so OKD will prune
that field; add a new property named configurationManagement under the existing
tuningOptions.properties (next to clientTimeout, maxConnections, etc.) with the
proper type/enum (e.g., type: string and enum: ["Dynamic","Manual"] or the exact
allowed values your operator expects), include any necessary
validation/defaults, then regenerate the CRD so
spec.tuningOptions.configurationManagement is persisted and reaches the
operator.

In `@pkg/operator/controller/ingress/deployment.go`:
- Around line 587-591: The code currently sets enableDCM = v from
unsupportedConfigOverrides.DynamicConfigManager, which lets a "true" override
enable DCM even when spec.tuningOptions.configurationManagement is not
"Dynamic"; change the logic so the parsed override only allows opting out: parse
unsupportedConfigOverrides.DynamicConfigManager (as done in
dynamicConfigOverride) and if parse succeeds and v == false then set enableDCM =
false, but do not set enableDCM = true when v == true; keep all other logic
intact (look for dynamicConfigOverride,
unsupportedConfigOverrides.DynamicConfigManager and enableDCM to update this
conditional).

---

Nitpick comments:
In `@go.mod`:
- Line 160: Add a short tracking comment next to the replace statement for the
openshift API fork in go.mod (the line containing "replace
github.com/openshift/api => github.com/Miciah/api
v0.0.0-20260312135711-6a8c07c9cf16") — e.g., a "// TODO: remove replace when
upstream openshift/api includes required changes (track PR/issue #...)" — so the
temporary fork is clearly marked for removal once upstream support is available.

In `@pkg/operator/controller/ingress/deployment_test.go`:
- Around line 1208-1228: Add three mirror test cases to the existing table that
use unsupportedConfigOverrides
`{"dynamicConfigManager":"true","maxDynamicServers":"7"}` for the combinations
where configurationManagement is omitted and where it is ForkAndReload (i.e.,
duplicate the blocks named
"featuregate-enabled-configurationManagement-omitted-dcm-override" and
"featuregate-enabled-configurationManagement-ForkAndReload-dcm-override" but
with dynamicConfigManager set to true), setting dcmEnabled and expectedEnv to
the values that assert DCM is enabled (the opposite of notSet). Ensure you
update the entries that reference unsupportedConfigOverrides, dcmEnabled,
configurationManagement, and expectedEnv so the table exercises the inverse/true
path of the dynamicConfigManager flag.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: cb9d4f60-f735-4342-a0c7-7455a5de6900

📥 Commits

Reviewing files that changed from the base of the PR and between 88b7301 and f625550.

⛔ Files ignored due to path filters (170)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/gogo/protobuf/AUTHORS is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/CONTRIBUTORS is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/LICENSE is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/Makefile is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/clone.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/custom_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/decode.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/deprecated.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/discard.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/duration.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/duration_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/encode.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/encode_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/equal.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/extensions.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/extensions_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/lib.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/lib_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/message_set.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/pointer_reflect.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/pointer_reflect_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/pointer_unsafe.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/pointer_unsafe_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/properties.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/properties_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/skip_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/table_marshal.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/table_marshal_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/table_merge.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/table_unmarshal.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/table_unmarshal_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/text.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/text_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/text_parser.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/timestamp.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/timestamp_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/wrappers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/proto/wrappers_gogo.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/gogo/protobuf/sortkeys/sortkeys.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/.ci-operator.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/.coderabbit.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/.golangci.go-validated.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/.golangci.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/AGENTS.md is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/Dockerfile.ocp is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/Makefile is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apiextensions/install.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/Makefile is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/doc.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/register.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/apps/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apps/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apps/v1/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/authorization/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/authorization/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/build/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/build/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/cloudnetwork/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/cloudnetwork/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/register.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_apiserver.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_authentication.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_cluster_image_policy.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_cluster_version.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_image_policy.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_infrastructure.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_ingress.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_insights.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_network.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha1/register.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_backup.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_cluster_image_policy.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_image_policy.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_insights.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/types_pki.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha2/types_insights.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/console/v1/types_console_sample.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/envtest-releases.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/README.md is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/install.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/v1alpha1/Makefile is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/v1alpha1/doc.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/v1alpha1/register.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/features.md is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/features/features.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/features/legacyfeaturegates.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/features/util.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/image/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/image/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/install.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1beta1/types_machine.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1beta1/types_machineset.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/network/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/network/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/networkoperator/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/networkoperator/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/oauth/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/oauth/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/types_console.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/types_ingress.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/types_network.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/register.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operatoringress/v1/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-Default.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-OKD.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operatoringress/v1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/project/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/project/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/quota/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/quota/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/route/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/route/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/samples/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/samples/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/security/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/security/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/template/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/template/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/user/v1/generated.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/user/v1/generated.protomessage.pb.go is excluded by !**/*.pb.go, !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (8)
  • go.mod
  • manifests/00-custom-resource-definition-CustomNoUpgrade.yaml
  • manifests/00-custom-resource-definition-DevPreviewNoUpgrade.yaml
  • manifests/00-custom-resource-definition-OKD.yaml
  • manifests/00-custom-resource-definition-TechPreviewNoUpgrade.yaml
  • manifests/00-custom-resource-definition.yaml
  • pkg/operator/controller/ingress/deployment.go
  • pkg/operator/controller/ingress/deployment_test.go

Comment thread manifests/00-custom-resource-definition-OKD.yaml
@gcs278
Copy link
Copy Markdown
Contributor

gcs278 commented Mar 18, 2026

/assign @candita
/assign @jcmoraisjr

{"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
{"ROUTER_MAX_DYNAMIC_SERVERS", true, "100"},
{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"},
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Rabbit comment but on another place: is it expected DCM being enabled even without feature gate allowing it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unsupportedConfigOverride overrides the featuregate and API field.

{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"},
},
},
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed a test (trying to) adding configurationManagement and dcmEnabled as false.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add the test cases. However, the API field is featuregated, so it does not exist (and therefore cannot be set) if the featuregate is not enabled.

Copy link
Copy Markdown
Contributor Author

@Miciah Miciah Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil {
// Config override can still be used to opt out from DCM.
enableDCM = v
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to Rabbit's comment, and related with another one I added in the tests. Although this is not part of the change, but shouldn't this override be under the feature gate, just like the API?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we enabled this as an unsupported option before we had the supported feature gate option. So we will still honor the unsupported override values even if the feature gate is disabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Candace said, the unsupported config override predates the featuregate. Besides that, unsupported config overrides generally do and should take precedence over any other configuration; that is implied by the word "override".

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2026
@Miciah Miciah force-pushed the NE-2523-implement-configurationManagement-API branch from f625550 to 048f2c8 Compare March 24, 2026 20:38
@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 24, 2026

@Miciah Miciah force-pushed the NE-2523-implement-configurationManagement-API branch from 048f2c8 to 60a6a81 Compare March 24, 2026 20:38
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2026
@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 24, 2026

@Miciah Miciah force-pushed the NE-2523-implement-configurationManagement-API branch from 60a6a81 to 43139b2 Compare March 24, 2026 20:39
@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 24, 2026

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/operator/controller/ingress/deployment_test.go (1)

1209-1246: Add one featuregate-enabled + override=true precedence case.

You already assert override precedence in several paths; adding a dynamicConfigManager:"true" case when featuregate is enabled and configurationManagement is omitted (or ForkAndReload) would close the remaining high-value gap.

Suggested test-case addition
 		{
 			name:                       "featuregate-enabled-configurationManagement-ForkAndReload-dcm-override",
 			unsupportedConfigOverrides: `{"dynamicConfigManager":"false","maxDynamicServers":"7"}`,
 			dcmEnabled:                 true,
 			configurationManagement:    operatorv1.IngressControllerConfigurationManagementForkAndReload,
 			expectedEnv:                notSet,
 		},
+		{
+			name:                       "featuregate-enabled-configurationManagement-omitted-dcm-override-true",
+			unsupportedConfigOverrides: `{"dynamicConfigManager":"true","maxDynamicServers":"7"}`,
+			dcmEnabled:                 true,
+			/* configurationManagement omitted. */
+			expectedEnv: []envData{
+				{"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
+				{"ROUTER_MAX_DYNAMIC_SERVERS", true, "7"},
+				{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"},
+			},
+		},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/operator/controller/ingress/deployment_test.go` around lines 1209 - 1246,
Add a test-case to the existing table that verifies override=true precedence
when the feature gate is enabled: insert a case similar to the other
dcmEnabled:true entries with unsupportedConfigOverrides set to
`{"dynamicConfigManager":"true"}`, dcmEnabled: true, leave
configurationManagement omitted (or add a separate case with
configurationManagement:
operatorv1.IngressControllerConfigurationManagementForkAndReload), and set
expectedEnv to the value that asserts the dynamicConfigManager override is
honored (i.e., the env var indicating DCM is enabled). Target the test table
entries using the fields unsupportedConfigOverrides, dcmEnabled,
configurationManagement, and expectedEnv so the new case follows the existing
"featuregate-enabled-..." group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@go.mod`:
- Line 235: Remove the ad-hoc replace directive that points
github.com/openshift/api to a personal fork/commit (the replace line referencing
github.com/Miciah/api v0.0.0-20260312135711-6a8c07c9cf16); either revert the
replace to use the official upstream module/version or, if the forked commit is
temporarily required, replace it with a documented justification and a link to
the upstream PR plus an explicit removal condition (and a TODO) so the
dependency is not shipped indefinitely.

---

Nitpick comments:
In `@pkg/operator/controller/ingress/deployment_test.go`:
- Around line 1209-1246: Add a test-case to the existing table that verifies
override=true precedence when the feature gate is enabled: insert a case similar
to the other dcmEnabled:true entries with unsupportedConfigOverrides set to
`{"dynamicConfigManager":"true"}`, dcmEnabled: true, leave
configurationManagement omitted (or add a separate case with
configurationManagement:
operatorv1.IngressControllerConfigurationManagementForkAndReload), and set
expectedEnv to the value that asserts the dynamicConfigManager override is
honored (i.e., the env var indicating DCM is enabled). Target the test table
entries using the fields unsupportedConfigOverrides, dcmEnabled,
configurationManagement, and expectedEnv so the new case follows the existing
"featuregate-enabled-..." group.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 23b4b050-64b3-43c9-ba22-90956429be68

📥 Commits

Reviewing files that changed from the base of the PR and between f625550 and 43139b2.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (8)
  • go.mod
  • manifests/00-custom-resource-definition-CustomNoUpgrade.yaml
  • manifests/00-custom-resource-definition-DevPreviewNoUpgrade.yaml
  • manifests/00-custom-resource-definition-OKD.yaml
  • manifests/00-custom-resource-definition-TechPreviewNoUpgrade.yaml
  • manifests/00-custom-resource-definition.yaml
  • pkg/operator/controller/ingress/deployment.go
  • pkg/operator/controller/ingress/deployment_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • manifests/00-custom-resource-definition-OKD.yaml
  • manifests/00-custom-resource-definition.yaml

Comment thread go.mod Outdated
@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 24, 2026

Before the rebase, ingress-operator was logging ERROR operator.init.controller-runtime.source.Kind wait/loop.go:87 if kind is a CRD, it should be installed before calling Start {"kind": "DNSRecord.ingress.operator.openshift.io", "error": "failed to get restmapping: no matches for kind \"DNSRecord\" in version \"ingress.operator.openshift.io/v1\""} and eventually terminating before completing its initialization. I notice that 5499634 added release.openshift.io/feature-set: Default to manifests/00-custom-resource-definition-internal.yaml, so I am hoping that rebasing resolves the DNSRecord CRD issue.

@Miciah Miciah force-pushed the NE-2523-implement-configurationManagement-API branch from 43139b2 to d12af90 Compare March 24, 2026 20:52
@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 24, 2026

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/operator/controller/ingress/deployment_test.go (1)

1165-1207: Add one happy-path maxDynamicServers case for ConfigurationManagement=Dynamic.

The matrix covers default, clamped, and invalid values, but not a valid positive override. A regression that ignores maxDynamicServers on the new API-driven Dynamic path and always falls back to "1" would still pass here.

Suggested test case
 		{
 			name:                       "featuregate-enabled-configurationManagement-ForkAndReload-max-servers",
 			unsupportedConfigOverrides: `{"maxDynamicServers":"7"}`,
 			configurationManagement:    operatorv1.IngressControllerConfigurationManagementForkAndReload,
 			dcmEnabled:                 true,
 			expectedEnv:                notSet,
 		},
+		{
+			name:                       "featuregate-enabled-configurationManagement-Dynamic-max-servers",
+			unsupportedConfigOverrides: `{"maxDynamicServers":"7"}`,
+			dcmEnabled:                 true,
+			configurationManagement:    operatorv1.IngressControllerConfigurationManagementDynamic,
+			expectedEnv: []envData{
+				{"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
+				{"ROUTER_MAX_DYNAMIC_SERVERS", true, "7"},
+				{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"},
+			},
+		},
 		{
 			name:                       "featuregate-enabled-configurationManagement-Dynamic-max-servers-lower-limit",
 			unsupportedConfigOverrides: `{"maxDynamicServers":"0"}`,
 			dcmEnabled:                 true,
 			configurationManagement:    operatorv1.IngressControllerConfigurationManagementDynamic,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/operator/controller/ingress/deployment_test.go` around lines 1165 - 1207,
Add a happy-path test case to the table in deployment_test.go that verifies a
positive maxDynamicServers override is honored for ConfigurationManagement =
Dynamic: add an entry (e.g. name
"featuregate-enabled-configurationManagement-Dynamic-max-servers-valid-value")
with unsupportedConfigOverrides `{"maxDynamicServers":"5"}`, dcmEnabled: true,
configurationManagement:
operatorv1.IngressControllerConfigurationManagementDynamic, and expectedEnv set
using envData to include {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
{"ROUTER_MAX_DYNAMIC_SERVERS", true, "5"} (and keep
{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"} if consistent with other Dynamic
expectations) so the test will fail if the code ignores the provided
maxDynamicServers override.
🤖 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/operator/controller/ingress/deployment_test.go`:
- Around line 1165-1207: Add a happy-path test case to the table in
deployment_test.go that verifies a positive maxDynamicServers override is
honored for ConfigurationManagement = Dynamic: add an entry (e.g. name
"featuregate-enabled-configurationManagement-Dynamic-max-servers-valid-value")
with unsupportedConfigOverrides `{"maxDynamicServers":"5"}`, dcmEnabled: true,
configurationManagement:
operatorv1.IngressControllerConfigurationManagementDynamic, and expectedEnv set
using envData to include {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
{"ROUTER_MAX_DYNAMIC_SERVERS", true, "5"} (and keep
{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"} if consistent with other Dynamic
expectations) so the test will fail if the code ignores the provided
maxDynamicServers override.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 9e1994e6-8485-4c4d-b746-2afac2e13b3a

📥 Commits

Reviewing files that changed from the base of the PR and between 43139b2 and d12af90.

📒 Files selected for processing (2)
  • pkg/operator/controller/ingress/deployment.go
  • pkg/operator/controller/ingress/deployment_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/operator/controller/ingress/deployment.go

@lihongan
Copy link
Copy Markdown

cc @ShudiLi

@candita
Copy link
Copy Markdown
Contributor

candita commented Mar 26, 2026

== NAME TestAll/parallel/TestRouteAdmissionPolicy
operator_test.go:2152: failed to observe expected conditions: timed out waiting for the condition

/test e2e-gcp-operator

@candita
Copy link
Copy Markdown
Contributor

candita commented Mar 26, 2026

=== FAIL: . TestCreateCluster/Teardown (1322.32s)
...
fixture.go:333: Failed to wait for infra resources in guest cluster to be deleted: context deadline exceeded
fixture.go:340: Failed to clean up 4 remaining resources for guest cluster

/test e2e-hypershift

@ShudiLi
Copy link
Copy Markdown

ShudiLi commented Mar 27, 2026

tested it with 4.22.0-0-2026-03-27-012542-test-ci-ln-54sgc12-latest

1.
% oc get clusterversion
NAME      VERSION                                                AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.22.0-0-2026-03-27-012542-test-ci-ln-54sgc12-latest   True        False         42m     Cluster version is 4.22.0-0-2026-03-27-012542-test-ci-ln-54sgc12-latest

2. By default, dcm is disabled, enable it by set spec.tuningOptions.configurationManagement with Dynamic
%oc -n openshift-ingress-operator get ingresscontroller default -ojsonpath="{.spec.tuningOptions.configurationManagement}"
Dynamic%  

3. create deployment, service and route
 % oc get route
NAME          HOST/PORT                                                             PATH   SERVICES      PORT          TERMINATION   WILDCARD
unsec-apach   unsec-apach-default.apps.ci-ln-54sgc12-76ef8.aws-2.ci.openshift.org          unsec-apach   unsec-apach                 None

4. check the haproxy.config
  cookie b94bb237dc742029fe83e6d395082b86 insert indirect nocache httponly dynamic
  server pod:appach-server-59c457d9d4-5bv2z:unsec-apach:unsec-apach:10.128.2.20:8080 10.128.2.20:8080 cookie 962c9c4d5a10a8476ed2486a721acf20 weight 1 check inter 5000ms
  server pod:appach-server-59c457d9d4-9nv8h:unsec-apach:unsec-apach:10.129.2.13:8080 10.129.2.13:8080 cookie 70ea94d714a811908fd249367321fa11 weight 1 check inter 5000ms
  dynamic-cookie-key b94bb237dc742029fe83e6d395082b86
  server-template _dynamic-pod- 1-1 172.4.0.4:8765 check inter 5000ms disabled

% oc -n openshift-ingress exec router-default-6856846c5f-jpg7d -- /bin/sh -c "echo \"show servers state ${BACKEND}\" | socat stdio /var/lib/haproxy/run/haproxy.sock" | sed 1d | grep -v '^#' | cut -d ' ' -f2-6 | sed -e 's/0$/STOP/' -e 's/1$/STARTING/' -e 's/2$/UP/' -e 's/3$/SOFTSTOP/'
be_http:default:unsec-apach 1 pod:appach-server-59c457d9d4-5bv2z:unsec-apach:unsec-apach:10.128.2.20:8080 10.128.2.20 UP
be_http:default:unsec-apach 2 pod:appach-server-59c457d9d4-9nv8h:unsec-apach:unsec-apach:10.129.2.13:8080 10.129.2.13 UP
be_http:default:unsec-apach 3 _dynamic-pod-1 10.131.0.20 UP

5. Disable the DCM by removing spec.tuningOptions.configurationManagement
6. Check the haproxy.config
  http-request add-header Forwarded for=%[src];host=%[req.hdr(host)];proto=%[req.hdr(X-Forwarded-Proto)]
  cookie b94bb237dc742029fe83e6d395082b86 insert indirect nocache httponly
  server pod:appach-server-59c457d9d4-5bv2z:unsec-apach:unsec-apach:10.128.2.20:8080 10.128.2.20:8080 cookie 962c9c4d5a10a8476ed2486a721acf20 weight 1 check inter 5000ms
  server pod:appach-server-59c457d9d4-9nv8h:unsec-apach:unsec-apach:10.129.2.13:8080 10.129.2.13:8080 cookie 70ea94d714a811908fd249367321fa11 weight 1 check inter 5000ms
  server pod:appach-server-59c457d9d4-kwwlr:unsec-apach:unsec-apach:10.131.0.20:8080 10.131.0.20:8080 cookie c41bda1361321b2c899ea9a0ecd2a801 weight 1 check inter 5000ms

# Secure backend, pass through

% oc -n openshift-ingress exec router-default-8497f84886-lfcz9 -- /bin/sh -c "echo \"show servers state ${BACKEND}\" | socat stdio /var/lib/haproxy/run/haproxy.sock" | sed 1d | grep -v '^#' | cut -d ' ' -f2-6 | sed -e 's/0$/STOP/' -e 's/1$/STARTING/' -e 's/2$/UP/' -e 's/3$/SOFTSTOP/'
be_http:default:unsec-apach 1 pod:appach-server-59c457d9d4-5bv2z:unsec-apach:unsec-apach:10.128.2.20:8080 10.128.2.20 UP
be_http:default:unsec-apach 2 pod:appach-server-59c457d9d4-9nv8h:unsec-apach:unsec-apach:10.129.2.13:8080 10.129.2.13 UP
be_http:default:unsec-apach 3 pod:appach-server-59c457d9d4-kwwlr:unsec-apach:unsec-apach:10.131.0.20:8080 10.131.0.20 UP

%

7. curl the route when dcm is enabled or disabled
% curl http://unsec-apach-default.apps.ci-ln-54sgc12-76ef8.aws-2.ci.openshift.org
It is a test!
% curl http://unsec-apach-test.apps.ci-ln-54sgc12-76ef8.aws-2.ci.openshift.org
It is a test!
% curl https://edge1-test.apps.ci-ln-54sgc12-76ef8.aws-2.ci.openshift.org -ks
It is a test!
% curl https://pass1-test.apps.ci-ln-54sgc12-76ef8.aws-2.ci.openshift.org -ks
It is a test!
% curl https://reencrypt1-test.apps.ci-ln-54sgc12-76ef8.aws-2.ci.openshift.org -ks
It is a test!
% 

8. enable dcm again and check the router log
% oc -n openshift-ingress get pods                            
NAME                              READY   STATUS    RESTARTS   AGE
router-default-6856846c5f-654gv   1/1     Running   0          2m52s
router-default-6856846c5f-zfbbq   1/1     Running   0          2m52s
%% oc -n openshift-ingress logs router-default-6856846c5f-654gv
I0327 03:10:32.905152       1 template.go:560] "msg"="starting router" "logger"="router" "version"="majorFromGit: \nminorFromGit: \ncommitFromGit: 8b94c5cd90c435e6b3cf137dc35fcbddb4c048a3\nversionFromGit: 4.0.0-646-g8b94c5cd\ngitTreeState: clean\nbuildDate: 2026-03-25T16:58:42Z\n"
I0327 03:10:32.905699       1 envvar.go:172] "Feature gate default state" feature="InformerResourceVersion" enabled=true
I0327 03:10:32.905713       1 envvar.go:172] "Feature gate default state" feature="WatchListClient" enabled=true
I0327 03:10:32.905718       1 envvar.go:172] "Feature gate default state" feature="ClientsAllowCBOR" enabled=false
I0327 03:10:32.905722       1 envvar.go:172] "Feature gate default state" feature="ClientsPreferCBOR" enabled=false
I0327 03:10:32.905725       1 envvar.go:172] "Feature gate default state" feature="InOrderInformers" enabled=true
I0327 03:10:32.905729       1 envvar.go:172] "Feature gate default state" feature="InOrderInformersBatchProcess" enabled=true
I0327 03:10:32.907108       1 metrics.go:156] "msg"="router health and metrics port listening on HTTP and HTTPS" "address"="0.0.0.0:1936" "logger"="metrics"
I0327 03:10:32.910450       1 router.go:213] "msg"="creating a new template router" "logger"="template" "writeDir"="/var/lib/haproxy"
I0327 03:10:32.910492       1 router.go:297] "msg"="router will coalesce reloads within an interval of each other" "interval"="5s" "logger"="template"
I0327 03:10:32.910801       1 router.go:367] "msg"="watching for changes" "logger"="template" "path"="/etc/pki/tls/private"
I0327 03:10:32.910844       1 router.go:288] "msg"="initializing dynamic config manager ... " "logger"="template"
I0327 03:10:32.910894       1 manager.go:757] "msg"="provisioning blueprint route pool" "logger"="manager" "name"="_blueprint-http-route" "namespace"="_hapcm_blueprint_pool" "size"=0
I0327 03:10:32.910903       1 manager.go:757] "msg"="provisioning blueprint route pool" "logger"="manager" "name"="_blueprint-edge-route" "namespace"="_hapcm_blueprint_pool" "size"=0
I0327 03:10:32.910911       1 manager.go:757] "msg"="provisioning blueprint route pool" "logger"="manager" "name"="_blueprint-passthrough-route" "namespace"="_hapcm_blueprint_pool" "size"=0
I0327 03:10:32.910918       1 manager.go:201] "msg"="haproxy Config Manager router will flush out any dynamically configured changes within some interval of each other" "interval"="1h0m0s" "logger"="manager"
I0327 03:10:32.910926       1 router.go:283] "msg"="router is including routes in all namespaces" "logger"="router"
I0327 03:10:32.923361       1 reflector.go:446] "Caches populated" type="*v1.EndpointSlice" reflector="github.com/openshift/router/pkg/router/controller/factory/factory.go:124"
I0327 03:10:32.928079       1 reflector.go:446] "Caches populated" type="*v1.Service" reflector="github.com/openshift/router/pkg/router/template/service_lookup.go:33"
I0327 03:10:32.928276       1 reflector.go:446] "Caches populated" type="*v1.Route" reflector="github.com/openshift/router/pkg/router/controller/factory/factory.go:124"
E0327 03:10:33.018479       1 haproxy.go:426] "Unhandled Error" err="can't scrape HAProxy: dial unix /var/lib/haproxy/run/haproxy.sock: connect: no such file or directory" logger="UnhandledError"
I0327 03:10:33.078417       1 router.go:664] "msg"="router reloaded" "logger"="template" "output"=" - Checking http://localhost:80 using PROXY protocol ...\n - Health check ok : 0 retry attempt(s).\n"
%

@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 27, 2026

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Mar 27, 2026

@Miciah: This pull request references NE-2523 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.22.0" version, but no target version was set.

Details

In response to this:

Bump openshift/api for configurationManagement API

Bump to openshift/api@582dc3d to get the spec.tuningOptions.configurationManagement API field to allow enabling or disabling the Dynamic Configuration Manager.

Implement configurationManagement API

Check the spec.tuningOptions.configurationManagement API field, and only enable the Dynamic Configuration Manager if the field is set to "Dynamic".

For now, the default behavior (when the field is omitted) is not to enable DCM. Once we are more confident that the feature is safe to enable, we will change the default behavior.


This PR depends on openshift/api#2757.

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.

@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 27, 2026

The build is failing with the following errors:

 # github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicyspec.go:12:26: undefined: configv1alpha1.ImageScope
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyrootoftrust.go:12:36: undefined: configv1alpha1.PolicyType
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyidentity.go:12:45: undefined: configv1alpha1.IdentityMatchPolicy
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policymatchexactrepository.go:12:29: undefined: configv1alpha1.IdentityRepositoryPrefix
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policymatchremapidentity.go:12:31: undefined: configv1alpha1.IdentityRepositoryPrefix
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policymatchremapidentity.go:13:31: undefined: configv1alpha1.IdentityRepositoryPrefix
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyspec.go:12:26: undefined: configv1alpha1.ImageScope
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicy.go:44:67: undefined: configv1alpha1.ClusterImagePolicy
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicy.go:51:73: undefined: configv1alpha1.ClusterImagePolicy
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicy.go:55:67: undefined: configv1alpha1.ClusterImagePolicy
../../vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicy.go:55:67: too many errors

I hope that bumping openshift/api to pull in openshift/api#2760 will resolve the errors.

@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 27, 2026

Hm, I already had openshift/api#2760 (it merged before #2757); I think openshift/client-go needs to be updated.

@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 27, 2026

Looks like openshift/client-go#365 resolves the issue.

Miciah added 2 commits March 27, 2026 12:06
Bump to github.com/openshift/api@582dc3d316b7705ae37fb750576454d387b2b53e
to get the spec.tuningOptions.configurationManagement API field to allow
enabling or disabling the Dynamic Configuration Manager.

Bump to github.com/openshift/client-go@743f664b82d17eca2c9474b6be635cf53e22d3ab
to resolve some build failures that were caused by the deletion of some
alpha APIs in openshift/api.

    go get github.com/openshift/api@582dc3d316b7705ae37fb750576454d387b2b53e \
           github.com/openshift/client-go@743f664b82d17eca2c9474b6be635cf53e22d3ab
    go mod tidy
    go mod vendor
    make update

* go.mod: Update.
* go.sum:
* manifests/*:
* vendor/*: Regenerate.

Modified-by: Miciah Dashiel Butler Masters <mmasters@redhat.com>
Check the spec.tuningOptions.configurationManagement API field, and only
enable the Dynamic Configuration Manager if the field is set to
"Dynamic".

For now, the default behavior (when the field is omitted) is not to
enable DCM.  Once we are more confident that the feature is safe to
enable, we will change the default behavior.

This commit implements NE-2523.

https://issues.redhat.com/browse/NE-2523

* pkg/operator/controller/ingress/deployment.go
(desiredRouterDeployment): Check
spec.tuningOptions.configurationManagement to enable DCM.

* pkg/operator/controller/ingress/deployment.go
(desiredRouterDeployment): Check that
spec.tuningOptions.configurationManagement is set to "Dynamic" before
enabling the Dynamic Configuration Manager.
* pkg/operator/controller/ingress/deployment_test.go
(TestDesiredRouterDeploymentDynamicConfigManager): Add the value for
spec.tuningOptions.configurationManagement to the test input, and add
test cases various different settings for configurationManagement.
@Miciah Miciah force-pushed the NE-2523-implement-configurationManagement-API branch from 6727eee to 6b6d84c Compare March 27, 2026 16:10
@Miciah
Copy link
Copy Markdown
Contributor Author

Miciah commented Mar 27, 2026

@jcmoraisjr
Copy link
Copy Markdown
Member

/approve
/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Mar 27, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcmoraisjr

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Mar 27, 2026

@Miciah: 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/e2e-aws-operator-techpreview 6b6d84c link false /test e2e-aws-operator-techpreview
ci/prow/e2e-vsphere-static-metallb-operator-gwapi 6b6d84c link false /test e2e-vsphere-static-metallb-operator-gwapi

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.

@ShudiLi
Copy link
Copy Markdown

ShudiLi commented Mar 30, 2026

/verified by @ShudiLi
tested it again with 4.22.0-0-2026-03-30-014309-test-ci-ln-4wh3nck-latest, marked it verified, thanks.

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Mar 30, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@ShudiLi: This PR has been marked as verified by @ShudiLi.

Details

In response to this:

/verified by @ShudiLi
tested it again with 4.22.0-0-2026-03-30-014309-test-ci-ln-4wh3nck-latest, marked it verified, thanks.

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.

@ShudiLi
Copy link
Copy Markdown

ShudiLi commented Mar 30, 2026

/retest-required

@openshift-merge-bot openshift-merge-bot Bot merged commit d212fd1 into openshift:master Mar 30, 2026
17 of 19 checks passed
@lihongan
Copy link
Copy Markdown

lihongan commented Apr 2, 2026

Change included in accepted release 4.22.0-0.nightly-2026-04-01-092906

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants