NETOBSERV-1377 Add controller to deploy netobserv network policy #690
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
OlivierCazade
force-pushed
the
network-policy
branch
2 times, most recently
from
bbc161d
to
ea0cfec
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
- Coverage 66.60% 66.30% -0.31%
==========================================
Files 70 73 +3
Lines 8115 8312 +197
==========================================
+ Hits 5405 5511 +106
- Misses 2315 2401 +86
- Partials 395 400 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
OlivierCazade
force-pushed
the
network-policy
branch
from
ea0cfec
to
1ce7586
Compare
| NetworkPolicy NetworkPolycy `json:"networkPolicy,omitempty"` | ||
| } | ||
|
|
||
| type NetworkPolycy struct { |
| } | ||
|
|
||
| type NetworkPolycy struct { | ||
| // Set `deploy` to `false` to disable network policy deployment. It is enabled by default. |
There was a problem hiding this comment.
We could add more information about what this policy does, such as:
This network policy better isolates the NetObserv components to prevent undesired connections to them. It is recommended to install it.
(or something in that vein)
Also, shouldn't we name that Enable, like we do generally for anything that can be turned on/off ?
| // +optional | ||
| Deploy *bool `json:"deploy,omitempty"` | ||
|
|
||
| // `additionalNamespaces` contains the interface names from where flows are collected. If empty, the agent |
There was a problem hiding this comment.
hmm I guess it's not the good text here :-)
There was a problem hiding this comment.
We should mention here in particular what users need to care about:
- If they use Kafka and installed it in a separate namespace, they must add it here
- Same for any exporter
We can expect users misconfigurations here... Probably something that we'll have to worry about in docs & troubleshooting
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&flowslatest.FlowCollector{}, reconcilers.IgnoreStatusChange). | ||
| Named("networkPolicy"). | ||
| Owns(&corev1.Namespace{}). |
There was a problem hiding this comment.
I don't think Namespace is required here?
| client.Client | ||
| mgr *manager.Manager | ||
| status status.Instance | ||
| currentNamespace string |
There was a problem hiding this comment.
looking at the code it doesn't seem that currentNamespace is useful here
| Advanced: &flowslatest.AdvancedProcessorConfig{ | ||
| Env: map[string]string{ | ||
| "GOGC": "200", | ||
| }, | ||
| ConversationHeartbeatInterval: &metav1.Duration{ | ||
| Duration: conntrackHeartbeatInterval, | ||
| }, | ||
| ConversationEndTimeout: &metav1.Duration{ | ||
| Duration: conntrackEndTimeout, | ||
| }, | ||
| ConversationTerminatingTimeout: &metav1.Duration{ | ||
| Duration: conntrackTerminatingTimeout, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I don't think conn-track settings has any use here, right? I guess you can remove them
OlivierCazade
force-pushed
the
network-policy
branch
from
1ce7586
to
f8d5c37
Compare
OlivierCazade
force-pushed
the
network-policy
branch
from
f8d5c37
to
73bf6b9
Compare
Description
Add controller to deploy netobserv network policy
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.