Skip to content
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

Antctl cnp anp support #1301

Merged
merged 3 commits into from
Oct 22, 2020
Merged

Antctl cnp anp support #1301

merged 3 commits into from
Oct 22, 2020

Conversation

GraysonWu
Copy link
Contributor

@GraysonWu GraysonWu commented Sep 25, 2020

Make antctl get netpol support ACNP and ANP. Fix issue #927.
After support ACNP and ANP, some name and namespace based functions no longer work. Here add a struct called NetworkPolicyQueryFilter which can be used to query NetworkPolicies and easy to extend in the future. The logic of the filter matching is that if a NetworkPolicy can match all provided attributes of a filter then they are matched.
Add type -T [np acnp anp] option to antctl get netpol.
Add more info into result of antctl get netpol like sourceType, Prioriy, TierPriority.

@vmwclabot
Copy link

@GraysonWu, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@antrea-bot
Copy link
Collaborator

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-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-commenter
Copy link

codecov-commenter commented Sep 25, 2020

Codecov Report

Merging #1301 into master will increase coverage by 10.15%.
The diff coverage is 58.82%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1301       +/-   ##
===========================================
+ Coverage   54.47%   64.63%   +10.15%     
===========================================
  Files         119      157       +38     
  Lines       11240    12627     +1387     
===========================================
+ Hits         6123     8161     +2038     
+ Misses       4526     3620      -906     
- Partials      591      846      +255     
Flag Coverage Δ
#integration-tests 44.80% <ø> (ø)
#kind-e2e-tests 50.66% <38.23%> (?)
#unit-tests 42.18% <68.75%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
pkg/antctl/antctl.go 100.00% <ø> (ø)
pkg/querier/querier.go 57.14% <ø> (+57.14%) ⬆️
pkg/support/dump.go 62.36% <0.00%> (ø)
.../agent/apiserver/handlers/networkpolicy/handler.go 56.52% <52.94%> (ø)
pkg/agent/controller/networkpolicy/cache.go 84.46% <66.66%> (+5.12%) ⬆️
...ntroller/networkpolicy/networkpolicy_controller.go 72.33% <66.66%> (+11.06%) ⬆️
pkg/agent/apiserver/handlers/ovsflows/handler.go 78.94% <100.00%> (+3.94%) ⬆️
pkg/apiserver/handlers/endpoint/handler.go 58.82% <0.00%> (-11.77%) ⬇️
pkg/agent/util/sysctl/sysctl_linux.go 0.00% <0.00%> (ø)
pkg/apis/networking/v1beta1/conversion.go 30.00% <0.00%> (ø)
... and 104 more

pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved
// getNetworkPolicy looks up and returns the cached NetworkPolicy.
// nil is returned if the specified NetworkPolicy is not found.
func (c *ruleCache) getNetworkPolicy(npName, npNamespace string) *v1beta1.NetworkPolicy {
func (c *ruleCache) getNetworkPolicy(npFilter v1beta1.NetworkPolicyQueryFilter) *v1beta1.NetworkPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we return the first netpol matched in this case rather than returning the list of all matches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two functions: getNetworkPolicy and getNetworkPolicies.
According to my understanding, previously, before supporting CNP and ANP, getNetworkPolicy is used to get a single NP with name and namespace. getNetworkPolicies is used to get a list of NP in one namespace or all namespace.
After supporting CNP and ANP, with NetworkPolicyQueryFilter introduced, I want to keep the functionality of these two functions. getNetworkPolicies still return a list of NP that match the filter. getNetworkPolicy should accept a NetworkPolicyQueryFilter with attributes that can locate only one NP and return this NP.
BTW I'm totally ok to remove function getNetworkPolicy since it can be covered by getNetworkPolicies now. If users want a single NP, they can pass a proper filter into getNetworkPolicies can get the first element of the returned list.

@GraysonWu GraysonWu force-pushed the antctl-cnp-anp branch 4 times, most recently from 5d5a68b to a7c300b Compare September 29, 2020 18:45
@GraysonWu GraysonWu force-pushed the antctl-cnp-anp branch 5 times, most recently from a133bb6 to 9a7eba6 Compare October 2, 2020 22:39
@GraysonWu GraysonWu force-pushed the antctl-cnp-anp branch 3 times, most recently from 3f2fc88 to fd4b1c6 Compare October 6, 2020 20:26
@codecov-io
Copy link

codecov-io commented Oct 6, 2020

Codecov Report

Merging #1301 into master will decrease coverage by 10.11%.
The diff coverage is 68.75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1301       +/-   ##
===========================================
- Coverage   64.40%   54.28%   -10.12%     
===========================================
  Files         159      120       -39     
  Lines       12681    11281     -1400     
===========================================
- Hits         8167     6124     -2043     
- Misses       3660     4563      +903     
+ Partials      854      594      -260     
Flag Coverage Δ
#integration-tests 44.87% <ø> (+0.03%) ⬆️
#kind-e2e-tests ?
#unit-tests 42.02% <68.75%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
pkg/antctl/antctl.go 100.00% <ø> (ø)
pkg/querier/querier.go 0.00% <ø> (-57.15%) ⬇️
pkg/agent/controller/networkpolicy/cache.go 79.81% <66.66%> (-5.18%) ⬇️
...ntroller/networkpolicy/networkpolicy_controller.go 57.77% <66.66%> (-9.26%) ⬇️
pkg/agent/apiserver/handlers/ovsflows/handler.go 75.00% <100.00%> (-3.95%) ⬇️
pkg/agent/agent_linux.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/agent/proxy/types/groupcounter.go 0.00% <0.00%> (-95.00%) ⬇️
pkg/controller/networkpolicy/tier.go 0.00% <0.00%> (-90.00%) ⬇️
pkg/apis/controlplane/register.go 0.00% <0.00%> (-85.72%) ⬇️
pkg/agent/proxy/types/types.go 0.00% <0.00%> (-84.62%) ⬇️
... and 104 more

pkg/agent/controller/networkpolicy/cache.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/cache.go Outdated Show resolved Hide resolved
pkg/antctl/antctl.go Outdated Show resolved Hide resolved
pkg/querier/querier.go Outdated Show resolved Hide resolved
pkg/querier/querier.go Outdated Show resolved Hide resolved
@GraysonWu GraysonWu force-pushed the antctl-cnp-anp branch 4 times, most recently from e0819c0 to c5c3c7a Compare October 15, 2020 21:19
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, some minor comments

pkg/antctl/antctl.go Outdated Show resolved Hide resolved
pkg/antctl/command_definition_test.go Outdated Show resolved Hide resolved
pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved
@GraysonWu GraysonWu force-pushed the antctl-cnp-anp branch 3 times, most recently from cdf8ef2 to b3119c6 Compare October 16, 2020 19:20
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two minor comments, otherwise LGTM

pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved
pkg/agent/apiserver/handlers/networkpolicy/handler.go Outdated Show resolved Hide resolved
GraysonWu and others added 3 commits October 21, 2020 12:46
After support CNP and ANP, some name and namespace based functions no longer work. Here add a struct called NetworkPolicyQueryFilter which can be used to query NetworkPolicies and easy to extend in future.
The logic of the filter match is that if a NetworkPolicy can match all provided attributes of a filter then they are matched.
Also add more info into result of antctl get netpol like sourceType, Prioriy, TierPriority.

codegen and fix UT

revert changes on this file, accidently submit it

change response while no resource are found fix e2e test

Add aliases to -r --reference option

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

Simplify the process of `func NewFilterFromURLQuery`

Co-authored-by: Dyanngg <[email protected]>
From pkg/apis/controlplane/v1beta1/types.go
To pkg/querier/querier.go
Change NetworkPolicyQueryFilter in map or argument from struct to pointer
Change -r reference to -T type
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Oct 22, 2020

/test-all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants