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 support for ExternalIP in AntreaProxy #4866

Merged
merged 1 commit into from
May 16, 2023

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Apr 16, 2023

This PR adds the ability to serve ExternalIP for AntreaProxy, allowing for
external client accesses to Services running in Kubernetes. In Kubernetes, an
ExternalIP is a feature that allows a Service to be accessed from outside the
cluster using a static IP address.

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

@hongliangl hongliangl added this to the Antrea v1.12 release milestone Apr 16, 2023
@hongliangl hongliangl added area/proxy Issues or PRs related to proxy functions in Antrea action/release-note Indicates a PR that should be included in release notes. labels Apr 16, 2023
@hongliangl hongliangl requested a review from tnqn April 20, 2023 13:52
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.

I guess there may be a misunderstanding between the ExternalIPs field and the ExternalIP type.

pkg/agent/config/service_config.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the 20230414-externalip branch 2 times, most recently from f2f668b to 30680da Compare May 3, 2023 13:00
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/route/interfaces.go Outdated Show resolved Hide resolved
pkg/agent/route/interfaces.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the 20230414-externalip branch 2 times, most recently from 1e69e33 to 2d2d6b7 Compare May 9, 2023 05:33
@luolanzone
Copy link
Contributor

@hongliangl could you check and improve the unit test code coverage for this patch? thanks.

@hongliangl
Copy link
Contributor Author

@hongliangl could you check and improve the unit test code coverage for this patch? thanks.

Sure

@hongliangl hongliangl requested a review from tnqn May 9, 2023 06:57
@luolanzone
Copy link
Contributor

@hongliangl please help to update the PR summary and commit messages with the necessary information about ExternalIP to help other reviewers to understand this change.

pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/fields.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/client_test.go Show resolved Hide resolved
pkg/ovs/openflow/testing/utils.go Outdated Show resolved Hide resolved
pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the 20230414-externalip branch 4 times, most recently from 07a0467 to 0c42fdb Compare May 9, 2023 11:22
@hongliangl
Copy link
Contributor Author

@hongliangl please help to update the PR summary and commit messages with the necessary information about ExternalIP to help other reviewers to understand this change.

Done

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/fields.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
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/openflow/fields.go Outdated Show resolved Hide resolved
pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved
@hongliangl hongliangl requested a review from tnqn May 10, 2023 23:56
@hongliangl hongliangl force-pushed the 20230414-externalip branch 2 times, most recently from bd3c699 to e21422c Compare May 11, 2023 02:38
// installs the flow which has a learn action to maintain the LB decision. The group with the groupID must be
// installed before, otherwise the installation will fail.
// externalAddress indicates that whether the Service is externally accessible, like NodePort, LoadBalancer and ExternalIP.
// nested indicates that whether the Service are backed by Endpoints of other Services.
Copy link
Member

Choose a reason for hiding this comment

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

It sounds the Service and other Services are sharing Endpoints, but we actually mean the Service's Endpoint is other Service's ClusterIPs, right?
Nested Service's ClusterIP -> Nested Service's Endpoint (Other Service's ClusterIP) -> Other Service's Endpoint

How about "nested indiciates the whether the Service are backed by Service IPs of other Services"? @jianjuns @hongliangl

Copy link
Contributor Author

@hongliangl hongliangl May 11, 2023

Choose a reason for hiding this comment

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

I think it is more readable by be backed by Service IPs of other Services.

Comment on lines +810 to +811
isLoad := spec.Header.Dst == true
isMatch := spec.Header.Dst == false
Copy link
Member

Choose a reason for hiding this comment

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

I actually don't understand the change, is it fixing an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to fix the action learn. Previously, action learn to string was wrong.

@hongliangl hongliangl force-pushed the 20230414-externalip branch 2 times, most recently from 78c35eb to b262e13 Compare May 11, 2023 03:03
@hongliangl hongliangl requested a review from tnqn May 11, 2023 07:03
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, let's see if @jianjuns @wenyingd have other comments

@hongliangl hongliangl force-pushed the 20230414-externalip branch 2 times, most recently from cffffcc to c9eb9e2 Compare May 11, 2023 07:38
wenyingd
wenyingd previously approved these changes May 11, 2023
Copy link
Contributor

@wenyingd wenyingd 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 hongliangl requested a review from tnqn May 11, 2023 08:22
tnqn
tnqn previously approved these changes May 11, 2023
This PR adds the ability to serve ExternalIP for AntreaProxy, allowing for
external client accesses to Services running in Kubernetes. In Kubernetes, an
ExternalIP is a feature that allows a Service to be accessed from outside the
cluster using a static IP address.

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

tnqn commented May 15, 2023

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

@hongliangl
Copy link
Contributor Author

/test-windows-proxyall-e2e
/test-windows-e2e

@hongliangl
Copy link
Contributor Author

/test-windows-proxyall-e2e

@tnqn
Copy link
Member

tnqn commented May 16, 2023

@jianjuns do you have any other comments on this one?

@jianjuns
Copy link
Contributor

@jianjuns do you have any other comments on this one?

No. Please merge.

@tnqn
Copy link
Member

tnqn commented May 16, 2023

/skip-conformance which failed on unrelated failure: a deleted service IP happened to be an available external server.

@tnqn tnqn merged commit 8dbb48c into antrea-io:main May 16, 2023
@hongliangl hongliangl deleted the 20230414-externalip branch May 16, 2023 05:51
ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023
This PR adds the ability to serve ExternalIP for AntreaProxy, allowing for
external client accesses to Services running in Kubernetes. In Kubernetes, an
ExternalIP is a feature that allows a Service to be accessed from outside the
cluster using a static IP address.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
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.

6 participants