Ginkgo: Added PolicyGeneration on Nightly tests by eloycoto · Pull Request #2258 · cilium/cilium

eloycoto · 2017-12-05T09:52:50Z

This issue covers a few test cases for issue #2029, not all yet, but I'll provide more test cases this week.

Summary:

Added a new policy generation tool under helpers/policygen folder.
Added exitCode on CMDRes
Added Kubectl.GetServiceHostPort method.
Added NightlyK8sPolicies test

Signed-off-by: Eloy Coto eloy.coto@gmail.com

houndci-bot · 2017-12-05T09:52:56Z

should not use dot imports

houndci-bot · 2017-12-05T09:52:56Z

should not use dot imports

houndci-bot · 2017-12-05T09:52:56Z

exported const HTTP should have comment (or a comment on this block) or be unexported

houndci-bot · 2017-12-05T09:52:56Z

exported var ConnTests should have comment or be unexported

aanm · 2017-12-05T16:56:13Z

Is this file a leftover?

If this file is added intentionally, it should be added to CODEOWNERS file, as this is the only change in this PR that is pulling in the requirement for Janitor's review.

If this file is here accidentally we may need to add it to .gitignore to avoid it popping up later.

Sorry, I was testing this in my local Jenkins and I did not remove it. My fault!

aanm · 2017-12-05T16:57:52Z

Why are we ignoring the error?

In this case, we only need if the exit was success or not. The error message is not essential.(A lot of them are supposed to fail)

Still, logging the errors in debug mode may be useful.

The main problem to log things that didn't work cause a lot of noise, and opens tasks like #2221

Have a lot of logs that says fail when it is supposed to fail creates a lot of confusion.

aanm · 2017-12-05T17:00:09Z

In this case, we only need if the exit was success or not. The error message is not essential.(A lot of them are supposed to fail)

Still, logging the errors in debug mode may be useful.

aanm · 2017-12-05T17:01:00Z

Magic numbers? Can you add a comment with the meaning of each number?

joestringer · 2017-12-05T23:34:01Z

I'm not sure why this is commented, but it would be good to either uncomment&fix, or open an issue and add a line above the comment that links to the GH issue so we can know why this commented code exists, and find out more.

houndci-bot · 2017-12-06T12:34:57Z

exported method GetConnectivyTest returns unexported type []policygen.connTestResultType, which can be annoying to use

houndci-bot · 2017-12-06T12:34:58Z

exported method GetConnectivyTest returns unexported type []policygen.connTestResultType, which can be annoying to use

nebril

Most of my comments are stylistic or nitpicks, but there are some bigger ones. Please ping me for any clarifications.

nebril · 2017-12-06T11:52:41Z

Still, logging the errors in debug mode may be useful.

nebril · 2017-12-06T11:52:44Z

Still, logging the errors in debug mode may be useful.

nebril · 2017-12-06T12:00:47Z

+	if err != nil {
+		return "", 0, err
+	}
+	return data.Spec.ClusterIP, int(data.Spec.Ports[0].Port), nil


This will panic if data.Spec.Ports is empty. Please add a check for it and return an error accordingly.

nebril · 2017-12-06T12:02:17Z

 // the command in the provided buffers. Returns false if the command failed
 // during its execution.
