-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NETOBSERV-1931: introduce flowcollector validation webhook #817
Conversation
@jotak: This pull request references NETOBSERV-1931 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.18.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. |
0a5e298
to
8bf3f51
Compare
Start by validating agent features
de07f2b
to
6a22acd
Compare
- Filters port validation - Warn on high logging veerbosity on critical components - FlowMetrics: warn on cardinality
6a22acd
to
59cfdab
Compare
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.
LGTM, 2 minor comments. This comments can be addressed later since this PR is blocking other work.
warnings = append(warnings, "The NetworkEvents feature is only supported with OpenShift") | ||
} | ||
if !fc.Spec.Agent.EBPF.Privileged { | ||
warnings = append(warnings, "The NetworkEvents feature requires eBPF Agent to run in privileged mode") |
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.
Should this be an error? We know it is not going to work.
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.
My rationale is that I'm setting warnings for things that won't work but won't break anything (e.g. a feature that will not be enabled), and errors for things that may break something or produce garbage / unexpected data (e.g. an invalid filter set on a metric will make the metric tell something completely different from the expectation)
} | ||
|
||
// MockOpenShiftVersion shouldn't be used except for testing | ||
func (c *Info) MockOpenShiftVersion(v string) { |
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.
Should this be in a test file ?
May be not as a method since this is for tests only.
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.
it manipulates non-exported data (e.g. apisMap
) so it has to be in the cluster package ; and it must be importable from other packages, so not in a *_test.go
file ; we can't do that, right?
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.
LGTM
From the user perspective, where does this warnings shows up in the console ?
It may be good to have these in the Status field 🤔
/approve |
thanks! |
[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
Start by validating agent features
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.