CORENET-6714: Enable Network Observability on Day 0#2752
CORENET-6714: Enable Network Observability on Day 0#2752OlivierCazade wants to merge 3 commits intoopenshift:masterfrom
Conversation
Add InstallNetworkObservability field to NetworkSpec to support automatic installation of the Network Observability operator during cluster creation. When omitted or set to true, the operator and FlowCollector are deployed at bootstrap time, enabling immediate network visibility for troubleshooting packet drops, latencies, and DNS tracking. When explicitly set to false, no automatic installation occurs (the operator can still be installed manually post-installation). This treats network observability as integral networking infrastructure, making it available by default rather than as a separate optional component.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@OlivierCazade: This pull request references CORENET-6714 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. |
|
Hello @OlivierCazade! Some important instructions when contributing to openshift/api: |
Review Summary by QodoAdd InstallNetworkObservability field for day-0 network observability
WalkthroughsDescription• Add InstallNetworkObservability optional boolean field to NetworkSpec • Field enables network observability by default when omitted or true • Update generated code and CRD manifests for new field • Support day-0 automatic Network Observability operator deployment Diagramflowchart LR
NetworkSpec["NetworkSpec CRD"] -- "adds field" --> InstallNetworkObservability["InstallNetworkObservability: *bool"]
InstallNetworkObservability -- "enables by default" --> NetworkObservability["Network Observability Operator"]
NetworkObservability -- "deploys at bootstrap" --> FlowCollector["FlowCollector"]
File Changes1. config/v1/types_network.go
|
Code Review by Qodo
1. InstallNetworkObservability not feature-gated
|
openshift-ci
bot
added
the
size/S
📝 WalkthroughWalkthroughThe change adds a new optional field 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml (1)
115-119: Keep description in sync with Go type and apply same clarity improvement.The CRD schema correctly reflects the Go type. The description has the same ambiguity issue noted in the Go file—"it does nothing" should be clarified to "automatic installation is skipped."
Ensure this description stays synchronized with any documentation changes made to
config/v1/types_network.go.📝 Suggested description improvement
installNetworkObservability: description: |- - InstallNetworkObservability is an optional field that enables network observability - when omitted or set to true. If the field is set to false, it does nothing. + InstallNetworkObservability is an optional field that enables automatic installation + of network observability when omitted or set to true. If the field is set to false, + automatic installation is skipped (the operator may still be installed manually post-installation). + This field is immutable after installation. type: boolean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml` around lines 115 - 119, Update the CRD description for the installNetworkObservability field so it matches the Go type and the clearer wording used in config/v1/types_network.go: replace "it does nothing" with "automatic installation is skipped" and ensure the sentence indicates the field is optional and defaults to true when omitted; target the installNetworkObservability field description in the CRD and keep this text synchronized with the corresponding struct/comment in config/v1/types_network.go.config/v1/types_network.go (1)
90-94: Clarify documentation wording and consider adding immutability notice.Two suggestions for the field documentation:
The phrase "it does nothing" is ambiguous. Consider clarifying to: "If the field is set to false, automatic installation of network observability is skipped."
Based on the PR objective (Day 0 enablement), if this field is intended to be immutable after installation like other fields in
NetworkSpec(e.g.,ClusterNetwork,ServiceNetwork,NetworkType), the documentation should explicitly state this.📝 Suggested documentation improvement
- // InstallNetworkObservability is an optional field that enables network observability - // when omitted or set to true. If the field is set to false, it does nothing. + // InstallNetworkObservability is an optional field that enables automatic installation + // of network observability when omitted or set to true. If the field is set to false, + // automatic installation is skipped (the operator may still be installed manually post-installation). + // This field is immutable after installation. // // +optional InstallNetworkObservability *bool `json:"installNetworkObservability,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_network.go` around lines 90 - 94, Update the InstallNetworkObservability field comment to remove ambiguity and, if intended to be immutable after initial installation like other NetworkSpec fields, explicitly state that; specifically, change the sentence "If the field is set to false, it does nothing." to "If the field is set to false, automatic installation of network observability is skipped." and add a second sentence such as "This field is immutable after initial installation (Day 0) similarly to ClusterNetwork, ServiceNetwork, and NetworkType." Ensure the comment sits above the InstallNetworkObservability *bool field in NetworkSpec and references consistency with ClusterNetwork, ServiceNetwork, and NetworkType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/v1/types_network.go`:
- Around line 90-94: Update the InstallNetworkObservability field comment to
remove ambiguity and, if intended to be immutable after initial installation
like other NetworkSpec fields, explicitly state that; specifically, change the
sentence "If the field is set to false, it does nothing." to "If the field is
set to false, automatic installation of network observability is skipped." and
add a second sentence such as "This field is immutable after initial
installation (Day 0) similarly to ClusterNetwork, ServiceNetwork, and
NetworkType." Ensure the comment sits above the InstallNetworkObservability
*bool field in NetworkSpec and references consistency with ClusterNetwork,
ServiceNetwork, and NetworkType.
In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml`:
- Around line 115-119: Update the CRD description for the
installNetworkObservability field so it matches the Go type and the clearer
wording used in config/v1/types_network.go: replace "it does nothing" with
"automatic installation is skipped" and ensure the sentence indicates the field
is optional and defaults to true when omitted; target the
installNetworkObservability field description in the CRD and keep this text
synchronized with the corresponding struct/comment in
config/v1/types_network.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f66427af-5dad-4322-a130-036759f2a2b3
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (2)
config/v1/types_network.gopayload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
config/v1/types_network.go
Outdated
| // InstallNetworkObservability is an optional field that enables network observability | ||
| // when omitted or set to true. If the field is set to false, it does nothing. | ||
| // | ||
| // +optional | ||
| InstallNetworkObservability *bool `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
1. installnetworkobservability not feature-gated 📘 Rule violation ✓ Correctness
A new field was added to the stable config.openshift.io/v1 NetworkSpec without +openshift:enable:FeatureGate=.... This makes the field unconditionally available on a stable v1 API, violating the requirement to gate new stable fields for controlled rollout.
Agent Prompt
## Issue description
A new field (`InstallNetworkObservability`) was added to the stable `config.openshift.io/v1` `NetworkSpec` without being gated by `+openshift:enable:FeatureGate=...`, which violates the requirement that new fields on stable v1 APIs must be behind a FeatureGate.
## Issue Context
This field currently appears in the ungated CRD manifest (`AAA_ungated.yaml`), meaning it is exposed unconditionally.
## Fix Focus Areas
- config/v1/types_network.go[90-94]
- config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yaml[116-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "installNetworkObservability": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "InstallNetworkObservability is an optional field that enables network observability when omitted or set to true. If the field is set to false, it does nothing.", | ||
| Type: []string{"boolean"}, | ||
| Format: "", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
2. Openapi json out of sync 🐞 Bug ✓ Correctness
The PR adds installNetworkObservability to the generated OpenAPI Go definitions, but the checked-in openapi/openapi.json still lacks the field under NetworkSpec, so OpenAPI consumers won’t see it and hack/verify-openapi.sh would diff-fail.
Agent Prompt
### Issue description
The PR updates `openapi/generated_openapi/zz_generated.openapi.go` to include the new `installNetworkObservability` field, but `openapi/openapi.json` is not updated and still lacks the field under `NetworkSpec`. This breaks consistency and will fail `hack/verify-openapi.sh`, and any tooling consuming `openapi/openapi.json` will miss the field.
### Issue Context
`hack/verify-openapi.sh` regenerates OpenAPI artifacts and diffs them against the committed versions.
### Fix Focus Areas
- openapi/openapi.json[8986-9034]
- hack/update-openapi.sh[9-13]
- hack/verify-openapi.sh[5-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
openshift-ci
bot
added
size/M
|
@OlivierCazade: This pull request references CORENET-6714 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. |
|
@OlivierCazade: This pull request references CORENET-6714 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. |
|
@OlivierCazade: This pull request references CORENET-6714 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml (1)
116-123: Description casing doesn't match enum values, and wording is ambiguous.Two issues with the description:
The description uses lowercase "enable" and "disable" but the enum values are CamelCase "Enable" and "Disable". This could confuse users when setting the field.
The phrase "it does nothing" is ambiguous. Based on the PR objectives, when set to "Disable", automatic installation is skipped. Consider clarifying to "network observability is not automatically installed" or similar.
Minor inconsistency: The existing
networkDiagnostics.modefield (line 144) uses"Disabled"(past tense), while this field uses"Disable"(present tense). Consider aligning with the existing pattern for consistency.📝 Suggested description improvement
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". + installNetworkObservability is an optional field that controls automatic installation + of the Network Observability operator during cluster creation. + When omitted or set to "Enable", network observability is automatically installed. + When set to "Disable", automatic installation is skipped (manual installation remains possible). + Valid values are "", "Enable", "Disable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml` around lines 116 - 123, The description for the installNetworkObservability field is inconsistent with its enum and ambiguous; update the description for installNetworkObservability to use the same casing as the enum values ("Enable", "Disable"), clearly state the behavior for each value (e.g., when set to "Enable" or omitted it enables automatic installation, when set to "Disable" network observability is not automatically installed), and consider aligning wording with the existing networkDiagnostics.mode choice (e.g., use "Disabled" vs "Disable") for consistency across the CRD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml`:
- Around line 116-123: The description for the installNetworkObservability field
is inconsistent with its enum and ambiguous; update the description for
installNetworkObservability to use the same casing as the enum values ("Enable",
"Disable"), clearly state the behavior for each value (e.g., when set to
"Enable" or omitted it enables automatic installation, when set to "Disable"
network observability is not automatically installed), and consider aligning
wording with the existing networkDiagnostics.mode choice (e.g., use "Disabled"
vs "Disable") for consistency across the CRD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f88d5b2c-e676-465b-b05d-b0d40c189d2f
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (2)
config/v1/types_network.gopayload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/types_network.go
|
@OlivierCazade: all tests passed! 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. |
|
@OlivierCazade: This pull request references CORENET-6714 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. |
| // Valid values are "", "Enable", "Disable". | ||
| // +kubebuilder:validation:Enum:="";Enable;Disable | ||
| // +optional | ||
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
"Disable" is misleading, because it is not disabling Network Observability; it does nothing. In the Enhancement Proposal, it uses the term "not enable" to mean it doesn't enable Network Observability.
What about using the values "true", "false" and ""? "false" would imply that it would not install Network Observability.
3 participants
config/v1: add installNetworkObservability field for day-0 installation
Summary
This PR adds the
installNetworkObservabilityfield to the Network CRD (config.openshift.io/v1) to support automatic installation of the Network Observability operator during cluster creation.Changes
InstallNetworkObservabilitystring field toNetworkSpecBehavior
When omitted or set to Enable, the Network Observability operator and FlowCollector are deployed at bootstrap time, providing immediate network visibility for troubleshooting packet drops, latencies, and DNS tracking.
When explicitly set to Disable, no automatic installation occurs (though the operator can still be installed manually post-installation).
Related
/cc @stleerh