Skip to content

NETOBSERV-2053: change FLP filtering API#1260

Closed
jotak wants to merge 1 commit intonetobserv:mainfrom
jotak:update-filter-api
Closed

NETOBSERV-2053: change FLP filtering API#1260
jotak wants to merge 1 commit intonetobserv:mainfrom
jotak:update-filter-api

Conversation

@jotak
Copy link
Member

@jotak jotak commented Mar 18, 2025

Description

  • Use AllOf+AnyOf
  • More explicit documentation about what it does
  • Move OutputTarget one level above in parent, to avoid users having to duplicate it in every rule

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labeled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@jotak
NETOBSERV-2053: change FLP filtering API
e6e0ed4 
- Use AllOf+AnyOf
- More explicit documentation about what it does
- Move OutputTarget one level above in parent, to avoid users having to
  duplicate it in every rule
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Mar 18, 2025 •
edited by openshift-ci bot
Loading

@jotak: This pull request references NETOBSERV-2053 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 epic to target either version "4.19." or "openshift-4.19.", but it targets "netobserv-1.9" instead.

Details

In response to this:

Description

  • Use AllOf+AnyOf
  • More explicit documentation about what it does
  • Move OutputTarget one level above in parent, to avoid users having to duplicate it in every rule

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
  • If so, make sure the JIRA epic is labeled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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.

@codecov
Copy link

codecov bot commented Mar 18, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 22.78481% with 61 lines in your changes missing coverage. Please review.

Project coverage is 62.57%. Comparing base (cd93472) to head (e6e0ed4).
Report is 90 commits behind head on main.

Files with missing lines Patch % Lines
...s/flowcollector/v1beta1/zz_generated.conversion.go 31.57% 24 Missing and 2 partials ⚠️
...pis/flowcollector/v1beta1/zz_generated.deepcopy.go 0.00% 19 Missing ⚠️
...pis/flowcollector/v1beta2/zz_generated.deepcopy.go 15.78% 15 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1260      +/-   ##
==========================================
- Coverage   62.63%   62.57%   -0.07%     
==========================================
  Files          76       76              
  Lines       11525    11567      +42     
==========================================
+ Hits         7219     7238      +19     
- Misses       3842     3865      +23     
  Partials      464      464
Flag Coverage Δ
unittests 62.57% <22.78%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
apis/flowcollector/v1beta1/flowcollector_types.go 100.00% <ø> (ø)
apis/flowcollector/v1beta2/flowcollector_types.go 100.00% <ø> (ø)
controllers/flp/flp_pipeline_builder.go 75.23% <100.00%> (+0.46%) ⬆️
...pis/flowcollector/v1beta2/zz_generated.deepcopy.go 39.49% <15.78%> (-0.10%) ⬇️
...pis/flowcollector/v1beta1/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
...s/flowcollector/v1beta1/zz_generated.conversion.go 33.15% <31.57%> (+0.06%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

OlivierCazade
OlivierCazade reviewed Mar 21, 2025
View reviewed changes
Copy link
Member

@OlivierCazade OlivierCazade left a comment

Choose a reason for hiding this comment

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

The code is LGTM, just some concerns about upgrades.

apis/flowcollector/v1beta1/flowcollector_types.go
// `anyOf` contains a list of rules evaluated individually: any satisfied rule results in a match.
// +optional
AllOf []FLPSingleFilter `json:"allOf"`
AnyOf []FLPFilterAnyOf `json:"anyOf"`
Copy link
Member

@OlivierCazade OlivierCazade Mar 21, 2025

Choose a reason for hiding this comment

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

Isn´t this a breaking change ?
In 1.8.0 we have a Allof directly inside the Filter field.

Assuming someone already has some Filters configured, will an upgrade succeed ?

@Amoghrd
Copy link
Member

Amoghrd commented Mar 21, 2025

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 21, 2025
@github-actions
Copy link

github-actions bot commented Mar 21, 2025

New images:

  • quay.io/netobserv/network-observability-operator:6f5bc7e
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-6f5bc7e
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-6f5bc7e

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:6f5bc7e make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-6f5bc7e

Or as a Catalog Source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: netobserv-dev
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-6f5bc7e
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

Amoghrd
Amoghrd reviewed Mar 21, 2025
View reviewed changes
apis/flowcollector/v1beta1/flowcollector_types.go
// `filters` lets you define custom filters to limit the amount of generated flows.
// These filters provide more flexibility than the eBPF Agent filters (in `spec.agent.ebpf.flowFilter`), such as allowing to filter by Kubernetes namespace,
// but with a lesser improvement in performance.
// [Unsupported (*)].
Copy link
Member

@Amoghrd Amoghrd Mar 21, 2025

Choose a reason for hiding this comment

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

Can remove the Unsupported references since its going GA in 1.9

@Amoghrd
Copy link
Member

Amoghrd commented Apr 23, 2025

@jotak Is this PR not required anymore because of #1434 implementation?

@openshift-merge-robot
Copy link
Collaborator

openshift-merge-robot commented Apr 23, 2025

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@jotak
Copy link
Member Author

jotak commented Apr 24, 2025

@Amoghrd correct, closing it

@jotak jotak closed this Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OlivierCazade OlivierCazade OlivierCazade left review comments

@Amoghrd Amoghrd Amoghrd left review comments

@jpinsonneau jpinsonneau Awaiting requested review from jpinsonneau

Assignees

No one assigned

Labels

jira/valid-reference needs-rebase ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jotak @openshift-ci-robot @Amoghrd @openshift-merge-robot @OlivierCazade