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 E2E test for L7 NetworkPolicy Logging #6275

Merged
merged 1 commit into from
May 14, 2024

Conversation

qiyueyao
Copy link
Contributor

@qiyueyao qiyueyao commented Apr 29, 2024

This PR adds an E2E test for L7 NetworkPolicy logging. It checks both allowed and dropped HTTP event logs under l7engine directory.

Resolving this issue from discussion.

test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
Comment on lines 400 to 397
if !strings.Contains(stdout, "http") || !strings.Contains(stdout, "alert") {
t.Logf("Audit log file does not contain enough entries yet")
return false, nil
}
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 do this after the decoding if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this part, and added checking the decoded object against matchers.

test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
@qiyueyao qiyueyao force-pushed the l7-logtest branch 3 times, most recently from 4ce8423 to 92cdfae Compare May 8, 2024 20:25
gotLogs = append(gotLogs, log)
}
}
assert.ElementsMatch(t, gotLogs, matchers)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably retry if the entries are not there (yet). You used to handle this case (you had some code before the decoding). We should do it here instead.

I would personally recommend replacing wait.PollUntilContextTimeout here with https://pkg.go.dev/github.com/stretchr/testify/assert#EventuallyWithT. This way you can keep using assert.ElementsMatch.

assert.EventuallyWithT(t, func(c *assert.CollectT) {
        stdout, stderr, err := data.RunCommandFromPod(antreaNamespace, antreaPodName, "antrea-agent", cmd)
        assert.NoError(t, err, ...)
        // ...
        assert.ElementsMatch(t, gotLogs, matchers)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, updated to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I gave you a bad idea here. This is not great, because we want the polling to fail immediately if either RunCommandFromPod or dec.Decode fails, and there is currently no way to do that with assert.EventuallyWithT. So I think we need to go back to wait.PollUntilContextTimeout here, as it lets us do that :/

That also means that instead of assert.ElementsMatch, you may to use something like slices.Equal

Copy link
Contributor Author

@qiyueyao qiyueyao May 9, 2024

Choose a reason for hiding this comment

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

Actually I was quite convinced, if RunCommandFromPod fail, it means the file did not exist yet and we should wait? dec.Decode probably shouldn't fail unless, after waiting, unrecognized characters are overwritten by logs..

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Ok, since we cannot use require.Error in this context (see stretchr/testify#1396), and assuming you want to keep using assert.EventuallyWithT, we should at least interrupt the iteration if there is a decoding error:

			if !assert.NoError(t, dec.Decode(&log)) {
				return
			}
			if slices.Contains(matchers, log) {
				gotLogs = append(gotLogs, log)
			}

(And same thing if RunCommandFromPod fails.)

If decoding errors are never supposed to happen, then I guess in practice it doesn't really matter that we have to wait for the full polling duration (10s) before failing the test.

But note that wait.PollUntilContextTimeout offers all the granularity we need:

stdout, stderr, err := data.RunCommandFromPod(antreaNamespace, antreaPodName, "antrea-agent", cmd)
if err != nil {
        return false, nil // not ready yet
}
// ...
if err := dec.Decode(&log); err != nil {
        return false, err // fail immediately
}
// ...
return !slices.Equal(), nil

Which is why I kind of regret asking you to change it :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha It's good for me to learn this, and I've changed to wait.PollUntilContextTimeout which makes sense.

test/e2e/l7networkpolicy_test.go Outdated Show resolved Hide resolved
This PR adds an E2E test for L7 NetworkPolicy
logging. It checks both allowed and dropped HTTP
event logs under l7engine directory.

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

/test-e2e

@antoninbas
Copy link
Contributor

/test-conformance
/test-networkpolicy

@antoninbas antoninbas merged commit 8dc54a6 into antrea-io:main May 14, 2024
52 of 57 checks passed
@qiyueyao qiyueyao deleted the l7-logtest branch May 15, 2024 06:47
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.

3 participants