eval command support #423
eval command support #423
Conversation
pkg/netpol/eval/check_eval.go
Outdated
| // analyzeANPCapturedRes when an admin-network-policy captures a connection , its result may be Allow (final- allowed conn), | ||
| // or Deny (final - denied conn) or Pass (to be determined by netpol/ banp) | ||
| func analyzeANPCapturedRes(anpRes int) (allowedOrDenied, pass bool, err error) { |
There was a problem hiding this comment.
the function name and its description is confusing... isn't it about checking if the result means allow by the ANP?
There was a problem hiding this comment.
the function name and return arg names are still confusing.. maybe consider a separate type for capturing all 3 options (allow/deny/pass) of return value ? instead of splitting into two return args, where sometimes their result is not relevant...
There was a problem hiding this comment.
tried to rename the returned values same as the return values of its calling func and added some documentation;
is this better?
this func is a helper that returns same values as its calling func allowedXgressConnectionByAdminNetpols
it is used only to avoid duplicate in the calling func.
Co-authored-by: Adi Sosnovich <[email protected]>
| // anpPortContains returns if the given AdminNetworkPolicyPort selects the input connection | ||
| // | ||
| //gocyclo:ignore | ||
| func anpPortContains(rulePorts *[]apisv1a.AdminNetworkPolicyPort, protocol, port string, dst Peer) (bool, error) { |
There was a problem hiding this comment.
does this function have some code used also in func (np *NetworkPolicy) ruleConnsContain that can be captured in helper functions instead?
pkg/cli/evaluate.go
Outdated
| case parser.ReplicaSet: | ||
| err = pe.InsertObject(obj.ReplicaSet) | ||
| case parser.Deployment: | ||
| err = pe.InsertObject(obj.Deployment) | ||
| case parser.DaemonSet: | ||
| err = pe.InsertObject(obj.DaemonSet) | ||
| case parser.StatefulSet: | ||
| err = pe.InsertObject(obj.StatefulSet) | ||
| case parser.ReplicationController: | ||
| err = pe.InsertObject(obj.ReplicationController) | ||
| case parser.Job: | ||
| err = pe.InsertObject(obj.Job) | ||
| case parser.CronJob: | ||
| err = pe.InsertObject(obj.CronJob) | ||
| // ns kind |
There was a problem hiding this comment.
if eval only supports pods, why adding these resources?
If the dir path only contains workload manifests and not pods, how would one infer the pod name for this command?
maybe this should be a separate issue, to extend eval support for workloads, but in this PR can just focus on pods?
the tests automation can add the pods resources and adjust the input, as if those manifests would exist for testing.
There was a problem hiding this comment.
not sure understood last sentence;
my tests dir paths contain manifests with StatefulSet workloads (I used replica name as the input of the test (pod name);
The tests failed without adding these cases (my manifests were not inserted to the policy-engine because of the lack of these cases; );
should I revert these changes and create new tests with pods only in the manifests?
There was a problem hiding this comment.
for these tests to work, you can add test automation (for eval tests) that add pods resources as if they existed in the manifests.. the changes here better be removed, until eval actually supports such resources, because in the current eval usage workload resources are not supported, and a user cannot infer the pod name to be used in this command when providing only workload resources.
There was a problem hiding this comment.
modified and added tests:
-
command_test.go: sincecommand-lineneeds the "dir" as an argument to execute the cmd;
for those tests (which don't contain pods in the manifests), I've generated a "temp dir" which is a copy of the original dir + generated yamls for pods (for the given src + dst arguments) so the command runs on the "temporary dir" successfully.
generating pod yaml for a workload considers also its labels (which may be used in the netpols rules and are essential for computing the result); -
eval_test.gouses the internalevalfuncs, so it works successfully with the original dirs.
in this file we had tests from parsed resources (that tests the "eval" functionality) ;
and I have added eval tests from dir paths.
There was a problem hiding this comment.
ok, please open a separate issue to consider supporting eval with input workloads, not just pods, and also add a comment for this test functionality added here, that it can be removed when this eval extension is added.
There was a problem hiding this comment.
done, new issue #440 ; + comments were added in the relevant files
…ied on github to the "general" tmp dir
pkg/netpol/eval/eval_test.go
Outdated
| dir: "anp_banp_core_test", | ||
| sourceNs: "network-policy-conformance-gryffindor", | ||
| sourcePod: "harry-potter", | ||
| destPod: "cedric-diggory", | ||
| destNs: "network-policy-conformance-hufflepuff", | ||
| port: "8080", | ||
| evalResult: true, | ||
| }, |
There was a problem hiding this comment.
where are the pod names coming from?
the test dir manifests do not contain pod resources.
There was a problem hiding this comment.
yes, the names are of the relevant workloads in the manifests.
I mentioned that the input-dir does not contain pods so it will generate a pod matching the workload resource with the src + dst names.
e.g. the generated src pod takes the metadata of the workload with the sourcePod param name. (i.w. its name, namespace and labels)
There was a problem hiding this comment.
ok, so the test attribute should be srcWorkload or dstWorkload, and also better add some documentation explaining that for these tests eval will be called with pod names owned by these workloads (pods which are added by policy engine ).
pkg/cli/command_test.go
Outdated
| protocol string | ||
| port string | ||
| evalResult bool | ||
| generateManifests bool // indicates if the test dir does not contain pods - to be generated |
There was a problem hiding this comment.
rename generateManifests to generatePodManifests
|
|
||
| const ( | ||
| tmpPattern = "temp-*" | ||
| exeMode = 0o777 |
There was a problem hiding this comment.
i put 600 for file access and 700 for creating the dir ( 600 on all failed)
| // copyDir copies files of network-policies from origDir into tempDir | ||
| // and generates into the tempDir : Pod yaml files for given src and dst peers from their workload resources | ||
| // in the origDir | ||
| func copyDir(origDir, tempDir, srcName, srcNs, dstName, dstNs string) error { |
There was a problem hiding this comment.
consider renaming copyDir to something like copyAndExtendDir or copyDirAndAddPods
| return "", osErr | ||
| } | ||
| } | ||
| // making temp directory in the temp path |
There was a problem hiding this comment.
why is this required? isn't it sufficient to just use TmpDir ?
| // copyFile copies origFile to tempFile | ||
| func copyFile(origFile, tempFile string) error { | ||
| contents, err := os.ReadFile(origFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = os.WriteFile(tempFile, contents, exeMode) | ||
| return err | ||
| } |
There was a problem hiding this comment.
is there an os func that can be used instead of this entire copyFile function?
There was a problem hiding this comment.
I didn't met such func that I can use to copy files in a more efficient or "shorter" way
| // Splitting the YAML into multiple documents | ||
| docs := splitYamlDocs(fileContents) | ||
|
|
||
| // we are interested in Metadata of the workload only. | ||
| // Iterate through objects to find the matching one | ||
| for _, doc := range docs { | ||
| var obj WorkloadMetadata | ||
| if err := yaml.Unmarshal(doc, &obj); err != nil { | ||
| return nil, err | ||
| } | ||
| if obj.Metadata.Name == podName && (obj.Metadata.Namespace == podNs || |
There was a problem hiding this comment.
can use instead the already existing functions from manifests.GetResourceInfosFromDirPath() and
parser.ResourceInfoListToK8sObjectsList() ?
There was a problem hiding this comment.
we may use these funcs but I think that the written solution without using them is shorter and more efficient for my goal.
-
I need to copy the
yamlfiles to the temp dir (as it is the arg of the command); in this case those funcs may not help and i will have to walk the dir anyway. -
I don't need to convert all objects in the dir to k8s objects, I just want the specific workload resources of the src and dst (just 2 objects of many converted - I have nothing to do with k8s objects - they are not the args of the cmd;)
-
I can not send those funcs only the workload files (assuming we can indicate what files are those; and ignore copying these files);
- the file with the workload resource may also contain
Namespaceobject and I must have this yaml in the output; because Ns labels may be used as selectors in the policy rules.
#380 adding support to eval command
issue #443
evalpkg (for my convenience, i have separated the funcs ofcheck.gofile into 2 files, the new file ischeck_eval.go; all the related funcs to eval command (checking if specific conn is allowed) are thereeval_test.goevalcommand with tests with ANPs (added relevant support for new policies (anps+banp) + for pods that are actually replica of a workload object that is not defined asPodin the yamls (e.g.StatefulSet))