-func (s *SSHMeta) Execute(cmd string, stdout io.Writer, stderr io.Writer) bool {
+func (s *SSHMeta) Execute(cmd string, stdout io.Writer, stderr io.Writer) (bool, error) {


I think that at this point returning bool is redundant. Can you change it so only error is returned?

nebril · 2017-12-06T12:03:28Z

 func (s *SSHMeta) ExecWithSudo(cmd string, stdout io.Writer, stderr io.Writer) bool {
 	command := fmt.Sprintf("sudo %s", cmd)
-	return s.Execute(command, stdout, stderr)
+	result, _ := s.Execute(command, stdout, stderr)


Returning only error in all Exec* methods would make sure that we don't ignore any errors.

nebril · 2017-12-06T13:31:14Z

s/connectivy/connectivity

nebril · 2017-12-06T13:31:32Z

s/connectivy/connectivity

nebril · 2017-12-06T13:34:05Z

Please change name of the variable to expectedTestResult or sth like that.

nebril · 2017-12-06T13:34:20Z

I don't get why do we need these two for loops. As I understand, DetermineStatus returns what result is expected from give testType? What does it matter which rules are matched first?

Easy, this is an example rule:

IngressRule on pod dest:
-- allow PORT 8080 --> ResultTimeout

EgressRule on src pod:
-- only access to /private/ url to port 80:

Testcase:

If src pod made an http request to http://destPod:80/private --> ResultTimeout Correct. (The rule that match is the IngressRule)

If src pod made an http request to http://destPod:80/public --> ResultAuth --> The expected was ResultTimeout

So, we need to check the egress before the ingress part to determine the status of the expected result

nebril · 2017-12-06T13:34:29Z

s/matchs/match

ianvernon · 2017-12-11T20:36:41Z

The error should be propagated; change log message on line 75 to be kub.logger.WithError(err).Errorf(...

ianvernon · 2017-12-11T20:39:17Z

Can you change this function to return an error similar to Execute on line 81 above? Would make the functions more uniform in their implementation.

ianvernon · 2017-12-11T20:40:21Z

comamnd --> command.

ianvernon · 2017-12-11T20:42:37Z

log message is a bit verbose. what about "HTTPAction returned unexpected exit code %d" ?

ianvernon · 2017-12-11T20:45:27Z

Create a GH issue and ref it here with GH-XXXX so that we know this is tracked.

ianvernon · 2017-12-11T22:04:45Z

Will the result always be a result timeout if the result was not successful?

ianvernon · 2017-12-11T22:09:38Z

fmt.Errorf("%s not implemented yet", t.Kind)

ianvernon · 2017-12-11T22:16:22Z

NetworkPolicyCreate --> CreateCiliumNetworkPolicy

ianvernon · 2017-12-11T22:17:39Z

ianvernon · 2017-12-11T22:18:35Z

Use consistency in comments esp. when referring to types. Change resutltType and resulttype --> ResultType

houndci-bot · 2017-12-13T13:01:01Z

package comment should be of the form "Package policygen ..."

houndci-bot · 2017-12-13T13:01:02Z

exported var ConnTests should have comment or be unexported

houndci-bot · 2017-12-13T13:01:02Z

package comment should be of the form "Package policygen ..."

houndci-bot · 2017-12-13T13:01:02Z

package comment should be of the form "Package policygen ..."

houndci-bot · 2017-12-13T13:01:02Z

exported const KubectlCmd should have comment (or a comment on this block) or be unexported

eloycoto · 2017-12-13T16:37:27Z

@ianvernon @nebril this PR should be ok, all the comments are now fixed.

Regards

ianvernon

Comments are fixed well-enough that I think this can be merged

eloycoto · 2017-12-14T07:31:27Z

@nebril any update on this please?

eloycoto · 2017-12-14T08:55:00Z

test this please

ianvernon · 2017-12-15T18:26:53Z

@eloycoto please rebase and fix merge conflicts.

- Added a new policy generation tool under helpers/policygen folder. - Added exitCode on CMDRes - Added Kubectl.GetServiceHostPort method. - Added NightlyK8sPolicies test Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Fix an issue when l3 was ResultTimeout in ingress and Egress has a deny policy. Egress rules are the first match, so it should check first the egress part instead ingress Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Fix L7 isssues on egress+ingress cases

Return only the error instead of sucess, err Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

eloycoto added area/CI Continuous Integration testing issue or flake kind/enhancement This would improve or streamline existing functionality. pending-review labels Dec 5, 2017

eloycoto requested review from ianvernon, nebril and tgraf December 5, 2017 09:52

eloycoto requested review from a team as code owners December 5, 2017 09:52

houndci-bot reviewed Dec 5, 2017

View reviewed changes

tgraf mentioned this pull request Dec 5, 2017

Proposal: Nightly test plan #2029

Closed

43 tasks

aanm reviewed Dec 5, 2017

View reviewed changes

joestringer reviewed Dec 5, 2017

View reviewed changes

eloycoto force-pushed the policyGen branch from 3e75f13 to f0069fc Compare December 6, 2017 08:12

houndci-bot reviewed Dec 6, 2017

View reviewed changes

nebril requested changes Dec 6, 2017

View reviewed changes

ianvernon suggested changes Dec 11, 2017

View reviewed changes

houndci-bot reviewed Dec 13, 2017

View reviewed changes

eloycoto force-pushed the policyGen branch 2 times, most recently from 2a7858a to 84a782b Compare December 13, 2017 16:35

ianvernon approved these changes Dec 13, 2017

View reviewed changes

eloycoto force-pushed the policyGen branch from 84a782b to 052adc2 Compare December 14, 2017 07:29

eloycoto force-pushed the policyGen branch 2 times, most recently from 8ff10ec to c765137 Compare December 14, 2017 11:39

nebril approved these changes Dec 15, 2017

View reviewed changes

eloycoto force-pushed the policyGen branch from c765137 to 5fe31a2 Compare December 15, 2017 11:12

eloycoto removed the pending-review label Dec 15, 2017

eloycoto added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 15, 2017

ianvernon added the stable/needs-backport label Dec 15, 2017

eloycoto force-pushed the policyGen branch 2 times, most recently from e85f152 to 1cc2848 Compare December 18, 2017 10:23

eloycoto added 7 commits December 18, 2017 15:23

Ginkgo: Added PolicyGeneration on Nightly tests

661d16c

- Added a new policy generation tool under helpers/policygen folder. - Added exitCode on CMDRes - Added Kubectl.GetServiceHostPort method. - Added NightlyK8sPolicies test Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Fix issues with cilium_ds path

7f5aad3

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Ginkgo: Fix issues with ingress/egress rules

c1ee347

Fix an issue when l3 was ResultTimeout in ingress and Egress has a deny policy. Egress rules are the first match, so it should check first the egress part instead ingress Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Ginkgo: Fix L7 issue

9a31c4e

Fix L7 isssues on egress+ingress cases

Ginkgo: helpers.Node refactoring Execute method

cc89159

Return only the error instead of sucess, err Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Ginkgo - Fix PR review comments

70179df

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Ginkgo: PolicyGen fix typos

33471a9

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

eloycoto force-pushed the policyGen branch from 1cc2848 to 33471a9 Compare December 18, 2017 14:23

tgraf merged commit cd56a2b into cilium:master Dec 18, 2017

tgraf mentioned this pull request Jan 19, 2018

Validate, complete & test network policy model #2594

Closed

28 tasks

tgraf removed the stable/needs-backport label Jan 20, 2018

Conversation

eloycoto commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nebril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eloycoto Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eloycoto Dec 6, 2017 •

edited

Loading