NETOBSERV-1426: detect external workloads / openshift subnets #559
Conversation
|
@jotak: This pull request references NETOBSERV-1426 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #559 +/- ##
==========================================
- Coverage 66.95% 65.68% -1.28%
==========================================
Files 65 65
Lines 8116 8363 +247
==========================================
+ Hits 5434 5493 +59
- Misses 2334 2512 +178
- Partials 348 358 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This is ready for review. Here's an example:
# ...
processor:
subnetLabels:
openShiftAutoDetect: true
customLabels:
- cidrs: ["172.217.20.164/32"]
name: "Google"... and then running some To detect external traffic, we can now query with filter on Src/DstSubnetLabel="" |
controllers/flp/flp_controller.go
Outdated
| CIDRs: podCIDRs, | ||
| }) | ||
| } | ||
| svcCIDRs := network.Spec.ServiceNetwork |
There was a problem hiding this comment.
u need to loop over serviceNetwork cidrs similar to what u did for clusterNetwork
Adding CNO yaml for reference
oc get networks.config.openshift.io cluster -o yaml
apiVersion: config.openshift.io/v1
kind: Network
metadata:
creationTimestamp: "2024-02-20T13:08:01Z"
generation: 2
name: cluster
resourceVersion: "5182"
uid: 336cbbd1-dfb4-4c0a-8bee-017777a48093
spec:
clusterNetwork:
- cidr: 10.128.0.0/14
hostPrefix: 23
externalIP:
policy: {}
networkType: OVNKubernetes
serviceNetwork:
- 172.30.0.0/16
status:
clusterNetwork:
- cidr: 10.128.0.0/14
hostPrefix: 23
clusterNetworkMTU: 1360
networkType: OVNKubernetes
serviceNetwork:
- 172.30.0.0/16
There was a problem hiding this comment.
unlike clusterNetwork, serviceNetwork is already a []string so I take it as is
There was a problem hiding this comment.
it seems there is a part for external service in https://docs.openshift.com/container-platform/4.14/rest_api/config_apis/network-config-openshift-io-v1.html#spec-externalip
There was a problem hiding this comment.
I haven't implemented it yet, asking some details to the SDN team bc it's in a different API object and I'm not sure what can be considered the source of truth
| // SubnetLabel allows to label subnets and IPs, such as to identify cluster-external workloads or web services. | ||
| type SubnetLabel struct { | ||
| // List of CIDRs, such as `["1.2.3.4/32"]`. | ||
| //+required |
There was a problem hiding this comment.
do u need CIDR api verification here ? this is an example
https://github.com/openshift/api/blob/master/network/v1/types.go#L34
There was a problem hiding this comment.
thanks yeah I'll look into it
There was a problem hiding this comment.
Actually this pattern only matches IPv4, and we allow IPv6 here. Adding regex for ipv6 is quite more complicated; I'd rather go with a validation webhook (there is a future task to implement that)
There was a problem hiding this comment.
this the check for the resource u read to pull cidrs from in CNO, I remember there were extension to verify IP CIDR that was under dev I will see if that is already there in such case that will be very light weight compared to verification webhook
There was a problem hiding this comment.
there is new cel check for CIDR pls check this slack thread
https://redhat-internal.slack.com/archives/C3VS0LV41/p1708517668688849
There was a problem hiding this comment.
as discussed on slack, using the CIDR check would break compatibility on older k8s/ocp. We need to wait that our last supported version has it
| // `openShiftAutoDetect` allows, when set to `true`, to detect automatically the machines, pods and services subnets based on the | ||
| // OpenShift install configuration and the Cluster Network Operator configuration. | ||
| //+optional | ||
| OpenShiftAutoDetect *bool `json:"openShiftAutoDetect,omitempty"` |
There was a problem hiding this comment.
is there a default ? false
There was a problem hiding this comment.
keeping nil as a default makes it more flexible, imagine if in the future we want to enable it by default, then we'll be able to tell "if it's nil => enabled" .. which will also work for folks upgrading from a previous version.
If we set a default "false", and we later change the default, people upgrading will still have their old default
controllers/flp/flp_controller.go
Outdated
| CIDRs: podCIDRs, | ||
| }) | ||
| } | ||
| svcCIDRs := network.Spec.ServiceNetwork |
There was a problem hiding this comment.
it seems there is a part for external service in https://docs.openshift.com/container-platform/4.14/rest_api/config_apis/network-config-openshift-io-v1.html#spec-externalip
|
@jpinsonneau I added the externalIP one like you suggested. According to the docs, it's generally only used in bare-metal clusters; but still doesn't hurt to add. |
There was a problem hiding this comment.
LGTM, thanks @jotak
Do you think that would be useful in CLI ? That could be part of the scripts to configure FLP properly
I guess yes, for instance an interesting use case for the CLI and this feature could be "show me all the external traffic" ie. traffic with empty Src/DstSubnetLabel ; that implies to do also in CLI the extraction of the ocp Network cidrs & reinjection in FLP config |
|
Created https://issues.redhat.com/browse/NETOBSERV-1578 for CLI |
|
/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:2f1a646 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-2f1a646Or 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-2f1a646
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
New changes are detected. LGTM label has been removed. |
github-actions
bot
removed
the
ok-to-test
|
(rebased without change) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #559 +/- ##
==========================================
- Coverage 66.95% 65.68% -1.28%
==========================================
Files 65 65
Lines 8262 8363 +101
==========================================
- Hits 5532 5493 -39
- Misses 2381 2512 +131
- Partials 349 358 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/label qe-approved |
|
@jotak: This pull request references NETOBSERV-1426 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 openshift-eng/jira-lifecycle-plugin repository. |
Configure columns & filters for subnet labels Fix reading machine network Document overlaps between customLabels and autoDetect Rebased & address feedback - rebased / bump FLP - read external ips config - read from config.Network rather than operator.Network, as it's considered the best source of truth
|
(rebased without change) |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak 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
New fields in FlowCollector API to enable detection of openshift subnets (hence be able to detect cluster-external IPs), and allow custom labelling based on IPs / subnets
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.