Conversation
|
[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 |
|
I have a couple of issues that must be fixed: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1071 +/- ##
=======================================
Coverage 62.46% 62.46%
=======================================
Files 77 77
Lines 11581 11581
=======================================
Hits 7234 7234
Misses 3889 3889
Partials 458 458
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/ok-to-test |
openshift-ci
bot
added
the
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:c9e12b3 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-c9e12b3Or 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-c9e12b3
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
github-actions
bot
removed
the
ok-to-test
- bpfman: mention prereq on bpfman operator - filters: mention important information on default behaviour, how to change it, and sampling - tail filters: mention how they compare with bpf filters - a few docs style nits - rename Go field FlowFilterRules=>Rules to match the json name
skrthomas
left a comment
skrthomas
left a comment
There was a problem hiding this comment.
A few review comments for Flows Format reference. on the flows-format.adoc, one out of scope comment I notice is line 468, the specs don't have backticks, so they aren't formatting correctly:
| Type of record: 'flowLog' for regular flow logs, or 'newConnection', 'heartbeat', 'endConnection' for conversation tracking
Should be:
| Type of record: flowLog for regular flow logs, or newConnection, heartbeat, endConnection for conversation tracking
|
|
||
| - Use `Drop` to drop every flow considered as duplicates, allowing saving more on resource usage but potentially loosing some information such as the network interfaces used from peer, or network events. + | ||
|
|
||
| - Use `Sample` to randomly keep only 1 flow on 50 (by default) among the ones considered as duplicates. This is a compromise between dropping every duplicates or keeping every duplicates. This sampling action comes in addition to the Agent-based sampling. If both Agent and Processor sampling are 50, the combined sampling is 1:2500. + |
There was a problem hiding this comment.
| - Use `Sample` to randomly keep only 1 flow on 50 (by default) among the ones considered as duplicates. This is a compromise between dropping every duplicates or keeping every duplicates. This sampling action comes in addition to the Agent-based sampling. If both Agent and Processor sampling are 50, the combined sampling is 1:2500. + | |
| - Use `Sample` to randomly keep only 1 flow on 50, which is the default, among the ones considered as duplicates. This is a compromise between dropping every duplicates or keeping every duplicates. This sampling action comes in addition to the Agent-based sampling. If both Agent and Processor sampling values are 50, the combined sampling is 1:2500. + |
| Description:: | ||
| + | ||
| -- | ||
| `FLPFilterSet` defines the desired configuration for FLP-based filtering satisfying all conditions |
There was a problem hiding this comment.
| `FLPFilterSet` defines the desired configuration for FLP-based filtering satisfying all conditions | |
| `FLPFilterSet` defines the desired configuration for FLP-based filtering satisfying all conditions. |
| Description:: | ||
| + | ||
| -- | ||
| `FLPSingleFilter` defines the desired configuration for a single FLP-based filter |
There was a problem hiding this comment.
| `FLPSingleFilter` defines the desired configuration for a single FLP-based filter | |
| `FLPSingleFilter` defines the desired configuration for a single FLP-based filter. |
|
|
||
| | `matchType` | ||
| | `string` | ||
| | Type of matching to apply |
There was a problem hiding this comment.
| | Type of matching to apply | |
| | Type of matching to apply. |
| | Network events, such as network policy actions, composed of nested fields: + | ||
| - Feature (such as "acl" for network policies) + | ||
| - Type (such as an "AdminNetworkPolicy") + | ||
| - Namespace (namespace where the event applies, if any) + | ||
| - Name (name of the resource that triggered the event) + | ||
| - Action (such as "allow" or "drop") + | ||
| - Direction (Ingress or Egress) |
There was a problem hiding this comment.
@jotak can we add a Note to this that Network Events is only available in 4.17 and 4.18 versions of OpenShift Container Platform?
docs/flows-format.adoc
Outdated
| | packet translation dst address | ||
| | `xlat_dst_address` | ||
| | no | ||
| | avoid | ||
| | n/a | ||
| | `XlatDstPort` | ||
| | number | ||
| | packet translation dst port | ||
| | `xlat_dst_port` | ||
| | no | ||
| | careful | ||
| | n/a | ||
| | `XlatSrcAddr` | ||
| | string | ||
| | packet translation src address | ||
| | `xlat_src_address` | ||
| | no | ||
| | avoid | ||
| | n/a | ||
| | `XlatSrcPort` | ||
| | number | ||
| | packet translation src port | ||
| | `xlat_src_port` | ||
| | no | ||
| | careful | ||
| | n/a |
There was a problem hiding this comment.
need to spell out "source" and "destination" in the description and begin with a capital P
| | List of User Defined Networks | ||
| | `udns` | ||
| | no |
There was a problem hiding this comment.
Same here, should we note that UDN is dev preview?
| It requires using the OVN-Kubernetes network plugin with the Observability feature. | ||
| IMPORTANT: This feature is available as a Developer Preview. + | ||
|
|
||
| - `PacketTranslation`: enable enriching flows with packet translation information, such as Service NAT. + |
There was a problem hiding this comment.
Commenting here but this comment is for line 218, I think we can remove Developer Preview for this and mention that its now Technology Preview instead.
|
New changes are detected. LGTM label has been removed. |
|
@skrthomas I'm not sure about mentioning tech/dev preview in the flow-format doc, as these texts also end up in the app |
* Regenerate doc * Add missing cardinality * Improve docs on newly added features - bpfman: mention prereq on bpfman operator - filters: mention important information on default behaviour, how to change it, and sampling - tail filters: mention how they compare with bpf filters - a few docs style nits - rename Go field FlowFilterRules=>Rules to match the json name * doc feedback * doc feedback
* Regenerate doc (#1071) * Regenerate doc * Add missing cardinality * Improve docs on newly added features - bpfman: mention prereq on bpfman operator - filters: mention important information on default behaviour, how to change it, and sampling - tail filters: mention how they compare with bpf filters - a few docs style nits - rename Go field FlowFilterRules=>Rules to match the json name * doc feedback * doc feedback * Update links for 1.8
4 participants
cc @skrthomas