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

Fix L7 NetworkPolicy e2e test failure #6138

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Mar 22, 2024

Fix L7 NetworkPolicy e2e test failure

Fix #6129

In the failure tests, the following function is called to
verify whether a connection should be allowed or denied.
To verify a connection should be denied, it requires 5 seconds.

func probeClientIPFromPod(data *TestData, pod, container string, baseUrl string) (string, error) {
	url := fmt.Sprintf("%s/%s", baseUrl, "clientip")
	hostPort, _, err := data.runWgetCommandFromTestPodWithRetry(pod, data.testNamespace, container, url, 5)
	if err != nil {
		return "", err
	}
	host, _, err := net.SplitHostPort(hostPort)
	return host, err
}

Before #5843, these e2e tests utilized the function PollImmediate
from k8s.io/apimachinery/pkg/util/wait, which immediately calls an
anonymous function including the above function. Since the timeout
is 5 seconds, and the ticker time is 1 second, and the anonymous
function runs immediately, the 5-second timeout is sufficient to
verify the denied state of a connection as mentioned above. However,
after #5843, the function Eventually from github.com/stretchr/testify/assert
is used with the same parameters, which implies that the anonymous
function runs after the first ticker time, leaving 4 seconds. 4 seconds
are insufficient to verify the denied state of a connection.

To resolve the issue, RunCommandFromPod called in
data.runWgetCommandFromTestPodWithRetry is called directly in function
Eventually to verify the connection state.

antoninbas
antoninbas previously approved these changes Mar 22, 2024
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.

LGTM, but it feels like the methodology is not great for validating denied connections. This kind of function (assert.Eventually, wait.Poll) is really meant to perform a single validation, and retry until the validation succeeds or until a timeout. In this case here, the probe function itself includes a retry mechanism, which is not ideal.

@antoninbas antoninbas requested a review from tnqn March 22, 2024 16:50
@tnqn
Copy link
Member

tnqn commented Mar 25, 2024

but it feels like the methodology is not great for validating denied connections. This kind of function (assert.Eventually, wait.Poll) is really meant to perform a single validation, and retry until the validation succeeds or until a timeout. In this case here, the probe function itself includes a retry mechanism, which is not ideal.

Agree with @antoninbas. It's obscure to rely on the difference between the two timeouts. It also wastes time to retry 5 times when the expectation is failure. Can we call RunCommandFromPod directly in Eventually?

@hongliangl
Copy link
Contributor Author

but it feels like the methodology is not great for validating denied connections. This kind of function (assert.Eventually, wait.Poll) is really meant to perform a single validation, and retry until the validation succeeds or until a timeout. In this case here, the probe function itself includes a retry mechanism, which is not ideal.

Agree with @antoninbas. It's obscure to rely on the difference between the two timeouts. It also wastes time to retry 5 times when the expectation is failure. Can we call RunCommandFromPod directly in Eventually?

@tnqn Done

tnqn
tnqn previously approved these changes Mar 25, 2024
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

Fix antrea-io#6129

In the failure tests, the following function is called to
verify whether a connection should be allowed or denied.
To verify a connection should be denied, it requires 5 seconds.

```go
func probeClientIPFromPod(data *TestData, pod, container string, baseUrl string) (string, error) {
	url := fmt.Sprintf("%s/%s", baseUrl, "clientip")
	hostPort, _, err := data.runWgetCommandFromTestPodWithRetry(pod, data.testNamespace, container, url, 5)
	if err != nil {
		return "", err
	}
	host, _, err := net.SplitHostPort(hostPort)
	return host, err
}
```

Before antrea-io#5843, these e2e tests utilized the function `PollImmediate`
from `k8s.io/apimachinery/pkg/util/wait`, which immediately calls an
anonymous function including the above function. Since the timeout
is 5 seconds, and the ticker time is 1 second, and the anonymous
function runs immediately, the 5-second timeout is sufficient to
verify the denied state of a connection as mentioned above. However,
after antrea-io#5843, the function `Eventually` from `github.com/stretchr/testify/assert`
is used with the same parameters, which implies that the anonymous
function runs after the first ticker time, leaving 4 seconds. 4 seconds
are insufficient to verify the denied state of a connection.

To resolve the issue, `RunCommandFromPod` called in
`data.runWgetCommandFromTestPodWithRetry` is called directly in function
 `Eventually` to verify the connection state.

Signed-off-by: Hongliang Liu <[email protected]>
@antoninbas
Copy link
Contributor

/test-all

@antoninbas
Copy link
Contributor

/test-conformance

@antoninbas antoninbas merged commit 91f374b into antrea-io:main Mar 25, 2024
51 of 56 checks passed
@hongliangl hongliangl deleted the 20240322-l7-issue branch March 26, 2024 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestL7NetworkPolicy e2e tests failing consistently
3 participants