-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NETOBSERV-1244: Added flow type transformation rule in FLP configuration #517
Conversation
@OlivierCazade: This pull request references NETOBSERV-1244 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.16.0" version, but no target version was set. In 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 kubernetes/test-infra repository. |
@@ -72,6 +72,9 @@ func (b *PipelineBuilder) AddProcessorStages() error { | |||
Type: api.AddKubernetesRuleType, | |||
}, { | |||
Type: api.ReinterpretDirectionRuleType, | |||
}, { | |||
Type: "add_kubernetes_infra", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO once FLP PR is merged: update FLP dependency and change this string using FLP API
/hold |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #517 +/- ##
==========================================
- Coverage 57.83% 57.65% -0.18%
==========================================
Files 70 70
Lines 9446 9458 +12
==========================================
- Hits 5463 5453 -10
- Misses 3653 3668 +15
- Partials 330 337 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
api/v1beta1/flowcollector_types.go
Outdated
@@ -664,7 +664,7 @@ type FlowCollectorConsolePlugin struct { | |||
// `portNaming` defines the configuration of the port-to-service name translation | |||
PortNaming ConsolePluginPortConfig `json:"portNaming,omitempty"` | |||
|
|||
//+kubebuilder:default:={{name:"Applications",filter:{"src_namespace!":"openshift-,netobserv","dst_namespace!":"openshift-,netobserv"},default:true},{name:"Infrastructure",filter:{"src_namespace":"openshift-,netobserv","dst_namespace":"openshift-,netobserv"}},{name:"Pods network",filter:{"src_kind":"Pod","dst_kind":"Pod"},default:true},{name:"Services network",filter:{"dst_kind":"Service"}}} | |||
//+kubebuilder:default:={{name:"Applications",filter:{"flow_type":"app"},default:true},{name:"Infrastructure",filter:{"flow_type":"infra"}},{name:"Pods network",filter:{"src_kind":"Pod","dst_kind":"Pod"},default:true},{name:"Services network",filter:{"dst_kind":"Service"}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 love it ! It will simplify the display a lot !
@@ -703,6 +709,11 @@ filters: | |||
component: autocomplete | |||
placeholder: 'E.g: Ingress, Egress, Inner' | |||
hint: Specify the direction of the Flow observed at the Node observation point. | |||
- id: flow_type | |||
name: Flow type | |||
component: autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use autocomplete
component you will need to implement getOptions
on plugin side:
https://github.com/netobserv/network-observability-console-plugin/blob/be4ea4532962056f32080cd29bc99ce1fe77ca89/web/src/utils/filter-definitions.ts#L253-L297
We could even improve that to something more generic for Namespaces
, Names
, Kinds
and this new FlowType
since all of these rely on autoCompleteCache.
That would require a new route, sending field name as argument and refactor resources.go accordingly. WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to text. I think this is enough since the placeholder already hint the user that this is only infra or app.
And I agree that as a follow up to the generic column, we could have a task to auto complete for generic column.
/ok-to-test |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:36aae92 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-36aae92 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-36aae92
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
7087f9c
to
6dff981
Compare
Updated operator to use this PR: Until it is merged, to build this branch it is required to use go replace to point at the FLP branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, thanks @OlivierCazade !
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:ed7e2e5 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-ed7e2e5 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-ed7e2e5
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
I'm also glad to see these awful infra/app filters looking so much better now :-) One thing that is not a blocker but I think I can do that quickly as a follow-up, if you all agree, is to find another name than "flow type" ... there's so many ways to interpret "type", it's too overloaded and could mean totally different things in different context. I think "layer" is quite common to refer to things like infrastructure / applications, wdyt? |
Changes looks good, flow layer will be better naming indeed 😺 |
Thinking about the upgrade process, users will still have their old quick filters. We might have to add in the release note that they could do the change manually in their config (although the old filters will continue to work) |
As discussed in scrum today, this will be tested post merge. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Added flow type transformation rule in FLP configuration
Dependencies
Related FLP PR:
netobserv/flowlogs-pipeline#554
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.