NETOBSERV-2486: Add Network Observability Operator support #8729
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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. |
WalkthroughAdds Network Observability: new cluster and host validation IDs and feature enum; registers a NetworkObservability feature; implements and wires a Network Observability operator with config, templates, manifest generation, validation hooks, tests, and OpenAPI/swagger enum updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
openshift-ci
bot
added
the
size/XL
openshift-ci
bot
added
the
api-review
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jpinsonneau 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 |
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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: 1
🤖 Fix all issues with AI agents
In
@internal/operators/networkobservability/templates/openshift/50_openshift-network-observability_ns.yaml:
- Around line 5-7: The manifest places finalizers under spec but Namespace
resources use metadata.finalizers; update the YAML so the existing "finalizers"
array (currently under "spec") is moved into "metadata.finalizers" (e.g., remove
the "spec:" block and add or append the "- kubernetes" entry under "metadata:"
-> "finalizers:"), ensuring the resource no longer contains an empty or invalid
spec and that metadata.finalizers contains the list.
🧹 Nitpick comments (2)
internal/operators/networkobservability/networkobservability_config.go (1)
41-44: Consider validating rather than silently normalizing invalid sampling values.The current implementation silently resets non-positive sampling values to 50. This might mask user configuration errors. Consider returning a validation error instead:
if config.Sampling <= 0 { return nil, fmt.Errorf("sampling rate must be positive, got %d", config.Sampling) }Alternatively, if silent normalization is intentional for backward compatibility or resilience, consider adding a log statement to make the correction visible.
🔧 Proposed validation approach
// Validate sampling rate if config.Sampling <= 0 { - config.Sampling = 50 + return nil, fmt.Errorf("sampling rate must be positive, got %d", config.Sampling) }internal/operators/networkobservability/networkobservability_templates.go (1)
13-19: Prefer a descriptive panic message.While panicking in
init()is appropriate for missing embedded assets (a compile-time error), the current panic propagates the raw error. A more descriptive message would aid debugging.♻️ Proposed enhancement
func init() { var err error templatesRoot, err = fs.Sub(templatesFS, "templates") if err != nil { - panic(err) + panic("failed to initialize templates filesystem: " + err.Error()) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (3)
vendor/github.com/openshift/assisted-service/models/cluster_validation_id.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/models/feature_support_level_id.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/models/host_validation_id.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (21)
internal/cluster/validation_id.gointernal/featuresupport/feature_support_level.gointernal/featuresupport/features_olm_operators.gointernal/host/validation_id.gointernal/operators/builder.gointernal/operators/networkobservability/networkobservability_config.gointernal/operators/networkobservability/networkobservability_manifests_test.gointernal/operators/networkobservability/networkobservability_operator.gointernal/operators/networkobservability/networkobservability_operator_test.gointernal/operators/networkobservability/networkobservability_suite_test.gointernal/operators/networkobservability/networkobservability_templates.gointernal/operators/networkobservability/templates/custom/flow_collector.yamlinternal/operators/networkobservability/templates/openshift/50_openshift-network-observability_ns.yamlinternal/operators/networkobservability/templates/openshift/50_openshift-network-observability_operator_group.yamlinternal/operators/networkobservability/templates/openshift/50_openshift-network-observability_subscription.yamlmodels/cluster_validation_id.gomodels/feature_support_level_id.gomodels/host_validation_id.gorestapi/embedded_spec.gosubsystem/operators_test.goswagger.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
subsystem/operators_test.gointernal/host/validation_id.gointernal/featuresupport/feature_support_level.gointernal/operators/networkobservability/networkobservability_config.gointernal/operators/networkobservability/networkobservability_suite_test.gomodels/host_validation_id.gointernal/operators/networkobservability/networkobservability_templates.gomodels/feature_support_level_id.gointernal/operators/networkobservability/templates/openshift/50_openshift-network-observability_operator_group.yamlmodels/cluster_validation_id.gointernal/operators/builder.gointernal/operators/networkobservability/templates/custom/flow_collector.yamlinternal/featuresupport/features_olm_operators.gointernal/cluster/validation_id.gointernal/operators/networkobservability/networkobservability_manifests_test.gointernal/operators/networkobservability/templates/openshift/50_openshift-network-observability_ns.yamlrestapi/embedded_spec.gointernal/operators/networkobservability/networkobservability_operator_test.gointernal/operators/networkobservability/templates/openshift/50_openshift-network-observability_subscription.yamlinternal/operators/networkobservability/networkobservability_operator.goswagger.yaml
🧬 Code graph analysis (9)
subsystem/operators_test.go (2)
internal/operators/networkobservability/networkobservability_operator.go (1)
Operator(27-34)internal/operators/networkobservability/networkobservability_config.go (1)
Name(9-9)
internal/host/validation_id.go (2)
models/host_validation_id.go (1)
HostValidationIDNetworkObservabilityRequirementsSatisfied(232-232)internal/cluster/validation_id.go (1)
AreNetworkObservabilityRequirementsSatisfied(61-61)
internal/featuresupport/feature_support_level.go (2)
models/feature_support_level_id.go (1)
FeatureSupportLevelIDNETWORKOBSERVABILITY(181-181)internal/featuresupport/features_olm_operators.go (1)
NetworkObservabilityFeature(1245-1245)
internal/operators/networkobservability/networkobservability_config.go (1)
internal/usage/manager.go (1)
Unmarshal(79-86)
models/host_validation_id.go (1)
internal/usage/manager.go (1)
Unmarshal(79-86)
internal/operators/builder.go (1)
internal/operators/networkobservability/networkobservability_operator.go (1)
NewNetworkObservabilityOperator(37-46)
internal/featuresupport/features_olm_operators.go (5)
internal/featuresupport/support_level_feature.go (2)
SupportLevelFeature(15-30)SupportLevelFilters(37-42)models/feature_support_level_id.go (2)
FeatureSupportLevelID(20-20)FeatureSupportLevelIDNETWORKOBSERVABILITY(181-181)internal/operators/networkobservability/networkobservability_config.go (2)
FullName(10-10)Name(9-9)models/support_level.go (1)
SupportLevelSupported(34-34)models/architecture_support_level_id.go (1)
ArchitectureSupportLevelID(20-20)
internal/cluster/validation_id.go (2)
models/cluster_validation_id.go (1)
ClusterValidationIDNetworkObservabilityRequirementsSatisfied(178-178)internal/host/validation_id.go (1)
AreNetworkObservabilityRequirementsSatisfied(79-79)
internal/operators/networkobservability/networkobservability_manifests_test.go (4)
internal/operators/networkobservability/networkobservability_operator.go (1)
NewNetworkObservabilityOperator(37-46)internal/operators/common/common.go (1)
GenerateManifests(42-87)internal/operators/networkobservability/networkobservability_config.go (6)
Namespace(11-11)SubscriptionName(12-12)SourceName(14-14)Source(13-13)GroupName(15-15)Name(9-9)models/monitored_operator.go (1)
MonitoredOperator(21-63)
🪛 YAMLlint (1.37.1)
internal/operators/networkobservability/templates/custom/flow_collector.yaml
[error] 2-2: syntax error: expected '', but found ''
(syntax)
🔇 Additional comments (35)
models/feature_support_level_id.go (1)
180-182: LGTM: Enum value addition follows pattern.The new
NETWORK_OBSERVABILITYfeature support level ID is correctly defined and integrated into the enum initialization. The changes align with the swagger model generation pattern used throughout the file.Also applies to: 198-198
models/host_validation_id.go (1)
231-233: LGTM: Validation ID addition follows pattern.The new
network-observability-requirements-satisfiedvalidation ID is correctly defined and integrated into the enum initialization. The naming convention and placement are consistent with other operator requirement validation IDs.Also applies to: 240-240
internal/operators/networkobservability/networkobservability_config.go (2)
8-16: LGTM: Operator metadata constants are well-defined.The constants follow standard patterns for OLM-based operators and align with OpenShift naming conventions.
18-24: LGTM: Config struct is well-structured.The configuration fields are clearly documented and use appropriate JSON tags. The struct design aligns with operator configuration patterns seen across the codebase.
subsystem/operators_test.go (1)
33-33: LGTM: Test updated to include network observability operator.The import and test assertion correctly integrate the network observability operator into the supported operators test, following the established pattern for other operators.
Also applies to: 93-93
internal/featuresupport/feature_support_level.go (1)
64-64: LGTM: Feature registration follows established pattern.The network observability feature is correctly registered in the OLM Operators features section. The registration follows the same pattern as other operator features and properly references the corresponding feature implementation.
internal/operators/networkobservability/templates/openshift/50_openshift-network-observability_operator_group.yaml (1)
1-6: LGTM!The OperatorGroup definition is correct. An empty spec is valid and appropriate for single-namespace operator deployment.
internal/operators/builder.go (2)
19-19: LGTM!Import follows alphabetical ordering and matches the established pattern.
93-93: LGTM!The operator registration follows the established pattern and is correctly positioned in the operator list.
internal/operators/networkobservability/templates/custom/flow_collector.yaml (2)
1-15: Template logic and resource structure are correct.The conditional rendering, eBPF configuration with templated sampling, and disabled Loki settings align with the PR objectives. The resource structure follows Kubernetes conventions.
6-6: The hardcoded "netobserv" namespace for FlowCollector is intentional.The test suite explicitly expects the FlowCollector metadata namespace to be "netobserv" (see
networkobservability_manifests_test.go:131). This is correct by design: while the operator itself (subscription and operator group) are deployed to{{ .Operator.Namespace }}, the FlowCollector resource must always be in the "netobserv" namespace where the network observability components run.internal/host/validation_id.go (2)
79-79: LGTM!The new validation ID constant is correctly defined and follows the established pattern.
150-151: LGTM!The validation ID is properly categorized under "operators" alongside other operator validation IDs.
internal/operators/networkobservability/networkobservability_suite_test.go (1)
10-13: LGTM!Standard Ginkgo test suite bootstrap is correctly implemented.
models/cluster_validation_id.go (1)
177-178: LGTM!The new validation ID constant follows the established naming pattern and is correctly integrated into the enum.
internal/cluster/validation_id.go (2)
61-61: LGTM!The validation ID constant is correctly defined and follows the established naming convention.
102-103: LGTM!The new validation ID is correctly categorized under "operators" alongside other operator requirement validations.
internal/operators/networkobservability/templates/openshift/50_openshift-network-observability_subscription.yaml (1)
7-7: No action needed. The "stable" channel is the correct and official channel for the netobserv-operator according to Red Hat OpenShift Network Observability documentation. Older channels have been deprecated and removed; subscriptions must use "stable".Likely an incorrect or invalid review comment.
swagger.yaml (3)
2735-2735: Addnetwork-observabilityto supported operators enumThe new operator name is consistent with existing naming (kebab-case, aligned with other OLM operators) and is a backward‑compatible addition to the
/v2/supported-operatorssurface.
4399-4399: AddNETWORK_OBSERVABILITYtofeature-support-level-idFeature ID naming matches established patterns (upper snake case, grouped with other operator-driven features) and keeps the feature support matrix extensible.
6973-6973: Add network observability requirement validation IDs (host & cluster)New
*-requirements-satisfiedentries for host and cluster validations are consistent with other operator-specific validations and are safe, additive enum extensions from an API perspective.Also applies to: 7362-7362
restapi/embedded_spec.go (1)
6274-6280: No issues found. The source specification (swagger.yaml) and generated file (embedded_spec.go) are consistent. The additions fornetwork-observabilityoperator,network-observability-requirements-satisfiedvalidation IDs, and the mirrored entries across hunks follow the expected structure.internal/operators/networkobservability/networkobservability_operator_test.go (1)
1-163: LGTM! Comprehensive test coverage.The test suite thoroughly validates all aspects of the Network Observability operator:
- Metadata and dependencies
- Cluster and host validation (both expect success)
- Preflight and host requirements (both expect zero resources)
- Manifest generation with correct keys
- Properties configuration (createFlowCollector, sampling)
- Feature support integration
All assertions are clear and align with the operator implementation.
internal/operators/networkobservability/networkobservability_manifests_test.go (4)
17-35: LGTM! Clean test setup and manifest validation.The test correctly verifies the three core OLM manifests (namespace, subscription, operator group) and appropriately handles the customManifest behavior when FlowCollector is not created.
37-92: LGTM! Thorough manifest content validation.The tests properly validate:
- YAML structural integrity through unmarshaling
- Namespace, subscription, and operator group metadata and spec fields against defined constants
The type assertions and field checks are appropriate for verifying manifest correctness.
94-166: LGTM! Comprehensive FlowCollector generation tests.The tests effectively validate:
- Conditional FlowCollector creation based on the
createFlowCollectorproperty- Custom and default sampling values (100 and 50 respectively)
- Loki configuration (always disabled per design)
- FlowCollector metadata (name="cluster", namespace="netobserv")
The test coverage ensures the conditional manifest generation works correctly.
168-198: LGTM! Good error resilience validation.The tests confirm that invalid or empty properties are handled gracefully:
- Manifest generation continues successfully
- Default configuration is used as fallback
This validates the error handling behavior in the operator's
GenerateManifestsmethod.internal/featuresupport/features_olm_operators.go (3)
12-12: LGTM! Import correctly added.The networkobservability import is properly placed in alphabetical order.
1244-1258: LGTM! Feature implementation follows established patterns.The NetworkObservabilityFeature correctly:
- Implements the SupportLevelFeature interface
- Uses networkobservability constants for name and feature ID
- Returns no incompatibilities (appropriate if operator works universally)
- Checks operator activation using the standard helper
The implementation is consistent with similar operators in this file.
Also applies to: 1263-1276
1259-1261: Add version check to getSupportLevel if Network Observability Operator has minimum OpenShift version requirements.The
getSupportLevelmethod unconditionally returnsSupportLevelSupportedwithout version validation. While other operators like Loki (4.17+) and MetalLB (4.11+) include version checks, Network Observability has no minimum version constant or validation in the operator implementation itself. Verify whether the operator truly supports all OpenShift versions or requires a minimum version; if a minimum is required, add a version check consistent with other operators.internal/operators/networkobservability/networkobservability_operator.go (5)
17-34: LGTM! Well-structured constants and operator metadata.The validation IDs, operator struct, and exported Operator variable are correctly defined:
- Consistent validation ID used for both cluster and host
- Standard operator struct with logger and templates
- Reasonable timeout (1 hour) for OLM installation
36-74: LGTM! Constructor and getters correctly implemented.The initialization and metadata methods are clean:
- Template loading with appropriate error handling for initialization
- No dependencies (standalone operator)
- Validation IDs properly exposed
All methods follow established patterns from other operators in the codebase.
76-118: LGTM! Solid validation and manifest generation logic.The implementation correctly handles:
- Validation methods return success (appropriate for operators with no special requirements)
- Robust config parsing with fallback to defaults on errors
- Graceful handling of missing operator in cluster
- Proper delegation to common manifest generator
The error handling pattern (log warnings, use defaults) ensures manifest generation succeeds even with invalid configuration.
120-154: LGTM! Well-documented properties and requirements.The implementation provides:
- Clear operator properties with descriptive text and sensible defaults
- Proper host requirements delegation with error logging
- Standard monitored operator integration
The property descriptions help users understand the configuration options.
156-184: LGTM! Clean resource requirements and metadata methods.The final methods are correctly implemented:
- Preflight requirements return zero resources (appropriate for this operator)
- Feature support ID correctly links to the feature definition
- Bundle labels properly extracted from the Operator metadata
All methods follow the established operator interface patterns.
...perators/networkobservability/templates/openshift/50_openshift-network-observability_ns.yaml
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8729 +/- ##
==========================================
+ Coverage 43.52% 43.59% +0.07%
==========================================
Files 411 414 +3
Lines 71306 71432 +126
==========================================
+ Hits 31034 31144 +110
- Misses 37509 37522 +13
- Partials 2763 2766 +3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@internal/operators/networkobservability/networkobservability_manifests_test.go:
- Around line 199-213: The test comment in the It block ("Handles invalid
sampling values gracefully") is misleading; update the comment to state that
invalid sampling triggers error handling rather than a supported
fallback—mention that ParseProperties rejects sampling <= 0, GenerateManifests
logs a warning and then uses safe defaults (createFlowCollector: false). Replace
the existing comment with something like: "Invalid sampling values trigger error
handling; the operator logs a warning and uses safe defaults
(createFlowCollector: false)." Ensure references to ParseProperties and
GenerateManifests remain clear so future readers know this is recovery behavior,
not a feature.
🧹 Nitpick comments (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (1)
47-92: Consider using comma-ok idiom for type assertions.The type assertions (e.g., lines 56, 69, 73, 89) will panic if the YAML structure doesn't match expectations. While this provides fail-fast behavior in tests, it produces unclear error messages. Consider using the comma-ok idiom for more informative test failures:
metadata, ok := nsData["metadata"].(map[string]interface{}) Expect(ok).To(BeTrue(), "metadata should be a map")Alternatively, leverage Gomega's nested matchers for clearer assertions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
internal/operators/networkobservability/networkobservability_config.gointernal/operators/networkobservability/networkobservability_manifests_test.gointernal/operators/networkobservability/networkobservability_templates.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/operators/networkobservability/networkobservability_templates.go
- internal/operators/networkobservability/networkobservability_config.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/operators/networkobservability/networkobservability_manifests_test.go
🧬 Code graph analysis (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (3)
internal/operators/common/common.go (1)
GenerateManifests(42-87)internal/operators/networkobservability/networkobservability_config.go (6)
Namespace(11-11)SubscriptionName(12-12)SourceName(14-14)Source(13-13)GroupName(15-15)Name(9-9)models/monitored_operator.go (1)
MonitoredOperator(21-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-service-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-service-rhel9-acm-ds-main-on-pull-request
🔇 Additional comments (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (1)
1-215: LGTM! Comprehensive test coverage for manifest generation.The test suite provides thorough coverage of the Network Observability manifest generation functionality:
- ✅ Validates all required OpenShift manifests (namespace, subscription, operator group)
- ✅ Tests YAML validity and structure
- ✅ Covers conditional FlowCollector creation based on configuration
- ✅ Verifies default value handling
- ✅ Tests error resilience (invalid JSON, empty properties, invalid sampling)
The test organization using Ginkgo contexts makes the test cases easy to understand and maintain.
internal/operators/networkobservability/networkobservability_manifests_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@internal/operators/networkobservability/networkobservability_manifests_test.go:
- Around line 110-165: The test is unsafe when asserting YAML-parsed values and
types: yaml.Unmarshal can return numbers as float64/int64 so direct equality
checks like Expect(ebpf["sampling"]).To(Equal(100)) can fail and the chained map
assertions (flowCollectorData -> spec -> agent -> ebpf -> loki) will panic if a
key is missing or of the wrong type. Fix by doing type-safe extraction with type
switches or conversions for sampling (e.g., convert numeric to float64 or int
via a helper or use Gomega's BeNumerically matcher) and add explicit ok checks /
Expect(...).To(HaveKey(...)) or Expect(...).ToNot(BeNil()) before casting each
nested map (flowCollectorData, metadata, spec, agent, ebpf, loki) so the test
fails with clear messages instead of panicking; update the two sampling
assertions and all nested assertions referencing flowCollectorData, spec, agent,
ebpf, and loki.
- Around line 47-92: The tests currently use unchecked type assertions (e.g.,
metadata := nsData["metadata"].(map[string]interface{}), and similar for
subData, ogData) which will panic if the manifest shape is unexpected; update
each of these to use the two-value type assertion form (e.g., metadata, ok :=
nsData["metadata"].(map[string]interface{})) and add Expect(ok).To(BeTrue(),
"describe which manifest/key was missing or wrong") or equivalent Expect
statements so failures surface as clear test errors; apply this pattern for all
occurrences in the Namespace, Subscription, and OperatorGroup tests that access
metadata/spec (nsData, subData, ogData) after calling
operator.GenerateManifests(cluster).
🧹 Nitpick comments (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (1)
47-92: Consider extracting a helper for manifest validation.The three tests (namespace, subscription, operator group) follow nearly identical patterns: unmarshal YAML, type-assert metadata, and validate specific fields. A helper function could reduce duplication and make the tests more maintainable.
💡 Example helper pattern
func unmarshalAndGetMetadata(manifest []byte) (map[string]interface{}, error) { var data map[string]interface{} if err := yaml.Unmarshal(manifest, &data); err != nil { return nil, err } metadata, ok := data["metadata"].(map[string]interface{}) if !ok { return nil, fmt.Errorf("metadata is not a map") } return metadata, nil }Then use it in tests:
metadata, err := unmarshalAndGetMetadata(nsManifest) Expect(err).ToNot(HaveOccurred()) Expect(metadata["name"]).To(Equal(Namespace))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
internal/operators/networkobservability/networkobservability_manifests_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/operators/networkobservability/networkobservability_manifests_test.go
🧬 Code graph analysis (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (3)
internal/operators/networkobservability/networkobservability_operator.go (1)
NewNetworkObservabilityOperator(37-46)internal/operators/networkobservability/networkobservability_config.go (6)
Namespace(11-11)SubscriptionName(12-12)SourceName(14-14)Source(13-13)GroupName(15-15)Name(9-9)models/monitored_operator.go (1)
MonitoredOperator(21-63)
🔇 Additional comments (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (1)
168-215: LGTM! Config parsing tests are well-structured.These tests properly validate error handling for invalid JSON, empty properties, and invalid sampling values. The comment clarification for the invalid sampling test (lines 199-214) accurately reflects the error-recovery behavior rather than a feature.
internal/operators/networkobservability/networkobservability_manifests_test.go
Show resolved
Hide resolved
internal/operators/networkobservability/networkobservability_manifests_test.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (1)
127-130: Comment is misleading regarding multi-document parsing.The comment states "it's a multi-document YAML" but
yaml.Unmarshalonly parses the first YAML document. This works currently because there's only one custom template (FlowCollector), but would silently ignore additional custom manifests if added later.If multi-document support is intended, consider using
yaml.NewDecoderwithDecodein a loop, or update the comment to clarify single-document parsing is intentional.♻️ Alternative for proper multi-document support
// Parse the first document from the custom manifest decoder := yaml.NewDecoder(bytes.NewReader(customManifest)) var flowCollectorData map[string]interface{} err = decoder.Decode(&flowCollectorData)This requires adding
"bytes"to imports.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
internal/operators/networkobservability/networkobservability_manifests_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/operators/networkobservability/networkobservability_manifests_test.go
🧬 Code graph analysis (1)
internal/operators/networkobservability/networkobservability_manifests_test.go (4)
internal/operators/networkobservability/networkobservability_operator.go (1)
NewNetworkObservabilityOperator(37-46)internal/common/test_configuration.go (1)
GetTestLog(598-602)internal/operators/common/common.go (1)
GenerateManifests(42-87)internal/operators/networkobservability/networkobservability_config.go (6)
Namespace(11-11)SubscriptionName(12-12)SourceName(14-14)Source(13-13)GroupName(15-15)Name(9-9)
🔇 Additional comments (6)
internal/operators/networkobservability/networkobservability_manifests_test.go (6)
1-9: LGTM!Imports are appropriate and follow standard Ginkgo test conventions. The yaml.v3 import is correctly used for YAML parsing throughout the tests.
11-24: LGTM!Test setup properly initializes fresh instances in
BeforeEachfor test isolation. Usingcommon.GetTestLog()ensures clean test output.
26-45: LGTM!Good coverage of basic manifest generation. The
Or(BeEmpty(), MatchRegexp(...))pattern correctly handles the case wherecustomManifestcontains only YAML document separators when FlowCollector is disabled.
47-96: LGTM!Manifest content validation is thorough with proper type assertion safety checks. Using package constants (
Namespace,SubscriptionName,SourceName,Source,GroupName) ensures test assertions stay synchronized with the implementation.
168-211: LGTM!Good coverage of default value behavior. The test properly validates that the default sampling value (50) is applied when not specified in properties, and that
loki.enabledremainsfalse.
214-261: LGTM!Excellent edge case coverage for error scenarios. The test names and inline comments clearly document that invalid inputs trigger error handling with safe defaults, not graceful degradation as a feature. This properly tests the operator's resilience to malformed configuration.
|
@jpinsonneau: 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. |
2 participants
Add Network Observability Operator support
NETOBSERV-2486
openshift-assisted/assisted-installer-ui#3351
Summary
This PR adds full support for the Network Observability Operator in the Assisted Installer. The Network Observability Operator enables network flow monitoring and observability in OpenShift clusters using eBPF technology. This implementation includes operator installation, configuration management, manifest generation, validation, and feature support level integration.
Key Features
Operator Configuration
The operator supports the following configurable properties:
createFlowCollector(boolean, default:false)false, only the operator will be installedsampling(integer, default:50)FlowCollector Configuration
When
createFlowCollectoris enabled, the operator generates a FlowCollector resource with:netobservsamplingpropertyenabled: false)Testing
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist