Add sort-by effectivePriority for antctl get networkpolicy #1530

Dyanngg · 2020-11-11T01:08:17Z

This PR aims to solve #1388

Adds an option --sort-by=effectivePriority for antctl get networkpolicy, which sorts the Antrea internal controlplane NetworkPolicy objects by the order in which they are evaluated.
antctl get networkpolicy displays tier priority and policy priority by default

antrea-bot · 2020-11-11T01:08:34Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-11-11T01:45:53Z

Codecov Report

Merging #1530 (f585845) into master (9d3d10b) will decrease coverage by 1.20%.
The diff coverage is 53.68%.

@@            Coverage Diff             @@
##           master    #1530      +/-   ##
==========================================
- Coverage   63.31%   62.11%   -1.21%     
==========================================
  Files         170      182      +12     
  Lines       14250    15420    +1170     
==========================================
+ Hits         9023     9578     +555     
- Misses       4292     4826     +534     
- Partials      935     1016      +81

Flag	Coverage Δ
e2e-tests	`26.18% <27.56%> (?)`
kind-e2e-tests	`52.23% <47.94%> (-3.16%)`	⬇️
unit-tests	`40.41% <14.24%> (-0.87%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
cmd/antrea-agent/options.go	`21.69% <0.00%> (+0.97%)`	⬆️
.../agent/apiserver/handlers/networkpolicy/handler.go	`58.33% <ø> (ø)`
...gent/controller/noderoute/node_route_controller.go	`46.06% <ø> (-0.41%)`	⬇️
.../agent/flowexporter/connections/conntrack_linux.go	`28.37% <0.00%> (-0.79%)`	⬇️
pkg/agent/stats/collector.go	`97.72% <ø> (ø)`
pkg/antctl/antctl.go	`100.00% <ø> (ø)`
pkg/antctl/transform/addressgroup/transform.go	`0.00% <0.00%> (ø)`
pkg/antctl/transform/controllerinfo/transform.go	`0.00% <ø> (ø)`
pkg/antctl/transform/networkpolicy/transform.go	`0.00% <0.00%> (ø)`
... and 71 more

pkg/agent/apiserver/handlers/networkpolicy/handler.go

antoninbas

maybe you can mention this command in the documentation as part of this PR, since it is a pretty small change in terms of LOCs? In antctl.md (and maybe in antrea-network-policy.md as well?).

pkg/agent/apiserver/handlers/networkpolicy/handler.go

pkg/antctl/command_definition_test.go

pkg/antctl/antctl.go

antoninbas · 2020-11-18T00:39:59Z

I think sorting functionality is typically done on the client-side instead of making the server do this work. This is the case for kubectl for example: kubernetes/kubernetes#80602 (comment)

One advantage of sorting client-side would be that we could support "sort by effective priority" when using antctl in controller-mode without having to introduce a new API. @tnqn what do you think?

tnqn · 2020-11-18T09:01:56Z

I think sorting functionality is typically done on the client-side instead of making the server do this work. This is the case for kubectl for example: kubernetes/kubernetes#80602 (comment)

One advantage of sorting client-side would be that we could support "sort by effective priority" when using antctl in controller-mode without having to introduce a new API. @tnqn what do you think?

I agree with @antoninbas, and the apiserver lib doesn't support --sort-by option so we don't have a way to specify the intention for antrea-controller.

Dyanngg · 2020-11-18T18:34:42Z

I think sorting functionality is typically done on the client-side instead of making the server do this work. This is the case for kubectl for example: kubernetes/kubernetes#80602 (comment)
One advantage of sorting client-side would be that we could support "sort by effective priority" when using antctl in controller-mode without having to introduce a new API. @tnqn what do you think?

I agree with @antoninbas, and the apiserver lib doesn't support --sort-by option so we don't have a way to specify the intention for antrea-controller.

So if we move this to the client side, does it mean we sort the NetworkPolicies by effective priority by default? since it does not take --sort-by option

antoninbas · 2020-11-18T18:53:20Z

I think sorting functionality is typically done on the client-side instead of making the server do this work. This is the case for kubectl for example: kubernetes/kubernetes#80602 (comment)
One advantage of sorting client-side would be that we could support "sort by effective priority" when using antctl in controller-mode without having to introduce a new API. @tnqn what do you think?

I agree with @antoninbas, and the apiserver lib doesn't support --sort-by option so we don't have a way to specify the intention for antrea-controller.

So if we move this to the client side, does it mean we sort the NetworkPolicies by effective priority by default? since it does not take --sort-by option

I believe the fact that we do not support flags for controller commands (resourceEndpoint) is more a current limitation of the framework. There is no technical reason not to support flags. Currently these commands all support --name and --namespace:
https://github.com/vmware-tanzu/antrea/blob/b1745fc8852578dae6e124cc48d3c3f607cdbce4/pkg/antctl/command_definition.go#L112

In theory all commands could support --sort-by (like kubectl does), but that would require implementing that functionality (using JSON paths). I believe it would be fine for now to extend the resourceEndpoint struct to support custom command-specific flags, and only add --sort-by for get netpol (with effectivePriority being the only supported value).

docs/antrea-network-policy.md

pkg/antctl/command_definition.go

pkg/antctl/transform/networkpolicy/transform.go

antoninbas

some minor comments, otherwise LGTM

thanks for making the change and implementing this client-side, we get "controller-mode" support for free :)

antoninbas · 2020-12-02T22:49:11Z

pkg/antctl/command_definition.go

@@ -127,6 +127,13 @@ func (e *resourceEndpoint) flags() []flagInfo {
 			usage:        "Filter the resource by namespace",
 		})
 	}
+	if e.groupVersionResource == &v1beta2.NetworkPolicyVersionResource {


in a future PR, we can consider extending this flag to other resources & other fields. Would you mind opening a Github issue for that, I think it would make a "good first issue" as well

Opened #1605

pkg/antctl/transform/networkpolicy/transform.go

antoninbas · 2020-12-02T23:24:29Z

pkg/agent/apiserver/handlers/networkpolicy/handler.go

+	sortBy := query.Get("sort-by")
+	if sortBy != "" && sortBy != sortByEffectivePriority {
+		return nil, fmt.Errorf("unsupported value for sort-by option. Current supported value is %s", sortByEffectivePriority)
+	}


@Dyanngg I am confused as to why the check is done on the server-side, and only for the agent?

You are totally right. Reverted that and added the check on the client-side

antoninbas

LGTM

Dyanngg · 2020-12-03T18:22:41Z

/test-all

* Add sort-by effectivePriority for antctl get networkpolicy * Address comments and move sort logic to the client side * Add value check for sort-by flag

Dyanngg requested review from abhiraut, antoninbas, tnqn and jianjuns November 11, 2020 01:08

vmwclabot added the cla-not-required label Nov 11, 2020

Dyanngg force-pushed the antctl-np-priority branch from 3079f01 to 3708ab2 Compare November 11, 2020 01:10

antoninbas added this to the Antrea v0.12.0 release milestone Nov 11, 2020

Dyanngg force-pushed the antctl-np-priority branch 3 times, most recently from d63205a to 1a02873 Compare November 11, 2020 07:25

tnqn reviewed Nov 16, 2020

View reviewed changes

pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved

pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the antctl-np-priority branch from 1a02873 to 8830b8d Compare November 16, 2020 21:17

Dyanngg requested a review from tnqn November 16, 2020 23:52

tnqn reviewed Nov 17, 2020

View reviewed changes

pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved

pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the antctl-np-priority branch from 8830b8d to 9849143 Compare November 17, 2020 18:42

antoninbas reviewed Nov 17, 2020

View reviewed changes

Dyanngg force-pushed the antctl-np-priority branch from 9849143 to 9357f8a Compare November 18, 2020 00:38

Dyanngg force-pushed the antctl-np-priority branch from 9357f8a to 01a0978 Compare November 18, 2020 18:49

Dyanngg force-pushed the antctl-np-priority branch 2 times, most recently from 543f1d5 to b809d06 Compare November 18, 2020 22:38

abhiraut reviewed Nov 24, 2020

View reviewed changes

docs/antrea-network-policy.md Outdated Show resolved Hide resolved

pkg/antctl/command_definition.go Outdated Show resolved Hide resolved

pkg/antctl/transform/networkpolicy/transform.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the antctl-np-priority branch from b809d06 to 0c15f32 Compare November 30, 2020 19:41

antoninbas reviewed Dec 2, 2020

View reviewed changes

Add sort-by effectivePriority for antctl get networkpolicy

f7e59da

Address comments and move sort logic to the client side

c02b813

Dyanngg force-pushed the antctl-np-priority branch from 0c15f32 to c02b813 Compare December 2, 2020 23:19

antoninbas reviewed Dec 2, 2020

View reviewed changes

Add value check for sort-by flag

f585845

antoninbas approved these changes Dec 2, 2020

View reviewed changes

Dyanngg mentioned this pull request Dec 3, 2020

Implement sort-by flag in more antctl resources for more fields #1605

Closed

Dyanngg requested review from tnqn and abhiraut December 3, 2020 05:23

abhiraut approved these changes Dec 3, 2020

View reviewed changes

Dyanngg merged commit 7d146ca into antrea-io:master Dec 4, 2020

Dyanngg deleted the antctl-np-priority branch December 4, 2020 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sort-by effectivePriority for antctl get networkpolicy #1530

Add sort-by effectivePriority for antctl get networkpolicy #1530

Dyanngg commented Nov 11, 2020

antrea-bot commented Nov 11, 2020

codecov-io commented Nov 11, 2020 •

edited

Loading

antoninbas left a comment

antoninbas commented Nov 18, 2020

tnqn commented Nov 18, 2020

Dyanngg commented Nov 18, 2020

antoninbas commented Nov 18, 2020 •

edited

Loading

antoninbas left a comment

antoninbas Dec 2, 2020

Dyanngg Dec 3, 2020

antoninbas Dec 2, 2020

Dyanngg Dec 2, 2020 •

edited

Loading

antoninbas left a comment

Dyanngg commented Dec 3, 2020

Add sort-by effectivePriority for antctl get networkpolicy #1530

Add sort-by effectivePriority for antctl get networkpolicy #1530

Conversation

Dyanngg commented Nov 11, 2020

antrea-bot commented Nov 11, 2020

codecov-io commented Nov 11, 2020 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Nov 18, 2020

tnqn commented Nov 18, 2020

Dyanngg commented Nov 18, 2020

antoninbas commented Nov 18, 2020 • edited Loading

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Dec 2, 2020

Choose a reason for hiding this comment

Dyanngg Dec 3, 2020

Choose a reason for hiding this comment

antoninbas Dec 2, 2020

Choose a reason for hiding this comment

Dyanngg Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Dyanngg commented Dec 3, 2020

codecov-io commented Nov 11, 2020 •

edited

Loading

antoninbas commented Nov 18, 2020 •

edited

Loading

Dyanngg Dec 2, 2020 •

edited

Loading