Add "antctl get bgppolicy" agent command #6646
Conversation
Atish-iaf
force-pushed
the
support-bgp-in-antctl
branch
from
b1b9ee0 to
187729c
Compare
rajnkamr
added
area/transit/bgp
Atish-iaf
force-pushed
the
support-bgp-in-antctl
branch
3 times, most recently
from
23b5852 to
47e4d33
Compare
Atish-iaf
changed the title
| bgpPolicyName, routerID, localASN, listenPort := bgpPolicyInfoQuerier.GetBGPPolicyInfo() | ||
| bgpPolicyResp := apis.BGPPolicyResponse{ |
There was a problem hiding this comment.
Can the client clearly identify when there is no effective BGPPolicy applied to the Node? In that scenario, antctl get bgppolicy should display an empty table, and not a table with one row of empty strings. So I want to make sure this is handled correctly by the current implementation?
Atish-iaf
force-pushed
the
support-bgp-in-antctl
branch
2 times, most recently
from
9fb88b7 to
6a521ec
Compare
pkg/antctl/command_definition.go
Outdated
| if reflect.Indirect(ref).IsZero() { | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
This is too specific to your use case IMO. A "zero" struct may not always mean "no object".
Honestly for this command, it would maybe make more sense to return a 404 Not Found in the API if there is no effective BGP policy.
There was a problem hiding this comment.
I think returning a 404 Not Found is not consistent as there are some other commands like antctl get addressgroup/appliedtogroup/netpol returning empty string as output without 404 when there is no resource. I think it is expected that a command should return zero exit code and run successfully when there is no resource. There is an e2e test for the same expectation.
antrea/test/e2e/antctl_test.go
Line 112 in b96d22f
There was a problem hiding this comment.
I think your initial views were also similar #6646 (comment)
There was a problem hiding this comment.
I think the comparison is not great
IMO, this command is more similar to antctl get addressgroup <name> than it is to antctl get addressgroup, because we are querying a specific object (the "effective" bgppolicy) and not a list of objects. And antctl get addressgroup <name> will return an error if there is no matching resource.
My original comment was more about antctl output formatting than it was about the API response.
We could in theory have the API return a 404, and the output be an empty table. But IMO, an error message like "there is no effective policy applied to the Node" with a non-zero error code is more appropriate. It is also easier to consume programmatically.
In any case, a 404 for the API response seems more appropriate than a "zero" struct.
There was a problem hiding this comment.
done, thanks @antoninbas
Atish-iaf
force-pushed
the
support-bgp-in-antctl
branch
from
6a521ec to
fbd7874
Compare
Atish-iaf
force-pushed
the
support-bgp-in-antctl
branch
4 times, most recently
from
719d0d3 to
ca18250
Compare
| @@ -122,7 +122,7 @@ func testAntctlAgentLocalAccess(t *testing.T, data *TestData) { | |||
| cmd := strings.Join(args, " ") | |||
| t.Run(cmd, func(t *testing.T) { | |||
| stdout, stderr, err := runAntctl(podName, args, data) | |||
| if err != nil && !strings.HasSuffix(stderr, "not enabled\n") { | |||
| if err != nil && !(strings.HasSuffix(stderr, "not enabled\n") || strings.HasSuffix(stderr, "there is no effective bgp policy applied to the Node\n")) { | |||
There was a problem hiding this comment.
I understand the change, but we need to find a better way to test this
The test is quite shallow anyway, as it doesn't really validate much, but anyway that is out-of-scope of the PR
Signed-off-by: Kumar Atish <kumar.atish@broadcom.com>
Atish-iaf
force-pushed
the
support-bgp-in-antctl
branch
from
ca18250 to
45a2cde
Compare
antoninbas
added
area/component/antctl
|
/test-all |
rajnkamr
added
the
kind/documentation
Add `antctl get bgppolicy` agent command to get effective BGP policy applied on the Node. The command is implemented using a new HTTP endpoint (`/bgppolicy`), which will return a `404 Not Found` error if no BGPPolicy has been applied on the Node. For antrea-io#6209 Signed-off-by: Kumar Atish <kumar.atish@broadcom.com>
Add
antctl get bgppolicyagent command to get effective BGP policy applied on the Node.For #6209