NETOBSERV-2545: Add quick filters for external traffic

[jotak]

Description

Now that external traffic detection starts getting quite accurate, adding a quick filter looks like a quick win:

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).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.47%. Comparing base (fa4e1a8) to head (5a730c7).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2271      +/-   ##
==========================================
- Coverage   73.55%   73.47%   -0.09%     
==========================================
  Files          88       88              
  Lines        9780     9780              
==========================================
- Hits         7194     7186       -8     
- Misses       2146     2149       +3     
- Partials      440      445       +5     

Flag	Coverage Δ
unittests	73.47% <ø> (-0.09%)	⬇️

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

Files with missing lines	Coverage Δ
api/flowcollector/v1beta2/flowcollector_types.go	100.00% <ø> (ø)

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jpinsonneau]

LGTM ! thanks !

[openshift-ci]

New changes are detected. LGTM label has been removed.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jpinsonneau. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• DOWNSTREAM_OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@jotak: This pull request references NETOBSERV-2545 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.

Details

In response to this:

Description

Now that external traffic detection starts getting quite accurate, adding a quick filter looks like a quick win:

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.

[openshift-ci-robot]

@jotak: This pull request references NETOBSERV-2545 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.

Details

In response to this:

Description

Now that external traffic detection starts getting quite accurate, adding a quick filter looks like a quick win:

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.

[openshift-ci-robot]

@jotak: This pull request references NETOBSERV-2545 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.

Details

In response to this:

Description

Now that external traffic detection starts getting quite accurate, adding a quick filter looks like a quick win:

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.

[Amoghrd]

/ok-to-test

[github-actions]

New images:

• quay.io/netobserv/network-observability-operator:c34a0f6
• quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-c34a0f6
• quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-c34a0f6

They will expire after two weeks.

To deploy this build:

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

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

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-sha-c34a0f6
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

[stleerh]

I'm trying to understand how this works and what flows I will see. Let's just take the "External ingress" case. The filter is:
src_subnet_label: '"",EXT:'

1. This assumes openShiftAutoDetect: true. It is the default, so it's likely the case, but this should be mentioned.
2. When does it set the label to "EXT:"? I don't see this documented anywhere.
3. If I define a custom label for an external CIDR, will it still work?
4. If I generate traffic outside of the cluster to a service, it creates two flows, one at the node level (external to router) and another at the service level (router to service). Is the first flow only considered "External ingress" or both?

[jotak]

@stleerh

I'm trying to understand how this works and what flows I will see. Let's just take the "External ingress" case. The filter is: src_subnet_label: '"",EXT:'

1. This assumes `openShiftAutoDetect: true`. It is the default, so it's likely the case, but this should be mentioned.

Right, good point. Also I haven't thought much about the non-openshift case, maybe I should make it openshift-only.

2. When does it set the label to "EXT:"?  I don't see this documented anywhere.

3. If I define a custom label for an external CIDR, will it still work?

I'd recommend to take this PR in the light of my blog here: netobserv/netobserv.github.io#29
Without custom labelling, these quick filters should work just by the "empty label" match, and "EXT" serves no purpose in that case.
But the idea with EXT is precisely for this to continue to work when users start creating custom labels. The goal is to make it a convention: if what you're labelling is for external traffic, then prefix it with EXT.
I don't think we have any way to enforce that rule, but we can push that as a convention.

You're right about the doc, I should add that in the subnet labels doc

4. If I generate traffic outside of the cluster to a service, it creates two flows, one at the node level (external to router) and another at the service level (router to service).  Is the first flow only considered "External ingress" or both?

yes, only the first

[stleerh]

Let's document the expectations and requirements, and provide clarity on what "External ingress" is (e.g. only the flow part of a conversation where the source is external). Perhaps, add a tooltip to point to an online documentation.

One way to remove the requirement that external CIDRs must begin with "EXT:" for this to work is to have a separate field for object type instead of overloading subnet label. It will need one for source and destination. The object type will have a fixed set of enum values like "pod", "service", "external", etc., similar to what openShiftAutoDetect does.

[jotak]

@stleerh

Let's document the expectations and requirements,

done

and provide clarity on what "External ingress" is (e.g. only the flow part of a conversation where the source is external). Perhaps, add a tooltip to point to an online documentation.

I'm not convinced extra doc is needed here, I don't feel there's anything unexpected in how it's done. Traffic between openshift-ingress and internal workloads IS cluster traffic, I don't think it's ambiguous.

One way to remove the requirement that external CIDRs must begin with "EXT:" for this to work is to have a separate field for object type instead of overloading subnet label. It will need one for source and destination. The object type will have a fixed set of enum values like "pod", "service", "external", etc., similar to what openShiftAutoDetect does.

yeah, we can improve that in a follow-up; but let's try to not create any breaking change, if users start to leverage the EXT prefix already. We could just make it so that when the object type is "external", the prefix is automatically added, but the end result is the same with the label

[Amoghrd]

/label qe-approved

[openshift-ci-robot]

@jotak: This pull request references NETOBSERV-2545 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.

Details

In response to this:

Description

Now that external traffic detection starts getting quite accurate, adding a quick filter looks like a quick win:

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.