NETOBSERV-1110: Enable support for Flow RTT #394
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,19 @@ type FlowCollectorIPFIX struct { | |
| OVNKubernetes OVNKubernetesConfig `json:"ovnKubernetes,omitempty" mapstructure:"-"` | ||
| } | ||
|
|
||
| // Agent feature, can be one of:<br> | ||
| // - `PKT_DROP`, to track packet drops.<br> | ||
| // - `DNS_TRACKING`, to track specific information on DNS traffic.<br> | ||
| // - `FLOW_RTT`, to track L4 latency. <i>Unsupported (*)</i><br> | ||
|
There was a problem hiding this comment. pls remove There was a problem hiding this comment. @msherif1234 I guess you are talking about the HTML blocks right ? That's not new to this PR: https://issues.redhat.com/browse/NETOBSERV-1104 ; it's related to asciidocs
jotak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // +kubebuilder:validation:Enum:="PKT_DROP";"DNS_TRACKING";"FLOW_RTT" | ||
| type AgentFeature string | ||
|
|
||
| const ( | ||
| PktDrop AgentFeature = "PKT_DROP" | ||
| DNSTracking AgentFeature = "DNS_TRACKING" | ||
| FlowRTT AgentFeature = "FLOW_RTT" | ||
|
There was a problem hiding this comment. if I am not mistaken Strings need to use Pascal not snake case ? PktDrop AgentFeature = "PacketsDrop"
DNSTracking AgentFeature = "DnsTacking"
FlowRTT AgentFeature = "FlowRtt"There was a problem hiding this comment. You're right, idk why I had in mind the best practice was upper case |
||
| ) | ||
|
|
||
| // `FlowCollectorEBPF` defines a FlowCollector that uses eBPF to collect the flows information | ||
| type FlowCollectorEBPF struct { | ||
| // Important: Run "make generate" to regenerate code after modifying this file | ||
|
|
@@ -219,19 +232,17 @@ type FlowCollectorEBPF struct { | |
| // +optional | ||
| Debug DebugConfig `json:"debug,omitempty"` | ||
|
|
||
| // List of additional features to enable. They are all disabled by default. Enabling additional features may have performance impacts. Possible values are:<br> | ||
| // - `PKT_DROP`: enable the packets drop flows logging feature. This feature requires mounting | ||
| // the kernel debug filesystem, so the eBPF pod has to run as privileged. | ||
| // If the `spec.agent.eBPF.privileged` parameter is not set, an error is reported.<br> | ||
| // - `DNS_TRACKING`: enable the DNS tracking feature. This feature requires mounting | ||
| // the kernel debug filesystem hence the eBPF pod has to run as privileged. | ||
| // If the `spec.agent.eBPF.privileged` parameter is not set, an error is reported.<br> | ||
| // - `FLOW_RTT` <i>Unsupported (*)</i>: allows enabling flow latency (RTT) calculations in the eBPF agent during TCP handshakes. | ||
| // This feature needs both INGRESS and EGRESS direction flow capture and will be disabled if they are not both enabled.<br> | ||
| // +optional | ||
|
There was a problem hiding this comment. should we add or we need to allow Features: block with no features ? // +kubebuilder:validation:Required
// +kubebuilder:validation:MinItems:=1There was a problem hiding this comment. I don't think we need that check. We could simply initialize it empty [] There was a problem hiding this comment. yeah I don't even think an empty array initialization is needed, we don't do that in other places (e.g. exporters list) |
||
| Features []AgentFeature `json:"features,omitempty"` | ||
| } | ||
|
|
||
| // `FlowCollectorKafka` defines the desired Kafka config of FlowCollector | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,14 +155,9 @@ func (c *Reconciler) reconcileOpenshiftPermissions( | |
| } else { | ||
| scc.AllowedCapabilities = AllowedCapabilities | ||
| } | ||
| if helper.IsPktDropEnabled(desired) || helper.IsDNSTrackingEnabled(desired) { | ||
| scc.AllowHostDirVolumePlugin = true | ||
| } | ||
|
There was a problem hiding this comment. This was added to handle special case where u start with both features on then u just disable one since we don't compare current vs desired we could endup clearing the AllowHostDirVolumePlugin there was an issue about this by @memodi There was a problem hiding this comment. There was a problem hiding this comment. @msherif1234 There was a problem hiding this comment. @msherif1234 if you're talking about the nil-check, it's not relevant anymore now, as the new There was a problem hiding this comment. can u try the steps mentioned in the bug shared above to be sure, the reason I added the explicit false to make scc desired different than actual anyway I kind of forget the details its been awhile just make sure the steps showing in the above issue are fine There was a problem hiding this comment. Just tried to reproduce that old ticket, and it works fine: I get no error on the daemonset, pods are updated as expected, flows are flowing with dns/drops info |
||
| actual := &osv1.SecurityContextConstraints{} | ||
| if err := c.Get(ctx, client.ObjectKeyFromObject(scc), actual); err != nil { | ||
| if errors.IsNotFound(err) { | ||
|
|
||
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.
This is a slice u need to allocate the space then copy content
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.
done