CORS-4308: Enable Network Observability during installation - #10382
Conversation
|
@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
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe installer adds optional network observability settings to install-config, preserves explicit policies during defaulting, updates the generated install-config schema, deep-copies the new fields, and passes the policy to the generated Network manifest. ChangesNetwork observability configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@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.
| 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.
barbacbd
left a comment
barbacbd
left a comment
There was a problem hiding this comment.
- you should squash or remove multiple commits here. Your first three commits are all overwritting/renaming the same variable.
- We generally only add to the install-config when absolutely necessary so I will take a closer look to see if this is that case.
- If this is a new feature, we default to previous settings. In this case I believe that the default should be disabled (unless that is going to break something or that is not the equivalent to a previous setting).
- It does happen but very rare for us to merge something with a local version of a dependency. If you wish to merge this, the api change will probably need to be applied first then we can change the reference in your final commit.
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
|
@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. |
|
@barbacbd thanks for your feedback.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 5009-5022: The schema allows an explicit empty
networkObservability object to bypass defaults and manifest emission; either
make installationPolicy required in the networkObservability schema (so
installationPolicy must be present) or change the defaulting logic so that when
networkObservability is present but has no installationPolicy (i.e.,
networkObservability: {}), the defaulting routine that sets installationPolicy
(see installconfig defaulting code) populates a value; after implementing the
schema or defaulting change, add a regression test that validates behavior for
networkObservability: {} to ensure installationPolicy is set and manifests are
emitted as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73a72032-3040-4194-8303-28237406c2b4
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/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/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
data/data/install.openshift.io_installconfigs.yamlgo.modpkg/asset/agent/installconfig_test.gopkg/asset/installconfig/installconfig_test.gopkg/asset/manifests/network.gopkg/types/defaults/installconfig.gopkg/types/defaults/installconfig_test.gopkg/types/installconfig.gopkg/types/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/types/defaults/installconfig.go
- pkg/asset/manifests/network.go
- go.mod
- pkg/types/installconfig.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/types/defaults/installconfig_test.go (1)
292-324: Add a regression case forNetworkObservability: {}with nilInstallationPolicyThe new tests cover
NetworkObservability == niland explicitDoNotInstall, but not the case whereNetworkObservabilityexists andInstallationPolicyis omitted. That case is important for defaulting safety.Suggested test case
+ { + name: "NetworkObservability present with nil InstallationPolicy", + config: &types.InstallConfig{ + Networking: &types.Networking{ + NetworkObservability: &types.NetworkObservability{}, + }, + }, + expected: defaultInstallConfig(), + },As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 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 292 - 324, Add a regression test case that exercises an InstallConfig where Networking.NetworkObservability is non-nil but its InstallationPolicy is nil: create a test entry similar to the existing cases using config: &types.InstallConfig{Networking: &types.Networking{NetworkObservability: &types.NetworkObservability{ /* no InstallationPolicy set */ }}}, and set expected to defaultInstallConfig() with NetworkObservability present and its InstallationPolicy defaulted as the code under test should produce; reference types.InstallConfig, types.Networking, types.NetworkObservability, InstallationPolicy and defaultInstallConfig to locate where to add the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/types/defaults/installconfig.go`:
- Around line 61-66: The current defaulting only sets
c.Networking.NetworkObservability when it's nil, so if
c.Networking.NetworkObservability exists but its InstallationPolicy is nil the
default is not applied; update the logic around
c.Networking.NetworkObservability to ensure that after ensuring the struct
exists (or creating it) you also check if
c.Networking.NetworkObservability.InstallationPolicy == nil and, if so, assign a
new variable set to types.NetworkObservabilityInstallAndEnable and assign its
address to InstallationPolicy; reference the existing symbols
c.Networking.NetworkObservability, InstallationPolicy, and
types.NetworkObservabilityInstallAndEnable when making this change.
---
Nitpick comments:
In `@pkg/types/defaults/installconfig_test.go`:
- Around line 292-324: Add a regression test case that exercises an
InstallConfig where Networking.NetworkObservability is non-nil but its
InstallationPolicy is nil: create a test entry similar to the existing cases
using config: &types.InstallConfig{Networking:
&types.Networking{NetworkObservability: &types.NetworkObservability{ /* no
InstallationPolicy set */ }}}, and set expected to defaultInstallConfig() with
NetworkObservability present and its InstallationPolicy defaulted as the code
under test should produce; reference types.InstallConfig, types.Networking,
types.NetworkObservability, InstallationPolicy and defaultInstallConfig to
locate where to add the new case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39f1f094-ddf3-4fc8-8190-15e509e670a3
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/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/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
data/data/install.openshift.io_installconfigs.yamlgo.modpkg/asset/agent/installconfig_test.gopkg/asset/installconfig/installconfig_test.gopkg/asset/manifests/network.gopkg/types/defaults/installconfig.gopkg/types/defaults/installconfig_test.gopkg/types/installconfig.gopkg/types/zz_generated.deepcopy.go
✅ Files skipped from review due to trivial changes (3)
- pkg/asset/installconfig/installconfig_test.go
- pkg/asset/manifests/network.go
- pkg/types/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/asset/agent/installconfig_test.go
| if c.Networking.NetworkObservability == nil { | ||
| installationPolicy := types.NetworkObservabilityInstallAndEnable | ||
| c.Networking.NetworkObservability = &types.NetworkObservability{ | ||
| InstallationPolicy: &installationPolicy, | ||
| } | ||
| } |
There was a problem hiding this comment.
Default policy is not applied when NetworkObservability exists but InstallationPolicy is omitted
Line 61 only defaults when NetworkObservability is nil. If a user sets networkObservability: {}, InstallationPolicy remains nil, so the “default Enable” behavior is skipped.
Proposed fix
- if c.Networking.NetworkObservability == nil {
- installationPolicy := types.NetworkObservabilityInstallAndEnable
- c.Networking.NetworkObservability = &types.NetworkObservability{
- InstallationPolicy: &installationPolicy,
- }
- }
+ if c.Networking.NetworkObservability == nil {
+ c.Networking.NetworkObservability = &types.NetworkObservability{}
+ }
+ if c.Networking.NetworkObservability.InstallationPolicy == nil {
+ installationPolicy := types.NetworkObservabilityInstallAndEnable
+ c.Networking.NetworkObservability.InstallationPolicy = &installationPolicy
+ }📝 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.
| if c.Networking.NetworkObservability == nil { | |
| installationPolicy := types.NetworkObservabilityInstallAndEnable | |
| c.Networking.NetworkObservability = &types.NetworkObservability{ | |
| InstallationPolicy: &installationPolicy, | |
| } | |
| } | |
| if c.Networking.NetworkObservability == nil { | |
| c.Networking.NetworkObservability = &types.NetworkObservability{} | |
| } | |
| if c.Networking.NetworkObservability.InstallationPolicy == nil { | |
| installationPolicy := types.NetworkObservabilityInstallAndEnable | |
| c.Networking.NetworkObservability.InstallationPolicy = &installationPolicy | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/defaults/installconfig.go` around lines 61 - 66, The current
defaulting only sets c.Networking.NetworkObservability when it's nil, so if
c.Networking.NetworkObservability exists but its InstallationPolicy is nil the
default is not applied; update the logic around
c.Networking.NetworkObservability to ensure that after ensuring the struct
exists (or creating it) you also check if
c.Networking.NetworkObservability.InstallationPolicy == nil and, if so, assign a
new variable set to types.NetworkObservabilityInstallAndEnable and assign its
address to InstallationPolicy; reference the existing symbols
c.Networking.NetworkObservability, InstallationPolicy, and
types.NetworkObservabilityInstallAndEnable when making this change.
|
@OlivierCazade Can you rebase and re-test? @barbacbd Let us know if you have additional comments. Trying to get this one moving again. |
Add support for controlling network observability installation via the networkObservability field in the install config. This field contains an installationPolicy that accepts "InstallAndEnable", "DoNotInstall", or empty string values, and defaults to "InstallAndEnable" when omitted. The field is propagated from the install config through to the cluster Network resource spec during manifest generation, allowing network observability to be installed by default with an opt-out mechanism.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
removed
the
needs-rebase
|
@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 "5.0.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@pkg/asset/installconfig/installconfig_test.go`:
- Line 132: Run the repository Go formatter via hack/go-fmt.sh . and ensure the
Networking composite literals around the affected test cases are formatted
consistently with surrounding fields.
In `@pkg/types/installconfig.go`:
- Around line 820-827: Update ValidateInstallConfig’s networking validation,
using the nested NetworkObservability.InstallationPolicy field, to accept only
the InstallAndEnable and NoAction constants before manifest generation; return a
validation error for any other value and add coverage for an invalid policy such
as “Bogus”.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 56806be7-6e55-450f-94c3-cd80b7d9e1e5
⛔ Files ignored due to path filters (2)
data/data/install.openshift.io_installconfigs.yamlis excluded by!data/data/install.openshift.io_installconfigs.yamlpkg/types/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
pkg/asset/agent/installconfig_test.gopkg/asset/installconfig/installconfig_test.gopkg/asset/manifests/network.gopkg/types/defaults/installconfig.gopkg/types/defaults/installconfig_test.gopkg/types/installconfig.go
💤 Files with no reviewable changes (1)
- pkg/types/defaults/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/types/defaults/installconfig_test.go
- pkg/asset/manifests/network.go
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run the repository Go formatter on these literals.
The closing Networking composite literals at Line 132, Line 233, and Line 289 are more indented than the surrounding fields. Run hack/go-fmt.sh . before merge.
As per coding guidelines, Go changes must use the repository formatting scripts.
Also applies to: 233-233, 289-289
🤖 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 `@pkg/asset/installconfig/installconfig_test.go` at line 132, Run the
repository Go formatter via hack/go-fmt.sh . and ensure the Networking composite
literals around the affected test cases are formatted consistently with
surrounding fields.
Source: Coding guidelines
| // NetworkObservability defines the configuration for network observability installation | ||
| type NetworkObservability struct { | ||
| // InstallationPolicy controls whether network observability is installed during cluster deployment. | ||
| // Valid values are "InstallAndEnable" and "NoAction". | ||
| // When set to "InstallAndEnable", network observability will be installed and enabled. | ||
| // When set to "NoAction", nothing will be done regarding network observability. | ||
| // +optional | ||
| InstallationPolicy *NetworkObservabilityInstallationPolicy `json:"installationPolicy,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Reject invalid InstallationPolicy values before manifest generation.
The change adds the enum marker, but the supplied code does not show an installer-side check in ValidateInstallConfig. pkg/asset/manifests/network.go casts the value directly to the API enum. If a value such as "Bogus" reaches that path, the installer can emit an invalid Network object and fail later. Validate Networking.NetworkObservability.InstallationPolicy against the two constants and test an invalid value.
This is the same validation gap raised in the previous review, updated for the nested field.
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'validateNetworking|NetworkObservability|InstallationPolicy' \
pkg/types/validation🤖 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 `@pkg/types/installconfig.go` around lines 820 - 827, Update
ValidateInstallConfig’s networking validation, using the nested
NetworkObservability.InstallationPolicy field, to accept only the
InstallAndEnable and NoAction constants before manifest generation; return a
validation error for any other value and add coverage for an invalid policy such
as “Bogus”.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@barbacbd This has been rebased, updated, and ready to be merged. Please review. The two other related PRs at openshift/api #2752 and openshift/cluster-network-operator#3087 are already merged. |
|
/retest |
|
/test e2e-aws-ovn |
sadasu
left a comment
sadasu
left a comment
There was a problem hiding this comment.
Looks good overall. I have a couple of Nits that will not hold merging.
| }, | ||
| expected: func() *types.InstallConfig { | ||
| c := defaultInstallConfig() | ||
| doNotInstall := types.NetworkObservabilityDoNotInstall |
There was a problem hiding this comment.
Nit: This can be define along with installationPolicy at line 24 and used here and at line 312.
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Who can /approve? |
| } | ||
| return c | ||
| }(), | ||
| }, |
There was a problem hiding this comment.
We need a test here that add a value "Test value" to installationPolicy to see what error message is reported.
…ues. If there is no `networkObservability` settings in install-config.yaml, then it shouldn't set anything in the Network config CR instead of setting it to InstallAndEnable. This is important since these two states have slightly different meanings. InstallAndEnable will enable on SNO, whereas the default (no setting) will not. Valid values for `installationPolicy` are "InstallAndEnable" and "NoAction".
|
/retest |
|
@OlivierCazade: The following test 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. |
|
/retest |
|
/test e2e-aws-ovn |
|
/lgtm |
|
/verified by @stleerh |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadasu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
verified
openshift-ci
Bot
added
the
approved
|
@sadasu: This PR has been marked as verified by 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. |
6 participants
networking: Add day-0 installation support for Network Observability
Summary
This PR implements day-0 installation support for Network Observability as described in the enhancement proposal.
Network Observability is enabled by default except for Single Node OpenShift (SNO). If you do not want Network Observability, add the following in the install-config.yaml file.
Please note that during Tech Preview, this feature is behind a feature gate called
NetworkObservabilityInstall. Therefore, this feature is not active until you first enable the feature gate.Summary by CodeRabbit
New Features
Bug Fixes