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 ACNP appliedto NodePort svc support #3997

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Conversation

GraysonWu
Copy link
Contributor

@GraysonWu GraysonWu commented Jul 13, 2022

Fixes #1580

With this PR users could use ACNP to control the external access of a NodePort Service.

  1. Add service field in appliedTo of ACNP.
  2. Add Service in AppliedToGroup and appliedToGroup with Service will span to all Nodes.
  3. Use groupIDs of a Service to do destination matching.
  4. Process ipBlock to ct_nw_src to do source matching.

Some API-wise restrictions:

  1. Only an ingress rule or a policy only has ingress rules can use service in its appliedTo.
  2. service field can't be used with other selectors in appliedTo.
  3. If a policy or an ingress rule is applied to Services, that only ipBlock can be used to select workloads.
  4. A policy or an ingress rule can't be applied to Services and other workloads at the same time.
    appliedTo like below is invalid:
appliedTo:
      - service:
           name: svc
           namespace: default
      - podSelector:
           matchLabels: 
               app: client

The reason for 3 & 4 is that we use service field to decide if a policy or ingress rule is a NodePort Service policy/rule. If it is a NodePort Service policy/rule, we can only use ct_nw_src, transformed from ipBlock, to do the source matching. The source matching logic is different from normal policy/rule and NodePort Service policy/rule.

Signed-off-by: wgrayson [email protected]

@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #3997 (c4034ad) into main (684dca3) will increase coverage by 2.63%.
The diff coverage is 62.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3997      +/-   ##
==========================================
+ Coverage   64.45%   67.08%   +2.63%     
==========================================
  Files         294      297       +3     
  Lines       43726    45846    +2120     
==========================================
+ Hits        28184    30758    +2574     
+ Misses      13252    12704     -548     
- Partials     2290     2384      +94     
Flag Coverage Δ *Carryforward flag
e2e-tests 40.54% <38.23%> (?)
integration-tests 35.84% <4.49%> (?) Carriedforward from bbf780c
kind-e2e-tests 50.50% <51.33%> (-0.14%) ⬇️
unit-tests 44.44% <56.13%> (+0.23%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/controller/networkpolicy/reject.go 83.51% <0.00%> (-2.36%) ⬇️
pkg/apis/controlplane/types.go 0.00% <ø> (ø)
pkg/controller/types/networkpolicy.go 100.00% <ø> (ø)
pkg/apis/controlplane/sets.go 22.22% <14.28%> (-6.90%) ⬇️
pkg/agent/openflow/network_policy.go 79.15% <26.53%> (-0.38%) ⬇️
...g/controller/networkpolicy/clusternetworkpolicy.go 68.18% <38.88%> (-1.04%) ⬇️
pkg/agent/openflow/pipeline.go 82.17% <40.74%> (+8.86%) ⬆️
...ntroller/networkpolicy/networkpolicy_controller.go 80.14% <46.15%> (-2.19%) ⬇️
pkg/apis/controlplane/v1beta2/sets.go 46.21% <71.42%> (-8.22%) ⬇️
pkg/agent/controller/networkpolicy/cache.go 89.02% <77.77%> (+0.23%) ⬆️
... and 83 more

@jianjuns jianjuns changed the title Add ACNP appliedto Nodeport svc support Add ACNP appliedto NodePort svc support Jul 13, 2022
pkg/controller/types/networkpolicy.go Outdated Show resolved Hide resolved
pkg/apis/controlplane/types.go Outdated Show resolved Hide resolved
pkg/apis/controlplane/v1beta2/types.go Outdated Show resolved Hide resolved
pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved
@GraysonWu
Copy link
Contributor Author

Looks like there is a race condition that will cause deadlock. Working on it. Will address @jianjuns 's comments in the same commit.

@GraysonWu GraysonWu force-pushed the np-on-svc branch 3 times, most recently from a881218 to 0dacb3f Compare July 19, 2022 02:07
@lgtm-com
Copy link

lgtm-com bot commented Jul 19, 2022

This pull request introduces 1 alert when merging 0dacb3f into 231b09d - view on LGTM.com

new alerts:

  • 1 for Off-by-one comparison against length

@lgtm-com
Copy link

lgtm-com bot commented Jul 19, 2022

This pull request introduces 1 alert when merging 0cc328f into 231b09d - view on LGTM.com

new alerts:

  • 1 for Off-by-one comparison against length

@GraysonWu GraysonWu force-pushed the np-on-svc branch 3 times, most recently from 652d2f6 to 8965521 Compare July 21, 2022 01:21
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
@tnqn
Copy link
Member

tnqn commented Jul 21, 2022

  1. If a policy or an ingress rule is applied to Services, that only ipBlock can be used to select workloads.
  2. A policy or an ingress rule can't be applied to Services and other workloads at the same time.
    appliedTo like below is invalid:

The reason for 3 & 4 is that we use service field to decide if a policy or ingress rule is a NodePort Service policy/rule. If it is a NodePort Service policy/rule, we can only use ct_nw_src, transformed from ipBlock, to do the source matching.

I'm not against to support IPBlocks as peer only given that we apply this to NodePort Service only which is typically accessed by external network. However, this explanation seems to say it's because of implementation restriction. I don't get why we we not use ct_nw_src for Pod IPs of a normal AddressGroup.

@tnqn tnqn added this to the Antrea v1.8 release milestone Jul 21, 2022
@GraysonWu
Copy link
Contributor Author

  1. If a policy or an ingress rule is applied to Services, that only ipBlock can be used to select workloads.
  2. A policy or an ingress rule can't be applied to Services and other workloads at the same time.
    appliedTo like below is invalid:

The reason for 3 & 4 is that we use service field to decide if a policy or ingress rule is a NodePort Service policy/rule. If it is a NodePort Service policy/rule, we can only use ct_nw_src, transformed from ipBlock, to do the source matching.

I'm not against to support IPBlocks as peer only given that we apply this to NodePort Service only which is typically accessed by external network. However, this explanation seems to say it's because of implementation restriction. I don't get why we we not use ct_nw_src for Pod IPs of a normal AddressGroup.

There are two reasons:

  1. If a client Pod on Node A tries to access Node B NodePort Service when the packet arrives Node B, it has already been SNAT'd to Node A IP. The client Pod IP can't be matched by nw_src or ct_nw_src on Node B.
  2. If the client Pod and the endpoint of the Service are the same Node when the client Pod initiates traffic to its local NodePort Service, the packet won't be SNAT'd. We can't use ct_nw_src to match it.

Let me know if I misunderstood something.

Other than the implementation reason, I totally agree with you that a NodePort Service is typically accessed by an external network. In-cluster Pods will usually use Service ClusterIP. IMO, supporting podSelector for a NodePort Service doesn't make too much sense.

@tnqn
Copy link
Member

tnqn commented Jul 22, 2022

There are two reasons:

  1. If a client Pod on Node A tries to access Node B NodePort Service when the packet arrives Node B, it has already been SNAT'd to Node A IP. The client Pod IP can't be matched by nw_src or ct_nw_src on Node B.
  2. If the client Pod and the endpoint of the Service are the same Node when the client Pod initiates traffic to its local NodePort Service, the packet won't be SNAT'd. We can't use ct_nw_src to match it.

1 makes sense. 2 is not true. ct_nw_src will just be original src IP if it's not SNAT'd.

@GraysonWu GraysonWu force-pushed the np-on-svc branch 8 times, most recently from 700e60a to 739e7df Compare July 27, 2022 21:57
@GraysonWu
Copy link
Contributor Author

/test-integration

@GraysonWu
Copy link
Contributor Author

/test-all

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

pkg/agent/controller/networkpolicy/reconciler.go Outdated Show resolved Hide resolved
pkg/agent/controller/networkpolicy/reject.go Outdated Show resolved Hide resolved
pkg/apis/controlplane/v1beta2/sets.go Show resolved Hide resolved
pkg/controller/networkpolicy/networkpolicy_controller.go Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
docs/antrea-network-policy.md Outdated Show resolved Hide resolved
@GraysonWu GraysonWu force-pushed the np-on-svc branch 2 times, most recently from 090b3a6 to 8c089ed Compare August 2, 2022 00:51
@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging 8c089ed into 98967d7 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

Address comments and add more validation UT cases.

Signed-off-by: wgrayson <[email protected]>
@tnqn tnqn added kind/feature Categorizes issue or PR as related to a new feature. action/release-note Indicates a PR that should be included in release notes. labels Aug 2, 2022
@GraysonWu
Copy link
Contributor Author

/test-all

@GraysonWu GraysonWu requested a review from tnqn August 3, 2022 01:35
tnqn
tnqn previously approved these changes Aug 4, 2022
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

@GraysonWu
Copy link
Contributor Author

/test-all

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 Aug 5, 2022

@jianjuns do you have more comments on this one?

@jianjuns
Copy link
Contributor

jianjuns commented Aug 5, 2022

@jianjuns do you have more comments on this one?

No.

@GraysonWu
Copy link
Contributor Author

/test-integration

@tnqn
Copy link
Member

tnqn commented Aug 8, 2022

/skip-integration tested manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodeport and Network Policies
3 participants