CORS-4308: Enable Network Observability during installation#10382
CORS-4308: Enable Network Observability during installation#10382OlivierCazade wants to merge 4 commits intoopenshift:mainfrom
Conversation
…erge. Must be changed before merging this branch.
|
@OlivierCazade: This pull request references CORS-4308 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. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughThese changes introduce a new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@OlivierCazade: This pull request references CORS-4308 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. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/types/defaults/installconfig_test.go (1)
290-315: Add the explicit empty-string case too.
""is part of the allowed enum, but the new table only exercisesniland"Disable". A case withInstallNetworkObservability: ptr("")would lock down whether defaults should preserve the empty string or normalize it to"Enable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/defaults/installconfig_test.go` around lines 290 - 315, Add a third table-driven test case exercising the explicit empty-string enum for Networking.InstallNetworkObservability: create a new entry in the test slice with name like "InstallNetworkObservability empty string", set config: &types.InstallConfig{Networking: &types.Networking{InstallNetworkObservability: func() *string { s := ""; return &s }()}}, and set expected to the result you expect (either keep empty string or normalize to "Enable") by cloning defaultInstallConfig() and adjusting c.Networking.InstallNetworkObservability accordingly; this ensures the behavior of InstallNetworkObservability for "" is locked down in the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 4799-4802: The enum currently allows an empty-string value ("")
alongside "Enable" and "Disable" but the schema description only documents
omitted, "Enable" and "Disable"; update the enum to remove the blank entry or
explicitly document the blank as equivalent to the intended behavior (e.g.,
treat "" as omitted or as "Enable") so there's no ambiguity—modify the enum list
(remove the line with - "") or add a sentence to the field description
clarifying how "" is interpreted.
- Around line 4794-4803: Add an explicit default for the
installNetworkObservability property in the CRD schema so schema-driven tooling
matches runtime behavior: update the installNetworkObservability property (the
block with description, enum, and type) to include a default: "Enable" entry
(ensuring the string value matches the enum entries), e.g., add default: Enable
under the same indentation as enum/type; keep the existing enum and type
unchanged.
In `@go.mod`:
- Line 393: Remove the temporary replace directive that points openshift/api to
the personal fork: delete the line "replace github.com/openshift/api
v0.0.0-20260228183123-9b2ee997d297 => github.com/OlivierCazade/api
v0.0.0-20260310172058-df85794acda8" from go.mod and, if needed, run `go get` or
`go mod tidy` to restore a proper official github.com/openshift/api revision so
all imports of github.com/openshift/api (e.g., config/v1, machine/v1,
machine/v1beta1, machineconfiguration/v1, operator/v1) resolve to the upstream
module rather than the fork.
In `@pkg/types/installconfig.go`:
- Around line 429-435: In validateNetworking(), add an explicit validation for
the InstallNetworkObservability field to reject any value other than the allowed
set ("", "Enable", "Disable"); locate the InstallNetworkObservability pointer on
the InstallConfig type and if non-nil check its value and append a field error
(using the same error accumulation approach used in validateNetworking) for the
json field "installNetworkObservability" when it is not one of the three allowed
strings so invalid values are rejected before manifest generation.
---
Nitpick comments:
In `@pkg/types/defaults/installconfig_test.go`:
- Around line 290-315: Add a third table-driven test case exercising the
explicit empty-string enum for Networking.InstallNetworkObservability: create a
new entry in the test slice with name like "InstallNetworkObservability empty
string", set config: &types.InstallConfig{Networking:
&types.Networking{InstallNetworkObservability: func() *string { s := ""; return
&s }()}}, and set expected to the result you expect (either keep empty string or
normalize to "Enable") by cloning defaultInstallConfig() and adjusting
c.Networking.InstallNetworkObservability accordingly; this ensures the behavior
of InstallNetworkObservability for "" is locked down in the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87623f3e-7184-4fb6-99f7-4eb911b3e0bb
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumpkg/types/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
data/data/install.openshift.io_installconfigs.yamlgo.modpkg/asset/manifests/network.gopkg/types/defaults/installconfig.gopkg/types/defaults/installconfig_test.gopkg/types/installconfig.go
| installNetworkObservability: | ||
| description: |- | ||
| InstallNetworkObservability is an optional field that enables network observability | ||
| when omitted or set to "Enable". If the field is set to "Disable", it does nothing. | ||
| Valid values are "", "Enable", "Disable". | ||
| enum: | ||
| - "" | ||
| - Enable | ||
| - Disable | ||
| type: string |
There was a problem hiding this comment.
Declare the default in the schema.
The PR defaults this field to "Enable" when it is omitted, but the CRD schema does not advertise that with a default: entry. That leaves schema-driven tooling and CRD defaulting out of sync with the documented behavior.
Suggested fix
installNetworkObservability:
description: |-
InstallNetworkObservability is an optional field that enables network observability
when omitted or set to "Enable". If the field is set to "Disable", it does nothing.
Valid values are "", "Enable", "Disable".
+ default: Enable
enum:
- ""
- Enable
- Disable
type: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| installNetworkObservability: | |
| description: |- | |
| InstallNetworkObservability is an optional field that enables network observability | |
| when omitted or set to "Enable". If the field is set to "Disable", it does nothing. | |
| Valid values are "", "Enable", "Disable". | |
| enum: | |
| - "" | |
| - Enable | |
| - Disable | |
| type: string | |
| installNetworkObservability: | |
| description: |- | |
| InstallNetworkObservability is an optional field that enables network observability | |
| when omitted or set to "Enable". If the field is set to "Disable", it does nothing. | |
| Valid values are "", "Enable", "Disable". | |
| default: Enable | |
| enum: | |
| - "" | |
| - Enable | |
| - Disable | |
| type: string |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/data/install.openshift.io_installconfigs.yaml` around lines 4794 - 4803,
Add an explicit default for the installNetworkObservability property in the CRD
schema so schema-driven tooling matches runtime behavior: update the
installNetworkObservability property (the block with description, enum, and
type) to include a default: "Enable" entry (ensuring the string value matches
the enum entries), e.g., add default: Enable under the same indentation as
enum/type; keep the existing enum and type unchanged.
| enum: | ||
| - "" | ||
| - Enable | ||
| - Disable |
There was a problem hiding this comment.
Avoid an undocumented empty-string mode.
The enum accepts "", but the description only defines behavior for omitted, "Enable", and "Disable". Please either document "" as equivalent to omitted/"Enable" or remove it from the allowed values so users do not get an ambiguous configuration state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/data/install.openshift.io_installconfigs.yaml` around lines 4799 - 4802,
The enum currently allows an empty-string value ("") alongside "Enable" and
"Disable" but the schema description only documents omitted, "Enable" and
"Disable"; update the enum to remove the blank entry or explicitly document the
blank as equivalent to the intended behavior (e.g., treat "" as omitted or as
"Enable") so there's no ambiguity—modify the enum list (remove the line with -
"") or add a sentence to the field description clarifying how "" is interpreted.
| sigs.k8s.io/cluster-api-provider-azure => github.com/mboersma/cluster-api-provider-azure v0.3.1-0.20251030205607-3161b9cc8d3e | ||
| ) | ||
|
|
||
| replace github.com/openshift/api v0.0.0-20260228183123-9b2ee997d297 => github.com/OlivierCazade/api v0.0.0-20260310172058-df85794acda8 No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== replace directive =="
sed -n '390,394p' go.mod
echo
echo "== repo imports that still resolve through github.com/openshift/api =="
rg -n -C1 '"github\.com/openshift/api/' --type=go -g '!vendor/**'Repository: openshift/installer
Length of output: 50375
Do not merge with the personal-fork openshift/api replace still in place.
This makes the branch depend on github.com/OlivierCazade/api instead of an official github.com/openshift/api revision. The codebase imports from github.com/openshift/api across numerous packages (config/v1, machine/v1, machine/v1beta1, machineconfiguration/v1, features, operator/v1, and more), all of which would be satisfied by the temporary fork if merged. This introduces a supply-chain and reproducibility risk to main and must be reverted before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 393, Remove the temporary replace directive that points
openshift/api to the personal fork: delete the line "replace
github.com/openshift/api v0.0.0-20260228183123-9b2ee997d297 =>
github.com/OlivierCazade/api v0.0.0-20260310172058-df85794acda8" from go.mod
and, if needed, run `go get` or `go mod tidy` to restore a proper official
github.com/openshift/api revision so all imports of github.com/openshift/api
(e.g., config/v1, machine/v1, machine/v1beta1, machineconfiguration/v1,
operator/v1) resolve to the upstream module rather than the fork.
| // InstallNetworkObservability is an optional field that enables network observability | ||
| // when omitted or set to "Enable". If the field is set to "Disable", it does nothing. | ||
| // Valid values are "", "Enable", "Disable". | ||
| // | ||
| // +kubebuilder:validation:Enum="";Enable;Disable | ||
| // +optional | ||
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validate install-config entry points =="
fd 'installconfig\.go$' pkg/types/validation -x sed -n '120,220p' {}
echo
echo "== references to the new field in validation code =="
rg -n -C3 'InstallNetworkObservability|validateNetworking\s*\(' pkg/types/validationRepository: openshift/installer
Length of output: 6902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the validateNetworking function starting from line 505
echo "=== validateNetworking function (full implementation) ==="
sed -n '505,700p' pkg/types/validation/installconfig.go
# Also check if InstallNetworkObservability is mentioned anywhere in the validation files
echo ""
echo "=== Search for InstallNetworkObservability in validation ==="
rg -n 'InstallNetworkObservability' pkg/types/validation/Repository: openshift/installer
Length of output: 9479
Add enum validation for InstallNetworkObservability in the validation logic.
The validateNetworking() function in pkg/types/validation/installconfig.go does not validate the InstallNetworkObservability field. The kubebuilder tag documents the enum but does not enforce it at the installer level. Add an explicit check to reject values outside "", "Enable", and "Disable" to prevent invalid values from reaching manifest generation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/installconfig.go` around lines 429 - 435, In validateNetworking(),
add an explicit validation for the InstallNetworkObservability field to reject
any value other than the allowed set ("", "Enable", "Disable"); locate the
InstallNetworkObservability pointer on the InstallConfig type and if non-nil
check its value and append a field error (using the same error accumulation
approach used in validateNetworking) for the json field
"installNetworkObservability" when it is not one of the three allowed strings so
invalid values are rejected before manifest generation.
|
@OlivierCazade: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
networking: Add day-0 installation support for Network Observability
github.com/openshift/apito include theinstallNetworkObservabilityfield in the Network CR spec. This dependency must be updated to point to the official release before merging.Summary
This PR implements day-0 installation support for Network Observability as described in the enhancement proposal.
Users can now enable Network Observability during cluster installation by setting the
installNetworkObservabilityfield in the install-config.yaml networking section. When enabled (or omitted, as it defaults to "Enable"), the installer will configure the cluster to deploy Network Observability on day-0.Summary by CodeRabbit
Release Notes
New Features
Tests