NETOBSERV-1244: Add Kubernetes Infra transform rule #554
Conversation
|
@OlivierCazade: This pull request references NETOBSERV-1244 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 kubernetes/test-infra repository. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
- Coverage 66.12% 65.77% -0.35%
==========================================
Files 102 102
Lines 7409 7445 +36
==========================================
- Hits 4899 4897 -2
- Misses 2220 2257 +37
- Partials 290 291 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@OlivierCazade: This pull request references NETOBSERV-1244 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 kubernetes/test-infra repository. |
| srcKubeInfo, err := kubernetes.Data.GetInfo(fmt.Sprintf("%s", outputEntry["SrcAddr"])) | ||
| if err != nil { | ||
| logrus.WithError(err).Tracef("can't find kubernetes info for IP %v", outputEntry["SrcAddr"]) | ||
| continue | ||
| } | ||
| dstKubeInfo, err := kubernetes.Data.GetInfo(fmt.Sprintf("%s", outputEntry["DstAddr"])) | ||
| if err != nil { | ||
| logrus.WithError(err).Tracef("can't find kubernetes info for IP %v", outputEntry["DstAddr"]) | ||
| continue | ||
| } | ||
|
|
||
| if objectIsApp(*srcKubeInfo, rule.Parameters) || objectIsApp(*dstKubeInfo, rule.Parameters) { |
There was a problem hiding this comment.
We should move GetInfo calls into objectIsApp function.
Let's say srcKubeInfo is app and dstKubeInfo is not available; we will miss K8S_FlowType whereas it could be identified as app since we know the source.
Also, when both src and dst are not identified, should we consider these as app ?
There was a problem hiding this comment.
Moved the GetInfo in obectIsApp thanks.
For now, every flow that is not tagged as app is tagged as infra.
| srcKubeInfo, err := kubernetes.Data.GetInfo(fmt.Sprintf("%s", outputEntry["SrcAddr"])) | ||
| if err != nil { | ||
| logrus.WithError(err).Tracef("can't find kubernetes info for IP %v", outputEntry["SrcAddr"]) | ||
| continue | ||
| } | ||
| dstKubeInfo, err := kubernetes.Data.GetInfo(fmt.Sprintf("%s", outputEntry["DstAddr"])) | ||
| if err != nil { | ||
| logrus.WithError(err).Tracef("can't find kubernetes info for IP %v", outputEntry["DstAddr"]) |
There was a problem hiding this comment.
Is it OK that SrcAddr and DstAddr are hard-coded? Should these field names be configurable?
There was a problem hiding this comment.
I added dedicated structure to configure rules.
I did not change other rules because I don't want to introduce API breaking change in this PR but I will create a following PR with the changes in the other rules.
OlivierCazade
force-pushed
the
1244
branch
2 times, most recently
from
76a6ef3
to
3defc50
Compare
| if rule.KubernetesInfra == nil { | ||
| logrus.Error("transformation rule: Missing Kubernetes Infra configuration ") | ||
| return | ||
| } |
There was a problem hiding this comment.
Should we provide a default behavior here ?
There was a problem hiding this comment.
The case here happens if a user added a K8sinfra rule but did not configure it.
In this case, for now we ignore the rule and log an error.
Co-authored-by: Julien Pinsonneau <91894519+jpinsonneau@users.noreply.github.com>
|
New changes are detected. LGTM label has been removed. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade 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
Add Kubernetes Infra transform rule
Dependencies
Related operator PR:
netobserv/network-observability-operator#517
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.