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

Add L7 Antrea native NetworkPolicy support in controller #4406

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

qiyueyao
Copy link
Contributor

@qiyueyao qiyueyao commented Nov 23, 2022

This PR add support for L7 Antrea native NetworkPolicy in the controller side.

  • Added support for passing L7Protocols to agent when processing ClusterNetworkPolicy and AntreaNetworkPolicy.
  • Added validation for Antrea native policy that checks L7Protocols is only used with Allow action, not used with toServices. When HTTP is set, then L4Protocol is only TCP or unset, and not with IGMP or ICMP.
  • Added UT.

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #4406 (c5f2c22) into main (19a7de0) will increase coverage by 0.37%.
The diff coverage is 42.59%.

❗ Current head c5f2c22 differs from pull request most recent head cb81da6. Consider uploading reports for the commit cb81da6 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4406      +/-   ##
==========================================
+ Coverage   63.56%   63.93%   +0.37%     
==========================================
  Files         400      400              
  Lines       56849    57086     +237     
==========================================
+ Hits        36137    36500     +363     
+ Misses      18035    17880     -155     
- Partials     2677     2706      +29     
Flag Coverage Δ *Carryforward flag
e2e-tests 38.46% <23.25%> (?)
integration-tests 34.62% <ø> (ø) Carriedforward from 19a7de0
kind-e2e-tests 40.18% <6.38%> (ø) Carriedforward from 19a7de0
unit-tests 49.78% <40.42%> (ø) Carriedforward from 19a7de0

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/networkpolicy/validate.go 45.75% <25.00%> (-4.71%) ⬇️
...kg/controller/networkpolicy/antreanetworkpolicy.go 89.14% <50.00%> (-1.77%) ⬇️
...g/controller/networkpolicy/clusternetworkpolicy.go 71.36% <100.00%> (-2.78%) ⬇️
pkg/controller/networkpolicy/crd_utils.go 77.25% <100.00%> (-16.05%) ⬇️
pkg/agent/openflow/pipeline.go 80.32% <0.00%> (+0.19%) ⬆️
...ntroller/networkpolicy/networkpolicy_controller.go 72.14% <0.00%> (+0.80%) ⬆️
... and 20 more

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, suggested one more validation.

pkg/controller/networkpolicy/validate.go Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
Comment on lines 778 to 779
if port.Protocol != nil && *port.Protocol != v1.ProtocolTCP {
return "layer 7 protocols can only be used when layer 4 protocol is TCP or unset", false
Copy link
Member

Choose a reason for hiding this comment

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

not all layer 7 protocols are based on TCP, though the only supported protocol http is. To avoid confusion and to be extensible, it should validate the l4 protocol is either unset or TCP if the l7 protocol is http, and the error message should say "HTTP protocol can only be used when layer 4 protocol is TCP or unset"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for protocols and ports. Added verification for toServices.

@tnqn
Copy link
Member

tnqn commented Nov 29, 2022

@qiyueyao #4380 has been merged, please rebase the PR when you update it, to make the diff clearer. The PR title could be updated too.

@qiyueyao qiyueyao changed the title Extend Antrea native policy Controller to Layer 7 Add L7 Antrea native NetworkPolicy support in controller Nov 30, 2022
@qiyueyao qiyueyao force-pushed the l7networkpolicy-controller branch 2 times, most recently from b0d6e2c to 69a0c9a Compare December 1, 2022 22:57
Add support for passing L7Protocols to agent when processing ACNP and ANP.
Add validation for Antrea native policy for L7Protocols (HTTP only) to be used with Ports/Protocols,
only supports Allow, and not used with toServices.
Add UT.

Signed-off-by: Qiyue Yao <[email protected]>
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 Dec 2, 2022

/test-all

@tnqn tnqn closed this Dec 2, 2022
@tnqn tnqn reopened this Dec 2, 2022
@vicky-liu vicky-liu added this to the Antrea v1.10 release milestone Dec 2, 2022
@tnqn tnqn merged commit fa332be into antrea-io:main Dec 2, 2022
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Jan 27, 2023
)

Add support for passing L7Protocols to agent when processing ACNP and ANP.
Add validation for Antrea native policy for L7Protocols (HTTP only) to be used with Ports/Protocols,
only supports Allow, and not used with toServices.
Add UT.

Signed-off-by: Qiyue Yao <[email protected]>
@qiyueyao qiyueyao deleted the l7networkpolicy-controller branch March 8, 2023 07:23
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
)

Add support for passing L7Protocols to agent when processing ACNP and ANP.
Add validation for Antrea native policy for L7Protocols (HTTP only) to be used with Ports/Protocols,
only supports Allow, and not used with toServices.
Add UT.

Signed-off-by: Qiyue Yao <[email protected]>
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.

3 participants