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

Support Internal Traffic Policy in AntreaProxy #2792

Merged
merged 1 commit into from Apr 1, 2022

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Sep 16, 2021

InternalTrafficPolicy is introduced in Kubernetes 1.21. Service Internal
Traffic Policy enables internal traffic restrictions to only route
internal traffic to Endpoints within the Node the traffic originated
from. The "internal" traffic here refers to traffic originated from Pods
in the current cluster. This can help to reduce costs and improve
performance.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2021

Codecov Report

Merging #2792 (f6804fc) into main (b9e37a8) will decrease coverage by 1.15%.
The diff coverage is 53.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2792      +/-   ##
==========================================
- Coverage   64.17%   63.01%   -1.16%     
==========================================
  Files         278      278              
  Lines       27825    27861      +36     
==========================================
- Hits        17856    17558     -298     
- Misses       8048     8438     +390     
+ Partials     1921     1865      -56     
Flag Coverage Δ
kind-e2e-tests 49.53% <40.90%> (-1.70%) ⬇️
unit-tests 43.38% <53.03%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/proxy/proxier.go 61.20% <53.03%> (-1.52%) ⬇️
...g/agent/apiserver/handlers/featuregates/handler.go 0.00% <0.00%> (-82.36%) ⬇️
...kg/apiserver/registry/system/supportbundle/rest.go 20.45% <0.00%> (-54.55%) ⬇️
pkg/support/dump.go 8.19% <0.00%> (-49.19%) ⬇️
...egator/apiserver/handlers/recordmetrics/handler.go 0.00% <0.00%> (-44.45%) ⬇️
pkg/support/dump_others.go 0.00% <0.00%> (-44.00%) ⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go 0.00% <0.00%> (-40.00%) ⬇️
...agent/apiserver/handlers/appliedtogroup/handler.go 0.00% <0.00%> (-40.00%) ⬇️
...gregator/apiserver/handlers/flowrecords/handler.go 0.00% <0.00%> (-40.00%) ⬇️
pkg/apiserver/handlers/loglevel/handler.go 0.00% <0.00%> (-38.47%) ⬇️
... and 27 more

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

what are we doing for e2e testing?
given that the feature gate is only enabled by default in K8s v1.22+, maybe we should have some temporary Antrea e2e tests for local internal traffic policy.

build/yamls/antrea-aks.yml Outdated Show resolved Hide resolved
docs/feature-gates.md Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the proxy-internal-traffic-policy branch 2 times, most recently from 4029309 to 19d01b6 Compare September 22, 2021 15:36
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@jianjuns
Copy link
Contributor

In the commit message:

The "internal" traffic here refers to traffic originated from Pod in the current cluster
The "internal" traffic here refers to traffic originates from Pods in the cluster

@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 19, 2022
@hongliangl hongliangl force-pushed the proxy-internal-traffic-policy branch 3 times, most recently from c313c53 to a440e31 Compare February 25, 2022 14:40
@hongliangl
Copy link
Contributor Author

@tnqn @jianjuns @antoninbas @wenyingd Could you review this PR when you are available? I have rebased this PR. I only added unit tests, and I will add e2e tests in another PR. Thanks a lot.

@hongliangl
Copy link
Contributor Author

/test-all
/test-windows-all

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 26, 2022
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl
Copy link
Contributor Author

/test-windows-proxyall-e2e
/test-flexible-ipam-e2e

@hongliangl
Copy link
Contributor Author

/test-ipv6-only-e2e

@tnqn tnqn added this to the Antrea v1.6 release milestone Mar 9, 2022
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 9, 2022
@hongliangl hongliangl removed the action/release-note Indicates a PR that should be included in release notes. label Mar 16, 2022
@hongliangl hongliangl removed this from the Antrea v1.6 release milestone Mar 16, 2022
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl hongliangl force-pushed the proxy-internal-traffic-policy branch from ca25c33 to 9bb09d0 Compare March 28, 2022 04:24
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the proxy-internal-traffic-policy branch from 9bb09d0 to 3a27beb Compare March 28, 2022 06:37
@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl
Copy link
Contributor Author

@jianjuns @tnqn Could you help review this PR if you are available? Thanks.

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

Just a few nits.

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the proxy-internal-traffic-policy branch from 3a27beb to 98697a7 Compare March 31, 2022 02:51
InternalTrafficPolicy is introduced in Kubernetes 1.21. Service Internal
Traffic Policy enables internal traffic restrictions to only route
internal traffic to Endpoints within the Node the traffic originated
from. The "internal" traffic here refers to traffic originated from Pods
in the current cluster. This can help to reduce costs and improve
performance.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor Author

@tnqn Do you have any comments about this PR?

@hongliangl
Copy link
Contributor Author

/test-all-features-conformance
/test-conformance
/test-e2e
/test-flexible-ipam-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-networkpolicy
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-ipv6-only-networkpolicy
/test-multicluster-e2e
/test-networkpolicy
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy
/test-windows-proxyall-e2e
/test-integration

@hongliangl hongliangl added the area/proxy Issues or PRs related to proxy functions in Antrea label Mar 31, 2022
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 31, 2022
@hongliangl
Copy link
Contributor Author

/test-integration
/test-multicluster-e2e
/test-windows-networkpolicy

// of the Service are different, install two groups. One group has all Endpoints, the other has only
// local Endpoints.
groupID := p.groupCounter.AllocateIfNotExist(svcPortName, true)
if err = p.ofClient.InstallServiceGroup(groupID, svcInfo.StickyMaxAgeSeconds() != 0, localEndpointUpdateList); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - I know it is not introduced by you, but what is the strategy to handle errors? Is it right just to ignore them?

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. area/proxy Issues or PRs related to proxy functions in Antrea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants