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 "antctl get bgproutes" agent command #6734

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

Atish-iaf
Copy link
Contributor

@Atish-iaf Atish-iaf commented Oct 11, 2024

Add antctl get bgproutes agent command to print the advertised BGP routes on the local Node.

The command is implemented using a new HTTP endpoint (/bgproutes), which will return a 404 Not Found error if no BGPPolicy has been applied on the Node.

For #6209

@Atish-iaf Atish-iaf added area/component/antctl Issues or PRs releated to the command line interface component area/transit/bgp Issues or PRs related to BGP support. labels Oct 11, 2024
@Atish-iaf Atish-iaf mentioned this pull request Oct 11, 2024
4 tasks
@Atish-iaf Atish-iaf marked this pull request as ready for review October 11, 2024 09:55
@Atish-iaf Atish-iaf marked this pull request as draft October 11, 2024 10:37
@Atish-iaf Atish-iaf marked this pull request as ready for review October 11, 2024 11:06
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.

Not done reviewing it yet, but at a high-level I do not think we should be handling "received routes" at the moment. Unless I am mistaken, this is not a concept which is really relevant to the (current) Antrea BGP implementation.

docs/antctl.md Outdated
Comment on lines 793 to 794
# Get the list of all received bgp routes
$ antctl get bgproutes --in
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not handle "received routes" in the Antrea BGP implementation, so I think this may be misleading to users
cc @hongliangl

Copy link
Contributor

Choose a reason for hiding this comment

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

we do not handle "received routes" in the Antrea BGP implementation

Exactly. At least right now, we don't care about the "received routes" from remote peers. In current implementation of BGPPolicy controller, we only advertise routes, for the "received routes", we do nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hongliangl
To track, better to create a separate issue to support received routes for future use-case , as received routes can be important for scenarios involving multiple BGP peers, dynamic route management, or multi-cluster setups,
as of now for basic CNI and Kubernetes networking, the focus is primarily on what the cluster advertises !

pkg/agent/apiserver/handlers/bgproute/handler.go Outdated Show resolved Hide resolved
docs/antctl.md Outdated
Comment on lines 793 to 794
# Get the list of all received bgp routes
$ antctl get bgproutes --in
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not handle "received routes" in the Antrea BGP implementation

Exactly. At least right now, we don't care about the "received routes" from remote peers. In current implementation of BGPPolicy controller, we only advertise routes, for the "received routes", we do nothing.

pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller_test.go Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller_test.go Show resolved Hide resolved
pkg/agent/controller/bgp/controller_test.go Outdated Show resolved Hide resolved
@luolanzone luolanzone added the action/release-note Indicates a PR that should be included in release notes. label Oct 12, 2024
@Atish-iaf Atish-iaf added this to the Antrea v2.2 release milestone Oct 15, 2024
@Atish-iaf Atish-iaf force-pushed the antctl-get-bgproutes branch 2 times, most recently from 84cd772 to 345ee83 Compare October 15, 2024 09:55
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
docs/antctl.md Outdated Show resolved Hide resolved
@Atish-iaf Atish-iaf force-pushed the antctl-get-bgproutes branch 3 times, most recently from f38c39d to 2aea77b Compare October 17, 2024 05:33
@Atish-iaf
Copy link
Contributor Author

/test-all

docs/antctl.md Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
docs/antctl.md Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
pkg/querier/querier.go Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
docs/antctl.md Outdated Show resolved Hide resolved
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.

only a handful of nits, otherwise LGTM

pkg/agent/apiserver/handlers/bgproute/handler.go Outdated Show resolved Hide resolved
pkg/agent/controller/bgp/controller.go Outdated Show resolved Hide resolved
@Atish-iaf Atish-iaf force-pushed the antctl-get-bgproutes branch 2 times, most recently from 996bfa1 to 633f3bf Compare October 19, 2024 03:42
@rajnkamr rajnkamr added the kind/documentation Categorizes issue or PR as related to a documentation. label Oct 21, 2024
@Atish-iaf Atish-iaf removed the kind/documentation Categorizes issue or PR as related to a documentation. label Oct 21, 2024
pkg/agent/apiserver/handlers/bgproute/handler.go Outdated Show resolved Hide resolved
pkg/antctl/antctl.go Outdated Show resolved Hide resolved
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.

one last comment, otherwise lgtm

Comment on lines 39 to 58
var ipv4Routes, ipv6Routes = false, false
rawQuery := r.URL.RawQuery
switch rawQuery {
case "ipv4-only=":
ipv4Routes = true
case "ipv6-only=":
ipv6Routes = true
case "":
ipv4Routes = true
ipv6Routes = true
default:
http.Error(w, "invalid query", http.StatusBadRequest)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use this variation:

		values := r.URL.Query()
		var ipv4Only, ipv6Only bool
		if values.Has("ipv4-only") {
				if values.Get("ipv4-only") != "" {
						// bad request
				}
				ipv4Only = true
		}
		if values.Has("ipv6-only") {
				if values.Get("ipv6-only") != "" {
						// bad request
				}
				ipv6Only = true
		}
		if ipv4Only && ipv6Only {
				// bad request
		}
		// ...
		bgpRoutes, err := bq.GetBGPRoutes(r.Context(), !ipv6Only, !ipv4Only)

I feel like the current validation is a bit too restrictive / low-level. We should accept both ipv4-only= and ipv4-only for example.

@Atish-iaf
Copy link
Contributor Author

/test-all

Add `antctl get bgproutes` agent command to print the advertised BGP routes.

The command is implemented using a new HTTP endpoint (`/bgproutes`),
which will return a `404 Not Found` error if no BGPPolicy has been applied
on the Node.

Signed-off-by: Kumar Atish <kumar.atish@broadcom.com>
@antoninbas
Copy link
Contributor

/test-all

@XinShuYang
Copy link
Contributor

/test-e2e
/test-conformance

@XinShuYang
Copy link
Contributor

/test-conformance

@XinShuYang
Copy link
Contributor

/test-all-features-conformance

@antoninbas antoninbas merged commit aeb03b0 into antrea-io:main Oct 23, 2024
54 of 58 checks passed
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/component/antctl Issues or PRs releated to the command line interface component area/transit/bgp Issues or PRs related to BGP support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants