Add e2e tests for BGPPolicy #6523
Conversation
hongliangl
added
the
area/test/e2e
hongliangl
force-pushed
the
20240712-bgppolicy-e2e
branch
2 times, most recently
from
7927513
to
f53eae6
Compare
test/e2e/bgppolicy_test.go
Outdated
| updatedLocalASN = int32(64513) | ||
|
|
||
| password = "password" | ||
| defaultBGPPolicySecretName = "antrea-bgp-passwords" // #nosec G101 |
There was a problem hiding this comment.
why is there "Policy" in this variable name? I think the secret name is global and not specific to a policy?
There was a problem hiding this comment.
Updated to use the const BGPPolicySecretName in pkg/agent/types/bgppolicy.go
test/e2e/bgppolicy_test.go
Outdated
| require.Equal(t, 0, rc) | ||
| } | ||
|
|
||
| func dumpFRRRouterBGPRoutes() []FRRRoute { |
There was a problem hiding this comment.
I think it's not ideal that this function doesn't return an error when applicable, as in case of error the caller will keep looping until the timeout is reached.
I know that it's because of a current limitation of assert.EventuallyWithT (at least until the next release of testify - see stretchr/testify#1396).
For now, I would recommend defining an additional helper function, checkFRRRouterBGPRoutes which will use wait.Poll... to call dumpFRRRouterBGPRoutes in a loop. In case of error, checkFRRRouterBGPRoutes can fail immediately. The function should also probably call t.Helper for improved test debuggability.
hongliangl
force-pushed
the
20240712-bgppolicy-e2e
branch
3 times, most recently
from
89926f3
to
f124d02
Compare
test/e2e/bgppolicy_test.go
Outdated
| func generateVtyshCommandsForConfiguringBGP(localASN, remoteASN int32) []string { | ||
| commands := []string{ | ||
| "configure terminal", | ||
| fmt.Sprintf("router bgp %d", localASN), | ||
| "no bgp ebgp-requires-policy", | ||
| "no bgp network import-check", | ||
| } | ||
| for _, node := range clusterInfo.nodes { | ||
| commands = append(commands, fmt.Sprintf("neighbor %s remote-as %d", node.ipv4Addr, remoteASN)) | ||
| commands = append(commands, fmt.Sprintf("neighbor %s password %s", node.ipv4Addr, bgpPeerPassword)) | ||
| } | ||
| commands = append(commands, | ||
| "exit", | ||
| "exit", | ||
| "write memory") | ||
| return commands | ||
| } | ||
|
|
||
| func generateVtyshCommandsForClearingBGP(localASN int32) []string { |
There was a problem hiding this comment.
not sure why you didn't define configureBGP and cleanupBGP instead? (both function would ultimately call runVtyshCommands).
test/e2e/bgppolicy_test.go
Outdated
| } | ||
|
|
||
| func dumpFRRRouterBGPRoutes() ([]FRRRoute, error) { | ||
| rc, stdout, stderr, err := exec.RunDockerExecCommand(externalInfo.externalFRRCID, "/usr/bin/vtysh", "/", nil, "show ip route bgp") |
There was a problem hiding this comment.
should call runVtyshCommands instead
test/e2e/bgppolicy_test.go
Outdated
| rc, stdout, stderr, err := runVtyshCommands(generateVtyshCommandsForConfiguringBGP(remoteASN, localASN)) | ||
| defer runVtyshCommands(generateVtyshCommandsForClearingBGP(remoteASN)) | ||
| t.Log(stdout) | ||
| t.Log(stderr) |
There was a problem hiding this comment.
maybe runVtyshCommands could be in charge of logging stdout / stderr, as you seem to be doing consistently every time you call the function
There was a problem hiding this comment.
moved to configureExternalBGPRouter and logged on failure.
test/e2e/bgppolicy_test.go
Outdated
|
|
||
| t.Log("Configure the remote FRR router with BGP") | ||
| rc, stdout, stderr, err := runVtyshCommands(generateVtyshCommandsForConfiguringBGP(remoteASN, localASN)) | ||
| defer runVtyshCommands(generateVtyshCommandsForClearingBGP(remoteASN)) |
There was a problem hiding this comment.
I assume this can handle the case where the previous command (runVtyshCommands(generateVtyshCommandsForConfiguringBGP(remoteASN, localASN))) failed / partially succeeded?
There was a problem hiding this comment.
changed to defer on success of configuration.
test/e2e/bgppolicy_test.go
Outdated
|
|
||
| t.Log("Configure the remote FRR router with BGP") | ||
| rc, stdout, stderr, err := runVtyshCommands(generateVtyshCommandsForConfiguringBGP(remoteASN, localASN)) | ||
| defer runVtyshCommands(generateVtyshCommandsForClearingBGP(remoteASN)) |
There was a problem hiding this comment.
the parameter name for generateVtyshCommandsForClearingBGP is localASN but you are passing remoteASN to the function here?
You can probably ignore this comment, as I think I know why: the variables here are named from the point of view of the K8s Node while the Vtysh functions operate on the "external" FRR router. So what is "remote" for the K8s Node is "local" for the router.
There was a problem hiding this comment.
changed to externalASN and nodeASN to avoid confusion.
test/e2e/bgppolicy_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func checkFRRRouterBGPRoutes(t *testing.T, expectedRouteStrings, notExpectedRouteStrings []string) { |
There was a problem hiding this comment.
nit: this function should be in charge of converting the routes to strings (using routesToStrings)
test/e2e/bgppolicy_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("Failed to get the expected BGP routes: %v", err) | ||
| } |
There was a problem hiding this comment.
should use require.NoError for consistency
There was a problem hiding this comment.
updated, and added mode details for debugging.
test/e2e/bgppolicy_test.go
Outdated
| if notExpectedRoutesSet.Len() != 0 && gotRoutesSet.IsSuperset(notExpectedRoutesSet) { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
this doesn't seem correct to me. I think it should be if gotRoutesSet.Intersection(notExpectedRoutesSet).Len() > 0. In other words: if any of the routes in notExpectedRoutesSet are in the output, it's bad.
Alternatively, it can be written as if gotRoutesSet.HasAny(notExpectedRouteStrings...) which is more readable and does not require you to create notExpectedRoutesSet. Personally I would also not create expectedRoutesSet and then you can use if gotRoutesSet.HasAll(expectedRoutesString) for the previous conditions. Giving you:
if !gotRoutesSet.HasAll(expectedRoutesStrings) {
return false, nil
}
if gotRoutesSet.HasAny(notExpectedRoutesStrings) {
return false, nil
}There was a problem hiding this comment.
it's much clearer, updated as suggested
|
I assume we will add e2e testing for Egress later? |
Signed-off-by: Hongliang Liu <[email protected]>
Signed-off-by: Quan Tian <[email protected]>
tnqn
force-pushed
the
20240712-bgppolicy-e2e
branch
from
f124d02
to
493ad5e
Compare
|
/skip-all |
Depends on #6488 #6203